Community
Participate
Working Groups
String s = "foo"; int start = 0; String s1 = s.substring(start); start++; String s2 = s.substring(start); Consider the above example. If I select "s.substring(start)", and do Extract Local Variable (with Replace All) selected, I get String s = "foo"; int start = 0; String x = s.substring(start); String s1 = x; start++; String s2 = x; In this case, s2 now has a different value.
to avoid this, in the general case, we'd have to use data flow analysis even then, it'd have to be very conservative. improvements possible - to be considered after 2.1
*** Bug 74943 has been marked as a duplicate of this bug. ***
*** Bug 76813 has been marked as a duplicate of this bug. ***
We should look into this for 3.1. I got tricked by it as well. At least we should issue an error in this case.
*** Bug 35788 has been marked as a duplicate of this bug. ***
*** Bug 56948 has been marked as a duplicate of this bug. ***
*** Bug 119142 has been marked as a duplicate of this bug. ***
*** Bug 132442 has been marked as a duplicate of this bug. ***
This is also an issue when the object reference on which the method is called is changed, e.g. in the code in comment 0 replace start++ with s= "newString". The refactoring changes incorrectly replace both instances of the substring expression with a local variable equivalent to s1, rendering the new assignment pointless and s2 incorrectly assigned. The dialog should at least warn if the expression to be extracted could cause side effects, i.e. it contains expressions of the following forms: a++, ++a, a+= n, a= b, a() (method calls and constructors) (assuming no concurrency issues). Without this the user is led to believe that the refactoring will be successful. Although it completes normally it does affect program behaviour, which refactorings aren't supposed to do. This seems a significant bug since it could without warning introduce logic errors that are hard to trace. It has also been around for a while and seems to be widely encountered. (These issues may be separate, but they seem to tie in with this bug or those marked as duplicates).
I reported one of the dups. I just hit this again in a very hard-to-figure-out situation. I had something like the following at one point of a fairly large routine: int matchUnsorted = (a0 == b0 ? 1 : 0) + (a1 == b1 ? 1 : 0); if (a0 > a1) { // Sort to ensure a0 <= a1 int tmp = a0; a0 = a1; a1 = tmp; } if (b0 > b1) { // Sort to ensure b0 <= b1 int tmp = b0; b0 = b1; b1 = tmp; } int matchSorted = (a0 == b0 ? 1 : 0) + (a1 == b1 ? 1 : 0); I then replaced (a0 == b0 ? 1 : 0) with a variable a0_eq_b0 and (a1 == b1 ? 1 : 0) with a1_eq_b1. (Those conditionals were used in a number of other places in the routine). Of course now matchUnsorted is always equal to matchSorted, whereas it wasn't before. Logic was broken by refactoring.
Of course the real problem with the example in the previous comment was bad style (changing the value of a non-counter variable partway through a method), but still...
+1 to at least show a warning.
*** Bug 176632 has been marked as a duplicate of this bug. ***
another very common example is extracting a local variable from the condition of a while statement: void f(){ while(b()) { } } will change to void f(){ boolean b = b(); while(b) { } } this is wrong since the result of b() could change between the iterations!! This is not a minor bug in my opinion..... same goes for do while.
*** Bug 217851 has been marked as a duplicate of this bug. ***
*** Bug 222527 has been marked as a duplicate of this bug. ***
*** Bug 322700 has been marked as a duplicate of this bug. ***
*** Bug 181127 has been marked as a duplicate of this bug. ***
Some more from me. Set<Object> set = new HashSet<Object>(); set.add(new Object()); set.add(new Object()); wanted Object o1 = new Object() Object o2 = new Object() set.add(o1); set.add(o2); but got after first refactoring Object object = new Object(); set.add(object); set.add(object); as for me, there is no reason at all to replace all ocurancies of code, because for me situations like someFn( realyBigExpression ); ... someOtherFn( realyBigExpression ); are bad smelling code. So if i need to use "realyBigExpression" for second time somewhere in my code i use and "extract Local Variable" refactoring. That means that in most cases i extract to variable a single expression. Some methaphor: When i saw Object object = new Object(); set.add(object); set.add(object); i felt like if i turn on my TV in leaving room and TV in kitchen turns on to. So, please, don't ocuppy your mind with data flow analysis, just give us some way to extract a single expression safely.
(In reply to comment #19) That already works today if you uncheck the option "Replace all occurrences..." in the Extract Local Variable dialog. Or you press Ctrl+1 and use the "Extract to local variable" quick assist (you can even assign a direct key binding).
Another common example: Color color = new Color((float) Math.random() * .5f + .5f, (float) Math.random() * .5f + .5f, (float) Math.random() * .5f + .5f); Select the first "(float) Math.random() * .5f + .5f", and go Refactor -> Extract to local variable. Call it "r". Now you have: float r = (float) Math.random() * .5f + .5f; Color color = new Color(r, r, r); Oops :-) This could be *much* worse if you have other examples of the same expression in the same scope but off the screen so you don't notice the change. Maybe common functions with side effects will need to be blacklisted too?
*** Bug 355283 has been marked as a duplicate of this bug. ***
I got hit by this today: int argLength = argTypes == null ? 0 : argTypes.length; long sourceLevel = compilerOptions().sourceLevel; if (argLength > 0 && sourceLevel >= ClassFileConstants.JDK1_5) { if (argTypes[--argLength].isVarArgs()) method.binding.modifiers |= ClassFileConstants.AccVarargs; if (CharOperation.equals(argTypes[argLength].name, ConstantPool.This)) { if (argLength != 0 || sourceLevel <= ClassFileConstants.JDK1_7) { problemReporter().illegalThis(argTypes[argLength], method); } } while (--argLength >= 0) { if (argTypes[argLength].isVarArgs()) problemReporter().illegalVararg(argTypes[argLength], method); } } Extracting argTypes[argLength] into a local left me with bad code.
ResultSet rs = ...; int i = 0; rs.getString(/*[*/i++/*]*/); // do extract local variable on "i++" rs.getString(i++); rs.getString(i++); The result is: int i = 0; int j = i++; rs.getString(j); rs.getString(j); rs.getString(j); Which is wrong because i++ has side effects. This case can be easily fixed by removing the following code from org.eclipse.jdt.internal.corext.refactoring.code.ExtractTempRefactoring.isLeftValue(): if (parent instanceof PrefixExpression) { PrefixExpression.Operator op= ((PrefixExpression) parent).getOperator(); if (op.equals(PrefixExpression.Operator.DECREMENT)) return true; if (op.equals(PrefixExpression.Operator.INCREMENT)) return true; return false; } Thanks, Jean--Noel
(In reply to comment #24) Hmm I wrote to fast I did not read enough. Please ignore the suggested fix in comment #24. To fix the issues described in comment #24, there should be another case in ExtractTempRefactoring.canReplace(): if (hasSideEffect(node)) { return false; } With the following implementation: private static boolean hasSideEffect(ASTNode node) { if (node instanceof PrefixExpression) { PrefixExpression.Operator op= ((PrefixExpression) parent).getOperator(); return op.equals(PrefixExpression.Operator.DECREMENT) || op.equals(PrefixExpression.Operator.INCREMENT); } return false; } I am not sure whether we should also take care of ParenthesizedExpression here too?
*** Bug 419421 has been marked as a duplicate of this bug. ***
Just ran into this again... I hit this problem about once every few months... private static void checkNoCycles(Node node, Set<Node> discovered, Set<Node> finished) { // ... } public void checkNoCycles() { for (Node child : nodes) { checkNoCycles(child, new HashSet<Node>(), new HashSet<Node>()); } } Select the first "new HashSet<Clause>()" (with the goal of moving the "new" invocations out of the loop), and extract to local variable. You end up with: public void checkNoCycles() { for (Node child : nodes) { var discovered = new HashSet<Node>(); checkNoCycles(child, discovered, discovered); } } This is semantically wrong. You should end up with: public void checkNoCycles() { for (Node child : nodes) { var discovered = new HashSet<Node>(); checkNoCycles(child, discovered, new HashSet<Node>()); } } then you should be able to repeat the process for the second "new" expression. In general, extract to local should *never* replace multiple "new" expressions with a single extracted local.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
