Fix js2-node-get-enclosing-scope on a statement function's name

[mishoo]

When called with a statement function's name node,
js2-node-get-enclosing-scope would errorneously return the function itself.
That's not really the scope where this name is defined (it should be the
function's parent scope).

This bug affects js2-refactor's rename variable functionality. Example:

(function(){
    function |foo() {
        var |foo = 5;
        console.log(|foo);
    }
    foo();
})();

(where '|' marks possible cursor position). Type M-x js2r-rename-var with
the cursor on one of the marked positions, and it'll offer to rename the
following occurrences (marked with underscores):

(function(){
    function _foo_() {
        var _foo_ = 5;
        console.log(_foo_);
    }
    foo();
})();

This is incorrect, as the name is shadowed inside the function. It should
instead rename these two:

(function(){
    function _foo_() {
        var foo = 5;
        console.log(foo);
    }
    _foo_();
})();

This patch fixes this.

[dgutov]

Any concerns about performance?

js2-node-get-enclosing-scope seems to be called a lot, through js2-node-top-level-decl-p, especially with js2-highlight-unused-variables-mode on.

[mishoo]

@dgutov it's just an initial extra-check, it's outside the loop, so I wouldn't worry about it. Besides, correctness is more important. ;-)

[dgutov]

It's outside the loop, but the function itself is called a lot.

[dgutov]

I was hoping you could so some benchmarking.

[mishoo]

Sorry, I won't bother. I seriously doubt it can add any measurable impact.

But, I now realize the description from my commit message could make one think that this is a hack needed for js2-refactor. That's not the case. Just to make it more clear that it's a bug, here's the example I used above, before my modification, with js2-highlight-unused-variables-mode on:

Because of this bug in get-enclosing-scope, it produces that incorrect warning; foo is obviously very initialized. ☺

[mishoo]

I added a couple of commits to this branch (enable lexical scope, and some cleanups). Here's some benchmarking before and after these commits (I still didn't benchmark js2-node-get-enclosing-scope, because I'm not sure how; besides, that is a bug, performance should simply not matter more than correctness).

These timings are for parsing a buffer of about 1700 lines (this file).

-------- dynamic scope (initial)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.793484382 2 0.1606025419999999)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.789422719 2 0.164317351)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.828467357 2 0.17550351799999997)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.81382005 2 0.18693292099999992)

-------- lexical scope

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.576688484 2 0.22133044000000046)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.562694449 2 0.22349454399999935)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.589671439 2 0.23338895199999943)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.5654962270000001 2 0.2413074169999998)

-------- after cleanup

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.2848497740000002 1 0.080605021)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.3688194150000002 2 0.184479457)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.38694573 2 0.17873525600000006)

ELISP> (benchmark-run 10 (js2-parse) nil)
(1.392318376 2 0.18782476300000006)

Feel free to take whatever you want from this. I live in js2-mode many hours a day, and when I fix or improve something, I do it for purley selfish reasons. :-) I'm happy to live on my fork if you don't want to include these in the official repo.

OTOH, if you feel you can trust me (and you should) then you can give me push rights to this repository. I'm using Emacs since '98, I'm a fairly competent Lisp programmer and somewhat of an authority on parsing JavaScript. I won't be able to put much time into it, but I need to fix bugs as I find them, because I need to trust my tools. ;-)

Have a great weekend!

[dgutov]

"remove unused properties from js2-script-node" looks definitely valuable (I mean to write it even before looking at the benchmarks), but orthogonal to this fix, isn't it?

The switch to lexical binding is discussed at #426.

[dgutov]

I'm not saying this is not a bug, but what about this example:

var f = function foo() {
  var foo = 4;
  return foo;
}

How does js2-refactor behave in this case?

[dgutov]

This is the example that might demonstrate the problem better instead, I think:

function foo() {
}

foo(); // <-- try to rename foo

[mishoo]

@dgutov your last example works perfectly here, what's the issue?

About the one in this comment, there's a bug there — js2-refactor should not rename all 3 occurrences, because foo is shadowed. However, given that the function name is not defined outside, I find the existing behavior actually more convenient; I think I would normally want all three of them to be changed (it's not incorrect, in any case: the logic of the code would remain the same).

[mishoo]

RE lexical binding: I wasn't aware of existing discussion. While not exaustive, I believe my benchmark was done correctly (recompiled bytecode, restarted emacs, before testing). Lexical binding must be faster, any compiler expert would say this (disclaimer: I'm no expert, but I know a thing or two).

[dgutov]

your last example works perfectly here, what's the issue?

