Community
Participate
Working Groups
I-20021016 linux-gtk: public class Woo { static void goo(char c) { System.out.println("char"); } static void goo(int i) { System.out.println("i"); } static int foo() { return 'a'; } public static void main(String[] args) { goo(foo()); } } 1. execute the class 2. observe the output is "int" 3. inline method foo 4. execute the class 5. observe the output is "char" characters are implicitely cast to int on return. When the method is inlined, the implicit cast must be made explicit. If there's no ambiguity the cast could be omitted. The fix should handle all kind of possible implicit conversions. There's a similar case: public class Zoo { static int foo() { return 'A'; } public static void main(String[] args) { System.out.println("output: " + foo()); } } Before inlining, the output is '65', after inlining, it's 'A'. In the past I often got bitten by implicit conversions of characters to int, which is hard to debug.
Created attachment 5197 [details] Patch fixing the bug The patch fixes the problem described in the bug report but there is one issue. If access to overloaded function is restricted then no explicit cast is really needed, in this case I still think it is safer to have cast in place. Example: public class Fruit { } public class Apple extends Fruit { } public class Woo { public static Fruit foo() { return new Apple(); } private static void goo(Apple apple) { } public static void goo(Fruit fruit) { } } public class Test { public static void main(String[] args) { Woo.goo(Woo.foo()); } } Woo.foo() will be inlined as (Fruit)new Apple() though goo(Apple apple) is private in class Woo.
Created attachment 5243 [details] final patch fixing the bug Previous version of the patch created unneeded explicit casts. This one doesn't have such problems.
Created attachment 5244 [details] unit tests for the submitted patch Eclipse 'Create Patch' command didn't include resources in generated patch. Probably, because I put them into separate folder.
Created attachment 5245 [details] Resources for unit tests
I will look at the patch (very likely not before beginning of next week).
Dmitry, nice work ! I released the patch after applying the following changes. - adapted code to JDT/UI guidelines (f prefix for fields and asymmetric assignment) - moved resolveInvokedType to ASTNodes and renamed it to getReceiverTypeBinding since it is a general useful method. - moved code from ASTResolving to new class TypeRules to avoid layer breakers. - shortened needsExplicitCast() a little bit since fInvocation == arguments(i) for 0 < i < number of arguments. No need to loop over it and to compare start offsets. Besides these changes I think the findMethodsInHierarchy method should consider interfaces as well. Currently it only considers super classes. Additionally it is sufficient to collect all methods with the same number of parameters. Method foo(Woo) doesn't overload method foo(Woo, int). Furthermore the method canImplicitlyCall can be optimized. Since we only replace one argument it is sufficient to check this one. Are you interested in implementing these changes/optimizations.
And a question: what is the purpose of // fixes bug #35905 if(expression instanceof CastExpression) { realArguments[i] = "(" + realArguments[i] + ")"; }
Dirk, thank you for the review! I thought about moving resolveInvokedType method to Bindings class and haven't done that only because wanted to limit my changes to as less number of files as possible. If you think ASTNodes serves that purpose better then I agree. Method findMethodsInHierarchy was designed to be more generic than was required for this fix and again that was done to simplify movement of the method into Bindings afterwards. Now, taking into account your comments, I think that following set of methods can be added to ASTNodes class: - findMethodsInHierarchy(ITypeBinding type, String methodName) - it will return all methods with specified name, considering interfaces too - findMethodsInHierarchy(ITypeBinding type, String methodName, int numberOfArguments) - the same as previous but methods are additionally filtered with number of arguments - findMethodsInSuperclasses(ITypeBinding type, String methodName) - this one is equal to current implementation of findMethodsInHierarchy. - findMethodsInSuperclasses(ITypeBinding type, String methodName, int numberOfArguments) - the patch must be modified to use this method. If you approve this change I will implement it. Regarding comment #7. The change got into the patch accidentially and was inteded to fix bug #35905. I'm definitely interested in further work on this bug.
I think we should start with one findMethodInHierarchy. It should take the type, the method name and the number of parameters. If we need another method we can add it later. Have you thought about optimizing "canImplicitlyCall" as well ? As said, I have released your fix to the HEAD so it is part of todays integration build. So I am expecting another patch from you ;-)).
I don't see a possibility how "canImplicitlyCall" can be optimized. Consider following code: class Base {} class Derived extends Base {} class Woo { static void goo(String s, Base b) { System.out.println("String"); } static void goo(Integer i, Derived d) { System.out.println("Integer"); } static Base foo() { return new Derived(); } public static void main(String[] args) { goo("String", foo()); } } If only second parameters of foo() methods are taken into account then it looks like explicit cast is needed, but it is not since the first parameter defines which overloaded method will be invoked. Actually, above example helped me to discover another case when the fix doesn't work. Here it is: class Woo { static void goo(String s) { System.out.println("String"); } static void goo(Integer i) { System.out.println("Integer"); } static Integer foo() { return null; } public static void main(String[] args) { goo(foo()); } } Inlining foo() will break the code. Method TypeRules.canAssign doesn't process null types correctly. I will fix it along with the "findMethodInHierarchy" change.
Created attachment 5298 [details] Code restructuring, fixed null types problem I moved findMethodsInSuperclasses to Bindings because it takes ITypeBinding as first parameter and ASTNodes class manipulates with AST nodes only. Fixed the null types problem described in comment #10. I'm not sure that additional check is neccessary in the following code: int argumentIndex= methodInvocation.arguments().indexOf(fInvocation); if (argumentIndex == -1) return false; // will never be executed Since the compilation unit is compiled without errors, state of bindings is up-to-date and consistent then argumentIndex will never be -1. Am I wrong? When will the status of the bug change to FIXED ? :)
Created attachment 5299 [details] Unit test for patch #5298
Looking at your comment #10 I get the feeling that findMethodInHierarchy should be coded in the following way: IMethod[] findMethodInHierarchy(ITypeBinding type, String methodName, ITypeBinding parameters[]) if (parameters[i] == null) then the parameter is ignored during method comparison. This will find all methods that cause a potential conflict. So instead of opimizing canImplicitlyCall we should optimize findMethodInHierarchy. What do you think ? Regarding the two questions: if (argumentIndex == -1) return false; // will never be executed is not needed. You are correct. When will the bug be closed: we close the bug when all issues enumerated in this bug are solved. IMO we are 95% done. Only little tweaking here and there <g>.
Created attachment 5315 [details] Generalized Bindings.findMethodsInSuperclasses I generalized method Bindings.findMethodsInSuperclasses as you suggested. The change added one more loop traversing method arguments (see Bindings.isEqualMethod) and array allocation when calling to Bindings.findMethodsInSuperclasses from CallInliner. This can decrease performance a little. I also removed "if (argumentIndex == -1)" check and added another one "if(methodInvocation.getExpression() == fTargetNode)" because target node can be a receiver expression in the method invocation (see TestParenthesis unit test). Unit test for the patch is attached to comment #12.
Created attachment 5324 [details] Considered interfaces, created binding predicate Dirk, Sorry for the 'spam' but there is no limit to code improvements :) First of all, I realized that interfaces should be considered too. Following code demonstrates why it is so: class Base {} class Derived extends Base {} interface I { public void foo(Derived d); } abstract class Goo implements I { public void foo(Base b) { } } class Woo extends Goo { public void foo(Derived d) { } } class Test { Base inlineMe() { return new Derived(); } void main() { Goo goo = new Woo(); goo.foo(inlineMe()); } } Actually, we don't need to traverse all superclasses and interfaces searching for ALL methods with the specified name and number of parameters. What we really want is to find ONE(any) method that after inlining can be confused with original invocation. That would save us many loop iterations and probably call to type.getInterfaces(). I also noticed that there is many Bindings.findMethod...s in Bindings class and the number tends to grow. That can make it difficult to find an appropriate method, especially for newbies like me :) Considering above ideas I decided to employ predicate pattern. I created new BindingPredicate interface and added several searching routines based on the predicates to Bindings class. Method CallInliner.needsExlplicitCast was simplified and canImplicitlyCall moved to new AmbiguousMethodPredicate class. What do you think? Is it 100% now? ;)
Will look into it tomorrow morning.
Dmitry, I looked at the code and functional wise I think we are done. However I don't like the additional methods on Bindings since I am not sure if they are really useful (for example we can't remove the existing find* method or even make them wrappers around the new methods). From looking at your new code I get the feeling that we are missing a type hierarchy visitor. The the different find methods would be different visitors and the Bindings class would only contain code to visit the hierarchy. What do you think about this ?
Created attachment 5370 [details] Replaced binding predicate with visitor I modified the patch and replaced predicate with visitor as you suggested. The new BindingVisitor is very simple but in the future it can be extended to the scale of ASTVisitor if needed. Take a look and let me know if it is what you meant.
Released path with two little changes: - renamed BindingVisitor to TypeBindingVisitor - made AmbiguousMethodAnalyzer an inner class of CallInliner (we don't use secondary top level types in Eclipse). Thanks for your contribution !!
Bindings.visitInterfaces(ITypeBinding, TypeBindingVisitor) didn't visit superinterfaces of interfaces recursively. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=7edc5f69024dacad9e60d721905913043dc5a57f
