Skip to content

Conversation

@bakkot
Copy link

@bakkot bakkot commented Dec 19, 2025

Fixes #117.

I ended up just writing this myself because existing parsers are concerned with location information and etc, which is not relevant here.

It's technically very slightly too permissive in that it always allows \8 and \9 in string literals, which are legal only in non-strict code, but I don't think this should matter in this application. I didn't bother with useful error messages because it's just eating the errors anyway.

@guybedford
Copy link
Collaborator

Thanks for posting this, I will review this in the next couple of weeks. Note that the JS implementation here runs in addition to the Wasm implementation, where the JS implementation is only used when Wasm is not otherwise supported on the system.

So - the implementation needs to be copied verbatim to the lexer.c code as well. Shouldn't be too hard to plug into AI to do that work given it's the same shape of code running.

@guybedford
Copy link
Collaborator

Oh, I see this is in the JS wrapper so it applies to the Wasm path - we just need to be careful to ensure both the JS entry point and the Wasm entry point support the feature I think?

One other thought I had here - Wasm might itself be disabled by turning off eval, I'm not sure though, in which case perhaps it is the JS path that matters for the feature.

@bakkot
Copy link
Author

bakkot commented Dec 20, 2025

I guess I need to copy this into lexer.js as well? I hadn't originally noticed that the code is duplicated. I can port this to lexer.c if you want but I strongly suspect it's faster to do it in the wrapper, since almost all of the time the wrapper is going to be able to use the JSON.parse fast path.

@guybedford
Copy link
Collaborator

That makes sense. Yeah so it just needs to be in both wrappers then most likely.

It would be nice to see some tests for the coverage cases though.

@bakkot
Copy link
Author

bakkot commented Dec 20, 2025

Happy to add tests. In test/_unit.js I guess?

@bakkot
Copy link
Author

bakkot commented Dec 23, 2025

Updated the pure-js impl and added a couple tests for other kinds of escape sequences. The existing tests already covered a few of them. And I added --disallow-code-generation-from-strings to the tests to confirm that it actually does fix the original issue.

Also, npm run test was previously using ; instead of &&, and therefore not actually failing unless test-js specifically failed. I changed it to && while I was here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoiding use of eval

2 participants