Avoid NPE when compilation unit is missing

[MeherSru]

What it does

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[iloveeclipse]

@MeherSru : thanks for your contribution.
Please make sure you have signed ECA (see check above).

Also please describe your problem & provide steps to reproduce.

[MeherSru]

@iloveeclipse : I’ve signed the ECA.

This change adds a defensive null check since TypeDeclaration.getCompilationUnitDeclaration() can return null. Without the check, dereferencing compUnitDecl.currentPackage could lead to a potential NullPointerException in edge or recovery scenarios.

[iloveeclipse]

I’ve signed the ECA.

It seems your email uses current IP address received via DHCP, and that will probably change in the future, so probably not a good choice for a long term.

Anyway, let wait for the ECA database update, I believe it needs some hours.

This change adds a defensive null check since TypeDeclaration.getCompilationUnitDeclaration() can return null. Without the check, dereferencing compUnitDecl.currentPackage could lead to a potential NullPointerException in edge or recovery scenarios.

Can you provide steps to reproduce or exception stack?

[robstryker]

Hi Andrey @iloveeclipse

There's no real stack trace for this. I was mucking around with the javac version of jdt, as well as a mixed environment where I was using some of ecj and some of javac. I just happened to notice that this particular call wasn't null-protected, while other calls to getCompilationUnitDeclaration() were null-protected.

See: CompilationUnitResolver, BindingKeyResolver, and ProblemHandler.

[iloveeclipse]

I just happened to notice that this particular call wasn't null-protected, while other calls to getCompilationUnitDeclaration() were null-protected.

Sure, it is clear why the NP check is needed, but the main reason I'm asking is to understand the root cause of NPE (as it was reported as NPE, see PR title). So far NPE at this place was not reported, and if you observed it, we've missed some use case and that use case then deserves a regression test.

@stephan-herrmann : the code deals with null annotations, so you may want to look into.

[robstryker]

To be more clear, there was no root cause, as there was no bug experienced in a running environment. This was source code level analysis / observation, and a PR created simply to align with other callers of the same method, who all performed null checks.

[robstryker]

Any chance we can get this merged? Or is source-code level analysis insufficient to get an NPE check added?

[iloveeclipse]

Any chance we can get this merged?

Would be OK from my POV, but ...

I’ve signed the ECA.

It seems your email uses current IP address received via DHCP, and that will probably change in the future, so probably not a good choice for a long term.

Whatever you did is not OK. Please check your mail linked to your eclipse.org account and the one you've used to sign ECA. They should be identical.

[MeherSru]

Hi @iloveeclipse,

Thanks for checking. At the time I created this PR, I had not yet signed the ECA. I signed it afterward using the same email address linked to my Eclipse account and Git commits. I have not opened a new PR for this change.

[stephan-herrmann]

@stephan-herrmann : the code deals with null annotations, so you may want to look into.

I currently don't have time nor energy for an extended discussion, but since you asked these are my comments:

• Without a reproducer we're basically guessing what might be happening, in which case we should be doubly careful not to draw any premature conclusions.
• During compilation TypeDeclaration.getCompilationUnitDeclaration() should never return null
  • the method depends on the scope being non-null, which in my experience may only be violated when DOM operations go back to resolution in the compiler after environment and scope have already been discarded
  • the only calls to missingNonNullByDefaultAnnotation() obtains their TypeDeclaration as this.scope.referenceContext -> obviously the scope was not yet discarded at that point
  • Ergo we have no explanation how null could enter the picture, and if it does we likely have a deeper problem, which would only get masked by the change until it surfaces somewhere else.
• The only null-check on a result of getCompilationUnitDeclaration() (in ProblemHandler.handle()) is a very weak indication. Perhaps the committer of https://bugs.eclipse.org/346175 / 7e8a00e was just overly cautious (I can see no specific justification), in which case it should not set the rule for all future calls.
• just aborting the current operation (here missingNonNullByDefaultAnnotation() isn't always a valid response to unexpected null
  • please see that compUnitDecl is consulted only to determine which of two possible variants of the problem should be reported. If that variable is null, we can still report the other variant.

It's all not a huge deal because neither missingNonNullByDefaultAnnotation() nor getCompilationUnitDeclaration() are central to the compiler: involved are very few incoming calls plus a compiler option likely never enabled by most users.