renaming does not respect scoping

[clausreinke]

consider this code

var x = 1;
(function(){
 var y = 2;  // renaming y to x should fail, because
 console.log(x);  // captured by renamed binding
 (function(){
   var x = 3;
   console.log(y);  // renamed occurrence captured
  })();
})();

Tern's renaming simply takes all occurences of y and replaces them with x, resulting in the erroneous result

var x = 1;
(function(){
 var x = 2;  // renaming y to x should fail, because
 console.log(x);  // captured by renamed binding
 (function(){
   var x = 3;
   console.log(x);  // renamed occurrence captured
  })();
})();

See estr for a scope-preserving renaming in Javascript (the repo has some more annoying test cases).

[marijnh]

See patch c595988 . I've made it simply return an error response in a situation like this.

[clausreinke]

Thanks. This seems to fix half of the issue - renaming y to x fails, with a useful error message (using ternjs.net in Chrome - in Opera, I can't rename anything, and there is no error message).

However, if I rename the first x to x1, I can then rename y to x, which captures the renamed occurrence of y in the innermost function. Also, if I add a line var x = 4 below the line var y = 2, I can rename y to x, irreversibly merging the yand x scopes (and doing this in a formal parameter list leads to repeated parameters, which are an error in strict mode).

Other scope-related corner cases: renaming to or from this or arguments is risky, because the binders are implicit and cannot be renamed.

[mishoo]

This is for an editor, not a completely automated refactoring tool. I'd say it should allow renaming anything to anything (it would be helpful to get a warning for funny cases, but not refuse to rename; after all, you have "undo"). For example sometimes I rename a var to foo.something(), or to a "string", and then drop the variable declaration.

(not using Tern yet, but I have a simple tool that has a similar feature, based on UglifyJS). EDIT: just noticed this is allowed on ternjs.net too — cool. :-)

[clausreinke]

Great, thanks! It is good to see you take these fundamental issues seriously - makes tern stand out from the average JS tool project.

That leaves only the implicit variables: renaming y to arguments or this will lead to accidental capture again. Renaming from arguments or this also cannot work, so should be blocked.

[marijnh]

My call is that providing too much 'safety' on a tool can get in the way and insult the intelligence of its users. Renaming from this already won't work because that's syntactically a keyword, not a variable. If someone wants to rename something to this or from/to arguments, I think that's their problem, and might even be a sane thing to do (renaming arguments to some captured variable when lifting out a piece of a function that was referring to the function's arguments variable).

[clausreinke]

Having worked on real refactorers before (lead role on Haskell, advisory on Erlang), my call is that there is no such thing as too much safety, only too little flexibility: if a user asks for renaming, the tool should try its best to do a renaming, and only that; if a user asks for a search and replace that might profit in some obscure way from being partially scope-aware, that is either a separate refactoring or a separate code transformation building on the same infrastructure.

In principle, you could separately offer the "scoped occurrence search" and "replace occurrences" phases of renaming, and users would know that they are doing something odd if they have to use that instead of simple renaming. But in our experience (the basic Haskell refactorer project ran for 3 years), there were always proper refactorings hiding behind those odd requests, and finding those offered the most benefit (eg, capturing arguments and lifting out a piece of a function).

Given the number of JS coders running into difficulties with this, I would not assume that tool users know what they are doing if they rename to this.

There is little enough safety that can be provided for refactoring JS, which, prior to ES5 strict, does not even respect static scoping, and since renaming touches static variables and dynamic properties, renaming in full is tricky to make safe (FYI, Tool-supported Refactoring for JavaScript has a sample analysis).