Community
Participate
Working Groups
See the TODOs in AccessAnalyzer and GetterSetterUtil and bug 335173 comment 20. Copying part of bug 335173 comment 20 for easy reference > 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(..). > > 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(..)).
*** Bug 386028 has been marked as a duplicate of this bug. ***
The issue in this bug introduces extra parentheses in a compound assignment case where the rhs is an infix expression, while replacing the field with getter/setter. Example: field+= 1 + 2; => setField(getField() + (1 + 2)); However, on save the extra parentheses are removed if the save action "Use parentheses in expressions: Only if necessary" is enabled. (In reply to comment #0) > > 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. In order to pass the common type of all operands, we also need to resolve type binding of the left operand, the getter, which again is a new node and is created and passed from different locations. Also, isExpressionStringType(..) and isExpressionIntegerType(..) used in NecessaryParenthesesChecker to check if the InfixExpression is associative, need to resolve the type binding of InfixExpression itself, which is a new node and doesn't have bindings. Dani/Markus, please suggest how these can be handled.
> (In reply to comment #0) > > > 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. > > In order to pass the common type of all operands, we also need to resolve > type binding of the left operand, the getter, which again is a new node and > is created and passed from different locations. The new getter indeed doesn't have a binding. But we're implementing the refactoring, so we know the type must be the type of AccessAnalyzer#fFieldBinding, right? > Also, isExpressionStringType(..) and isExpressionIntegerType(..) used in > NecessaryParenthesesChecker to check if the InfixExpression is associative, > need to resolve the type binding of InfixExpression itself, which is a new > node and doesn't have bindings. Again, the binding we need for our analysis doesn't have to come from the passed InfixExpression node. The new API in NecessaryParenthesesChecker can take the InfixExpression's type as an additional parameter. To compute the type, we have to look at the InfixExpression's operands, and maybe apply some knowledge of JLS7 15.18 to handle string conversions and numeric promotions.
Created attachment 230005 [details] Proposed Patch Added a new method NecessaryParenthesesChecker#needsParenthesesForRightOperand(Expression expression, InfixExpression infixExpression, ITypeBinding leftOperandType) for the cases where parent is a new InfixExpression node with no bindings, to be used in a compound assignment case as mentioned in comment #2. It takes 'leftOperandType' as it is also required by needsParenthesesInInfixExpression(..). The 'expression' node must have bindings as it will be used to get the type of the right operand. Markus, can the 'expression' node be an unparented node and still have bindings? We need to mention if the 'expression' node can be unparented in the javadoc comment. Extracted the common code to be shared by needsParentheses(..) and needsParenthesesForRightOperand(..) to a private method needsParentheses(..). Updated needsParenthesesInInfixExpression(..) to check if parent node has bindings and accordingly obtain leftOperandType, rightOperandType, parentInfixExprType and isAllOperandsHaveSameType. Added method getInfixExpressionType(..) to return the type of infix expression based on its operands and operator. If operands are of same type, that type is returned. If operator is '+' and either operand is String, the String type is returned. If the left and right operand types are different after that, we assume that parentheses are needed. Cases with other operators are covered by using OperatorPrecedence helper, hence not included here. Also, updated the tests for the changes. Markus, please have a look and let me know the review comments so that I can update the patch accordingly.
> Markus, can the 'expression' node be an unparented node and still have bindings? No, an unparented node cannot have bindings, since only the ASTParser can attach bindings to nodes. The patch generally looks good, but please apply these changes before releasing: 1.) needsParenthesesForRightOperand: - needs another restriction in the Javadoc: the infixExpression must not have additional operands - parameter 'expression' should be called 'rightOperand' 2.) needsParentheses(.., boolean hasBindings) doesn't need the boolean. 'leftOperandType != null' is equivalent to 'hasBindings'
Thanks Markus. (In reply to comment #5) > 1.) needsParenthesesForRightOperand: > - needs another restriction in the Javadoc: the infixExpression must not > have additional operands > - parameter 'expression' should be called 'rightOperand' Done. > 'leftOperandType != null' is equivalent to 'hasBindings' Updated. 'leftOperandType == null' is equivalent to 'hasBindings' Committed with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=82e7a99c8429022aaab5edd6cdaa0b55d97f74df
.
