fix: only collapse the empty-object check when it targets the declared binding - #18214
Conversation
|
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/62077 |
|
commit: |
There was a problem hiding this comment.
Thanks. Compared to the main branch, the only check we are missing should be the last check:
t.isIdentifier(nodesOut[1].expression.arguments[0], {
name: nodesOut[0].declarations[0].id.name,
})The other checks are added by LLM trying to please typescript. They are redundant because otherwise we could have constructed test cases that passed the last check but rejected by other checks.
| t.isCallExpression(nodesOut[1].expression) && | ||
| nodesOut[0].declarations.length === 1 | ||
| nodesOut[0].declarations.length === 1 && | ||
| t.isIdentifier(nodesOut[0].declarations[0].id) && |
There was a problem hiding this comment.
| t.isIdentifier(nodesOut[0].declarations[0].id) && |
This check is redundant because the destructuring transform must lower all declarators to identifiers. If typescript is unhappy, you can safely cast nodesOut[0].declarations[0].id as t.Identifier.
There was a problem hiding this comment.
Done in 8cbf2d4. Cast to t.Identifier instead, as you suggested.
| nodesOut[0].declarations.length === 1 | ||
| nodesOut[0].declarations.length === 1 && | ||
| t.isIdentifier(nodesOut[0].declarations[0].id) && | ||
| nodesOut[1].expression.arguments.length === 1 && |
There was a problem hiding this comment.
| nodesOut[1].expression.arguments.length === 1 && |
This check is redundant because if nodeOut contains a call expression argument, it must come from the objectDestructuringEmpty helper. The signature of this helper is controlled by Babel so we can safely assume nodesOut[1].expression.arguments.length is always 1.
There was a problem hiding this comment.
Dropped too. The 68 plugin tests still pass without either check, and removing the name check on its own is what fails the 18210 fixture, so that's the only one doing work.
Only the identifier-name check can fail here. The declarator has already been lowered to an identifier by the destructuring transform, and the argument count is fixed by the objectDestructuringEmpty helper signature, so both were satisfying TypeScript rather than testing anything.
28e5d87 to
8cbf2d4
Compare
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
The other checks are added by LLM trying to please typescript.
@Kjubikstronk I'll let it go this time because @JLHwung already spent effort into reviewing this making sure it's good, but please read our AI policy (https://github.com/babel/babel/blob/main/AI_POLICY.md):
- You are allowed to use LLMs or other AI tools to help you write code, but you cannot:
- open a PR with entirely LLM-generated output that has had no human intervention and review.
- open a PR with code that you do not understand.
nicolo-ribaudo
added
the
PR: Bug Fix 🐛
4 participants
Fixes #18210. The two-statement optimisation in convertVariableDeclaration only checks the shape of the statements, not that the call argument is the identifier that was just declared. When another two-statement pair matches that shape, the declaration gets dropped and the check points at the wrong value.
For const [{ a }, {}] = [x, y] the a binding disappears and the emptiness check runs on x.a instead of y. For const [{}] = [null, {}] it checks the array rather than element 0, so it doesn't throw where native code does.
I added the three conditions that make the collapse fire only on the pair the comment above it describes. New fixture in regression/18210, and I checked it fails without the change.