Bug 198032 - JavaScript Variable hidden from Refactor Rename
JavaScript Variable hidden from Refactor Rename
Status: VERIFIED FIXED
Product: javascript
Classification: Unclassified
Component: Editor
7.0
PC Windows Vista
: P2 (vote)
: 7.3
Assigned To: Petr Pisl
issues@javascript
: 7.0.1_WAIVER_APPROVED, 7.1_WAIVER_APPROVED, 7.2_WAIVER_APPROVED
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-23 08:33 UTC by bht
Modified: 2013-01-15 13:24 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Testcase (527 bytes, text/x-c)
2011-04-23 08:33 UTC, bht
Details

Note You need to log in before you can comment on or make changes to this bug.
Description bht 2011-04-23 08:33:41 UTC
Created attachment 107913 [details]
Testcase

Please refer to attached testcase. I hope there is a rewarding quick fix.
Comment 1 bht 2011-05-04 17:35:23 UTC
This is a bad trap - please fix in 7.01
Comment 2 bht 2011-05-23 08:10:54 UTC
The fix for this will probably also fix issue 197817.
Comment 3 Petr Pisl 2011-05-25 12:16:39 UTC
It's problem in infrastructure and I'm afraid that it will not be easy to fix.
Comment 4 bht 2011-05-25 17:19:46 UTC
I am requesting the removal of the refactoring feature in the JavaScript editor until this bug is fixed.

This bug is breaking JavaScript code without warning. The JavaScript developer does not have any awareness of breaking their code with refactoring all, and there is no means of discovering the errors. That is a fundamental problem that no IDE should have at any production stage.

It might be difficult to fix this bug. I was hoping for an easy fix, but I did not say that, meaning "please fix it only if it is easy to fix".
Comment 5 Petr Jiricka 2011-05-26 07:27:43 UTC
> The JavaScript developer does not have any awareness of breaking their code with refactoring all

I believe for other languages, we had a dialog saying "refactoring is approximate, please check the diff". Would it help in this case too?

> I was hoping for an easy fix, but I did not say that

No worries, this will be fixed. Just not for 7.0.1 - there is not enough time in the 7.0.1 schedule for this complex fix. I suggest to leave this bug open with P2 priority and request a waiver for 7.0.1.
Comment 6 bht 2011-05-26 07:58:21 UTC
IMHO JavaScript refactoring is approximate only where the scope/reachability is not well defined.

However in the private scope, refactoring is 100% defined, and a failure is catastrophic. Please try the test case. If it doesn't work here, then this is just extreme poor quality, and not worth taking the risk to even use it.

So please, please switch refactoring off or fix this bug in this release.
Comment 7 Petr Pisl 2011-05-26 14:07:48 UTC
The problem in the infrastructure is that there is no context for variables. This is big problem. There has to be added this information and this is not easy. 

Unfortunately this behavior is here from NB 6.5.
Comment 8 Petr Jiricka 2011-06-03 11:55:24 UTC
Right, the behavior has been the same for several years. I still think refactoring is useful even in the current limited form - this is the first request to disable it that we got in several years. Going forward, there is a plan to fix it (involving essentially a complete rewrite of the JavaScript editor), so this will be addressed. However, such a change is not doable for 7.0.1, therefore I am requesting a waiver for 7.0.1.
Comment 9 bht 2011-06-03 20:00:31 UTC
Refactoring is only useful if does not break our source code.

In this case, the refactoring breaks the source code with no means available to detect and repair the damage. Then, a simple search and replace is more useful than refactoring.

The fact that "this is the first request to disable it that we got in several years" is the reflection of the fact above, so not many developers except the reporter use refactoring in a meaningful way. Otherwise they would have found this rather obvious bug.

More generally, not may people cared to use the NetBeans JavaScript editor at all. I don't know exactly why, possibly because of other severe bugs, eg bug 159704 which was raised by the same reporter. Likewise why would one want to rewrite the editor if nobody except the reporter complains?

I appreciate that there have been plans to rewrite the JavaScript editor for years. If the timeline is not very short term, eg in the range of weeks, then IMHO it would be preferable to limit the damage before such a rewrite not just carry on with business as usual ie waiver.
Comment 10 Petr Jiricka 2011-06-06 07:05:17 UTC
> More generally, not may people cared to use the NetBeans JavaScript editor at all. 

Not true, our statistics show that more than a quarter of all users use the JavaScript editor.
Comment 11 Petr Jiricka 2011-06-08 08:23:31 UTC
Sorry, any change in this area is too large and risky for NetBeans 7.0.1. I am marking the waiver as approved.
Comment 12 Petr Pisl 2011-09-19 13:29:02 UTC
I have returned to this bug. I'm trying to solve it in the old infrastructure, because it looks like the new one, will not be ready in the next release.

At first I have to admit that I'm not big expert in writing JS code. I have change a little the example that you have provided: 

function level0() {
  var var_a = "level0";// invoking refectoring through CTRL+R here
                
  function level1_1(){
    var_a = var_a;
    document.write("level1_1: " + var_a +  "<br/>");
  }
  function level1_2(){
    var var_a;
    document.write("level1_2: " + var_a +  "<br/>");
  }
  function level1_3 (){
    var_a = var_a;
    document.write("level1_3: " + var_a +  "<br/>");
  }
  function level1_4 (){
    this.level2_1 = function(){
      var_a = var_a;
      document.write("level2_1: " + var_a +  "<br/>");
    }
  }
                
  level1_1();
  level1_2();
  level1_3();
  var o = new level1_4();
  o.level2_1();
}
level0();

After executing the code the output is:
level1_1: level0
level1_2: undefined
level1_3: level0
level2_1: level0

If I do the code right, that only var_a shouldn't be refactored in level1_2 method. When you press ctrl+r in var_a declaration only the var_a in level1_1 method are offered to refactor. So from my example there should be also refactored var_a in level1_3 and level2_1 methods, but this doesn't correspond with comment in your example. So at first i would like to clarify it.
Comment 13 bht 2011-09-19 20:30:05 UTC
Yes Petr, I agree with your last statements.

My choice of naming wasn't perfect, here is another version of the example:

/*
Variable hidden from refactor rename
*/
function level0() {
    var first;// Try rename refactor from here
    function level1_1(){
        first = "a";
    }
    function level1_2(){
        var first = "a";// This hidden declaration causes the damage.
    }
    function level1_3(){
        first = "a";// hidden - cannot be refactored from level0
    }
    function level1_4(){
        this.level2_1 = function(){
            first = "x";// hidden - cannot be refactored from level0
        }
    }
}

As you say, "var first" should only be protected from refactoring in function level1_2() which is the only place where it is declared again on a nested level.
Comment 14 Petr Pisl 2012-03-27 14:04:21 UTC
In the new JS support it's going to work. The occurrences are already implemented. The refactoring not yet.
Comment 15 Petr Pisl 2012-05-10 12:27:07 UTC
Instant rename also implemented in the new JS support.
Comment 16 Petr Pisl 2012-09-12 14:12:41 UTC
This issue can be closed for now. The "standard" refactoring is not implemented yet in the new JS editor, but the instant rename works as expected.
Comment 17 Vladimir Riha 2013-01-15 13:24:02 UTC
verified

Product Version: NetBeans IDE 7.3 RC1 (Build 201301142100)
Java: 1.7.0_10; Java HotSpot(TM) Client VM 23.6-b04
Runtime: Java(TM) SE Runtime Environment 1.7.0_10-b18
System: Linux version 3.2.0-35-generic-pae running on i386; UTF-8; en_US (nb)


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo