Community
Participate
Working Groups
Support for annotations in package-info.java is incomplete in the case where the compiler works off a SearchableEnvironment as its name environment. This surfaces in the CompilationUnitProblemFinder seeing fewer problems than the builder. This affects, e.g., quickfixes invoked from the Problems view. I originally observed this with upcoming annotations for bug 331647, but it can be reproduced also with package level @Deprecated annotations. For interpreting @Deprecated annotations in package-info.java the compiler (class PackageBinding) checks for existence of the faked package-info type. During building this type is properly found and annotations are found, too. However, when the name environment is a SearchableEnvironment it uses the JavaModel, finds no such type, and hence throws JME. After reading through some history of this behaviour (bug 258145, bug 99662) I see that not showing this fake type in the model is intended. So the unfortunate event is that the lack of the fake type is fed back into the compiler, since the compiler's analysis depends on seeing package-info.java. I propose to fix this issue by exceptional handling in SearchableEnvironment: Instead of using the inexistent IType we can construct an answer from the existing compilation unit for package-info.java. This immediately makes the compiler happy, but requires special attention in a few more places, because CU-based NameEnvironmentAnswers are unexpected in some situations: (1) The HierarchyResolver throws AbortCompilation when accept()ing an ICompilationUnit. Aborting at this stage can cause NPE later during compile (I've seen missing scopes & bindings). For this I see two simple possible fixes: (a) catch that AbortCompilation within LookupEnvironment.askForType(PackageBinding,char[]) IFF the name is PACKAGE_INFO_NAME, or (b) check for PACKAGE_INFO_NAME within HierarchyResolver.accept(ICompilationUnit, ..) as to avoid throwing the AbortCompilation in the first place. I implemented (a) and for my test it works fine. (2) In some situations also the Compiler doesn't expect calls to accept(ICompilationUnit,..) of which I have observed two symptoms: (a) addCompilationUnit(..) may be called while unitsToProcess is null -> NPE (b) when the CU for package-info.java is added to unitsToProcess during beginToCompile, the added CU may be mistaken to be the current unit. This can be observed when resolving any type within the given package: instead of the requested unit the package-info.java is returned. Both issues can be easily fixed within class Compiler.
Created attachment 189521 [details] tests and proposed fixes This patch contains tests for the above issues: CompilationUnitTests.testDeprecatedFlag10() for the original bug. TypeHierarchyTests.testPackageInfo01() for fup (1) ReconcilerTests.testAnnotations6() for fup (2) The proposed fixes are as described in comment 0.
Olivier, Srikanth, what do you think about the patch?
Jay, please review this patch.
As another cross reference: this issue seems to be exactly that portion of bug 214948 that remained unsolved by then, see bug 214948 comment 18 and bug 214948 comment 30.
Patch looks good to me. Just one question on handling of the JME. Are there cases of JME that we don't want to return an NameEnvironmentAnswer. I am just wondering if we can check for the JavaModelStatus of the JME to be absolutely sure that the scenario is the correct one. Again, I am just wondering and not sure if that would be required.
(In reply to comment #5) > Patch looks good to me. Just one question on handling of the JME. Are there > cases of JME that we don't want to return an NameEnvironmentAnswer. I am just > wondering if we can check for the JavaModelStatus of the JME to be absolutely > sure that the scenario is the correct one. Again, I am just wondering and not > sure if that would be required. You mean like doing: if (jme.isDoesNotExist() && String.valueOf(TypeConstants.PACKAGE_INFO_NAME).equals(typeName)) { Sounds fair (and surely won't hurt). So will this be a +1 from you? Thanks
(In reply to comment #2) > Olivier, Srikanth, what do you think about the patch? I gave a quick once over and it looks ok to me. Did you consider the filter API suggestion in https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c18 and discussed further in https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c22 and decided not to pursue it for some reason ? I wonder if that would have been cleaner and more general than the current fix ? (We are approaching the API freeze very soon though.)
(In reply to comment #6) > You mean like doing: > if (jme.isDoesNotExist() && > String.valueOf(TypeConstants.PACKAGE_INFO_NAME).equals(typeName)) { > > Sounds fair (and surely won't hurt). > So will this be a +1 from you? > Thanks Yes, that's what I meant. isDoesNotExist() looks for ELEMENT_DOES_NOT_EXIST or ELEMENT_NOT_ON_CLASSPATH. If that's what we want, we can use that. On the other hand we can also look for a specific constant. For instance, when I run the new test in CompilationUnitTests, I see the JavaModelStatus as ELEMENT_DOES_NOT_EXIST. With this change, +1 from me.
(In reply to comment #7) > Did you consider the filter API suggestion in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c18 and discussed further > in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c22 and decided > not to pursue it for some reason ? I saw two proposals: - Make package-info.java visible as an IType. This would break many clients. No go. - Make annotations visible via IPackageFragment. This doesn't help the compiler because there's no way the compiler can see an IPackageFragment. The LookupEnvironment can only be asked for types and an answer from the name environment must have a binary type, or a source type, or a compilation unit. > I wonder if that would have been cleaner and more general than > the current fix ? (We are approaching the API freeze very soon > though.) The specific question in this bug is: how can the compiler see package annotations. The only notion that compiler and model share for talking about package-info.java is as a compilation unit. Thus I do think for this particular issue my solution is "clean". It is not very general, though. So if other clients still want the annotation information at the IPackageFragment that's fine with me but apparently that's orthogonal to the compiler POV. For anybody interested in seeing annotations via the IPackageFragment it shouldn't be difficult to transfer annotations from the CU to the package fragment within PackageFragment.buildStructure(..), assuming we want to make IPackageFragment extend IAnnotatable. Do you want me to prepare a patch for that, too (maybe on a separate bug)? But as I said, this doesn't seem to help the compiler. Actually, it would be quite awkward if the compiler asks for a package with its annotations and for that the PackageFragment would have to invoke a compiler to find the annotation inside the CU. Better just return a handle to the CU and let the compiler proceed as normal. For my proposed patch I'll still wait for a final nod from you, Srikanth.
(In reply to comment #9) > (In reply to comment #7) > > Did you consider the filter API suggestion in > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c18 and discussed further > > in > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=214948#c22 and decided > > not to pursue it for some reason ? > > I saw two proposals: > - Make package-info.java visible as an IType. > This would break many clients. No go. Hence the original suggestion to retain the current behavior by filtering it out by default, and exposing it only when a client explicitly authorizes the model prior to query. > For my proposed patch I'll still wait for a final nod from you, Srikanth. I am OK with the patch.
Released for 3.7M6
Created attachment 189884 [details] additional test & fix Using this fix in the field revealed that I missed one case: If a working copy for package-info.java exists NameLookup did not answer the fake type. This was inconsistent to the case where no working copy exists. The attached patch adds a fix for this by answering a fresh not-open handle.
Verified for 3.7M6 using build I20110307-1300.
