Skip to content
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

Adds null check before calling nullable repository variable method to avoid NPE #6682 #6694

Conversation

snreinert
Copy link
Contributor

@snreinert snreinert commented Nov 13, 2023

Fixes #6682. The repository value that is passed to the ClassPathProviderImpl constructor is null when the file that triggered ClassPathProviderImpl::findClassPath is a Java source file in a JDK repo directory structure.

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Nov 13, 2023
@apache apache locked and limited conversation to collaborators Nov 14, 2023
@apache apache unlocked this conversation Nov 14, 2023
@mbien
Copy link
Member

mbien commented Nov 14, 2023

@snreinert thanks for looking into this!

@neilcsmith-net @MartinBalin this looks like a potential candidate for last-minute NB 20 inclusion assuming it gets reviewed in time. Running all tests now (edit: all tests green, removing label again). Would need rebase to delivery obviously.

@mbien
Copy link
Member

mbien commented Nov 14, 2023

@snreinert if you are available, could you rebase your commit on top of the delivery branch and force push into this PR? After that we can switch the merge target from master to delivery.

Comment on lines -206 to 209
if (!repository.isAnyProjectOpened()) {
if (repository != null && !repository.isAnyProjectOpened()) {
//if no project is open, java.base may not be indexed. Fallback on default queries:
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment might need adjustment to:
//if no project is open or the JDK is not modular, java.base may not be indexed. Fallback on default queries:

I assume repository is always null for openjdk 8 checkouts. cc @jlahoda

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For openjdk 8 (and maybe prior) checkouts, the repository is null so the comment inside that block wouldn't apply. Would non-modular JDKs ever have a non-null repository?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this was an attempt to avoid unnecessary parsing from source when none of the OpenJDK project is open and hence not indexed. There's probably nothing better we could do for JDK 8.

@mbien mbien removed the ci:all-tests [ci] enable all tests label Nov 14, 2023
@mbien
Copy link
Member

mbien commented Nov 14, 2023

on second thought, lets keep it as is targeting master - no need to rush anything.

@mbien mbien added this to the NB21 milestone Nov 14, 2023
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Comment on lines -206 to 209
if (!repository.isAnyProjectOpened()) {
if (repository != null && !repository.isAnyProjectOpened()) {
//if no project is open, java.base may not be indexed. Fallback on default queries:
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this was an attempt to avoid unnecessary parsing from source when none of the OpenJDK project is open and hence not indexed. There's probably nothing better we could do for JDK 8.

@mbien
Copy link
Member

mbien commented Jan 11, 2024

merging this one too for NB21, congrats to your first contribution @snreinert! && thanks for review @lahodaj

@mbien mbien merged commit 91b2846 into apache:master Jan 11, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting a breakpoint in jdk8u repo Java source prevents Java debug session from starting
3 participants