But how? I don't have js2-refactor installed, but one would assume that, if foo as the function name is considered to be defined inside its scope, the code analysis should consider the foo call below it to come from somewhere else.

[mishoo]

But how? I don't have js2-refactor installed, but one would assume that, if foo as the function name is considered to be defined inside its scope, the code analysis should consider the foo call below it to come from somewhere else.

Sorry, I don't understand what you mean here.. Renaming should replace both occurrences in your example, and it works as expected.

[dgutov]

However, given that the function name is not defined outside, I find the existing behavior actually more convenient

To my mind, this sounds like building with a bug on top of a bug. js2-refactor should deal with shadowing, no matter the function name's scope, I think. But ok, that's not the problem here.

[dgutov]

Renaming should replace both occurrences in your example, and it works as expected

I'm asking how can a reasonable implementation of this work, given the current behavior reported in this issue.

[dgutov]

While not exaustive, I believe my benchmark was done correctly (recompiled bytecode, restarted emacs, before testing).

Seems so. And here lexical-binding is slower, as you can observe.

Lexical binding must be faster, any compiler expert would say this (disclaimer: I'm no expert, but I know a thing or two).

"The leading experts" on Emacs Lisp compiler agree that it can be faster, but not every possible optimizations are in place, for this to happen consistently.

Anyway, let's move that discussion into the issue where it belongs. Could you remove the unrelated commits from this branch?

[mishoo]

OK, if I understand correctly hope this explains. Here's the code that js2-refactor uses to get a variable's defining scope:

(defun js2r--name-node-defining-scope (name-node)
  (unless (js2r--local-name-node-p name-node)
    (error "Node is not on a local identifier"))
  (js2-get-defining-scope
   (js2-node-get-enclosing-scope name-node)
   (js2-name-node-name name-node)))

If there is no foo variable inside the function, it worked properly even before my fix, because the function node's symbol table would not contain a foo symbol that would trick get-defining-scope to think it's the node that defines foo. Hope it explains.

Anyway, let's move that discussion into the issue where it belongs. Could you remove the unrelated commits from this branch?

I could, but I believe it would be easier for you to just cherry-pick the commit you need, and close this PR. I'm staying on my fork anyway, I feel it's snappier with lexical-binding on (will notify you if I encounter any issues with it).

[mishoo]

@dgutov alright, I moved the other two commits on another branch in my repo (devel), and left here only the fix to get-enclosing-scope. Apologizes.. I'm sometimes arrogant, I know I am, and for no good reason :-/ Thanks for maintaining this project!

[dgutov]

Okay, let me address this now:

I still didn't benchmark js2-node-get-enclosing-scope, because I'm not sure how;

Exercising the features which call it should be enough. js2-node-top-level-decl-p is called from the iMenu code. So timing js2-build-imenu-index calls might give one result.

Another would be to enable js2-highlight-unused-variables-mode and time parsing with it on.

If both (or at least the later) don't show a significant change, the performance is fine.

besides, that is a bug, performance should simply not matter more than correctness

There's no reason why it shouldn't inform the implementation of the fix, though.

Anyway, I'll need some time to think about the problem.

[dgutov]

my fork anyway, I feel it's snappier with lexical-binding on

Are you sure that's not just because of js2-script-node cleanup?

[dgutov]

@mishoo Apology accepted, and of course you have reasons to be proud. Unfortunately, by itself, that doesn't help me much when reviewing patches: like many programmers, I trust the process better than people.

Not only would I be happy to share commit access, but to even pass on the maintainership to someone else (@felipeochoa has been the prime candidate until now, if he's interested). For that, though, I'd like to see certain compatibility in the development practice. Caring about performance and writing tests are the primary things, I guess.

[dgutov]

@mishoo So, the patch looks good, and the performance effects on js2-highlight-unused-variables-mode seems to be within the margin of error.

Could you add a test, then? See the references to js2-test-scope-of-nth-variable-satisifies-predicate in existing examples.

[felipeochoa]

Re: maintainership, thanks @dgutov but I don't think I can commit to that responsibility right now. If it lessens your burden I'd accept commit access for my own PRs, though

[dgutov]

@felipeochoa Only if you really want to. Your PRs are basically the easiest. :)

[dgutov]

@mishoo To repeat: the patch looks good, and the performance effects on js2-highlight-unused-variables-mode seems to be within the margin of error.

Could you add a test? See the references to js2-test-scope-of-nth-variable-satisifies-predicate in existing examples.

[mishoo]

@dgutov I added a test. Sorry for the several years delay :)) I completely forgot about it.

[dgutov]

Not a problem, thanks for getting around to it. :)