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

fix: invalid identifiers #133

Merged
merged 2 commits into from
Oct 6, 2024
Merged

fix: invalid identifiers #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7e5d63
fix: invalid identifiers
j4k0xb Sep 28, 2024
a317fd3
Add more invalid identifier tests
jehna Oct 6, 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
33 changes: 33 additions & 0 deletions src/plugins/local-llm-rename/visit-all-identifiers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,36 @@ e.b;
})
);
});

test("should handle invalid identifiers", async () => {
const code = `const a = 1`;
const result = await visitAllIdentifiers(code, async () => "this.kLength");
assert.equal(result, "const thisKLength = 1;");
});

test("should handle space in identifier name (happens for some reason though it shouldn't)", async () => {
Copy link
Contributor Author

@j4k0xb j4k0xb Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

humanify/src/plugins/local-llm-rename/unminify-variable-name.ts

Line 25 in 73f0424

gbnf`A good name would be '${/[a-zA-Z] [a-zA-Z0-9]{2,12}/}'`

maybe because the regex has a space
and would ^ $ make a difference?

Copy link
Owner

@jehna jehna Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This regex is actually a representation of gbnf, which is a superset of regex that allows strings edit: spaces in between. If it were a normal regex, all variable names should have a space in them, right?

To me this seems like an issue with the llama.cpp gbnf handling

Copy link
Owner

Choose a reason for hiding this comment

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

Another thing I'll need to check from the previous issues is that whether the users were using local or openai mode. Local mode is much more robust, openai does not have any actual guarantees about returning a valid identifier name.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jehna From memory, for at least some of the ones I was looking at/debugging, I believe they were using openai, not local.

const code = `const a = 1`;
const result = await visitAllIdentifiers(code, async () => "foo bar");
assert.equal(result, "const fooBar = 1;");
});

test("should handle reserved identifiers", async () => {
const code = `const a = 1`;
const result = await visitAllIdentifiers(code, async () => "static");
assert.equal(result, "const _static = 1;");
});

test("should handle multiple identifiers named the same", async () => {
const code = `
const a = 1;
const b = 1;
`.trim();
const result = await visitAllIdentifiers(code, async () => "foo");
assert.equal(
result,
`
const foo = 1;
const _foo = 1;
`.trim()
);
});
4 changes: 2 additions & 2 deletions src/plugins/local-llm-rename/visit-all-identifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { parseAsync, transformFromAstAsync, NodePath } from "@babel/core";
import * as babelTraverse from "@babel/traverse";
import { Identifier, isValidIdentifier, Node } from "@babel/types";
import { Identifier, toIdentifier, Node } from "@babel/types";

const traverse: typeof babelTraverse.default.default = (
typeof babelTraverse.default === "function"
Expand Down Expand Up @@ -39,7 +39,7 @@ export async function visitAllIdentifiers(
const surroundingCode = await scopeToString(smallestScope);
const renamed = await visitor(smallestScopeNode.name, surroundingCode);

let safeRenamed = isValidIdentifier(renamed) ? renamed : `_${renamed}`;
let safeRenamed = toIdentifier(renamed);
while (renames.has(safeRenamed)) {
safeRenamed = `_${safeRenamed}`;
}
Expand Down
Loading