FIX: Resolve Alias Recursion Issue by Preventing Duplicate Rule Application #1649
Conversation
Actually, it was more flawed than that, but it's hard to put into words. Whenever it processed a rule like |
Basically for rules like ( Previously it did following shenanigans (see post above): |
|
The new expected values also correspond to the behavior observed with aliases under Ubuntu: |
|
And here a screenshot that the alias mentioned in #1647 now works as expected, regardless of whether its useful: |
|
Good catch on this. Nice test coverage, too. |
|
I've incorporated the feedback from d0sboots. Please let me know if there’s anything else to adjust! |
| if (localAlias && !currentlyProcessingAliases.includes(localrule)) { | ||
| const appliedAlias = applyAliases(localAlias, depth + 1, [localrule, ...currentlyProcessingAliases]); | ||
| if (localAlias && !currentlyProcessingAliases.has(localrule)) { | ||
| const appliedAlias = applyAliases(localAlias, depth + 1, new Set([localrule, ...currentlyProcessingAliases])); |
There was a problem hiding this comment.
I really wanted to just merge, but I can't let this go... XD
Don't create a brand-new Set, that will be the same O(N^2) behavior as the array you just replaced. Add the item to the set you have.
Edit: For this to work properly (because there are other checks that happen after), you also have to remove it after you recurse.
There was a problem hiding this comment.
I need to think this through, I'm just in love with immutables. But you're right ofc that this is a little expensive.
For the global rules you have to copy, since a global should be able to be applied multiple times. For the local one it might suffice to just add it.
There was a problem hiding this comment.
If you really can't stand to do mutable stuff, I'm fine with you changing it back to an array. There's no point in doing something more verbose if you're going to be N^2 anyway, and most people won't have lots of deep aliases.
There was a problem hiding this comment.
Oh don't merge, there is another bug that will make local rules be applied globally I believe. Sorry that I haven't thought this through carefully before. I was just focused on this single bug.
There was a problem hiding this comment.
It shouldn't be hard to fix though, but I'd like to write some more tests to catch it.
There was a problem hiding this comment.
There is a point in doing it more verbose: readability. Sets, in contrasts to Arrays, are unique and often used for lookup. So it helps with getting the codes intention.
I think mutating the set for the local aliases is totally fine, but copying is needed for the global ones. I don't believe you can circumvent that.
I will probably finish this tomorrow. It is easy to mess up.
After global rules are applied, subsequent calls to applyAlias should just consider global rules with the exception of a global rule modifying the first token/command. I believe the function might need some restructuring of the execution flow to handle all edge cases.
There was a problem hiding this comment.
Regardless of how you end up restructuring, the following should work for mutation without copying: Add the key to the set immediately before recursion, and remove it immediately after.
A quick sketch of why this works: You abort if the key was already present, so there's never an issue of double-removing a key. Thus, the removal always properly reverts to the prior state.
There was a problem hiding this comment.
Ah that's a great approach. I'll probably use that, but then also "unify" the application of local and global rules inside one loop / reduce, to avoid certain edge cases.
Closes #1647
Previously, the check was only to ensure that the right side of an alias rule does not repeat, which is not effective for endlessly expanding aliases such as buy="buy -l". Now, the implementation ensures a single alias rule is not recursively applied multiple times by remembering and checking the entire rule.
Updates expected values in tests; this change may disrupt existing setups with endless recursive aliases. Please verify if the new expected values align with your expectations. It would be possible to avoid applying the last substitution if it would result in infinite recursion, but this approach would not comply with the POSIX standard.