Open Mathekatze opened 2 months ago
d0sboots
commented
2 months ago I'm not quite going to mark this WONTFIX, but the behavior is unlikely to change here. Apparently we expand aliases recursively (unlike bash); this error is not the nicest for if that forms a loop, but it does explain the issue. :)
catloversg
commented
2 months ago I remember seeing this bug reported a couple of months ago. Back then, I thought about writing a detectRecursiveAlias to detect this problem and print a useful error message to the terminal instead of letting it throw an "too much recursion" error. When I asked for the specific expected behavior of some test cases, Hoekstraa said something along the lines of "look at POSIX spec". To be honest, that's not useful at all. In the end, I scrapped the plan and forgot about this problem. There are problems with our "alias" feature:
I tried to check old issues/PRs/discussions (on Discord), read the POSIX spec, test it on bash, write test cases, decide the expected behavior, think about the potential change in the behavior, etc. Things got messy really fast. I think there are 2 ways to solve it:
@d0sboots What do you think about [2]?
d0sboots
commented
2 months ago 2 is basically what we already have, just with a bad error message. XD
If you want to catch the exception and try to detect if it was a stack overflow, go ahead.
catloversg
commented
2 months ago I don't think we have a working detection of infinite recursion. In applyAliases (src\Alias.ts), we check currentlyProcessingAliases, but that check is flawed.
d0sboots
commented
2 months ago Maybe you misunderstood me. All known JS engines have working detection of infinite recursion. You can see that detection functioning in the bug report here, via the pasted exception.
All that remains is to show a better message for this case.
catloversg
commented
2 months ago Oh, I got it now. I thought about implementing a custom "detector" for our alias/global alias, but catching the JS error is a good idea.
StormSurge95
commented
2 months ago I know nothing about POSIX compliance...but why not simply add the recursive alias info into the help text for alias and then have something along the lines of:
if (aliasValue.includes(` ${aliasKey} `)) showRecursiveAliasError()To the second part, because the recursion can happen through more than one alias.
cmfrydos
commented
2 months ago You'd keep track of every rule already applied and don't apply a rule twice. Also implementing the posix rule that only unquoted words are potentially replaced by an alias could help here, since this would also allow writing an alias in the style TO tried to do.
I doubt js really has an infinite recursion detection algorithm, since that would solve the halting problem. The most it probably does is to stop when the function call stack overflows. That's why TO experience a slight lag before the error was thrown.
cmfrydos
commented
2 months ago wait, keeping track of the aliases should already be in: https://github.com/bitburner-official/bitburner-src/issues/630 and https://github.com/bitburner-official/bitburner-src/pull/741/. So this might just need a second look.
cmfrydos
commented
2 months ago I have fixed a small flaw in the recursive check for alias application. However, I have not modified the function to ensure only unquoted commands are aliased, as I am still reviewing the POSIX quoting rules and how they are interpreted by the game's terminal.
Additionally, I recommend not attempting to catch the "too much recursion" exception. This error varies by browser, leading to potential inconsistencies (more details here: MDN - InternalError: too much recursion).
The function applyAliases(origCommand: string, depth = 0, currentlyProcessingAliases: string[] = []) already effectively tracks the recursion depth and could be used for this purpose. This approach was previously replaced by a more advanced check introduced in #741. The old depth check could be reinstated as a fallback.
Please review my change and confirm if the new recursion check aligns with your expectations.
cmfrydos
commented
2 months ago I know nothing about POSIX compliance...but why not simply add the recursive alias info into the
helptext foraliasand then have something along the lines of:if (aliasValue.includes(` ${aliasKey} `)) showRecursiveAliasError()This would prevent an alias definition like buy="buy -l" entirely. I believe the alias system was never supposed to throw exceptions, but should just stop substituting when it runs into a recursion.
bcox
commented
1 month ago It might make sense for an alias only be able to call a 'base' command. alias scan=scan-analyze // scan from the command line runs scan-analyze. alias connected=scan // connected runs the base command scan, not the alias. That should get rid of the recursion mess, unless I am missing something.
cmfrydos
commented
1 month ago The recursion is fine now; I fixed a small flaw in #1649. However, I noticed another issue just before merging, but I didn’t have time to address it last month. I'll have some free time at the start of next week and can finish it then. Apologies for the delay.
stoltzld
commented
1 month ago I know nothing about POSIX compliance...but why not simply add the recursive alias info into the
helptext foraliasand then have something along the lines of:if (aliasValue.includes(` ${aliasKey} `)) showRecursiveAliasError()This would prevent an alias definition like buy="buy -l" entirely. I believe the alias system was never supposed to throw exceptions, but should just stop substituting when it runs into a recursion.
Is there a reason to allow recursion by default? Why not add a flag to alias to allow processing recursion and don't do recursion otherwise....I could see how aliasing a command to a command with a specific option would be useful.
version of the game: Bitburner v2.6.2 (633da3830)
I have aliased
buy="buy -l". Now, when I type "buy" into the terminal and press enter, the game hangs briefly, then gives the following error:bitburnerSave_1725813539_BN1x1.json.gz