Bug 525203

Comment 1 Noopur Gupta 2017-11-14 03:52:50 EST

Manoj, can you please have a look?

Comment 2 Manoj N Palat 2017-12-18 08:48:33 EST

Created attachment 271943 [details]
Test Project

(In reply to Fred Bricon from comment #0)
> All jars in the classpath should be available as completion items, when
> autocompleting modules in module-info.java. 

@Fred: All the packagefragmentroots in classpath are considered for the module completion in required directives and checked for modules - and consequently all the modules will be shown as an option for completion.

> But you can't autocomplete on modules in module-info.java unless the
> dependencies are already on the modulepath. Chicken, meet egg.

Not really. required directive allows autocomplete on modules which are not on module path - the philosophy adopted here is that while creating the module-info.java file, it is assumed that the dependencies are being created and hence  module-path is not expected to be a given. Dependencies are filled up from the existing work space.

> So ideally, all dependencies from the classpath should be available for
> module autocompletion. 

All dependencies are available from the classpath. I am attaching a project which has a jar called second in its class path. second has a module-info though it is in the class path. go to the module-info.java of the parent project, which 525203 and then complete the  "requires s" directive. "second" will be shown as an option to complete.
LMK if I missed out something here.

Comment 3 Fred Bricon 2017-12-18 11:14:02 EST

Created attachment 271949 [details]
Test project exhibiting autocompletion failure for automodules

Attached is a project with junit and jupiter jars added to the classpath. I expect autocompletion to return junit (matching junit 4.12's default automodule name) and org.junit.jupiter.api (matching jupiter's Automatic-Module-Name in that jar's MANIFEST.MF).
Classpath jars containing a module-info class do work indeed. (your example used a hard coded path to your second.jar)

Comment 4 Manoj N Palat 2017-12-19 00:53:26 EST

(In reply to Fred Bricon from comment #3)
> Created attachment 271949 [details]
> Test project exhibiting autocompletion failure for automodules
> 
> Attached is a project with junit and jupiter jars added to the classpath. I
> expect autocompletion to return junit (matching junit 4.12's default
> automodule name) and org.junit.jupiter.api (matching jupiter's
> Automatic-Module-Name in that jar's MANIFEST.MF).
> Classpath jars containing a module-info class do work indeed. (your example
> used a hard coded path to your second.jar)

@Fred: Thanks for the test project. From the module-info.java of that,..
module demo {
	exports pack1;
	requires java.sql;
	//try autocompleting junit (matching junit 4.12's default automodule) 
	//or org.junit.jupiter.api (matching jupiter's Automatic-Module-Name in that jar's MANIFEST.MF)
	requires second;//completes but module not automatically available to modulepath, should provide quickfix
}

From the completion point of view, the issue is about the automatic modules as you have mentioned in the first two comments, ie:
"	//try autocompleting junit (matching junit 4.12's default automodule) 
	//or org.junit.jupiter.api (matching jupiter's Automatic-Module-Name in that jar's MANIFEST.MF)"

However, given the nature of automatic modules, should we want to have it in the completion in the first place? From JLS 9, 7.7.1:

"While automatic modules are convenient for migration, they are unreliable in the sense
that their names and exported packages may change when their authors convert them
to explicitly declared modules. A Java compiler is encouraged to issue a warning if
a requires directive refers to an automatic module. An especially strong warning is
recommended if the transitive modifier appears in the directive."

My take would be not to provide automatic module names at completion. However, open to suggestions.
@Jay, Stephan: Since you guys have worked on the automatic modules implementation, please chime in if you have any comments on the above.

Now to the third comment: 
"	requires second;//completes but module not automatically available to modulepath, should provide quickfix"

As mentioned, from the completion works but the quick fix is not available. Have created bug 528930 for the same.

Comment 5 Sasikanth Bharadwaj 2017-12-19 01:17:30 EST

It isn't an automatic module unless there's a requires directive specifying it's name, so that way not proposing the name is fine, the user is fully aware of what he is doing, given all the warnings associated with the usage of automatic modules. But from a user pov, it looks odd that we do not propose what's available. Do we have proposals that come with warnings?

Comment 6 Till Brychcy 2017-12-19 02:27:53 EST

Fred describes the problem:

- Maven puts the dependency on the classpath or modulepath according to whether it is mentioned in the module-info.java or not, so from the (maven) user point of view, the module-info.java is the place where the location of dependencies is configured (we could have done that, too, remember the discussion in bug 520300). But this will be hard without completion.

- Many external dependencies will probably be automatic modules for years to come, no matter what the spec says.

So I agree with Fred, that completion should work for automatic modules, too.

Comment 7 Jay Arthanareeswaran 2017-12-19 03:05:25 EST

Given the non-trivial nature of the naming protocol for automatic modules (in some cases, at least), I don't think we should burden the user with figuring out what a valid module name would be.

Comment 8 Manoj N Palat 2017-12-19 03:19:40 EST

Thanks Sasi, Till and Jay for the comments and fast response. So then lets have it. Setting a target now (M5 hopefully)

Comment 9 Eclipse Genie 2017-12-19 09:13:50 EST

New Gerrit change created: https://git.eclipse.org/r/114397

Comment 10 Eclipse Genie 2017-12-20 00:30:21 EST

Gerrit change https://git.eclipse.org/r/114397 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=124c6007c2e3c0b7813d2d7198a1af4300e07d61

Comment 11 Fred Bricon 2017-12-20 16:02:07 EST

Created attachment 271992 [details]
Completion using default automodule name instead of Automatic-Module-Name

Testing from master, it doesn't seem to work properly, all completion items for automodules return the default automodule name, Automatic-Module-Name from MANIFEST.MF is ignored.

Comment 12 Manoj N Palat 2017-12-21 22:26:49 EST

(In reply to Fred Bricon from comment #11)
> Created attachment 271992 [details]
> Completion using default automodule name instead of Automatic-Module-Name
> 
> Testing from master, it doesn't seem to work properly, all completion items
> for automodules return the default automodule name, Automatic-Module-Name
> from MANIFEST.MF is ignored.

I cannot reproduce this on master - locally am able to get the appropriate name - from MANIFEST.MF if "Automatic-Module-Name: org.junit.jupiter.api" is defined, else the default name.

Could you please attach the reproducible project?

Comment 13 Fred Bricon 2017-12-26 10:05:05 EST

I tested from sources last time, but I don't have my laptop available, not until another week.
But I couldn't reproduce that issue with yesterday's I-Build, on another machine, so I guess that's good news.

Now I'm eager to see bug #528930 being fixed :-)

Comment 14 Manoj N Palat 2018-01-02 03:29:12 EST

(In reply to comment #13)
> I tested from sources last time, but I don't have my laptop available, not until
> another week.
> But I couldn't reproduce that issue with yesterday's I-Build, on another
> machine, so I guess that's good news.
I am taking this as solved for now :) 
[pl reopen/open a follow-up bug if you see this/related issue]

Comment 15 Sasikanth Bharadwaj 2018-01-24 04:10:10 EST

Verified for 4.8 M5 using I20180123-2000 build

Comment 16 Till Brychcy 2018-01-24 04:14:27 EST

(In reply to Fred Bricon from comment #11)
> Created attachment 271992 [details]
> Completion using default automodule name instead of Automatic-Module-Name
> 
> Testing from master, it doesn't seem to work properly, all completion items
> for automodules return the default automodule name, Automatic-Module-Name
> from MANIFEST.MF is ignored.

That was probably Bug 530142

Comment 17 Manoj N Palat 2018-01-29 22:05:30 EST

*** Bug 528958 has been marked as a duplicate of this bug. ***