Summary: | Refactor - expression detection incorrect | ||
---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Peter Burka <peter_burka> |
Component: | Core | Assignee: | Jerome Lanneluc <jerome_lanneluc> |
Status: | VERIFIED FIXED | QA Contact: | |
Severity: | normal | ||
Priority: | P3 | CC: | akiezun, jerome_lanneluc, Olivier_Thomann |
Version: | 2.0 | ||
Target Milestone: | 2.0 F4 | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Whiteboard: |
Description
Peter Burka
2002-06-17 17:38:33 EDT
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. |