Community
Participate
Working Groups
Build Identifier: 20110916-0149 When using the "Remove unnecessary casts" clean up options the following snippet is incorrectly changed. // Original public static Object create(final int a, final int b) { return (Double) ((double) (a * Math.pow(10, -b))); } // After Cleanup public static Object create(final int a, final int b) { return ((double) ); } Reproducible: Always Steps to Reproduce: Create class with following snippet; public static Object create(final int a, final int b) { return (Double) ((double) (a * Math.pow(10, -b))); } Run Source -> Clean Up (ensure "Remove unnecessary casts" is part of the operation. Code is changed to; public static Object create(final int a, final int b) { return ((double) ); }
This used to work but got broken during 3.7, most likely because of bad fix for bug 335173. Deepak, please take a look.
*** Bug 371597 has been marked as a duplicate of this bug. ***
Created attachment 211133 [details] proposed fix + test (In reply to comment #1) > This used to work but got broken during 3.7, most likely because of bad fix for > bug 335173. Fix for bug 335173 did cause the regression, but the fix was not necessarily bad in the strict sense... The problem was that things go wrong when a node is move via a call to ASTRewrite.createMoveTarget(ASTNode) and a subsequent replace, and then later used as a replace target in ASTRewrite.replace(ASTNode, ASTNode, TextEditGroup). i.e. NodeA is replaced by NodeB NodeB is then replaced by NodeC I am guessing that this is a known limitation of moving nodes via ASTRewrite.createMoveTarget(ASTNode), but I am not 100% sure. In the attached patch I avoid the above situation by directly replacing NodeA by NodeC. But maybe the ASTRewrite infrastructure should handle this ? Markus, thoughts ?
> I am guessing that this is a known limitation of moving nodes via > ASTRewrite.createMoveTarget(ASTNode). Yes, I've clarified the Javadoc. ASTRewrite#createMoveTarget(..) indeed only works with complete moves where the original node is removed/replaced and the resulting node really gets inserted into the AST. The "if (astNodes[1] == toReplace)" only covers one branch -- that looks suspicious. In fact, your patch only works sometimes, depending on the order of the casts in fUnnecessaryCasts (a HashSet with unpredictable order). But instead of adding an "if (astNodes[0] == expression)" branch, we better avoid the whole delaying through nodesToRewrite. The loops that find the "top" and "down" nodes already tried to collapse multiple moves, but this broke down with the fix for bug 335173. I now rewrote the loops to process both nested CastExpressions and nested ParenthesizedExpression together in one sweep. That also has the advantage that parentheses removal works reliably for multiple parentheses, see CleanUpTest#testUnusedCodeBug335173_2(). Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=79ac82614e6dc71d8a7fcf802eee07098026adb8
Thanks Markus! > I now rewrote the loops to process both nested CastExpressions and nested > ParenthesizedExpression together in one sweep. That also has the advantage that > parentheses removal works reliably for multiple parentheses, see > CleanUpTest#testUnusedCodeBug335173_2(). Initially I was trying to do the same, but then I stopped because of bug 335173 comment 6. More specifically because of the following agreement we had there.. > we should remove parentheses if only if they were necessary > for correctness in the original code. In testUnusedCodeBug371078_2() we do remove more parenthesis than required. By the above logic the result there should be "Object o= ((i));". Also, do we really need the if condition on line 494 if (!NecessaryParenthesesChecker.needsParentheses(castExpression, downChild.getParent(), downChild.getLocationInParent())) { ... doesn't it always evaluate to true ?
> > we should remove parentheses if only if they were necessary > > for correctness in the original code. Good catch, thanks. This should fix all known problems and a few more cases where we removed parentheses that were not strictly due to the cast: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d408bbe5619cd56c3f9fd541c8f62a13090ac9d4
Thanks! Looks good now.
Verified in I20120312-1800.
