search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
161 stars 130 forks source link

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

Closed robserm closed 1 week ago

robserm commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

robserm commented 2 weeks ago

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

stephan-herrmann commented 2 weeks ago

@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 commented 2 weeks ago

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:

anyone?

jarthana commented 2 weeks ago

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

jarthana commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

No, not at all.

jarthana commented 2 weeks ago 
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 commented 2 weeks ago

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 https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3063#issuecomment-2404919830

noopur2507 commented 1 week ago

It seems this commit is causing test failure in JDT UI - https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1722.