Avoid free variable conflicts within descendant scopes instead of global #5398

edemaine · 2022-01-25T20:57:47Z

Instead of a global referencedVars option that is computed at the lexer level (via all IDENTIFIER tokens), recurse through the AST to mark each scope with all variables used within it and descendant scopes.

Likely also more efficient via Sets instead of Arrays.

Also fix some missing children in For nodes, which otherwise caused this rewrite to fail.

Fixes #4865 by causing parameter renaming in fewer (more predictable) scenarios.

I've separated into separate commits for:

Actual changes.
Changes to the compiler that result from these changes (running cake build a second time), so you can see the reduced renaming.
Rebuilding the browser version.

Instead of a global `referencedVars` option that is computed at the lexer level (via all IDENTIFIER tokens), recurse through the AST to mark each scope with all variables used within it and descendant scopes. Likely also more efficient via `Set`s instead of `Array`s. Also fix some missing `children` in `For` nodes, which otherwise caused this rewrite to fail. Fixes jashkenas#4865 by causing parameter renaming in fewer (more predictable) scenarios.

GeoffreyBooth · 2022-01-25T22:28:06Z

This seems fine. Any idea why the prior implementation was written the way that it was? Or put another way, are there any disadvantages of this refactor?

Also wouldn’t referencedVars tell us a key thing we would need to know to implement const, at least within a function scope?

edemaine · 2022-01-25T22:36:47Z

Any idea why the prior implementation was written the way that it was? Or put another way, are there any disadvantages of this refactor?

@lydell wrote the original, and in #4865 (comment) says:

Why? Because it’s simple. Tracking scope is more complicated for no gain. And even if we did that it wouldn’t help if the name is in the same scope.

So the main reason was simplicity. I imagine the original initialization is also somewhat faster (array scan vs tree scan), though lookups should be faster here; and the original approach was more robust to mistakes in children lists (but those would be bugs that should be fixed anyway).

I agree that the new approach doesn't help renaming if the name is in the same scope (or descendant scope), but in that scenario the renaming is not surprising, whereas the current nonlocal behavior is very surprising (and especially annoying with jsdoc).

@lydell, do you agree that this is a good approach?

Also wouldn’t referencedVars tell us a key thing we would need to know to implement const, at least within a function scope?

Yes, I had the same thought. I imagine findReferencedVars could be extended to do the static analysis first pass that I mentioned in #5377 (comment), if we also extend to block scopes.

GeoffreyBooth · 2022-01-26T04:25:42Z

I imagine the original initialization is also somewhat faster

I ran cake bench on this branch and on main and sometimes this branch was faster, sometimes main was. So the performance impact seems to be negligible.

I want to leave this open for a few days in case @lydell or anyone else wants to review or discuss it, but it looks good to me.

Yes, I had the same thought. I imagine findReferencedVars could be extended to do the static analysis first pass that I mentioned in #5377 (comment), if we also extend to block scopes.

I guess the issue with potentially outputting a declaration/first assignment “in place,” as opposed to moving it up to where the var line is now, is that the current code might be relying on the hoisting; like if something then a = 1, where a needs to be in the function scope and not in that if block scope. So we probably can’t avoid doing the work of tracking block scopes in addition to function scopes, if we want to ever output const. But I feel like doing the work to track block scopes will probably yield some benefits.

lydell · 2022-01-26T07:22:24Z

I won’t be reviewing, but thanks for asking!

STRd6 · 2022-04-17T23:49:56Z

I'm excited for this! Is there anything I can do to help?

GeoffreyBooth · 2022-04-19T03:10:03Z

I’m excited for this! Is there anything I can do to help?

This one was pretty much ready to merge, but I was hoping it could get another reviewer. If you don’t mind reviewing it, and updating it per latest main, I think that’s all it needs.

While you review, my main concern with this PR and with #5395, though more so with #5395, was that I found the code related to scope-tracking quite dense and hard to follow. It could benefit from a lot more commentary, from you or @edemaine if you have time to better explain what’s going on. See the review of #5395 to get a sense of the parts I found confusing.

edemaine · 2022-04-19T15:29:26Z

Sorry for the long hiatus. In some ways, coming back to this code after a long delay is good for seeing how confusing it is. 🙂

It could benefit from a lot more commentary, from you or @edemaine if you have time to better explain what’s going on.

I added a bunch of comments to document how this specific PR works. There's still a lot to explain how the Scope class works, but that's really a task for #5395, which will be my next task (not sure when exactly).

I also merged with main, so this should be ready to review/merge.

GeoffreyBooth · 2022-04-28T03:11:27Z

src/coffeescript.coffee

@@ -106,6 +100,7 @@ exports.compile = compile = withPrettyErrors (code, options = {}) ->
    ast.tokens = tokens
    return ast

+  nodes.findReferencedVars()


What is this doing? Populating the referencedVars property of the root and all children? Can we add a comment explaining this?

GeoffreyBooth · 2022-04-28T03:12:23Z

src/nodes.coffee

+  # `findReferencedVars` recurses through the source parse tree to mark every
+  # "scope" parse node (`Root` and `Code`s) with all referenced
+  # (accessed or assigned) variables in that source scope, so that every scope
+  # knows what to avoid when generating new variable names.
+  # Specifically, it takes in an array of `scopeNodes`,
+  # each of which already has a `@referencedVars` attribute that is a `Set`.
+  # For each found reference to an identifier (see `IdentifierLiteral`'s
+  # override for `findReferencedVars`), it adds the name (as a `String`)
+  # to each scope node's `referencedVars` Set.


Great comment, just updating typography to match (most, if not all) other comments.

Suggested change

# `findReferencedVars` recurses through the source parse tree to mark every

# "scope" parse node (`Root` and `Code`s) with all referenced

