Bug 122709 - variable renaming affects variables in other methods
variable renaming affects variables in other methods
Status: VERIFIED FIXED
Product: ruby
Classification: Unclassified
Component: Editing
6.x
All All
: P1 (vote)
: 6.x
Assigned To: Torbjorn Norbye
issues@ruby
release60_fixes_candidate1, release60...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-25 18:22 UTC by jamespb
Modified: 2007-12-12 16:15 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Fix merged to release60 (745 bytes, patch)
2007-11-26 21:09 UTC, Torbjorn Norbye
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jamespb 2007-11-25 18:22:20 UTC
Given this code:

  class MatchHashOrAttribute < Match
    def initialize arg
      @matches = {}
      arg.each_pair do |foo, val|
        @matches[foo] = Match.match_factory(val)
      end
    end
    
    def match? rhs
      result = false
      @matches.each_pair do |foo, val|
        if Hash === rhs
          matched = val.match?(rhs[foo])
        else
          (matched = val.match?(rhs.send(foo))) rescue nil
        end
        if matched
          result = true
        else
          result = false
          break
        end
      end
      result
    end
  end
    
Put the cursor on the first 'name'.  All the instances of 'name' are highlighted in both methods, and if you hit ctrl-r
on any one of them all of them are renamed in both methods.

IMHO, this is a showstopper for final release; if the other variables are off the screen, there's no way for the user to
know that they've just changed variables names all over their code.
Comment 1 jamespb 2007-11-25 18:23:25 UTC
build 5469

Product Version: NetBeans Ruby IDE 20071124202613
Java: 1.6.0_01; Java HotSpot(TM) Client VM 1.6.0_01-b06
System: Windows Vista version 6.0 running on x86; Cp1252; en_US (nbrubyide)
Userdir: C:\Users\James\AppData\Roaming\.nbrubyide\dev
Comment 2 Torbjorn Norbye 2007-11-26 01:27:15 UTC
Thanks for the report. I don't see 'name' referenced in the code; were you referring to one of the dynamic vars perhaps
(foo, val) ?  Also, your build is 6.1 not 6.0, but this code hasn't changed since 6.0 so unlike the other lexing bug you
filed this has a high chance of being a problem in 6.0 as well. I'll investigate tomorrow.
Comment 3 jamespb 2007-11-26 01:53:45 UTC
Sorry about that - 'name' was the original value, 'foo' was the new value.
Comment 4 Torbjorn Norbye 2007-11-26 21:03:25 UTC
Fixed in trunk:
IDE:-------------------------------------------------
IDE: [11/26/07 9:00 PM] Committing started
cvs server: scheduling file `nestedblocks2.rb.testRename9.rename' for addition
cvs server: scheduling file `nestedblocks2.rb' for addition
cvs server: scheduling file `nestedblocks2.rb.testNestedBlocks2.occurrences' for addition
cvs server: use 'cvs commit' to add these files permanently
Checking in test/unit/src/org/netbeans/modules/ruby/RenameHandlerTest.java;
/cvs/ruby/editing/test/unit/src/org/netbeans/modules/ruby/RenameHandlerTest.java,v  <--  RenameHandlerTest.java
new revision: 1.5; previous revision: 1.4
done
IDE: [11/26/07 9:00 PM] Diffing finished
Checking in test/unit/src/org/netbeans/modules/ruby/OccurrencesFinderTest.java;
/cvs/ruby/editing/test/unit/src/org/netbeans/modules/ruby/OccurrencesFinderTest.java,v  <--  OccurrencesFinderTest.java
new revision: 1.9; previous revision: 1.8
done
Checking in src/org/netbeans/modules/ruby/AstUtilities.java;
/cvs/ruby/editing/src/org/netbeans/modules/ruby/AstUtilities.java,v  <--  AstUtilities.java
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb,v
done
Checking in test/unit/data/testfiles/nestedblocks2.rb;
/cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb,v  <--  nestedblocks2.rb
initial revision: 1.1
done
RCS file: /cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb.testRename9.rename,v
done
Checking in test/unit/data/testfiles/nestedblocks2.rb.testRename9.rename;
/cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb.testRename9.rename,v  <--  nestedblocks2.rb.testRename9.rename
initial revision: 1.1
done
RCS file: /cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb.testNestedBlocks2.occurrences,v
done
Checking in test/unit/data/testfiles/nestedblocks2.rb.testNestedBlocks2.occurrences;
/cvs/ruby/editing/test/unit/data/testfiles/nestedblocks2.rb.testNestedBlocks2.occurrences,v  <-- 
nestedblocks2.rb.testNestedBlocks2.occurrences
initial revision: 1.1
done
IDE: [11/26/07 9:00 PM] Committing finished
IDE: [11/26/07 9:00 PM] Diffing finished
Comment 5 Torbjorn Norbye 2007-11-26 21:09:04 UTC
Now the question is whether we push this into the release branch.

James argued that we should, and I'm inclined to agree. I wouldn't hold the release for this single bug fix, but if we
are respinning the build for anything else, I'd like to include this fix.  It's really simple.  Basically, a "break"
statement was intended to jump out of an iteration, but because this break statement was inside a switch-statement, it
only broke out of the switch. What we need is a labelled break:

Index: AstUtilities.java
===================================================================
RCS file: /cvs/ruby/editing/src/org/netbeans/modules/ruby/AstUtilities.java,v
retrieving revision 1.17
diff -u -r1.17 AstUtilities.java
--- AstUtilities.java   4 Nov 2007 09:04:47 -0000       1.17
+++ AstUtilities.java   26 Nov 2007 21:04:09 -0000
@@ -1798,7 +1798,8 @@
         }
 
         Node leaf = path.root();

+      while_loop:
         while (it.hasNext()) {
             Node n = it.next();
             switch (n.nodeId) {
@@ -1813,7 +1814,7 @@
             case NodeTypes.SCLASSNODE:
             case NodeTypes.MODULENODE:
                 leaf = n;
-                break;
+                break while_loop;
             }
         }
 
This code is basically walking up the AST path looking for the current enclosing method/class/module. When it sees it,
it's supposed to stop - but because of this bug, it didn't, and as a result, the whole AST tree was included in the
search for block nodes - which is clearly wrong. I've added new unit tests to detect this scenario.

I will attach a patch for 6.0 if somebody wants to review it. The fix can be tested in deadlock.netbeans.org build #5489
or later.
Comment 6 Torbjorn Norbye 2007-11-26 21:09:45 UTC
Created attachment 53509 [details]
Fix merged to release60
Comment 7 Jiri Skrivanek 2007-11-28 12:23:49 UTC
Verified in trunk.
Comment 8 pgebauer 2007-12-03 19:47:30 UTC
The fix has been ported into the release60_fixes branch.

Checking in AstUtilities.java;
/cvs/ruby/editing/src/org/netbeans/modules/ruby/AstUtilities.java,v  <--  AstUtilities.java
new revision: 1.17.4.1; previous revision: 1.17
done


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