Community
Participate
Working Groups
The JSR-269 spec, very unfortunately, requires certain information to be available to annotation processors even when the code being compiled contains semantic errors such as missing types. Here's the relevant text from the javax.lang.model.element package javadoc: "During annotation processing, operating on incomplete or erroneous programs is necessary; however, there are fewer guarantees about the nature of the resulting model. If the source code is not syntactically well-formed, a model may or may not be provided as a quality of implementation issue. If a program is syntactically valid but erroneous in some other fashion, the returned model must have no less information than if all the method bodies in the program were replaced by "throw new RuntimeException();". If a program refers to a missing type XYZ, the returned model must contain no less information than if the declaration of type XYZ were assumed to be "class XYZ {}", "interface XYZ {}", "enum XYZ {}", or "@interface XYZ {}". If a program refers to a missing type XYZ<K1, ... ,Kn>, the returned model must contain no less information than if the declaration of XYZ were assumed to be "class XYZ<T1, ... ,Tn> {}" or "interface XYZ<T1, ... ,Tn> {}" Note that this spec is ambiguous, is probably not perfectly implementable even in principle, and does not provide any real benefit to users; however, it is unlikely that we can get Sun to substantially change it. The 6.0 javac compiler does implement the described functionality. To implement this in Eclipse, I think we need annotation bindings (of some sort) to be available in the compiler even when the annotated types are unresolved. I don't know if this is possible or practical, but this bug report can serve as a discussion forum. I have added a test case, org.eclipse.jdt.compiler.apt.tests.NegativeTests, that demonstrates the problem. The test case currently fails (and therefore is not part of the TestAll suite).
Kent asked in email: > So what do these missing annotation type mirrors know? > If its nothing, then we could likely answer an empty > problem type & have similar behaviour. The "spec" (such as it is) only requires that as much information is provided as would be there if the missing types were replaced with empty types. So if the annotation type itself is missing, "nothing" is a fine answer. I have not explored what happens when, e.g., a well-defined annotation is on a missing type: @interface Anno { String value() default "foo"; } public class X { @Anno("spud") MissingType mt; } Here, by my reading of the "spec", we are supposed to provide as much information as if MissingType were defined as class MissingType {}. So in that case, presumably there should be a mirror for @Anno("spud") that knows that it has a value(), knows that value is "spud", and so forth. But I don't know whether that level of detail is important.
We already handle correctly the case where a well-defined annotation is on a missing type. I have updated the test case to include this situation. Note that get this far without failing earlier tests, one must set _testFailingCases to "false" in NegativeModelProc.java and rebuild the processor jar; or do the equivalent in the debugger.
The requirement for this has just been explained to me in more detail and I have to say that it is a bit more compelling than I thought. Basically, there are a lot of processor (e.g. for EJB) that do something like this: class C { @Generate GeneratedType myField; } Where the annotation processor is supposed to see the @Generate annotation and produce a definition of GeneratedType. But if GeneratedType does not yet exist and myField is therefore erased from the definition of C, then there is nothing for the annotation to live on, and if the annotation disappears then GeneratedType is not ever produced. Catch-22.
Comment #3 is more compelling. A question though: is the processor supposed to be able to know the name and package of "GeneratedType" somehow in this case, or does it just need to know that there exists a field with that annotation on it? A couple corner cases off the top of my head: @Generate foo.Bar myField; @Generate GeneratedType myField1, myField2;
Good point. We certainly can't determine the qualified name of a missing type. (The "spec" is mute on this point...) Whether having the simple name is valuable or required is not clear to me; I'll report back.
javac represents the missing types as ErrorType, so there is no way to recover its simple or qualified name. I've updated the test case to verify that the model includes the field with the missing type, rather than erasing it as at present.
Either the annotation type or the type of the annotated element, or both, may be unresolved. All these items do show up when we visit the compiler internal AST to search for annotations (in AnnotationDiscoveryVisitor); they disappear later on when we inspect bindings. In the case of an annotation of unknown type on an field of known type, when we call FieldBinding.getAnnotations(), the annotation holder contains NO_ANNOTATIONS. In the case of an annotation on a field of unknown type, the containing SourceTypeBinding does not even have an entry for the field in its "fields" array.
See also related Bug 198085.
Tentatively targeting 3.4. Need to double check whether this is mandated strictly for 3.4, or could be deferred to later. The underlying compiler changes are far from trivial.
It appears this is mandated for 3.4. Also, I believe the binary scenarii are already covered (need APT to confirm this); i.e. when the missing type is being referenced from binaries. The gap remains in sources, when some missing types are detected and referenced from sources, all affected signatures are simply discarded (no binding when it would contain a reference to a missing type).
Created attachment 83553 [details] Changes in progress
Created attachment 83825 [details] More advanced patch Passes NegativeTest (slightly tuned). TypeReference will always answer the closestMatch, but record the problem reference binding, so it can be checked when accessing members down the road.
Created attachment 83870 [details] More advanced patch v3
Created attachment 84003 [details] More advanced patch v4
Patch 4 seems promising in my testing. I found a case where it still fails compared to javac: in package a, declare public annotations @MarkerA and @MarkerB. Then: Test.java: package b; import a.MarkerA; // but not MarkerB class Test { @MarkerA @MarkerB void foo() {} } When inspecting annotations on foo(), we detect MarkerA but we do not report the existence of MarkerB. Philippe, do you want me to report bugs in this patch within this Bugzilla entry, or start new Bugzilla entries for each one?
I've updated jdt.core.compiler.apt.tests.NegativeTests to cover the case mentioned in Comment #14. The updated test passes with javac but fails with Eclipse.
In my last comment, I meant to refer to comment 15, not 14, of course. I have added more failing examples to the NegativeTests suite. Since there are a lot of failures, I suggest setting NegativeModelProc._reportFailingCases to "false" in the debugger, so as to be able to step through to the case of interest. Note that if NegativeModelProc is edited, it is necessary to recreate the processor jar file with apttestprocessors.jardesc, for the change to take effect.
Providing new testcases is the way to go, thanks Walter. No need to have separate bugs, until this patch is finalized and released. I will soon attach a better version of the patch. I would now only recover missing and non visible types (as opposed to everything, which led to some nasty inconsistencies). Would you be able to check APT's expectations ? What about the case where some class would extend a type variable (forbidden) ? class X<T> { class Member extends T {} } Case where a static method would return a type variable which it cannot see ? class X<T> { static T foo() { return null; } } In both cases, I wouldn't be resilient anymore.
Created attachment 84439 [details] More advanced patch v5
Martin ? Question about recovered bindings in the DOM. Since 3.2.2, when we introduced compiler recovery for missing binary types, the DOM surfaced more bindings than in the past (typically a #foo(Zork) method became available when talking to a binary type). The purpose of this present bug is to add more resilience for source bindings. When enabling it, I am seeing cases where the DOM tests are checking absence of any recovered bindings in sources until the ASTParser explicitly requests binding recovery. Should we preserve this behavior ? Or rather like we did near 3.2.2 when offering more recovered bindings in binaries, simply surface more bindings per default (incidentally recovered binary bindings didn't even think they were recovered if you asked them). My reading of the DOM spec is that in presence of errors, some bindings may be missing, but this doesn't mean they MUST be missing. Am I interpreting it the way you expect it to behave ? The ASTParser recovery mode would still be useful in some advanced cases where the compiler is not yet recovering for free... Thoughts ?
It is also my understanding that in case of error its unspecified which bindings show up and which not. The AST parser is free to decide this and users shouldn't make any assumptions here. The only thing the user can rely on is that the returned bindings are complete, consistent and they exist. Examples for this are the guessed bindings for method invocations and for variables that aren't actually visible. For 3.2 the 'allow binding recovery' flag got added to the AST parser. With this one enabled, you would additionally get bindings of non existing types. Like in private void foo() { A myField; } you would get a field binding that contains a pseudo type A, located in the same package, without any members and no corresponding Java element. Such bindings are marked as 'recovered' We need this flag for example in 'Organize Imports' where we never want to add an import to a pseudo type. IMO, it would make sense to keep this distinction. That means - the 'recovered binding' flag should only be used for pseudo types (or method/field in the future). Such 'recovered' bindings should only appear when 'allow binding recovery' is enabled. - adding or guessing bindings for elements with errors is fine to do. The 'recovered' is not set for those. Does that make sense? Note that the shared AST for the active Java editor is always one with recovered bindings. That's the one reconcile participants get and that, since 3.4, clients can request with the new API org.eclipse.jdt.ui.SharedASTProvider.getAST(...)
Created attachment 84626 [details] More advanced patch v6 Martin - can you give it a try with JDT/UI tests ? It should convert compiler bindings if present (no need to enable special recovery for it), hence more bindings by default (similar to what occurred when introducing recovery for missing binary bindings). Also, method/field holding onto missing types are not tagged as recovered themselves. Also fixed missing binary types to properly be tagged as recovered. Also contains a fix for bug 212034
Released some regression tests to assert handling of missing packages for qualified names (ProblemTypeAndMethodTest#test016-023)
Created attachment 85731 [details] More advanced patch v7 Should better deal with DOM types and secondary errors. Still couple issues with incremental builder, and need to investigate APT NegativeTests. It will reconstruct missing types from qualified names now.
Created attachment 87237 [details] More advanced patch v8 This new patch addresses all issues in the test suites (~700) except 2 in the incremental builder. It strictly aligns missing types in sources on binaries, and thus exhibit suboptimal behavior when doing incremental development. e.g. first define: class Y extends Zork {} // Zork missing then incrementally define: class X extends Y It will flag X as indirectly referring to missing Zork through binaries, and abort the build. This is the behavior we have for inconsistent binaries, which are mostly immutable (i.e. one can safely assume a build path inconsistency). For source dev, this behavior is most annoying. Need to take the patch to the next level, by making the source scenario handle missing types in a different way. I am thinking of actually generating classfiles for the missing types, so that it would not look as inconsistent. My current thinking is that a missing type would be generated as a synthetic member type, so that we could compile and run them (like problem types).
APT tests Negative4 and Negative5 are still failing with this patch. Negative4 tests the ability to recover methods such as class A { Zork rawZork() { return null; } Zork<String> zorkOfString() { return null; } } where Zork is undefined. With this patch, we successfully recover rawZork(), but we omit zorkOfString() from the list of methods in A. The JSR269 spec requires "If a program refers to a missing type XYZ<K1, ... ,Kn>, the returned model must contain no less information than if the declaration of XYZ were assumed to be 'class XYZ<T1, ... ,Tn> {}' or 'interface XYZ<T1, ... ,Tn> {}'." I think that means we need to be able to synthesize genericized error types, ie Zork<T1>. Negative5 tests the ability to recover unresolved superinterfaces and superclasses. With this patch we are succeeding on raw interfaces but failing on generic interfaces: e.g., in class C { class C1 implements M1 { } class C2 implements M2<M3>, M4<M5> { } } we are able to recover the superinterfaces of C1 but not C2. This does not satisfy the spec, but I have noted that javac also fails to recover unresolved superinterfaces. Similarly, we are able to recover the superclass of a class if it is raw, but for unresolved generic superclasses we recover Object. Would it be helpful for me to break Negative5 into a dozen or so individual test cases?
also see bug 159448
Created attachment 88973 [details] More advanced patch v9 Up to date version of v8 (with latest changes from HEAD)
Created attachment 89990 [details] More advanced patch v10 Passes all JDT/Core tests, deals with incremental building using new classfile attribute. New tests: EfficiencyTests#testMissingType001 IncrementalTests#testNewJCL IncrementalTests#testMissingType001-003 ProblemTypeAndMethodTest#test002-033 Many existing tests updated.
Passes the same compiler.apt tests that v9 did. Still fails Negative4, because of failure to find method with Zork<String> return type; and still fails Negative5, likely for reasons internal to APT.
Created attachment 90154 [details] More advanced patch v11 Still should not handle parameterized missing type (sorry Walter, will get there soon). It should better reflect usage of construct with missing type (field, method, constructor); and better account for type mismatch where missing types are involved. Added ProblemTypeAndMethodTest#test034-063 Martin - could you pls check consequences on JDT/UI tests ?
Created attachment 90169 [details] Results and analysis from running JDT UI/refactoring tests on the patch
*** Bug 219573 has been marked as a duplicate of this bug. ***
Re: comment 32 Thanks Martin for your feedback. Basically, I agree with everything but the last point on recovered DOM bindings. I will soon post a revised patch where it should produce less secondary errors. On recovered DOM bindings, to be strictly compatible, we would have to leave in place bindings denoting missing types from binaries (since these got enabled since 3.2.2 and no one ever complained of more bindings). Also, these missing binary types were not even tagged as being recovered. This was quite inconsistent. I would rather produce more bindings by default, and still use an option for the DOM to do an extra effort to produce more bindings than the compiler would normally issue (and it can still produce more bindings over time).
Created attachment 90307 [details] More advanced patch v12 This addresses Martin's comment 32
Actually, I wasn't aware of the bindings for missing types until you mentioned it. I assume the reason why we didn't notice such bindings is as they are quite rare. Maybe also because they probably have more information, like a correct qualified name, so they behave almost like existing bindings. However, it's quite easy to detect the new bindings. And, the recovered types are really just a simple name. So the chance is high that tooling not prepared for recovered bindings will find such types and process them as if they were normal bindings. Note that the AST we use for quick fix and for all tooling in the active editor is already an AST with recovered bindings enabled. So most of our tooling will automatically profit from the new functionality. As we are very cautious to not break any existing clients, I would suggest to follow the 'recovered bindings' settings for the 'new' recovered bindings. No need to change anything with missing binary types.
Missing binary types should still be flagged as recovered, if not how can you explain the absence of members ? As for missing source types, I hear you, but given we have no usecase demonstrating our new behavior would not actually be a feature, and this again is lots of work to fix up the DOM binding to no longer mirror compiler bindings; and also considering the spec which doesn't preclude bindings in broken code... I would rather wait and see until someone objects to this evolution (since no one objected to the binary evolution).
Also missing binary types are very weak, they don't know their modifiers or their kind (class/interface/enum/annotation type) etc... except for the qualified name, they are fairly useless.
I verified that the latest patch fixes the test failures from duplicated errors.
Created attachment 90374 [details] More advanced patch v13 This patch addresses some possible side-effects caused by earlier errors on subsequent resolutions. Added ProblemTypeAndMethodTest#test064-071
Released patch v13 to HEAD for integration
Created attachment 90499 [details] Extra patch for parameterized v01 Simple recovery for parameterized single type reference, Zork<String> --> Zork
Created attachment 90846 [details] Extra patch for parameterized v02 Better patch for recovering parameterized types (able to recover: Zork<String>).
Released for 3.4M6. Will close this bug since all original requirements are now supported. Subsequent issues should be filled as separate bugs. Fixed
*** Bug 198085 has been marked as a duplicate of this bug. ***
The WTP Dali project had a few test failures after moving to the eclipse build eclipse-SDK-I20080305-1100. These were testing the functionality of fields/methods with unresolving types, so we will just be modifying the tests to match the new functionality. Our tooling can now handle things in the source code that we couldn't handle before. I don't think this warrants a new bug, I just have a few questions about the changes and wanted you to know that this did have an affect on adopters(though that affect appears to be positive for us) We have code in several places that checks for null type bindings. Are there ever cases now where org.eclipse.jdt.core.dom.Type.resolveBinding() or org.eclipse.jdt.core.dom.Expression.resolveTypeBinding() would return null? I am wondering if this is an api breaking change since the javadocs state: '@return the type binding, or <code>null</code> if the binding cannot be resolved.' Or maybe the definition of 'resolved' is just ambiguous, thus is covered by the new MissingTypeBinding functionality. Mainly wondering if we can remove our null checks and if those javadocs need to be updated. A little more info, we parse the ast with resolveBindings=true and bindingsRecovery=false. Since we have bindingsRecovery set to false, should we even be getting a type binding for a field who's type does not resolve? These type bindings return true for isRecovered(). I must admit I do not fully understand bindingsRecovery :) Hopefully you can shed some light on this for me.
Changed the target milestone since comment #44 says "Released for 3.4M6"
As mentioned in previous comments (comment 36) I also think we should be more conservative here and hide the recovered bindings unless 'recovered bindings' is enabled. I think comment 46 and our own experiences in UI and APT show that there is existing code that doesn't expect recovered bindings to appear. I filed bug 222735 to track the issue.
Verified for 3.4M6 using I20080324-1300.
