RFR: 8263926: JavacFileManager.hasExplicitLocation fails with NPE while compiling [v2]

[Jonathan Gibbons]

On Thu, 17 Jun 2021 16:56:55 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> The compiler fails with NullPointerException while building the maven project provided by the user in [JDK-8263926](https://bugs.openjdk.java.net/browse/JDK-8263926).
>> 
>> Actually, the patch is simple. We only need to add a `null` check before using the `msym.sourceLocation`.
>> 
>> 
>> -            if (fileManager.contains(msym.sourceLocation, fo)) 
>> +            if (msym.sourceLocation != null && fileManager.contains(msym.sourceLocation, fo)) 
>> 
>> 
>> But we can't verify whether the problem is solved because the code provided by the user is complex so that we have no way to wtite a regression test.
>> 
>> Recently, I deeply researched the relationship of the source code provided by the user 
>> and construct a minimal test case to verify the patch. 
>> The minimal test was attached to the JBS just now.
>> The attached test reproduces the bug by using the shell script instead of maven.
>> And the test case `SourceLocationNotExist.java` at this patch is another way to reproduce the bug. 
>> The steps in `SourceLocationNotExist.java` is the same as the shell script mentioned above.
>> 
>> Given that the bug is hard to be reproduced, I give some comments here.
>> 
>> 1. We need to construct a jar file which has both `*.java` files and `*.class` files and has no named module.
>>     First, I compile the file `test/TestUnnamedModule.java` into `test/TestUnnamedModule.class`.
>>     Then  I use the `jar` tool to construct the jar file `test.jar`, which contains both `test/TestUnnamedModule.java` and `test/TestUnnamedModule.class`.
>>     Commands are like: 
>>     `javac test/TestUnnamedModule.java` and 
>>     `jar -c -f test.jar test/TestUnnamedModule.class test/TestUnnamedModule.java`
>> 
>> 2. The last modified time of the `*.java` files should be newer than that of the `*.class` files. 
>>     Because the compiler implicitly choose the last modified file if both source files and class files are present.
>>     We need the compiler the choose the `*.java` files to reproduce the bug.
>>     So before using the `jar` command, we need to update the modified time of the `*.java` files to let it newer than `*.class` files.
>>     Command is like: `touch test/TestUnnamedModule.java`.
>> 
>> 3. We need to use the class of the `test.jar` in a named module.
>>     I construct a module named `use`, which has a module file `module-info.java` and a source file `TestUse.java`.
>>     You can see the `TestUse.java` has a statement `import test.TestUnnamedModule;` to use the `test.jar`.
>> 
>> 4. The `module-info.java` of the module `use` can't have the directive `requires test`. Because the name `test` is the automatic module name of the jar file `test.jar`.
>> 
>> 5. When compiling the module `use` mentioned above, we must use the option `-sourcepath` and the `test.jar` is not  the argument of the `-sourcepath` and `--module-path`.
>>     Command is like: `javac -classpath test.jar -d use -sourcepath use use/module-info.java use/TestUse.java`
>> 
>> We can see that these five requirements are difficult to meet.
>> - Generally, our jar files shouldn't have the `*.java` files and only have the `*.class` files. But some libs that the user uses have these `*.java`files unexpectedly .
>> - And the class files are always compiled from the source files so that it is almost impossible that the last modified time of the `*.java` files is newer than `*.class` files.
>> - Generally, if we use the module system to construct our source code, we should compile the source code by using option `--module-path` instead of `-classpath`.
>> Command is like: javac **--module-path** test.jar -d use -sourcepath use use/module-info.java use/TestUse.java
>> instead of: javac **-classpath** test.jar -d use -sourcepath use use/module-info.java use/TestUse.java
>> However, the option `-classpath` is used by the maven at this situation (which no `requires` directive is in the `module-info.java`).
>> 
>> It is a combination of the java compiler(has no null check), the maven building tools(use `-classpath`), 
>> the extra libraries(have `*.java` files in the jar file) and the user's moduled project codes(have no corresponding module directive).
>> 
>> Anyway, we actually meet it now which is provided by the user.
>> Hope you are not confused by these descriptions.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adjust the comments

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Modules.java line 539:

> 537:         try {
> 538:             JavaFileObject fo = tree.sourcefile;
> 539:             if (msym.sourceLocation != null && fileManager.contains(msym.sourceLocation, fo)) {

What is the module for which the `sourceLocation` is `null` ... in other words, it is reasonable for this method to be called with a module with no source location, or is this check just hiding some other bug elsewhere?

-------------

PR: https://git.openjdk.java.net/jdk/pull/4523