Community
Participate
Working Groups
Build ID: I20090313-0100 (3.5M6) Steps To Reproduce: If the text lines between LINE 4 and LINE 10 are selected, then the refactor menu should offer Extract Constant as a menu item. ------------------------- Bug.java ------------ import java.util.Comparator; class Bug { { //LINE 4 new Comparator<String>(){ @Override public int compare(String o1, String o2) { return 0; }} //LINE 10 ; } }
See also bug 94639.
Bug 94639 is something different. This bug is about two problems: 1. Extract Constant is not available if the selection includes whitespace in front of the expression. 2. If only the expression is selected, the refactoring stops with a fatal error because ConstantChecks#isLoadTimeConstant(IExpressionFragment) is borked: The implementation should also consider anonymous class creations and lambda expressions: It should skip parameters, local variables, and fields defined inside the expression, and references to those variables.
You can assign this one to me
About point 1, I think this is a more general behavior/issue: if the selection includes heading spaces/tabs, the resulting refactoring or Quick assist is different. For instance, selecting: <space>System.out.println("Test) ... gives different refactoring/Quick assist than selecting: System.out.println("Test) Is this an issue? If yes, I think it should be logged in a separate bug.
Created attachment 243481 [details] patch to review Here is a (simple) patch for point 2 to review.
Filed bug 435772 for the whitespace selection issue (problem 1. from comment 2). (In reply to Jerome Cambon from comment #5) * Formatting problems: See section/link "coding conventions" in http://www.eclipse.org/jdt/ui/dev.php . Unfortunately, the referenced Sun/Oracle code conventions are broken links, see references in https://wiki.eclipse.org/Coding_Conventions#cite_note-1 * ConstantChecks.LoadTimeConstantChecker#visit(ReturnStatement) doesn't change anything. * The visitor should not accept all expressions that contain an anonymous or lambda. If you e.g. append .compare(toString(), System.getProperty("abc")) to the anonymous from comment 0, then the whole expression should not be extractable into a constant because 'toString()' is not a load time constant.
Created attachment 243615 [details] patch to review, including Markus's comments This should include all your comments, hope the coding conventions are ok now. As commented in the patch, I think "visit(FieldDeclaration node)" method is not needed, should be captured by "visit(VariableDeclarationFragment node)".
This contribution complies with http://www.eclipse.org/legal/CoO.php
Markus, could you please review the patch ?
Comment on attachment 243615 [details] patch to review, including Markus's comments * The 3 fields in ExpressionChecker are in the wrong class. The initializer in the field declaration of fLambdaAnonymousNames is hard to understand and unnecessary for the second constructor. Better initialize fields in all constructors if they can't be completely initialized in the initializer. * I don't see the point of fAnonymous and fLambda. Why are they necessary? * The example from comment 6 is now blocked, but for a wrong reason. The problem is not the anonymous as method invocation target, but the invocation of the non-static method toString(). If you append '.compare("ABC", System.getProperty("abc"))' to the anonymous, then the expression should be extractable. * node.getName().toString() is a no-go, see Javadoc. To store entities, you would use resolveBinding(). But I don't think any of this is necessary here. A complete analysis would be a lot of work to implement. ExtractConstantRefactoring#checkSource(..) already reports such problems, and I think that's good enough. LoadTimeConstantChecker should just avoid diving into AnonymousClassDeclaration and LambdaExpression nodes and let the compiler find the more complex problems inside those nodes. * In this code snippet, the assertion is not necessary for two reasons: Assert.isTrue(param instanceof SingleVariableDeclaration); SingleVariableDeclaration varDeclaration= (SingleVariableDeclaration) param; 1. The Javadoc of MethodDeclaration#parameters() already guarantees it's a list of SingleVariableDeclaration. 2. The cast would fail anyway at run time, so the assert is completely redundant. * When I select the anonymous class instance creation expression in comment 0, then the Extract Constant dialog shows an info text that is not correct (may be an old problem). * The patch causes test failures in AssistQuickFixTest18. Please add tests in a new class ExtractConstantTests18 (copy ExtractConstantTests17.java and adapt).
Created attachment 247141 [details] New patch to review Considering your comments, I finally ended with a very simple patch, which only skip anonymous and lambda checking in ConstantChecks.java. For code such as the one described in comment 6, the Extract Constant is proposed, with an error about toString(). This seems acceptable to me. I also fixed AssistQuickFixTest18 test failure. I tried to add a new test, but unfortunately ExtractConstantTests17.java (that you suggested to start from) doesn't seem to test anything.
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. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. 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.
