Summary: | [compiler][model] incomplete support for package-info.java when using SearchableEnvironment | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Stephan Herrmann <stephan.herrmann> | ||||||
Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | amj87.iitr, jarthana, Olivier_Thomann, srikanth_sankaran | ||||||
Version: | 3.7 | Flags: | jarthana:
review+
|
||||||
Target Milestone: | 3.7 M6 | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Whiteboard: | |||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 331647 | ||||||||
Attachments: |
|
Description
Stephan Herrmann
2011-02-22 12:28:44 EST
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. |