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

[Jan Lahoda]

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
>