ASTParser should detect and report when no suitable system library has been configured

[robserm]

I have a case when I got NullPointerException, it seems that it is thrown when we are parsing more than one file and java lib is not set (when calling method parser.setEnvironment() includeRunningVMBootclasspath flag is set to false, changing to true solves the problem).

java.lang.NullPointerException: Cannot read field "declaringClass" because "mb" is null
at org.eclipse.jdt.internal.compiler.ast.ReferenceExpression.generateCode(ReferenceExpression.java:375)
at org.eclipse.jdt.internal.compiler.ast.LocalDeclaration.generateCode(LocalDeclaration.java:174)
at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:387)
at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.generateCode(AbstractMethodDeclaration.java:323)
at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:759)
at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.generateCode(TypeDeclaration.java:829)
at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.generateCode(CompilationUnitDeclaration.java:412)
at org.eclipse.jdt.internal.compiler.Compiler.process(Compiler.java:935)
at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:1173)
at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:786)
at org.eclipse.jdt.core.dom.CompilationUnitResolver$ECJCompilationUnitResolver.resolve(CompilationUnitResolver.java:92)
at org.eclipse.jdt.core.dom.ASTParser.createASTs(ASTParser.java:1071)
at npe.JDTParsingExample.main(JDTParsingExample.java:52)

Steps to reproduce:
download and unzip the attached project
make sure you are under Java 17
then run:

gradlew run

As a result on the console, you should get the listed exception. The project is using jdt.core version 3.39.0

npe_while_parsing.zip

[srikanth-sankaran]

Are you able to attach a plain SDK project that shows the problem - that will speed up investigation and potential resolution. Thanks in advance

[robserm]

@srikanth-sankaran what do you mean by saying "plain SDK project"? did you check the attachment?
it consists of 3 files, the first one JDTParsingExample calls parser.createASTs(..) and there are two more files in resources which are parsed. I tried to keep it as much simple as possible. Please let me know if it is ok or do you need the example project reproducing in some other form.

[srikanth-sankaran]

A plain SDK project wouldn't involve gradle for example.

@jarthana - are you able to extract a reproducer from the attachment ? Any help would be appreciated. TIA

[jarthana]

This is more of a missing classpath entry problem. We are unable to find java.lang.Object because we are not passing a valid JDK library - this is required for the setEnvironment() invocation. And because we are bypassing the compiler, we are not really reporting about the missing system library or system class files.
@robserm you can fix this in line number 22 of JDTParsingExample.java, either by passing a valid JavaSE as a classpath entry or change the last argument to true, which will let the compiler pick the running JDK. I am closing. Should you still face issues, please feel free to reopen with more details.

[robserm]

@jarthana thank you so much quick reaction, I am completely aware that if I change the includeRunningVMBootclasspath flag to true solves the problem (I wrote this in the first comment) , but in my opinion it is only a workaround, the code SHOULD NOT throw NullPointerException when the project is misconfigured
Don't you think that it will be better to protect the code against throwing NullPointerException?

[jarthana]

This is very specific to the standalone parser. Other components of JDT don't suffer the problem because they are handled at a higher level and we don't even reach here. But I agree with you on avoiding the NPE, though.

If we introduce a null check, it's going to be redundant for most of the compiler scenarios. Better option will be to avoid reaching this at all in case of standalone parser. But without the system classes, what do you expect to happen?

@jarthana thank you so much quick reaction, I am completely aware that if I change the includeRunningVMBootclasspath flag to true solves the problem (I wrote this in the first comment)

Oops! Sorry, I wasn't paying attention. I only tried to convert this into a simple testcase but then ended up investigating :)

[robserm]

Well, in standalone mode misconfiguration I think this could be a common problem. Throwing NPE gives an impression that something went wrong with the parser and does not give any hint that what is the source of the problem.
In most of the cases the compiler just returns 'compile error' for such cases. And this is what I would expect to happen in this case.

Because each class in java requires system class to compile maybe there should be some mechanism to detect if there are system classes on classpath and if not fallback to includeRunningVMBootclasspath=true or immediately report 'compile error'.

Beside that leaving the code unprotected with NPE is in my opinion not the safest way to solve this issue. Even if this happen right now only in standalone mode you cannot be sure that it will not happen later in some other scenarios.

[stephan-herrmann]

Beside that leaving the code unprotected with NPE is in my opinion not the safest way to solve this issue. Even if this happen right now only in standalone mode you cannot be sure that it will not happen later in some other scenarios.

I'm actually against adding null checks for all situations, because

• every null check needs a reasonable else branch. Code generation cannot simply skip some part. So what else part should we add?
• in this particular situation where method java.lang.Object.getClass() is missing, code generation cannot possibly produce any useful result
• the compiler has several constellations where a null pointer definitely signals a code bug. Mindlessly inserted null checks will give the impression that every use of a given field / method requires a null check and soon it will be impossible to tell apart where null is an expected value vs. result of some code bug. Yes, there are situations where I want to see NPE, in order to be alerted that somewhere upstream a bug exists.

[robserm]

@stephan-herrmann I completely understand your point of view and agree, so what is your proposition? how this problem should be solved?

