Update scope info after destructuring transform #14494

peey · 2022-04-26T10:59:25Z

Q	A
Fixed Issues?	Fixes #14438
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes + Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

Introduces a test suite to demonstrate #14438 and provides a fix that makes all tests pass.

59/66 introduced tests failing

Via crawling the scope after the transfomrations are made. 10/66 tests passing instead of earlier 7/66.

babel-bot · 2022-04-26T11:03:01Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51754/

peey · 2022-04-26T11:03:36Z

Would appreciate any comments which could help bring up the number of passing tests.

This d40dce6 already makes 3 more tests pass which weren't before.

E.g. one of the failing tests is https://github.com/babel/babel/blob/main/packages/babel-plugin-transform-destructuring/test/fixtures/destructuring/spread/input.js . I'm not sure where that is handled in this transform plugin so that its scope bindings may be taken care of.

Also test assumptions / preconditions

1. Update tests: some were spurious failures 2. Address failing tests: introduce scope crawling in for statement cases

peey · 2022-04-26T12:27:11Z

This is now fixed. If someone can help me with the failing CI test (I don't understand the failure) then that'd be great!

JLHwung

Nice work. Left some nit comments on code and testing approach.

packages/babel-plugin-transform-destructuring/src/index.ts

JLHwung · 2022-04-26T12:34:11Z

packages/babel-plugin-transform-destructuring/src/util.ts

@@ -637,6 +637,7 @@ export function convertVariableDeclaration(
  } else {
    path.replaceWithMultiple(nodesOut);
  }
+  path.scope.crawl();


Q: Is Line 625 - 633 still required now that we crawl the scope?

I don't understand the plugin source very well but removing them doesn't cause any tests to fail. I'll remove them, let me know if you think we should keep them.

packages/babel-plugin-transform-destructuring/test/api.js

packages/babel-plugin-transform-destructuring/src/index.ts

packages/babel-plugin-transform-destructuring/src/util.ts

- nit: improve variable naming style - remove some manual scope updation code which isn't necessary now that we're crawling

JLHwung · 2022-04-26T13:20:56Z

I'm unsure about this, git checkout master -- src to undo all additions to src and TEST_GREP="regression-14438" make test-only leaves 27 tests failing

The test is passing because in the second round traverse, V8 has not GC'ed the cached NodePath in traverse.cache so the cached Scope is used. But generally we should not depend on the GC behaviour. To verify that you can add traverse.cache.clear() between those calls and see if the failing test is passing (incorrectly).

One concern I have is that this IMO is a universal-ish check so I wouldn't want to go and put it in all options.json files.

You don't have to put in in all options.json. Options.json in these folders will be applied to their sub tests:

allowArrayLike                 assumption-iterableIsArray     destructuring                  sourcemap
assumption-arrayLikeIsIterable assumption-objectRestNoSymbols regression

Ref: babel#14494 (comment)

JLHwung · 2022-04-26T13:35:10Z

packages/babel-plugin-transform-destructuring/test/api.js

+        traverse.cache.clear();
+        traverse(transformed.ast, {
+          VariableDeclarator(path) {
+            const b = path.scope.getBinding(path.get("id").node.name);


Further improvement idea: To make it a universal check, we should loop through Object.keys(path.getBindingIdentifiers()). The current check is ok for transform-destructuring since path.get("id") is always an identifier after transformed.

Sure, but if the bindings don't exist, would that be returned in path.getBindingIdentifiers()?

At the AST level, instead of LHS of the variable declarator we can also go for all identifiers that are present. But I couldn't think of a way to exclude globals or member expressions (e.g. slice in arr.slice or Array global) which definitely won't have scope bindings.

path.getBindingIdentifiers() extracts ids from the AST, it does not access the scope info.

At the AST level, instead of LHS of the variable declarator we can also go for all identifiers that are present.

hasBinding will respect globals. path.isReferencedIdentifier() can exclude referenced identifiers. I think we can start from VariableDeclarator / Function params only.

The most strict check will be to compare the scope info after transform against a new scope created from scratch (without traverse cache), but I guess it will fail many, if not most current tests.

peey · 2022-04-26T13:38:21Z

I'm trying what you suggested, but I get ": .Program is not a valid Plugin property" (BABEL_UNKNOWN_PLUGIN_PROPERTY) as an error. Is there anywhere else where babel uses the babel-helper-fixture as you've suggested?

JLHwung · 2022-04-26T13:40:02Z

I'm trying what you suggested, but I get ": .Program is not a valid Plugin property" (BABEL_UNKNOWN_PLUGIN_PROPERTY) as an error. Is there anywhere else where babel uses the babel-helper-fixture as you've suggested?

My bad, it should be:

module.exports = (babel) => {
  return {
    visitor: {
    Program: {
      exit(programPath) {
        // so that this plugin is always called after the transform-destructuring plugin
        programPath.traverse(checkScopeVisitor)
      }
    }
    }
  }
}

See suggestions in JLHwung's review

peey · 2022-04-26T13:49:10Z

You're right. It works (tested that they're failing without the fix), and it works better!

Any thoughts on where the checkScopeInfo.js file should be placed? It's not src, we don't want to distribute it after building, it doesn't contain a test suite so I don't know if it can be placed under test. I've currently created a res and placed it under that but I can move to a more appropriate place.

packages/babel-plugin-transform-destructuring/test/fixtures/allowArrayLike/options.json

To generalize the check

peey · 2022-04-26T14:42:23Z

All checks pass. Thanks for all the comments. Should we merge?

JLHwung

Thanks

nicolo-ribaudo

I dislike path.scope.crawl() because it's a very expensive operation. Would just using path.scope.registerDeclaration on the new nodes work to fix this bug?

peey · 2022-05-04T17:50:30Z

@nicolo-ribaudo Thanks. I had tried your suggestion, but this transform in particular is written in a way that all the nodes are prepared at once and then attached to AST at once

Based on my try, I think

There's a tradeoff between the code simplicity and performance as far as this transform is concerned. Currently the code is easy to maintain but makes a few operations (like scope registration) awkward.
path.scope.crawl shouldn't be needed in any other transform

Do we have any perf tests so we can measure the exact impact on performance?

nicolo-ribaudo · 2022-05-04T18:04:02Z

Do we have any perf tests so we can measure the exact impact on performance?

We have some, but @JLHwung is probably the best person to tell you about them.

Let's merge this PR now since it improves correctness, we can iterate on performance later.

peey added 2 commits April 26, 2022 16:24

Introduce test suite for babel#14438

e53065d

59/66 introduced tests failing

Attempt to fix babel#14438

d40dce6

Via crawling the scope after the transfomrations are made. 10/66 tests passing instead of earlier 7/66.

peey added 2 commits April 26, 2022 17:11

Improve tests

d84141c

Also test assumptions / preconditions

Fix babel#14438

84d6c4a

1. Update tests: some were spurious failures 2. Address failing tests: introduce scope crawling in for statement cases

peey changed the title ~~[WIP] Attempt to Fix 14438~~ Fix 14438 Apr 26, 2022

JLHwung reviewed Apr 26, 2022

View reviewed changes

JLHwung changed the title ~~Fix 14438~~ Update scope info after destructuring transform Apr 26, 2022

JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: traverse (scope) labels Apr 26, 2022

Take care of comments in JLHwung's review

d503eb7

- nit: improve variable naming style - remove some manual scope updation code which isn't necessary now that we're crawling

Clear traverse scope cache for through testing

ce4a6bf

Ref: babel#14494 (comment)

JLHwung reviewed Apr 26, 2022

View reviewed changes

Move test to runnable plugin

8d1bb65

See suggestions in JLHwung's review

JLHwung reviewed Apr 26, 2022

View reviewed changes

packages/babel-plugin-transform-destructuring/test/fixtures/allowArrayLike/options.json Outdated Show resolved Hide resolved

LVal.get('id') -> LVal.getBindingIdentifiers()

999fd51

To generalize the check

JLHwung approved these changes May 2, 2022

View reviewed changes

nicolo-ribaudo reviewed May 4, 2022

View reviewed changes

nicolo-ribaudo approved these changes May 4, 2022

View reviewed changes

nicolo-ribaudo merged commit 97d1967 into babel:main May 4, 2022

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 4, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update scope info after destructuring transform #14494

Update scope info after destructuring transform #14494

peey commented Apr 26, 2022 •

edited by nicolo-ribaudo

babel-bot commented Apr 26, 2022 •

edited

peey commented Apr 26, 2022

peey commented Apr 26, 2022

JLHwung left a comment

JLHwung Apr 26, 2022

peey Apr 26, 2022 •

edited

JLHwung commented Apr 26, 2022

JLHwung Apr 26, 2022

peey Apr 26, 2022

peey Apr 26, 2022

JLHwung Apr 26, 2022

JLHwung Apr 26, 2022

peey commented Apr 26, 2022 •

edited

JLHwung commented Apr 26, 2022

peey commented Apr 26, 2022

peey commented Apr 26, 2022

JLHwung left a comment

nicolo-ribaudo left a comment

peey commented May 4, 2022

nicolo-ribaudo commented May 4, 2022

Update scope info after destructuring transform #14494

Update scope info after destructuring transform #14494

Conversation

peey commented Apr 26, 2022 • edited by nicolo-ribaudo

babel-bot commented Apr 26, 2022 • edited

peey commented Apr 26, 2022

peey commented Apr 26, 2022

JLHwung left a comment

Choose a reason for hiding this comment

JLHwung Apr 26, 2022

Choose a reason for hiding this comment

peey Apr 26, 2022 • edited

Choose a reason for hiding this comment

JLHwung commented Apr 26, 2022

JLHwung Apr 26, 2022

Choose a reason for hiding this comment

peey Apr 26, 2022

Choose a reason for hiding this comment

peey Apr 26, 2022

Choose a reason for hiding this comment

JLHwung Apr 26, 2022

Choose a reason for hiding this comment

JLHwung Apr 26, 2022

Choose a reason for hiding this comment

peey commented Apr 26, 2022 • edited

JLHwung commented Apr 26, 2022

peey commented Apr 26, 2022

peey commented Apr 26, 2022

JLHwung left a comment

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

peey commented May 4, 2022

nicolo-ribaudo commented May 4, 2022

peey commented Apr 26, 2022 •

edited by nicolo-ribaudo

babel-bot commented Apr 26, 2022 •

edited

peey Apr 26, 2022 •

edited

peey commented Apr 26, 2022 •

edited