Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Resolve Alias Recursion Issue by Preventing Duplicate Rule Application #1649

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

FIX: Resolve Alias Recursion Issue by Preventing Duplicate Rule Application #1649

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa20c25
fix alias recursion problem by not applying the same rule twice
cmfrydos Sep 12, 2024
68d86fa
fix small typo
cmfrydos Sep 12, 2024
c17f3d8
Improved tests
cmfrydos Sep 12, 2024
b469baf
Use Set for currently processing aliases and skip failing tests
cmfrydos Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Alias.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export function substituteAliases(origCommand: string): string {
* @param currentlyProcessingAliases any aliases that have been applied in the recursive evaluation leading to this point
* @return { string } the provided command with all of its referenced aliases evaluated
*/
function applyAliases(origCommand: string, depth = 0, currentlyProcessingAliases: string[] = []) {
function applyAliases(origCommand: string, depth = 0, currentlyProcessingAliases: Set<string> = new Set()) {
if (!origCommand) {
return origCommand;
}
Expand All @@ -98,17 +98,17 @@ function applyAliases(origCommand: string, depth = 0, currentlyProcessingAliases
// (unless there are any reference loops or the reference chain is too deep)
const localAlias = Aliases.get(commandArray[0]);
const localrule = commandArray[0] + "->" + localAlias + "(l)";
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]));
Copy link
Collaborator

@d0sboots d0sboots Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@d0sboots d0sboots Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be hard to fix though, but I'd like to write some more tests to catch it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based attitude

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

commandArray.splice(0, 1, ...appliedAlias.split(" "));
}

// Once local aliasing is complete (or if none are present) handle any global aliases
const processedCommands = commandArray.reduce((resolvedCommandArray: string[], command) => {
const globalAlias = GlobalAliases.get(command);
const rule = command + "->" + globalAlias + "(g)";
if (globalAlias && !currentlyProcessingAliases.includes(rule)) {
const appliedAlias = applyAliases(globalAlias, depth + 1, [rule, ...currentlyProcessingAliases]);
const globalrule = command + "->" + globalAlias + "(g)";
if (globalAlias && !currentlyProcessingAliases.has(globalrule)) {
const appliedAlias = applyAliases(globalAlias, depth + 1, new Set([globalrule, ...currentlyProcessingAliases]));
resolvedCommandArray.push(appliedAlias);
} else {
// If there is no alias, or if the alias has a circular reference, leave the command as-is
Expand Down
12 changes: 10 additions & 2 deletions test/jest/Alias/Alias.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,20 @@ describe("substituteAliases Tests", () => {
expect(result).toEqual("'a'");
});

it("Should not substitute quoted commands III", () => {
it.skip("Should not substitute quoted commands III", () => {
parseAliasDeclaration("a=b");
parseAliasDeclaration("b='c'");
parseAliasDeclaration("c=d");
const result = substituteAliases("a");
//expect(result).toEqual("'c'"); // Currently FAILS
expect(result).toEqual("'c'");
});

it.skip("Should not substitute quoted commands IV", () => {
parseAliasDeclaration("a=b");
parseAliasDeclaration('b="c"');
parseAliasDeclaration("c=d");
const result = substituteAliases("a");
expect(result).toEqual('"c"');
});

it("Should only change local aliases if they are the start of the command", () => {
Expand Down