[stephan-herrmann]

@stephan-herrmann I completely understand your point of view and agree, so what is your proposition? how this problem should be solved?

I think the best would be to detect the situation in the org.eclipse.jdt.core.dom.* layer, throw an IllegalStateException, so that 3rd party clients have a chance to handle this in a meaningful way. Note that ASTParser.createASTs() already declares

	/** ...
	 * @exception IllegalStateException if the settings provided
	 * are insufficient, contradictory, or otherwise unsupported
	 */

This is the slot we should integrate with.

Reopening - I'll also adjust the ticket title accordingly.

[stephan-herrmann]

If we restrict the fix to Java 9+ then a simple strategy could be to add a check in org.eclipse.jdt.core.dom.ASTParser.getClasspath() like this

		if ((this.bits & CompilationUnitResolver.RESOLVE_BINDING) != 0) {
			String compliance = this.compilerOptions.get(JavaCore.COMPILER_COMPLIANCE);
			if (CompilerOptions.versionToJdkLevel(compliance) >= ClassFileConstants.JDK9)
				if (!allClasspaths.stream().anyMatch(cp -> cp.getModule(TypeConstants.JAVA_DOT_BASE) != null))
					throw new IllegalStateException("Missing system library"); //$NON-NLS-1$
		}

TODO:

• write unit tests for the above
• investigate tests that trigger the above exception (I see 40+ such tests below RunDOMTests), perhaps they are indeed ill-configured?

anyone?

[jarthana]

I will add this test along with your fix. I almost have one in my setup already.

[jarthana]

I have been trying to find why only this particular scenario fails. It appears this is the only case that gets past the compilation stage and tries getJavaLandObject() during codegen. Also, to steer the execution down this path, we must have a compilation unit, followed by another with no explicit imports. I am also seeing (in the CUScope) the java.lang.* import binding as missing binding. Should we even really let this get this far. Let me take another look.

[jarthana]

Adding little more details: The first unit gets reported for missing import binding. But yet, we set the this.environment.root.defaultImports, which is used for the 2nd unit and as a result we bypass the getDefaultImports() and therefore don't tag it as ignoreFurtherInvestigation. That's how we end up at codegen which we shouldn't have reached at all.

[jarthana]

Pull #3063 provides a different approach. It's simple. Fix is not to set the environment.root.defaultImports if there's a problem. Side effect is, every compilation unit will get these problems. I also noticed we are not doing creating any missing binding for module import but haven't touched that part.

Edit: Actually, I don't see any difference in behavior in the IDE. All files on the editor see the problem about missing java.lang.Object, but only the first one get it on the problems view. So, I guess we are safe. @stephan-herrmann wdyt?

[stephan-herrmann]

Pull #3063 provides a different approach. It's simple. Fix is not to set the environment.root.defaultImports if there's a problem. Side effect is, every compilation unit will get these problems. I also noticed we are not doing creating any missing binding for module import but haven't touched that part.

Edit: Actually, I don't see any difference in behavior in the IDE. All files on the editor see the problem about missing java.lang.Object, but only the first one get it on the problems view. So, I guess we are safe. @stephan-herrmann wdyt?

So you are proposing a middle ground between a point-fix for the actual symptom (a trivial null check) and fix at the front door to avoid the broken situation from the outset (throw ISE when invoked with incomplete configuration), right?

Anything better than fixing one particular symptom is good. Perhaps #3063 will even avoid havoc in other situations, not coming from DOM.

In the particular DOM scenario I'd still prefer one exception over a useless result. When requesting bindings what is the benefit of creating lots of compilation units, where all bindings will have problems? Some clients of DOM might not even lock at the problems in the CompilationResult, so its not clear that the root cause will ever be visible.

Maybe we should do both? I'll take a quick look at the failures resulting from my proposed change.

@jarthana do you mind if I push an additional commit on your PR?

[jarthana]

@jarthana do you mind if I push an additional commit on your PR?

No, not at all.

[jarthana]

if ((this.bits & CompilationUnitResolver.RESOLVE_BINDING) != 0) {
			String compliance = this.compilerOptions.get(JavaCore.COMPILER_COMPLIANCE);
			if (CompilerOptions.versionToJdkLevel(compliance) >= ClassFileConstants.JDK9)
				if (!allClasspaths.stream().anyMatch(cp -> cp.getModule(TypeConstants.JAVA_DOT_BASE) != null))
					throw new IllegalStateException("Missing system library"); //$NON-NLS-1$
		}

If the JDK is added as a classpath (as opposed to modulepath), will this still work? It's been a while since I worked on this area.
Also, just noticed that we have no test that expects "invalid environment settings" that the getClasspath() throws.

[stephan-herrmann]

If the JDK is added as a classpath (as opposed to modulepath), will this still work? It's been a while since I worked on this area.

In the UI we don't even allow to move a modular JRE to the classpath, but by modification of .classpath this is possible, indeed.

For more details see #3063 (comment)

[noopur2507]

It seems this commit is causing test failure in JDT UI - eclipse-jdt/eclipse.jdt.ui#1722.