Community
Participate
Working Groups
Build F3 Enter the following class into a Java editor: 1: public class Foo { 2: public Object foo() { 3: if ( ((Object)null).toString() == null ) { 4: return (Object)null; 5: } 6: return null; 7: } 8: } If you select "(Object)null" on line 3, and choose 'Refactor'->'Extract Local Variable', the action complains that "An expression must be selected to activate this refactoring. If you select the same expression on line 4, the refactoring is allowed. If you select "((Object)null)" on line 3, the refactoring is alos allowed. All three of these are identical expressions, and should all be permitted.
Moving to JDT/UI as they own refactor.
AST problem the ifStatement.toString is as follows if ((Object)null.toString()==null) {return (Object)null;} note the missing brackets around (Object)null in the infix (==) expression the brackets are lost somewhere during AST creation so the code that finds the selected expression gets confused
Adam, why do you need the toString() to be correct? Don't you just need correct positions?
toString just ilustrates the bug i do not need it to be correct (it would not hurt but it's P5 for me) the bug is that a ParenthesizedExpression node is missing around the CastExpression node
I will investigate.
i must've gotten tootoo much sun on my head today: quite naturally, once the ParenthesizedExpression is there, there should be no toString problem.
The problem is in ASTConverter#convert (org.eclipse.jdt.internal.compiler.ast.Expression expression). 'checkForParenthesis' must be done before convert the class cast expression.
This is not a good fix. Now code like: void foo() { char x; x = (char)3; } doesn't work anymore, because the (char) is converted to a parenthesis expression. But you are right, the fix is in this area. I think we need to check if we have a parenthesis AND if the expression is not a cast expression. I will investigate a fix and I post it very soon.
The right fix is to change two things: 1) Instead of: if (expression instanceof org.eclipse.jdt.internal.compiler.ast.CastExpression) { return convert((org.eclipse.jdt.internal.compiler.ast.CastExpression) expression); } if (checkForParenthesis(expression)) { return convertToParenthesizedExpression(expression); } it should be: if (checkForParenthesis(expression)) { return convertToParenthesizedExpression(expression); } if (expression instanceof org.eclipse.jdt.internal.compiler.ast.CastExpression) { return convert((org.eclipse.jdt.internal.compiler.ast.CastExpression) expression); } You were right for that, David :-). But in the checkForParenthesis(...) method we should replace: return (scanner.currentPosition >= end) && (rightParentCount == leftParentCount); with: return (scanner.currentPosition > end) && (rightParentCount == leftParentCount); >= instead of > makes a big difference. This method is supposed to check if an expression is surrounded with parenthesis. So: ((char) 3) is fine but: (char) 3 is not. If you keep the >= both will say they are parenthesis expressions. An expression is a parenthesis expression iff: - there is the same number of opening and closing parenthesis - the first token is a opening parenthesis - the last token is a closing parenthesis. The > is checking the last condition. The last token means that the scanner.currentPosition is beyond the end of the source range. Then the next token would be an EOF. I can release this fix if necessary. It is trivial. All regression tests are green.
DOM AST is wrong, we should fix it for F4, all the more as it breaks refactoring.
Olivier - shouldn't rather the AstConverter#checkForParenthesis rather check that the last token read before EOF was a closing parenthesis, no matter about its position. Your fix is probably ok, but you rely on the fact that the scanner will jump out of bounds when reaching EOF, which is more vulnerable IMO. Something like: private boolean checkForParenthesis (org.eclipse.jdt.internal.compiler.ast.Expression expression) { /* * We need to handle multiple parenthesis */ int start = expression.sourceStart; int end = expression.sourceEnd; scanner.resetTo(start, end); int dangling = 0, token; boolean first = true; try { while (true) { token = scanner.getNextToken(); switch (token) { case Scanner.TokenNameLPAREN : dangling ++; break; case Scanner.TokenNameRPAREN : if (first) return false; dangling --; break; case Scanner.TokenNameEOF : if (first) return false; return dangling == 0; default : if (first) return false; if (dangling == 0) return false; } first = false; } } catch (InvalidInputException e){ } return false; }
If you prefer your code, fine. You can release it.
I ran the regression tests with this new version and everything is green.
Adam, we will get a patch from philippe so that we can run our refactoring test suite to get confidence in the fix.
From Adam, regarding to refactoring test suites: our tests run smoothly i activated the prevously disabled tests for 20693 Finding references to variables does not find all occurances 20520 Refactor - expression detection incorrect
Fix got approved, releasing into 20020621
Verified.
