RFR: JDK-8217868: Crash for overlap between source path and patch module path

[Jonathan Gibbons]

I'm not a big fan on initializing the module system twice, but in the 
overall scheme of things, it's not the worst that can happen.

At some point, it may be worth revisiting this, but for now, this looks OK.

You'll have (trivial) merge conflicts; one of the files (for the old 
doclet world) has gone away.

-- Jon


On 02/12/2019 05:52 AM, Jan Lahoda wrote:
> Hi Jon,
>
> Thanks for the comments!
>
> On 12.2.2019 02:27, Jonathan Gibbons wrote:
>> Why the call of modules.newRound() in JavadocTool line 202?
>>
>> As I read it, nothing much should have happened to the modules up to
>> this point ... the code has just been parsing files, hasn't it?
>
> ElementsTable.scanSpecifiedItems is calling initModules at the end, 
> and is invoked from:
>
>             etable.packages(packageNames)
>                     .classTrees(classTrees.toList())
>                     .scanSpecifiedItems();
>
>
> I tried to remove that, but it turned out 
> ElementsTable.findModuleOfPackageName needs the module system to be 
> set-up, and is (eventually) called from ElementsTable.getFilesToParse. 
> So I've incline to let the module system set-up and then set it up 
> again, using newRound() to clear it between.
>
> (If needed, we could keep the javadoc tool(s) untouched, by having one 
> more check in Modules.)
>
>>
>> The use case looks interesting, because it does look like a
>> configuration error and so I was expecting more conflict detection, but
>> maybe that's a different separable problem.  I guess if ever we had a
>> patch path for a module mX that pointed to a directory containing a
>> module-info.java containing mY, and we needed mX, we would get some sort
>> of "wrong module found" error. Here, we have the patch path matching the
>> sourcepath, but presumably we can't go round pairwise checking paths for
>> silly combinations.
>
> I guess we could (and maybe should) have a task to introduce a warning 
> for cases like this. But javac knows from which module it was reading 
> the file from, so seemed more reliable to simply reuse that knowledge.
>
> Jan
>
>>
>> -- Jon
>>
>>
>> On 02/07/2019 08:42 AM, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Consider code like this:
>>> ---src/module-info.java
>>> module m { uses test.Test; }
>>> ---src/test/Test.java
>>> package test; public class Test { }
>>>
>>> And javac invocation like:
>>> javac -sourcepath src --patch-module m2=src module-info.java
>>>
>>> While analysing the module-info, javac will try to lazily/implicitly
>>> load the test.Test class. And when Modules.setCompilationUnitModules
>>> is called for test.Test, it will eventually call singleModuleOverride,
>>> which will look at --patch-module to find if the given source is on
>>> any module patch. And it will find out it is a module patch for m2,
>>> but as javac never heard about m2 before, it will crash.
>>>
>>> Seems that the biggest underlying problem is that even though javac
>>> knows which modules owns the class, it will try to look for the owner
>>> on the patch path. The proposed patch fixes that, and uses the correct
>>> owning module. This requires also a little cleanup in javadoc.
>>>
>>> Webrev: cr.openjdk.java.net/~jlahoda/8217868/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217868
>>>
>>> How does this look?
>>>
>>> Thanks,
>>>     Jan
>>