-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASTParser should detect and report when no suitable system library has been configured #3047
Comments
Are you able to attach a plain SDK project that shows the problem - that will speed up investigation and potential resolution. Thanks in advance |
@srikanth-sankaran what do you mean by saying "plain SDK project"? did you check the attachment? |
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 |
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. |
@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 |
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?
Oops! Sorry, I wasn't paying attention. I only tried to convert this into a simple testcase but then ended up investigating :) |
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. 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. |
I'm actually against adding null checks for all situations, because
|
@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 /** ...
* @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. |
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? |
I will add this test along with your fix. I almost have one in my setup already. |
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. |
Adding little more details: The first unit gets reported for missing import binding. But yet, we set the |
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? |
No, not at all. |
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 For more details see #3063 (comment) |
It seems this commit is causing test failure in JDT UI - eclipse-jdt/eclipse.jdt.ui#1722. |
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
The text was updated successfully, but these errors were encountered: