Enable NonNullByDefault for lspe4.plugin #1009
Conversation
sebthom
force-pushed
the
null-safety
branch
4 times, most recently
from
01484a1
to
562fd0c
Compare
org.eclipse.lsp4e/src/org/eclipse/lsp4e/callhierarchy/CallHierarchyView.java
Outdated
Show resolved
Hide resolved
From my side, yes, I like the idea. I still need to go through the PR to have a better idea. |
|
I'd be fine with @NonNullByDefault as much as possible. |
sebthom
force-pushed
the
null-safety
branch
8 times, most recently
from
8b8cd52
to
42620c8
Compare
sebthom
changed the title
|
@rubenporras I am aware that this is a huge PR. Enabling annotation based null analysis affects everything, so I had to adjust all classes of the plugin. I hope no other PRs are merged before this one, as that would lead to substantial effort in resolving merge conflicts. |
sebthom
force-pushed
the
null-safety
branch
4 times, most recently
from
de42a41
to
0753ef3
Compare
|
@sebthom , you just asked for the review at the beginning of my summer holidays ;) I will check it today. |
@rubenporras no worries, I hope you are well rested :) |
| @@ -21,12 +21,12 @@ | |||
| </arguments> | |||
| </buildCommand> | |||
| <buildCommand> | |||
| <name>org.eclipse.m2e.core.maven2Builder</name> | |||
| <name>org.eclipse.pde.ds.core.builder</name> | |||
There was a problem hiding this comment.
could we undo the changes in this file?
There was a problem hiding this comment.
We could but evertime I checkout or switch a branch with Eclipse, the entries get modified/reordered, so I guessed that is how new Eclipse or m2e versions like it.
| org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jdt.annotation.Nullable | ||
| org.eclipse.jdt.core.compiler.annotation.nullable.secondary= | ||
| org.eclipse.jdt.core.compiler.annotation.nullanalysis=enabled | ||
| org.eclipse.jdt.core.compiler.annotation.owning=org.eclipse.jdt.annotation.Owning | ||
| org.eclipse.jdt.core.compiler.annotation.resourceanalysis=disabled |
There was a problem hiding this comment.
Is resourceanalysis=disabled related to this PR? If so, how?
There was a problem hiding this comment.
that is the default setting. it is set by eclipse 2024-06 automatically.
org.eclipse.lsp4e/src/org/eclipse/lsp4e/callhierarchy/CallHierarchyContentProvider.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/callhierarchy/CallHierarchyContentProvider.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/codeactions/LSPCodeActionsMenu.java
Show resolved
Hide resolved
| this.contextInformationLanguageServersFuture.get(CONTEXT_INFORMATION_TIMEOUT, TimeUnit.MILLISECONDS); | ||
| } catch (ResponseErrorException | ExecutionException e) { | ||
| if (!CancellationUtil.isRequestCancelledException(e)) { // do not report error if the server has cancelled the request | ||
| if (!CancellationUtil.isRequestCancelledException(e)) { // do not report error if the server has cancelled |
There was a problem hiding this comment.
the indentation of this comment is a bit weird now (at least on the GitHub review. Could we revert the change, or make it shorter so that the auto-format leaves it all in one line?
There was a problem hiding this comment.
I could, but this is done by the current Eclipse formatter settings and I prefer to use the formatter settings because otherwise we have to manually update readjust the code everytime when we do code changes and use the formatter.
There was a problem hiding this comment.
I would actually really appreciate if we could harmonize the formatter settings of all LSP4E modules (they currently differ) and in a single commit reformat the whole project with these settings.
| } catch (OperationCanceledException | ResponseErrorException | ExecutionException | CancellationException e) { | ||
| if (!CancellationUtil.isRequestCancelledException(e)) { // do not report error if the server has cancelled the request | ||
| if (!CancellationUtil.isRequestCancelledException(e)) { // do not report error if the server has cancelled | ||
| // the request |
There was a problem hiding this comment.
same comment about the format of the comment
...sp4e/src/org/eclipse/lsp4e/operations/linkedediting/LSPLinkedEditingReconcilingStrategy.java
Show resolved
Hide resolved
|
@rubenporras thanks for the review and your trust! |
|
Let cross fingers 👍 |
I'd propose the introduction of NonNullByDefault Eclipse annotation on package level for improved null safety.
The current code base is very inconsistent regarding null value handling and contains hidden null pointer issues that e.g. result in issues like this #913
My goal is to progressively enable the NonNullByDefault on package level(s).
I have good experience in TM4E moving to NonNullByDefault. I found hidden bugs in the control flow and it now gives me increased confidence in working with the API and during refactorings.
@mickaelistria @rubenporras do you support this change?
Reference: https://help.eclipse.org/latest/topic/org.eclipse.jdt.doc.user/tasks/task-using_null_annotations.htm