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.

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

Update scope info after destructuring transform #14494

Merged
merged 8 commits into from May 4, 2022
Merged

Conversation

peey
Copy link
Contributor

@peey peey commented Apr 26, 2022

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.

peey added 2 commits Apr 26, 2022
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
Copy link
Collaborator

@babel-bot babel-bot commented Apr 26, 2022

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

@peey
Copy link
Contributor Author

@peey peey commented Apr 26, 2022

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.

peey added 2 commits Apr 26, 2022
Also test assumptions / preconditions
1. Update tests: some were spurious failures
2. Address failing tests: introduce scope crawling in for statement
   cases
@peey peey changed the title [WIP] Attempt to Fix 14438 Fix 14438 Apr 26, 2022
@peey
Copy link
Contributor Author

@peey peey commented Apr 26, 2022

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!

Copy link
Contributor

@JLHwung JLHwung left a comment

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

packages/babel-plugin-transform-destructuring/src/index.ts Outdated Show resolved Hide resolved
@@ -637,6 +637,7 @@ export function convertVariableDeclaration(
} else {
path.replaceWithMultiple(nodesOut);
}
path.scope.crawl();
Copy link
Contributor

@JLHwung JLHwung Apr 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@peey peey Apr 26, 2022

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
packages/babel-plugin-transform-destructuring/test/api.js Outdated Show resolved Hide resolved
packages/babel-plugin-transform-destructuring/src/index.ts Outdated Show resolved Hide resolved
packages/babel-plugin-transform-destructuring/src/util.ts Outdated Show resolved Hide resolved
@JLHwung JLHwung changed the title Fix 14438 Update scope info after destructuring transform Apr 26, 2022
@JLHwung JLHwung added PR: Bug Fix 🐛 pkg: traverse (scope) labels Apr 26, 2022
- nit: improve variable naming style
- remove some manual scope updation code which isn't necessary now that
  we're crawling
@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 26, 2022

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

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

@JLHwung JLHwung Apr 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@peey peey Apr 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@peey peey Apr 26, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JLHwung JLHwung Apr 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@JLHwung JLHwung Apr 26, 2022

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@peey peey commented Apr 26, 2022

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
Copy link
Contributor

@JLHwung JLHwung commented Apr 26, 2022

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
Copy link
Contributor Author

@peey peey commented Apr 26, 2022

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.

@peey
Copy link
Contributor Author

@peey peey commented Apr 26, 2022

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

JLHwung
JLHwung approved these changes May 2, 2022
Copy link
Contributor

@JLHwung JLHwung left a comment

Thanks

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

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
Copy link
Contributor Author

@peey peey commented May 4, 2022

@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

  1. 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.
  2. 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
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 4, 2022

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.

@nicolo-ribaudo nicolo-ribaudo merged commit 97d1967 into babel:main May 4, 2022
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: traverse (scope) PR: Bug Fix 🐛
Projects
None yet
4 participants