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 #2747

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Oct 21, 2022

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

requires eclipse-jdtls/eclipse.jdt.ls#2279

add new preference java.compile.nullAnalysis.mode.

  • When set to interactive, 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 (default), the null analysis will be disabled.

@CsCherrYY
Copy link
Contributor Author

CsCherrYY commented Oct 26, 2022

Since switching between enabled and disabled mode causes the deadlock of buildship easily (see eclipse-jdtls/eclipse.jdt.ls#2279 (comment)) and there are some regressions about null analysis encountered (microsoft/vscode-java-debug#1247 and #2743), I suggest we make disabled the default value and once the buildship deadlock bug has been addressed, we can make interactive 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
Copy link
Member

@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.

This part of the change looks pretty good. Just some minor things to address.

package.json Outdated
"automatic"
],
"default": "disabled",
"markdownDescription": "Specify how to enable the annotation-based null analysis. When set to `interactive`, you will have a chance to turn on the null analysis when the null annotations are detected in your project dependencies.",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need to explain what happens when interactive is set. If you look at other options we have (eg. java.configuration.updateBuildConfiguration, java.project.importOnFirstTimeStartup) that use interactive, we never go into details. Maybe it's understood that the setting implies some kind of UI prompt when the capability is about to change/enable.

Also, there's 3 other places in previous changes where we use ".. if it exists in your project dependencies". I would avoid this in favour of just ".. if it exists in project dependencies"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me remove the detail description of interactive and make the phase more readable.

README.md Outdated Show resolved Hide resolved
Signed-off-by: Shi Chen <chenshi@microsoft.com>
@rgrunber rgrunber added this to the End November milestone Nov 1, 2022
@rgrunber rgrunber merged commit 1d8c85d into redhat-developer:master Nov 1, 2022
@jdneo
Copy link
Collaborator

jdneo commented Nov 11, 2022

Is java.compile.nullAnalysis.enabled a typo? I didn't find that setting. Do you mean to java.compile.nullAnalysis.mode? @CsCherrYY

@CsCherrYY CsCherrYY deleted the cs-null-mode branch November 11, 2022 06:35
@CsCherrYY
Copy link
Contributor Author

CsCherrYY commented Nov 11, 2022

looks like an outdated configuration name, let me correct it.

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