Community
Participate
Working Groups
Steps to reproduce: 1. Invoke "Introduce Indirection" on 'g' method in code below 2. ClassCastException is throws (see part of the log below) class IntroduceIndirectionBug3<T extends IntroduceIndirectionBug3<T>> { // Invoke "Introduce Indirection" on 'g' void g(T t) { } void f(T t) { t.g(null); } } (Thanks to Yilong Li for helping with the bug report.) java.lang.ClassCastException: org.eclipse.jdt.internal.core.TypeParameter cannot be cast to org.eclipse.jdt.core.IType at org.eclipse.jdt.internal.corext.refactoring.code.IntroduceIndirectionRefactoring.checkFinalConditions(IntroduceIndirectionRefactoring.java:545) at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:85) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:121) at org.eclipse.ltk.core.refactoring.PerformChangeOperation.run(PerformChangeOperation.java:209) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2344) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:87) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121)
Issue is reproducible using Build id: I20130115-1300. The refactoring throws ClassCastException.
Created attachment 236095 [details] proposed patch This contribution complies with http://www.eclipse.org/legal/CoO.php
Noopur, please review whenever you have some spare time during your Java 8 work.
Comment on attachment 236095 [details] proposed patch With the patch, the refactoring results in compilation errors if "Redirect all method invocations" is checked. Please note that in the given example, different methods are created depending on the checked state of the above check box, which is wrong.
Hello Noopur, I am a little over my head with this one. Is there an easy way to convert a org.eclipse.jdt.core.dom.TypeBinding into a org.eclipse.jdt.core.dom.TypeParameter. In the example in the bug report the TypeBinding is <T extends Test<T>> I found some code that attempted to do that in IntroduceFactoryRefactoring.copyTypeParameters() but it failed for the example above.
Created attachment 236184 [details] 2nd attempt This contribution complies with http://www.eclipse.org/legal/CoO.php I figured out a different way to fix this bug that doesn't create code that doesn't compile. Please take a look and let me know if it is correct.
Created attachment 236185 [details] 2nd attempt This contribution complies with http://www.eclipse.org/legal/CoO.php for some reason .gitignore files leaked into the patch.
Comment on attachment 236185 [details] 2nd attempt With this patch: - Refactoring: public class Test<T extends Test<T>> { void foo(T t) { } void f(T t) { t.foo(null); // Invoke "Introduce Indirection" on 'foo' } } Adds the new method: public static <T extends Test<T>> void foo(Test<T> test, T t) { test.foo(t); } The type of the first parameter is 'Test<T>', which is fine as #foo is present in 'Test<T>'. But in the original code, the invocation expression has type 'T' (which extends Test<T>). Markus, is the result correct or should it be: public static <T extends Test<T>> void foo(T test, T t) { test.foo(t); } - In the above example, select "foo" in "t.foo(null);" and invoke Introduce Indirection. Uncheck "Redirect all method invocations" and click OK => We get CCE. - Nikolay, you can also try some examples with checked and unchecked state of "Replace all..", selecting method declaration and method invocation, with the selected method as static and non-static, with different target types chosen in refactoring wizard, navigating in the refactoring wizard with 'Preview' and 'Back' buttons etc.
Created attachment 236271 [details] 3rd time's a charm This contribution complies with http://www.eclipse.org/legal/CoO.php Oops... I am not doing too well am I? Sorry about that. Lets try this patch.
Comment on attachment 236271 [details] 3rd time's a charm With this patch, select "foo" in "t.foo(null);" and invoke Introduce Indirection. Uncheck "Redirect all method invocations" and click Preview. Click Back and then click OK. We get the exception: java.lang.IllegalArgumentException: Node is not inside the AST at org.eclipse.jdt.core.dom.rewrite.ASTRewrite.validateIsCorrectAST(ASTRewrite.java:582) at org.eclipse.jdt.core.dom.rewrite.ASTRewrite.createTargetNode(ASTRewrite.java:698) at org.eclipse.jdt.core.dom.rewrite.ASTRewrite.createMoveTarget(ASTRewrite.java:741) at org.eclipse.jdt.internal.corext.refactoring.code.IntroduceIndirectionRefactoring.updateMethodInvocation(IntroduceIndirectionRefactoring.java:989) ... Before the next patch, please make sure you have tested various scenarios (some are already mentioned in comment #8).
Created attachment 236477 [details] Lets try this This contribution complies with http://www.eclipse.org/legal/CoO.php Hello Noopur, This latest problem you found is technically unrelated to the CCE in the original bug. I don't know if it should be fixed as a separate patch. Anyway. I have submitted a patch which addresses both.
(In reply to Nikolay Metchev from comment #11) > This latest problem you found is technically unrelated to the CCE in the > original bug. I don't know if it should be fixed as a separate patch. > Anyway. I have submitted a patch which addresses both. Thanks for the patch Nikolay. The IAE problem is not related to CCE. But since we found it while fixing CCE and there is no other bug for this, let us fix it along with this bug itself. (In reply to Noopur Gupta from comment #10) > select "foo" in "t.foo(null);" and invoke Introduce Indirection. > Uncheck "Redirect all method invocations" and click Preview. > Click Back and then click OK. > We get the exception: > java.lang.IllegalArgumentException: Node is not inside the AST When we click Preview, we remove the cached CU rewrite from the cache in #createChangeAndDiscardRewrite(ICompilationUnit). But fSelectionMethodInvocation still belongs to the discarded CU rewrite's AST. Hence on clicking Back and OK/Preview again, when we need the CU rewrite for fSelectionMethodInvocation, #getCachedCURewrite(ICompilationUnit) creates a new CU rewrite with a new CUNode/AST. And now, the existing AST of fSelectionMethodInvocation is different from the new CU rewrite's AST -> IAE. Either we should not discard the CU rewrite from the cache or, #getCachedCURewrite should use the existing CUNode/AST of fSelectionMethodInvocation while creating the new CU rewrite. Nikolay, added a null check for typeBinding in #getExpressionType(MethodInvocation) and released your patch for CCE with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=b74c4b80110ff14b76d1482504cba63a96d3d309 Released fix for IAE with #getCachedCURewrite using existing AST of fSelectionMethodInvocation as: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ace0a97be4318ccbde62328ddc0b8374bf710d7c
Thanks Noopur, I am still learning... Sorry about the multiple incorrect attempts.
(In reply to Noopur Gupta from comment #12) > Nikolay, added a null check for typeBinding in > #getExpressionType(MethodInvocation) and released your patch for CCE with: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=b74c4b80110ff14b76d1482504cba63a96d3d309 This only works for the original example, where T extends IntroduceIndirectionBug3, but it's not correct in the general case: package snippet; class IntroduceIndirectionBug4<T extends M> { // Invoke "Introduce Indirection" on 'm2.m()' void h(M2 m2) { m2.m(); } void f(T t) { t.m(); } } class M { void m() { } } class M2 extends M { void m() { } } Fixed this with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d6ed4ae23d3c8539db8731b3658c9bf520c513d2 Nikolay, could you please attach another test for this?
Created attachment 236811 [details] unit test This contribution complies with http://www.eclipse.org/legal/CoO.php Here is a unit test for the test case you have presented. Looking at your code it made me come up with more test cases. This one for example now complains that the refactoring is not possible. However if you swap Runnable and M it does work.: package p; class Test<T extends Runnable & M> { // Invoke "Introduce Indirection" on 'm2.m()' void h(M2 m2) { m2.m(); } void f(T t) { t.m(); } } interface M { void m(); } interface M2 extends M { void m(); }
Created attachment 236846 [details] more unit tests + more generic fix This contribution complies with http://www.eclipse.org/legal/CoO.php I figured out a more generic fix.
(In reply to Nikolay Metchev from comment #15) > class Test<T extends Runnable & M> { Yeah, that's the point where we usually give up. To resolve this, we would have to re-write too much of the implementation. And we would complicate the code for a use case that is not expected to occur frequently enough to justify the cost. And luckily, we even detect the problem before we produce compile errors.
(In reply to Nikolay Metchev from comment #16) > Created attachment 236846 [details] [diff] > more unit tests + more generic fix The patch looks fine though it traverses the complete hierarchy of each type bound until it finds the method, but that is required and would happen for very rare use cases. Released the fix and tests with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=36efc3dc84f50d4a0a35ccc6e62305532fd82c78
