fix(babel-traverse): deopt path.evaluate() on native call errors#18003
fix(babel-traverse): deopt path.evaluate() on native call errors#18003raghavwahi wants to merge 4 commits into
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/61623 |
|
commit: |
|
I would prefer if we deopted every time we pass an object to |
|
I have a deopt fix on my local machine, and I will commit it to this branch. |
|
We have to ban |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac7d21b8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| typeof right === "object" && | ||
| Object.hasOwn(right, "toString") | ||
| ) { |
There was a problem hiding this comment.
Guard null before
Object.hasOwn in + deopt check
This branch treats typeof right === "object" as safe, but null also has type "object"; calling Object.hasOwn(null, "toString") throws TypeError and crashes path.evaluate() for inputs like "" + null instead of returning a confident value. This is a regression introduced by the new deopt guard and should explicitly exclude null before calling Object.hasOwn.
Useful? React with 👍 / 👎.
| (typeof left === "object" && | ||
| typeof right === "string" && | ||
| Object.hasOwn(left, "toString")) || |
There was a problem hiding this comment.
Guard null before
Object.hasOwn in == deopt check
Both sides of this == guard rely on typeof x === "object", which includes null; expressions such as "" == null or null == "" now throw TypeError from Object.hasOwn instead of evaluating normally. Since this path runs inside evaluate(), the new check introduces a crash for previously supported inputs.
Useful? React with 👍 / 👎.
| func === String && | ||
| args.length > 0 && | ||
| Object.hasOwn(args[0], "toString") | ||
| ) { |
There was a problem hiding this comment.
Handle
null/undefined in String(...) deopt guard
The new String special-case calls Object.hasOwn(args[0], "toString") without checking for null/undefined, so common calls like String(undefined) and String(null) now throw TypeError during evaluation. That changes behavior from returning a confident constant to crashing, which is an urgent regression in constant folding.
Useful? React with 👍 / 👎.
Summary
path.evaluate()crashes withRangeError: Maximum call stack size exceededon input like:The
CallExpressionbranch of the evaluator invokes the real native function (here,String(obj)) viafunc.apply(context, args). If the evaluator has previously produced an object whose own coercion methods recurse infinitely (theObjectExpressionbranch allows function-valued properties, soobj.toString === String.prototype.toUpperCaseis reachable), that native call blows the stack instead of being caught.This wraps the native call in
try/catchand deopts on any thrown error, which matches howpath.evaluate()already gives up in other uncertain cases (e.g., inherited methods — see existingshould not evaluate inherited methodstest).Change
After the fix,
path.evaluate()on the issue's input returns{ confident: false, value: undefined, deopt: <path> }— no crash.Tests
Added a regression test in
packages/babel-traverse/test/evaluation.jsusing the issue's exact input. Fullyarn jest packages/babel-traversesuite is green (652 passed, 1 skipped),make lintclean.Note for reviewers - related, out-of-scope crash
If a caller stores the result of
path.evaluate().valuefor anObjectExpressionlike{ toString: "".toUpperCase }and later coerces that returned object to a string in their own code, it will still crash with the sameRangeError— because the returned value is a real JS object with a malicioustoString. This PR intentionally does not address that path, because:path.evaluate()itself, which is now fixed.Happy to follow up in a separate PR / issue if maintainers want broader hardening of the
ObjectExpressionbranch.