Community
Participate
Working Groups
Umbrella bug for Java 9 support in JDT Core Java Model
Created attachment 264522 [details] Draft patch This is the first cut draft patch. This is not complete by any means, but I just wanted to give an idea of what I think is needed from Java model. At a high level, I think we need two different kind of Java elements. One should be represent the module description for the module-info. This will be a child of ITypeRoot, equivalent to IType. I have added this in the attached path, in the form of IModuleDescription. The second is the notion of a Java module, that a module project represents. Now, this needs more discussion. My idea for this is that it should: 1. provide the packages within this module 2. provide the packages exported within this module 3. provide a handle to the other "required" modules 4. provide the services provided and used. I am not yet sure what the intended use for this, though. For now, I will work on completing the former. It still needs support on element handle, source range etc.
Created attachment 264910 [details] Updated model changes This adds more stuff to the previous patch. The patch has pretty much everything wrt to the module descriptor, except handle/identifier for new elements.
Created attachment 264911 [details] Code select changeds Patch to be applied on top of the previous model related patch. But these need to be synced up with the latest BETA_JAVA9 branches as it collides with some of Sasi's recent changes.
New Gerrit change created: https://git.eclipse.org/r/83513
Gerrit change https://git.eclipse.org/r/83513 was merged to [BETA_JAVA9]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=623763b4b76f1ec0b4fe3fc56f794a085f27cd95
(In reply to Eclipse Genie from comment #5) > Gerrit change https://git.eclipse.org/r/83513 was merged to [BETA_JAVA9]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=623763b4b76f1ec0b4fe3fc56f794a085f27cd95 This patch caused three tests to fail in TypeHierarchyTests and I have commented out them for now. My investigation so far shows there is some undesired dependency between tests. I can emulate the same failure without my fix by opening some Java elements explicitly in tests.
The following stack trace comes up when I start eclipse: at java.io.DataInputStream.readBoolean(DataInputStream.java:244) at org.eclipse.jdt.internal.core.JavaModelManager$VariablesAndContainersLoadHelper.loadBoolean(JavaModelManager.java:3647) at org.eclipse.jdt.internal.core.JavaModelManager$VariablesAndContainersLoadHelper.loadPath(JavaModelManager.java:3737) at org.eclipse.jdt.internal.core.JavaModelManager$VariablesAndContainersLoadHelper.loadContainers(JavaModelManager.java:3709) at org.eclipse.jdt.internal.core.JavaModelManager$VariablesAndContainersLoadHelper.loadProjects(JavaModelManager.java:3767) at org.eclipse.jdt.internal.core.JavaModelManager$VariablesAndContainersLoadHelper.load(JavaModelManager.java:3601) at org.eclipse.jdt.internal.core.JavaModelManager.loadVariablesAndContainers(JavaModelManager.java:3438) at org.eclipse.jdt.internal.core.JavaModelManager.startup(JavaModelManager.java:5176) at org.eclipse.jdt.core.JavaCore.start(JavaCore.java:5878) at org.eclipse.osgi.internal.framework.BundleContextImpl$3.run(BundleContextImpl.java:774) at org.eclipse.osgi.internal.framework.BundleContextImpl$3.run(BundleContextImpl.java:1) at java.security.AccessController.doPrivileged(Native Method) at org.eclipse.osgi.internal.framework.BundleContextImpl.startActivator(BundleContextImpl.java:767) at org.eclipse.osgi.internal.framework.BundleContextImpl.start(BundleContextImpl.java:724) at org.eclipse.osgi.internal.framework.EquinoxBundle.startWorker0(EquinoxBundle.java:933) at org.eclipse.osgi.internal.framework.EquinoxBundle$EquinoxModule.startWorker(EquinoxBundle.java:309) at org.eclipse.osgi.container.Module.doStart(Module.java:581) at org.eclipse.osgi.container.Module.start(Module.java:449) at org.eclipse.osgi.framework.util.SecureAction.start(SecureAction.java:468)
(In reply to Manoj Palat Limited Availability Till Nov 2 from comment #7) > The following stack trace comes up when I start eclipse: This is probably related to the module source path container, but it will help if you can debug and post which container causes this.
Created attachment 264999 [details] Projects to reproduce the scenario @Jay - please find the reproducible zip
I agree that we need an IJavaElement that represents the module declaration in the module-info.java. But I don't think we need any children of this element. Here are the main purposes of the Java model elements: - Present a hierarchical structure of named elements that constitute a Java code base - Serve as target element for user and API client operations, e.g. search for references, open type hierarchy, open call hierarchy, rename, open declaration - Serve as context element for search results (e.g. the Search view shows the IMethod in whose body the search engine found a reference to a field, type, package, etc.) - Offer a stable handleIdentifier string that can be used to store and re-create the element IModuleDescription should be renamed to IModule. All other element types could also be called I*Description; the appendix doesn't add valuable information. The module element is structurally similar to the IPackageDeclaration element, but I guess that element was only called *Declaration to avoid confusion with the other IPackage* element types. IModule should extend IJavaElement, ISourceReference. None of the methods on IMember is required on a module. There are a few that could have a reasonable implementation (getClassFile(), getCompilationUnit(), getTypeRoot(), isBinary()), but like for IPackageDeclaration, that information is trivially available by inspecting the type of getParent(). On IMember, these methods are interesting because members can be nested recursively. But that's not the case for package declarations and modules. The children of IModuleDescription all represent references to other elements, not declarations that have individual names. They are comparable to references inside an IMethod's body. Such details are better left to an AST (which we currently don't need, but which could be added later). Caveat: For bug 506369, I've added a reference to the type IModuleDescription in JDT UI. When you rename IModuleDescription to IModule, please leave an "interface IModuleDescription extends IModule" for a few days, so that we can update the UI without breakages.
(In reply to Markus Keller from comment #10) > IModuleDescription should be renamed to IModule. All other element types > could also be called I*Description; the appendix doesn't add valuable > information. The module element is structurally similar to the > IPackageDeclaration element, but I guess that element was only called > *Declaration to avoid confusion with the other IPackage* element types. As I called out in comment #1, I wanted to specifically make that distinction between a module description and a module, esp. because at this point I wasn't sure of what a "module" provides to the client. In other words, a IJavaProject can "have" an IModuleDescription and "be" a IModule. I have no problem in having just one element (and call it IModule) if we are absolutely sure that we want to put every module related API in just one. On the other hand, having a separate IModule gives us the opportunity to provide additional APIs for the client, hypothetically at least. > IModule should extend IJavaElement, ISourceReference. None of the methods on > IMember is required on a module. There are a few that could have a > reasonable implementation (getClassFile(), getCompilationUnit(), > getTypeRoot(), isBinary()), but like for IPackageDeclaration, that > information is trivially available by inspecting the type of getParent(). On > IMember, these methods are interesting because members can be nested > recursively. But that's not the case for package declarations and modules. That's about half the methods declared in IMember :). Let me also throw in the getSourceRange() from ISourceReference. And who knows, there could be modifiers (add getFlags() here) and Javadoc (add getJavadocRange() here). There's nothing in the docs of IMember that binds us to only those that can be nested. So, I am inclined to take the convenient approach here. > The children of IModuleDescription all represent references to other > elements, not declarations that have individual names. They are comparable > to references inside an IMethod's body. Such details are better left to an > AST (which we currently don't need, but which could be added later). I had a discussion with Manoj some time back, and IIRC, he said search could use individual elements for better indexing/search.
New Gerrit change created: https://git.eclipse.org/r/83749
(In reply to Eclipse Genie from comment #12) > New Gerrit change created: https://git.eclipse.org/r/83749 This patch addresses two issues: 1) Binary modules were not offered as part of completion. This was a regression introduced by the first commit through this bug. 2) code select would only work on the last segment of the module name, for e.g., in my.mod, select only works on "mod" and not on "my". This was due to the fact that module name, unlike packages and types, is a single identifier. The '.' are not really separators but part of the identifier. Added some test. But more tests on the way for model and code select.
Gerrit change https://git.eclipse.org/r/83749 was merged to [BETA_JAVA9]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e1882fb7451cd4afded135030307a827a4d3a76c
(In reply to Eclipse Genie from comment #14) > Gerrit change https://git.eclipse.org/r/83749 was merged to [BETA_JAVA9]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=e1882fb7451cd4afded135030307a827a4d3a76c @jay: this is causing a regression wrt completion due to the NameLookup change - from if (!prefixMatcher.matches(name, root.getElementName().toCharArray())) { to this if (!CharOperation.equals(name, root.getElementName().toCharArray())) { Could you please explain the rationale behind this change? [planning to revert this particular change]
(In reply to Manoj Palat from comment #15) > Could you please explain the rationale behind this change? > [planning to revert this particular change] Now, I realize that fix was incorrect. I found problems in code select when multiple modules having same prefix. But I wrongly diognized the cause to be the prefix-equals itself, but may have been due to the "prefixMatch" being passed when not appropriate. Please feel free to fix it and while you are at, please add tests for completion as well, which will greatly assist anyone changing code in this area. For the record, the commit in question is: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=205390a257a809cd8183a868edb9ecc9fe0ef192
@Jay: (as discussed offline) I believe that the char[][] usesServices of ModuleDescriptionInfo is not sufficient since we can have multiple uses with each uses potentially have a QualifiedTypeReference as well. It may be better to have a separate struct similar to PackageExportInfo to enclose this for a)for uniformity and b) for any additional keywords/changes that can come in future.
I have added the remaining model elements via commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA9&id=4fbf882e97b6977b3cda5aba254e7b63df299e14 The new elements need better documentation and perhaps better names too. Will keep this bug open.
@Jay - adding another reason for comment 17 req :) - if you use the patch (wip at the time of writing this) in bug 501162 comment 1 and search for pack1 in a module-info.java file that contains atleast the following: exports pack1; uses pack1.X11; provides pack22.I22 with pack1.X11; we can see that the search results view is non-uniform with uses match having module itself as the IJElement. [An alternate is to display all search results wrt module - but since we have finer grain IJE (except for uses), the results are shown against the corresponding lowest parent]. So, again, my suggestion would be to define an IJE for uses as well for consistency at the least. Let know your take on this.
In the end, we kept it simple by having a java element only for the module description and not for the directives inside. Other features could/do use other means such as DOM for manipulating a module info. If UI comes up with new requirements, we can handle them via new bugs.
@Jay: See bug 501162 comment 2 - second point - reproduced below: -- start --- Searching for references to a module, mostly works: I have two modules Service and Consumer, the latter saying "requires Service". Searching for references to "Service" ... 1. Opens a dialog asking me which element I want to search for (showing lots of *types* of that name, plus the module at the bottom) 2. Search finds the reference after "requires", good. => Search is good, code select may be broken? Is that a known issue? -- end --- I also see a TODO in SelectionRequestor.acceptModule against this bug (related?). The issue here is the SelectionEngine.acceptedAnswer is false and it defaults to the text search for IType (in this case its a module). We should be looking only for a module since we know its IJavaSearchConstants.MODULE
Another selection issue (on top of pending changes in several bugs): //--- module jdk.jdeps { // ... provides java.util.spi.ToolProvider with com.sun.tools.javap.Main.JavapToolProvider, com.sun.tools.jdeps.Main.JDepsToolProvider; } //--- Selecting "java.util.spi.ToolProvider" does not work. In sibling modules with similar "provides" it works OK. Open type works for that type.
(In reply to Manoj Palat from comment #21) > @Jay: See bug 501162 comment 2 - second point - reproduced below: I think the issue is we do not set the SelectionEngine#acceptedAnswer flag even after successfully resolving a module element. I will provide a fix shortly probably along with the one reported in comment #22, of which I know nothing about yet.
(In reply to Jay Arthanareeswaran from comment #20) > In the end, we kept it simple by having a java element only for the module > description and not for the directives inside. Other features could/do use > other means such as DOM for manipulating a module info. > > If UI comes up with new requirements, we can handle them via new bugs. Yes, more seems to be needed, see bug 522293.
This can be considered done. Further issues can be taken up through separate bugs.
To close the loop regarding comment 22: this has found a new home in bug 521287
*** Bug 496163 has been marked as a duplicate of this bug. ***
