coffeescript

coffeescript copied to clipboard

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:

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

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?

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 https://github.com/jashkenas/coffeescript/issues/4865#issuecomment-360115017 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 https://github.com/jashkenas/coffeescript/issues/5377#issuecomment-1020511526, if we also extend to block scopes.

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.

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

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

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 https://github.com/jashkenas/coffeescript/pull/5395, though more so with https://github.com/jashkenas/coffeescript/pull/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 https://github.com/jashkenas/coffeescript/pull/5395 to get a sense of the parts I found confusing.

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.