# (accessed or assigned) variables in that source scope, so that every scope

# knows what to avoid when generating new variable names.

# Specifically, it takes in an array of `scopeNodes`,

# each of which already has a `@referencedVars` attribute that is a `Set`.

# For each found reference to an identifier (see `IdentifierLiteral`'s

# override for `findReferencedVars`), it adds the name (as a `String`)

# to each scope node's `referencedVars` Set.

# `findReferencedVars` recurses through the source parse tree to mark every

# “scope” parse node (`Root` and `Code`s) with all referenced

# (accessed or assigned) variables in that source scope, so that every scope

# knows what to avoid when generating new variable names.

# Specifically, it takes in an array of `scopeNodes`,

# each of which already has a `@referencedVars` attribute that is a `Set`.

# For each found reference to an identifier (see `IdentifierLiteral`’s

# override for `findReferencedVars`), it adds the name (as a `String`)

# to each scope node’s `referencedVars` Set.

GeoffreyBooth · 2022-04-28T03:13:59Z

src/nodes.coffee

@@ -515,9 +528,16 @@ exports.Root = class Root extends Base
    super()

    @isAsync = (new Code [], @body).isAsync
+    @referencedVars = new Set  # all referenced variable names in this scope


Suggested change

@referencedVars = new Set # all referenced variable names in this scope

# This set contains all the referenced variable names in this scope.

@referencedVars = new Set

GeoffreyBooth · 2022-04-28T03:17:07Z

src/nodes.coffee


  children: ['body']

+  findReferencedVars: (scopeNodes = []) ->
+    # This is called at the top level to start the recursion,


Is there any way to make this “automatic,” in the sense that we don’t need to remember to call findReferencedVars on the root, outside of nodes.coffee, to trigger this recursion? I assume we probably can’t call it in Root‘s constructor, as that would be too early? Could it happen within compile or compileNode, as maybe those get called after all the child nodes are added?

GeoffreyBooth · 2022-04-28T03:17:42Z

src/nodes.coffee

@@ -3904,6 +3929,7 @@ exports.Code = class Code extends Base
    @isGenerator = no
    @isAsync     = no
    @isMethod    = no
+    @referencedVars = new Set  # all referenced variable names in this scope


Suggested change

@referencedVars = new Set # all referenced variable names in this scope

# This set contains all the referenced variable names in this scope.

@referencedVars = new Set

GeoffreyBooth · 2022-04-28T03:18:59Z

src/nodes.coffee

@@ -5342,7 +5373,7 @@ exports.For = class For extends While
    @addBody body
    @addSource source

-  children: ['body', 'source', 'guard', 'step']
+  children: ['body', 'name', 'index', 'source', 'guard', 'step']


Why are these added? I don’t see the connection to the “referenced vars” change. Not opposed, but I’m wondering if this should be here (or is part of a separate goal).

GeoffreyBooth · 2022-04-28T03:19:28Z

src/repl.coffee

@@ -49,7 +47,8 @@ replDefaults =
      isAsync = ast.isAsync
      # Invoke the wrapping closure.
      ast    = new Root new Block [new Call ast]
-      js     = ast.compile {bare: yes, locals: Object.keys(context), referencedVars, sharedScope: yes}
+      ast.findReferencedVars()


Per my comment above, I’m wondering if we could do without this call, that it could happen automatically as part of assembling the nodes tree.

edemaine added 3 commits January 25, 2022 15:54

Changes to compiler caused by this PR

1ee19f9

Browser build

7b2bdf4

edemaine mentioned this pull request Jan 25, 2022

constructor renaming parameters #4865

Open

GeoffreyBooth approved these changes Jan 26, 2022

View reviewed changes

edemaine mentioned this pull request Apr 12, 2022

Support export destructured assignment #5100

Open

edemaine added 2 commits April 19, 2022 11:15

Merge remote-tracking branch 'upstream/main' into referenced-vars

6456b4f

Add comments

fed48fe

Rebuild

2ab383d

edemaine force-pushed the referenced-vars branch from d7e6609 to 2ab383d Compare April 19, 2022 15:33

GeoffreyBooth reviewed Apr 28, 2022

View reviewed changes

edemaine mentioned this pull request Feb 11, 2023

break used as value for detecting broken loops, simpler global ref resolution DanielXMoore/Civet#359

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid free variable conflicts within descendant scopes instead of global #5398

Avoid free variable conflicts within descendant scopes instead of global #5398

edemaine commented Jan 25, 2022 •

edited

Loading

GeoffreyBooth commented Jan 25, 2022

edemaine commented Jan 25, 2022 •

edited

Loading

GeoffreyBooth commented Jan 26, 2022

lydell commented Jan 26, 2022

STRd6 commented Apr 17, 2022

GeoffreyBooth commented Apr 19, 2022

edemaine commented Apr 19, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

GeoffreyBooth Apr 28, 2022

	@referencedVars = new Set # all referenced variable names in this scope
	# This set contains all the referenced variable names in this scope.
	@referencedVars = new Set

Avoid free variable conflicts within descendant scopes instead of global #5398

Are you sure you want to change the base?

Avoid free variable conflicts within descendant scopes instead of global #5398

Conversation

edemaine commented Jan 25, 2022 • edited Loading

GeoffreyBooth commented Jan 25, 2022

edemaine commented Jan 25, 2022 • edited Loading

GeoffreyBooth commented Jan 26, 2022

lydell commented Jan 26, 2022

STRd6 commented Apr 17, 2022

GeoffreyBooth commented Apr 19, 2022

edemaine commented Apr 19, 2022

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

GeoffreyBooth Apr 28, 2022

Choose a reason for hiding this comment

edemaine commented Jan 25, 2022 •

edited

Loading

edemaine commented Jan 25, 2022 •

edited

Loading