Community
Participate
Working Groups
Build ID: I20070323-1616 Steps To Reproduce: Hi, I've got a curious buggy behaviour of 'Organize Imports' in one of my source files, using Eclipse 3.3M6. I'm extending a class that has protected static final String fields in its superclass, and I'm using these fields for some comparisons in my subclass. Now since they are in the superclass and are protected, they are visible and all's fine. However, each time I do 'Organize Imports', they are imported with an import static statement, which results in a compile error. I have tried to thin the code out a little, so that the example is smaller, but everytime I took out any code, the problem dissappeard. So in order to make it reproducible, I have packaged the entire project (just a few classes anyway). To reproduce: - import the attached project - ignore compile errors because of missing referenced plugins - open the type NewHaskellContributionWP - Ctrl-Shift-O - two new import statements appear on the top of the import list: import static org.eclipse.jdt.ui.wizards.NewTypeWizardPage.PACKAGE; import static org.eclipse.jdt.ui.wizards.NewTypeWizardPage.TYPENAME; (This happens also when the missing other plugins are available, and there are no other compile errors than those two.)
Created attachment 65330 [details] zipped workspace project to reproduce
Created attachment 65373 [details] ast view Looking at the class in the ASTView, it can be seen that the binding of the superclass ('NewTypeWizardPage') is not complete. It has no fields or methods. The compiler, however can find the fields 'TYPENAME' and 'PACKAGE'.
The type itself has no declared fields or methods, but if you go in: @Override protected void handleFieldChanged( final String fieldName ) { super.handleFieldChanged( fieldName ); if( TYPENAME.equals( fieldName ) ) { info.setName( getTypeName() ); } else if( PACKAGE.equals( fieldName ) ) { info.setPackageName( getPackageText() ); } doStatusUpdate(); } and you look at the binding for TYPENAME and PACKAGE, they are there. If you add org.eclipse.jdt.core in the list of required plugins, then the organize import is fixed. I don't see what we can change at the core level to get rid of the errors. We don't have a way to report that the resolution of the fields or methods failed for the superclass and therefore it is pointless to try to organize imports. In this case, you need to filter these two imports. Moving back to JDT/UI
How did you find out that 'org.eclipse.jdt.core' is missing in the list of required plugins? I think there's no error at all on files of project that would tell the user that something is missing. That's most important here. Maybe the reference to 'NewTypeWizardPage' should already say something. I also don't understand why 'NewTypeWizardPage' has no bindings at all. Why is there a binding on 'PACKAGE', with declaring type 'NewTypeWizardPage'? With the recovered bindings, wouldn't it be possible to be more complete here? I don't see what we can do in JDT/UI here. We have to rely that we get correct bindings or none or marked as recovered.
There is a difference in resolving all fields or one specific field. When we try to get declared fields on the superclass, it fails since one of the return types of the field cannot be resolved. When we try to resolve the binding for PACKAGE or TYPENAME, we try to resolve a specific field in the superclass and this works so we set a binding. Now, what binding do you want us to return as recovered? The superclass binding ?
Martin, Why do you need all declared fields or all declared methods for the superclass? The user never requested them. The only two fields that are referenced in the superclass can successfully be resolved. So the DOM/AST tree and bindings are reflecting this. In order to resolve all fields or all methods from the superclass, you need to get org.eclipse.jdt.core on the classpath. The user doesn't need to add it since he never used any field or method from the superclass that requires that project to be on the classpath. So why do you need that ?
To evaluate what imports are required, I need to know what references can be found in the scope or not. If they can be found in the scope, no import is needed. This has to be done for all type/field/and method references so that the minimal set of imports can be determined. Our algorithm to find elements in the scope (ScopeAnalyzer) walks through the super type bindings and looks at the declared fields and methods. As a user of bindings and the AST I most expect to get consistent results. If there's a filed that has a declaring type, this filed also needs to show up as field in the declaring type. Only exception would be if it is marked as recovered. Second, but less important is that that the bindings should be as complete as possible.
We could add a call that would return all declared fields and methods that could be resolved instead of returning a empty list in both cases. The problem is that you are doing operations that require the classpath to contain more types than what the user is doing. So the user doesn't need to add org.eclipse.jdt.core plugin on the classpath since he never explicitely refers to any of the types in this plugin. IF the user would try to refer to a method or a field that has a type inside org.eclipse.jdt.core, he would get compile errors. I could get an example for fields since all "visible" fields don't have a type inside org.eclipse.jdt.core. But if you add a call to setPackageFragment(...), then you would end up with such a compile error: The type org.eclipse.jdt.core.IPackageFragment cannot be resolved. It is indirectly referenced from required .class files de.leiffrenzel.cohatoe.pde.ui/INTERNAL/java/src/de/leiffrenzel/cohatoe/pde/ui/internal/wizard NewHaskellContributionWP.java This is not a new problem. It has always been like this. If we add a new method on binary type binding that would return all the methods and fields that can be resolved without adding more types to the classpath, we would get "consistent" results since the getDeclaredFields() and getDeclaredMethods() would at least contain the fields and methods that the user can referenced without compile errors, but this would not solved the problem of reporting to the user that not all declared fields or all declared methods have been returned. The current APIs don't let us report an exception. This is not a recovered binding since the type binding itself is perfectly fine.
> The problem is that you are doing operations that require the classpath to > contain more types than what the user is doing. So the user doesn't need to add > org.eclipse.jdt.core plugin on the classpath since he never explicitely refers > to any of the types in this plugin. As a side-remark: I think this happens also with other features where the IDE helpfully "knows more than I know myself". When I trigger Code Assist in the example class in some code block, I don't get setPackageFragment(..) in the list. But it's in the list which I get with twice Ctrl-O. My feeling here would be that I rather want these proposals (if they are what I need, then I'll see compile errors, and these will then remind me that I still need to import the missing plugin dependency).
Martin - The root of this problem is your algorithm to compute the imports. You should not be asking for all fields & methods from a supertype. You do not need them. Ask for the ones that you need by name & you will have the same behaviour as the compiler, code assist, etc.
Kent, can you elaborate? - how do I look up an element by name using the public AST API? - how and when would I know that I shouldn't access fields from a ITypeBinding?
(In reply to comment #11) > - how do I look up an element by name using the public AST API? Why would you need to look up an element? Each expression or declaration has already a binding or null. So I don't see what will bring you to get all fields/methods from the superclass. Did I miss something ?
> Why would you need to look up an element? see my comment 7. To determine what static imports are really required, I iterate through all static references in the code. A static import is required when the reference can't be found in the scope (but only through an import) Note thats not the same as the 'unused import' warning tells me. For example I need to find out how many times java.util.Math.* is used. If only once, then Organize imports might decide to change it to java.util.Math.max. Yes, each element has a binding. The question is, how did the compiler find this binding. Through an import or not?
When you collect all bindings from the current unit, you know for each binding: - its declaring class, - its modifiers Then you don't actually need to get the fields or methods from the hierarchy. You only need to know if the binding can be resolved through the hierarchy and a check on the declaring class and the modifiers should be sufficient.
I guess I could because I already have the binding; but the ScopeAnalyzer is more general. It can also resolve element from a given name or give all bindings visible at a given location (like code assist, but with bindings). What about all the other clients that get fields from super types? Do want them to be rewritten too? The binding API make clear promises. It's a bug in the implementation, if the spec is not fulfilled.
Sorry, comment 14 doesn't work, here's an example: (paste it like that to a Java project) package a; public class A { public static int x; } //----- package b; public class B { private static int x; } //----- package a; import static a.A.x; // static import in needed, but 'A' is visible from inside 'C' import b.B; public class C extends B { int u= x; }
should be: package a; public class A { public static int x; } package b; public class B extends a.A{ private static int x; } package a; import static a.A.x; // static import in needed, // but 'A' is visible from inside 'C' public class C extends b.B { int u= x; }
(In reply to comment #15) > The binding API make clear promises. It's a bug in the implementation, if the > spec is not fulfilled. The binding API makes clear promises as long as the classpath to resolve the bindings is ok. Here you want to resolve the supertype within the context of the subclass. Tell me where this is specified to work in the spec. If you would try the same resolution with the compiler, it would not work. There is no magic in the AST world. It works according to the context that you give. In this case the context is missing the project org.eclipse.jdt.core. It has always been the case. Why complaining about it now?
(In reply to comment #16 and comment #17) > Sorry, comment 14 doesn't work, here's an example: I don't see why my comment 14 is wrong. I said that the type hierarchy is enough and the declaring class and the modifiers of the bindings resolved for C are enough. I think this is still true. the binding for 'x' in "int u = x;" has a.A has the declaring class. So I don't see why with this information it is not enough to decide what import you need. How do the fields and the methods from the superclass help you in this case? What did I miss? The ScopeAnalyser assumes that methods or fields from the superclass can be resolved in the context of the subclass. This assumption is wrong. This is also a implementation bug.
In my example you see a binding to A.x. So it seems the static import in not really required as A is a super type of C. But when you follow the lookup order, and by looking at all fields of B, you will find out that x would resolve to a b.x, if it weren't private. So the static import is necessary. This could only be found out by looking at fields of the super class. I know that bindings are limited as soon as there are build path problems. But in this case, how can I know? There's no compile error, everything seems to work as you explained in comment 8. So everything would be fine if there were at least the fields and methods with resolvable types. If it's legal for the compiler, we should also get the bindings (as you said, it's not even the 'recovered' case) What do you mean by 'The ScopeAnalyzer assumes that methods or fields from the superclass can be resolved in the context of the subclass'?
(In reply to comment #20) > I know that bindings are limited as soon as there are build path problems. But > in this case, how can I know? There's no compile error, everything seems to > work as you explained in comment 8. There is no way to report a compile error with the existing API and there is no exception we can return to report the unresolved issue to the user without breaking existing API. > So everything would be fine if there were at least the fields and methods with > resolvable types. If it's legal for the compiler, we should also get the > bindings (as you said, it's not even the 'recovered' case) But this would not solve this issue. The number of fields or methods would not match the actual number of fields or methods. I would simply return the ones that can be resolved. > What do you mean by 'The ScopeAnalyzer assumes that methods or fields from the > superclass can be > resolved in the context of the subclass'? This is exactly what you are doing in the ScopeAnalyser. You try to resolve the fields and methods from the superclass using the context (classpath) of the subclass. There is absolutely no guarantee that this works. You might want to request a new API to retrieve individual fields given a type binding and a name. Same for methods. This would be a new bug report.
> But this would not solve this issue. The number of fields or methods would not > match the actual number of fields or methods. I would simply return the ones > that can be resolved. I think it would solve the issue. The fields 'PACKAGE' and 'TYPENAME' would in the field list. setPackageFragment(..) would not, but it's also not requested (if it was requested there would be a compile error). So we get the same view of a type as the compiler does. > This is exactly what you are doing in the ScopeAnalyser. You try to resolve the > fields and methods from the superclass using the context (classpath) of the > subclass. There is absolutely no guarantee that this works. I don't understand. We are not mixing any classpaths here. All the Scope Analyzer answers: what elements are visible at a given source location. In our special scenario: 'PACKAGE' and 'TYPENAME' are available. setPackageFragment(..) not. But let's take this discussion offline.
This would require new APIs on ITypeBinding in order to be able to get: - a field given its name - a method given its selector. Philippe, Adding these two methods would be the only way to fix this issue cleanly. This could also be useful methods for other cases. I know this is really late for API addition, but we might want to consider these two.
Its not the only way this can be fixed. The algorithm to compute which imports are necessary can be changed into a trial & error one. No new API is needed and it could turn out to be faster. Instead of making sure that only necessary imports are added, why not add an import for every reference outside the class - ignore searching the hierarchy to see if an import is necessary. Then use the unused import markers to remove the ones that are not needed.
The suggestion for comment 24 gets in trouble as soon as there are naming conflicts --- package a; import static java.lang.Math.PI; public class E extends F { double foo() { return PI; } } class F { public static double PI= Math.PI; } class G { double foo() { return PI; } } Here you would end up with two conflicting imports import static java.lang.Math.PI; import static a.F.PI; -- Organize import and Scope Analzser are central piece of functionality for quick fix and refactoring. Used, tested, improved and in daily action for over 3 years and seems to be working with compiling code. Stop blaming the algorithm. It doesn't make any crazy assumptions. It doesn't mix binding environments. It gets an AST with no compile error in a project that compiles as well. It uses one binding environment returned by the compiler and just expects to find the same elements the compiler found. There must be hundert's of other algorithms that do similar things. Let's work on improving the information returned by the AST. Better without new API because you also want to help existing code. I think what Olivier wrote in comment 8 would bring us a big step forward: Add all resolvable members to the type binding. When we find out that this is not good enough, we can also think of having all members, by using 'recovered' types for the unresolvable parts. But lets start with the first idea.
re: comment 25 > "Stop blaming the algorithm... It gets an AST with no compile error in a project that compiles as well." This is the point. It does compile well, and NO error must be reported as per language spec. > "It uses one binding environment returned by the compiler and just expects to find the same elements the compiler found." This is wrong. It triggers more resolution than original user code, which doesn't need/resolve the broken areas from supertype. This is the reason why the algorithm you use got fingerpointed; and also this is the reason why you have zero error reported in the first place. > "There must be hundert's of other algorithms that do similar things." If so, they are wrong. > "Let's work on improving the information returned by the AST. Better without new API because you also want to help existing code." There is no regression in the DOM AST. I am all for more resilience to error issues, but these are going to require significant enhancements which are hard to accomodate with 3.3 endgame. I'd rather defer such enhancements to 3.4. Side-note: as a concession, if field/method accessors in DOM AST are not resilient (in case of badness, could return the subset of resolvable ones). I thought this was already the case, may want to use availableFields/availableMethods underneath instead (and document the latter as not only being used by codeassist). This may improve this specific issue, but I think there is a general problem in algorithm exploiting the DOM AST and causing more resolution than original usercode. To me, the immediate action required is to improve the organize import algorithm to make it more lightweight; and avoid causing more resolution than what original user code needs so there is no inconsistency. I suspect the algorithm at work is dealing with various hiding situations, and maybe eagerly initializing its contexts and therefore triggering more resolution than original user code. > "The suggestion for comment 24 gets in trouble as soon as there are naming conflicts". Wouldn't it simply work if not adding imports for things declared inside the current unit.
Discussing with Martin, this issue should be deferred until 3.4. The real need is for organize import to avoid trying to mimic compiler lookups (and thus cause more resolution than actually needed by compiler). This would require an extra API on DOM reference nodes, to indicate that resolution used some imports to perform (would need to be added on compiler InvocationSite). Once this is added, Martin would adjust the Scope analyzer to avoid causing unncessary resolutions. Also, DOM AST should provide some way for users to realize that some of the lazy requests failed due to resolution issues. Some channel API should be added (or use existing requestor but need to worry about existing clients to it). Independantly, and could be good enough for 3.3, more resilience on some DOM AST apis would help. A separate bug should be entered for the 3.3 resilience work.
I filed bug 185306 as mentioned in the last paragraph of comment 27
Defer as unfortunately we had not enough time to look at this...
