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

Add an option to enable/disable null analysis #2279

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Oct 21, 2022

Signed-off-by: Shi Chen chenshi@microsoft.com

related to redhat-developer/vscode-java#2747

add new preference java.compile.nullAnalysis.mode, which type is FeatureStatus.

  • When set to interactive (default), a notification will show once null annotation types are detected when project is imported or null analysis related configuration changed.
  • When set to automatic, the null analysis will be enabled.
  • When set to disabled, the null analysis will be disabled.

@CsCherrYY
Copy link
Contributor Author

CsCherrYY commented Oct 24, 2022

When changing the null analysis settings, the users can easily encounter the buildship deadlock bug (eclipse/buildship#1148 and eclipse/buildship#1185). Since this PR will send a notification to let user change the settings, I'll investigate more about the bug before marking it ready for review.

update: I just left a comment in buildship side: eclipse/buildship#1185 (comment), since the dead lock can be easily reproduced when we change the JDT compiler options and we will pop up a notification to guide user to change the null analysis options, I suggest we hold this PR now and keep eyes on the bug.

You can now easily reproduce this:

  1. open a new java project
  2. set java.compile.nullAnalysis.nonnull and java.compile.nullAnalysis.nullable to empty array
  3. reload window
  4. the deadlock can be observed
  5. reload window again, everything works well

cc: @rgrunber @testforstephen @jdneo

@testforstephen
Copy link
Contributor

Let's wait and see if there are more regression issues reported about null annotation analysis. If so, I'd rather disable it by default.

@CsCherrYY CsCherrYY changed the title Set interactive mode by default for null analysis Add an option to enable/disable null analysis Oct 26, 2022
@CsCherrYY CsCherrYY marked this pull request as ready for review October 26, 2022 08:35
return this.updateAnnotationNullAnalysisOptions(true);
case interactive:
if (this.hasAnnotationNullAnalysisTypes()) {
String cmd = "java.compile.nullAnalysis.setMode";
Copy link
Contributor

Choose a reason for hiding this comment

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

This command can be implemented via delegateCommand, and store it in workspace cache.

@rgrunber
Copy link
Contributor

Do we want this in for the 1.17.0 release ? If so, we could let the release slip to the 31st (Monday).

@CsCherrYY
Copy link
Contributor Author

@rgrunber Let's wait for more feedback and hope the buildship bug can be fixed. I suggest this can be included in the next release.

Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Just a small fix to make regarding the message prompt.

Signed-off-by: Shi Chen <chenshi@microsoft.com>
Signed-off-by: Shi Chen <chenshi@microsoft.com>
@rgrunber rgrunber merged commit 9f54736 into eclipse-jdtls:master Nov 1, 2022
@rgrunber rgrunber added this to the End November milestone Nov 1, 2022
@CsCherrYY CsCherrYY deleted the cs-annotation-default branch November 11, 2022 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants