Bugzilla – Full Text Bug Listing
| Summary: | [clean up][quick assist][inline] Fix detection and creation of unnecessary parentheses | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||||||||||||
| Component: | UI | Assignee: | Deepak Azad <deepakazad> | ||||||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||
| Priority: | P3 | CC: | markus.kell.r, rthakkar | ||||||||||||||||||||||
| Version: | 3.7 | Flags: | markus.kell.r:
review+
|
||||||||||||||||||||||
| Target Milestone: | 3.7 M6 | ||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||
| OS: | Windows 7 | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 371078 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Markus Keller
Bug 334876 also needs a closer look. This example shows some problems when inlining plus() or minus():
public class PrecedenceParentheses {
int plus(int a, int b) {
return a + b;
}
int minus(int a, int b) {
return a - b;
}
void bar() {
int i;
i= plus(1 + 2, 3);
i= plus(1, 2 + 3);
i= minus(1, 2);
i= minus(1 - 2, 3);
i= minus(1, 2 + 3);
i= minus(1, 2 - 3);
i= minus(1 + 2, 3 + 4);
}
}
(In reply to comment #1) > Bug 334876 also needs a closer look. The fix for Bug 334876 is fine, but Invert conditions quick assist does need a closer look, e.g. Bug 120230 Created attachment 187952 [details] fix + tests v0.5 There are 2 parts of the problem - Have one commmon implementation for deciding whether an expression needs parentheses or not. - Use this common implementation everywhere. The Implentation in ExpressionsFix.UnnecessaryParenthesisVisitor#canRemoveParenthesis() looks decent enough. At least I could tweak it a bit to make it reusable and fix problems with bug 332019, bug 330415, bug 120230. (I have not tried to make this perfect as per the spec yet) There are few other places where we need to use this common implementation. e.g inline constant and inline variable refactoring Besides correctness, we also have to consider aesthetics, e.g.
-----------------------------------------------------------
package expression_in;
public class TestConditionalExpression {
int i(Object s, int k) {
return k == 3 ? s.hashCode() : 3;
}
void f(int p) {
int u = i(this, p);
}
}
-----------------------------------------------------------
What should be the result after inlining 'i'
- int u = p == 3 ? this.hashCode() : 3;
or
- int u = (p == 3 ? this.hashCode() : 3);
Both are correct but some might prefer the second option. There are other places as well where we might choose aesthetics over correctness.
Markus, what do you think ?
The goal is to "only do what we're told to do", so no guessing about aesthetics. We should never add parentheses unless they are needed for correctness. In comment 3, the result should be: - int u = p == 3 ? this.hashCode() : 3; On the other hand, we should remove parentheses if only if they were necessary for correctness in the original code. E.g. here, the quick fix to remove the unnecessary cast should remove the parentheses around 'n': void bar (Integer n) { int i= ((Integer) n).intValue(); } But in cases like the following, the unnecessary parentheses should stay, since they were not the target of the quick fix: int i= (((Integer) n)).intValue(); //=> (n).intValue() int i= ((Integer) (n)).intValue(); //=> (n).intValue() The cases where we add unnecessary parentheses should be very limited. The user can always add them later if he wants. Created attachment 188224 [details] fix + tests v0.9 - There was some implementation in ASTNodes.substituteMustBeParenthesized and ASTNodes.needsParentheses. I have consolidated all this to MissingNecessaryParenthesesChecker.isParenthesesNeeded(Expression, ASTNode, StructuralPropertyDescriptor), and used this everywhere. - With this patch all the known problems are fixed - that is bug 332019, bug 330415, bug 120230, bug 322597. Plus some tests are also updated as extra parentheses are no longer added. (In reply to comment #6) > The goal is to "only do what we're told to do", so no guessing about > aesthetics. Fair enough. The patch tries to do this only. Next step is to find holes in MissingNecessaryParenthesesChecker.isParenthesesNeeded() as per the spec. > But in cases like the following, the unnecessary parentheses should stay, since > they were not the target of the quick fix: > int i= (((Integer) n)).intValue(); //=> (n).intValue() > int i= ((Integer) (n)).intValue(); //=> (n).intValue() .. and also look out for cases like this. Created attachment 188311 [details]
fix + tests v1.0
This patch should be good to go. The implementation in MissingNecessaryParenthesesChecker.needsParentheses() looks quite solid now.
I have tried to use this common implementation in as many places as possible, but there still might be few places where we add extra parentheses. We can fix these cases as and when we find them in separate bugs.
Created attachment 188316 [details]
fix + tests v1.0.1
Found and fixed 2 additional cases in AdvancedQuickAssistProcessor.
Created attachment 188426 [details]
fix + tests v1.0.2
Sigh... I had missed fixing the following 2 cases in previous patches
- 15.17.1 While integer multiplication is associative when the operands are all of the same type, floating-point multiplication is not associative.
- 15.18.2 Integer addition is associative when the operands are all of the same type, but floating-point addition is not associative.
- Also renamed MissingNecessaryParenthesesChecker to NecessaryParenthesesChecker
This should be the last patch :)
Ping. Sorry, I have not yet checked the whole patch, but here are some points that need to be fixed anyway: * The check in InlineConstantRefactoring you marked with TODO is really necessary. Modified example from bug 265448 (which introduced this test): public class Constants { static final long two = 1 + 1; //inline two static long much = two * Integer.MAX_VALUE; public static void main(String[] args) { System.out.println(much); } } The example can easily be adapted to show that the check is also necessary in InlineTempRefactoring and in SourceProvider. Additional caveat: In all three places, the arguments "element.getParent(), element.getLocationInParent()" are wrong in the patch. Should be "cast, CastExpression.EXPRESSION_PROPERTY". To see this, modify my example to declare "two = 2 * Integer.MAX_VALUE:". * Javadoc of NecessaryParenthesesChecker#needsParentheses(Expression, ASTNode, StructuralPropertyDescriptor) should be a bit more detailed, e.g. "Does the <code>expression</code> need parentheses when inserted into <code>parent</code> at <code>locationInParent</code>?" * Change in AccessAnalyzer adds unnecessary parentheses. * I don't get how NecessaryParenthesesChecker#needsParentheses(Expression) can do something useful. By definition, an expression never needs parentheses at its current location. Created attachment 189008 [details] fix + tests v1.0.3 (In reply to comment #12) > Sorry, I have not yet checked the whole patch, but here are some points that > need to be fixed anyway: > > * The check in InlineConstantRefactoring you marked with TODO is really > necessary. Modified example from bug 265448 (which introduced this test): > > public class Constants { > static final long two = 1 + 1; //inline two > static long much = two * Integer.MAX_VALUE; > > public static void main(String[] args) { > System.out.println(much); > } > } > > The example can easily be adapted to show that the check is also necessary in > InlineTempRefactoring and in SourceProvider. Additional caveat: In all three > places, the arguments "element.getParent(), element.getLocationInParent()" are > wrong in the patch. Should be "cast, CastExpression.EXPRESSION_PROPERTY". > To see this, modify my example to declare "two = 2 * Integer.MAX_VALUE:". Oops, the TODOs were left by mistake. I also thought that the checks were needed. But they helped in catching the wrong arguments issue :) Fixed this for all three refactorings. > * Change in AccessAnalyzer adds unnecessary parentheses. > > * I don't get how NecessaryParenthesesChecker#needsParentheses(Expression) can > do something useful. By definition, an expression never needs parentheses at > its current location. Right, got rid of this method. Changes in AccessAnalyzer and GetterSetterUtil are not perfect yet, and I am working on these. AccessAnalyzer - Change causes 2 test failures SefTests.testCompoundWrite2() and SefTests.testCompoundWrite4(). This is a bit tricky as in this case the binding for the method invocation is null which causes the analysis to fail in NecessaryParenthesesChecker.isExpressionIntegerType(Expression). GetterSetterUtil - I do not have test cases for this change yet. I also fixed String concatenation cases. See CleanUpTest.testRemoveParenthesesBug335173_20() Created attachment 189072 [details] fix + tests v1.0.4 (In reply to comment #13) > AccessAnalyzer - Change causes 2 test failures SefTests.testCompoundWrite2() > and SefTests.testCompoundWrite4(). This is a bit tricky as in this case the > binding for the method invocation is null which causes the analysis to fail in > NecessaryParenthesesChecker.isExpressionIntegerType(Expression). > > GetterSetterUtil - I do not have test cases for this change yet. I think we will have to live with extra parentheses for these 2 cases, because bindings are not available in these cases as the InfixExpression is a newly created node, see AccessAnalyzer.java line 142 and GetterSetterUtil.java line 261. I do not see a way to solve this problem. Markus, do you ? See also tests SefTests#TestCompoundWrite2, SefTests#TestCompoundWrite4, GetterSetterQuickFixTest#testInvisibleFieldToGetterSetterBug335173_2. The patch includes a couple of minor changes from the previous one. (In reply to comment #6) > But in cases like the following, the unnecessary parentheses should stay, since > they were not the target of the quick fix: > int i= (((Integer) n)).intValue(); //=> (n).intValue() > int i= ((Integer) (n)).intValue(); //=> (n).intValue() > Somewhat similar to these cases - Extract method, Extract constant, Extract local, Introduce Parameter ------------------------------------------------------------------------ package p; public class A { public void foo(){ int x= 1 - (2 + 3); } } ------------------------------------------------------------------------ Do we remove the parentheses when selection is ‘2 + 3’? Do we remove the parentheses when selection is ‘(2 + 3)’? I think it is ok to remove in this case - What if there is an extra parentheses ------------------------------------------------------------------------ package p; public class A { public void foo(){ int x= 1 - ((2 + 3)); } } ------------------------------------------------------------------------ Do we remove the parentheses when selection is ‘2 + 3’? Do we remove the parentheses when selection is ‘(2 + 3)’? Do we remove the parentheses when selection is ‘((2 + 3))’? We can probably remove 1 pair of parentheses in these cases. (In reply to comment #14) > > This is a bit tricky as in this case the > > binding for the method invocation is null which causes the analysis to fail in > > NecessaryParenthesesChecker.isExpressionIntegerType(Expression). [..] > I do not see a way to solve this problem. Markus, do you ? Yes, I do (agree;-). If there are compile errors, we better add more parentheses than too few. Users can easily remove them later. (In reply to comment #15) The Extract... cases are a bit different from Inline and Remove Cast. My statement about leaving unnecessary parentheses only applies to the latter. For Extract..., we should: a) Remove 1 pair of unnecessary parentheses around the replacing name / method invocation (because those were only there to group the extracted code). b) Remove 1 pair of unnecessary parentheses around the extracted expression (because the Extract... operation adds a grouping that replaces the parentheses) > Somewhat similar to these cases > - Extract method, Extract constant, Extract local, Introduce Parameter > ------------------------------------------------------------------------ > package p; > public class A { > public void foo(){ > int x= 1 - (2 + 3); > } > } > ------------------------------------------------------------------------ > Do we remove the parentheses when selection is ‘2 + 3’? > Do we remove the parentheses when selection is ‘(2 + 3)’? > I think it is ok to remove in this case Agree. > - What if there is an extra parentheses > ------------------------------------------------------------------------ > package p; > public class A { > public void foo(){ > int x= 1 - ((2 + 3)); > } > } > ------------------------------------------------------------------------ > Do we remove the parentheses when selection is ‘2 + 3’? => int extracted= 2 + 3; => int x= 1 - (extracted); > Do we remove the parentheses when selection is ‘(2 + 3)’? => int extracted= 2 + 3; => int x= 1 - extracted; > Do we remove the parentheses when selection is ‘((2 + 3))’? => int extracted= (2 + 3); => int x= 1 - extracted; > We can probably remove 1 pair of parentheses in these cases. Yes, I would remove 1 pair in the extracted expression and 1 pair in the source. Created attachment 189172 [details] fix + tests v1.0.5 Fixed cases mentioned in comment 15. Markus, good to go ? (I think if you are happy with NecessaryParenthesesChecker.java then we can commit this patch, and fix remaining cases, if any, in separate bugs.) Created attachment 189197 [details]
fix + tests v1.0.6
sigh... To test the patch I used it to remove unused parentheses in o.e.jdt.ui.
The patch removed the parentheses in the following leading to a compilation error. This is fixed in this patch.
String s= "some string" + (1 - 2)
(I will also run our JUnits after removing all unused parentheses to test if the patch causes any semantic changes.)
(In reply to comment #18) > (I will also run our JUnits after removing all unused parentheses to test if > the patch causes any semantic changes.) All tests pass, patch is good! Yipee, a few nitpicks and a GO at the end! Please move NecessaryParenthesesChecker to org.eclipse.jdt.internal.corext.dom, since it is of general interest. In NecessaryParenthesesChecker#needsParentheses(..), the special check for "parentExpression instanceof ParenthesizedExpression" should be moved into locationNeedsParentheses(..). An inline method test that uses non-integral types would be interesting, e.g. TestPlusPlus with all int -> double and <NumberLiteral> -> <NumberLiteral>.1 The TODO in AccessAnalyzer should be clarified to tell that the new "exp" node doesn't have bindings (same in GetterSetterUtil). I first thought the "missing bindings" problem was about cases where the refactored code has compile errors, but this is actually only an implementation problem. The problem can be solved by passing additional information to NecessaryParenthesesChecker#needsParentheses(..). The additional information is only needed if the parentNode is a new InfixExpression node (not from a resolved AST), and it would be enough to just pass - the operator and - the type of the other operands to needParentheses(..), or null if not all operands have the same type. You'd have to refactor needsParenthesesInInfixExpression(..) a bit and probably turn isAllOperandsHaveSameType(..) into getCommonOperandsType(..). But it's OK with me if you release this patch now and tackle the TODO later (doesn't have to be for M6). Most callers of NecessaryParenthesesChecker#needsParentheses(..) won't need to be touched, but special cases like AccessAnalyzer will have to call a new method (which will share most of the implementation with the current needsParentheses(..)). Created attachment 189391 [details] final fix + tests (In reply to comment #20) > An inline method test that uses non-integral types would be interesting, e.g. > TestPlusPlus with all int -> double and <NumberLiteral> -> <NumberLiteral>.1 Added a test as it does not hurt, but floating point addition is also covered in CleanUpTest.testRemoveParenthesesBug335173_19(). > The TODO in AccessAnalyzer should be clarified to tell that the new "exp" node > doesn't have bindings (same in GetterSetterUtil). I first thought the "missing > bindings" problem was about cases where the refactored code has compile errors, > but this is actually only an implementation problem. Filed bug 337680 for this. Committed the patch to HEAD. . Verified on Mac with I20110307-2110 |