-
Notifications
You must be signed in to change notification settings - Fork 783
Null annotations - observations and discussion #4321
Comments
Another observation I want to add regards JUnit test cases: JUnit tests
assuming the above this will give me compiler warnings which state that
because the non null |
While looking at the PRs i found in #4319 something where the code has been changed to satisfy the null annotation compiler:
has been changed to
Do we really have to iterate over the entryset whenever we loop through a map? |
Class members of type Map<K,V>
If thats the case then it is not correctly handled by the Eclipse Annotation processor. But this would be solved as soon EEAs are used. Implicit null checks This could very easily lead to a change in programming style which is required to adopt to the null annotations: To express the semantically blank check a It depends. Dependency injection For dependency injected variable the null check should perhaps be deactivated at all (if possible). We should have a look at which options are available using the Eclipse nullness annotation processor. |
If you need the key and the value you should use an entrySet (this does not depend on nullness annotations at all). As long as a map get function could return |
I would like to add another one to the list: Inline Field InitializationIs there any trick to make JDT be just a little smart about those? (argh, just realized it's the same as #4321 (comment) - this not only applies to unit tests...) |
I agree that the JDT compiler should recognize that your |
Right, the warning vanishes. But it gets worse, so no, I can't do that if I'm using OSGi service references - because then it will give me an error on my service instance field: If I then tell it that my field is @nullable (which is a lie, because it will never be null), I get the error down there because JDT feels pretty confident that the contract is violated: Big Fail! Plus: the "Dependency injection" point described in #4321 (comment) hits. To me this whole nullable story with JDT feels pretty half-baked. The annotations are nice for documenting the API, but rather a pain when trying to use them for static analysis. |
Dependency injection Using nullness checks involves that we need to differ between an optional injected reference (e.g. OSGi DS using a cardinality of 0..1 or 0..n) and a mandatory injected reference. And surely also between "we trust that injected variables are injected" and "we want to check that the mandatory injection has been done". But let's assume we never want to check that the injection framework does it work correctly. Just added this case for completeness. I assume we can neglect the optional injected reference because this one should be marked as nullable as it could be missed. Agree? For OSGi there are at least two cases:
Dependency Injection per field The variable is set and unset by the injection framework.
Initialize the variable with a dummy object is possible but doesn't make sense as we rely on the injection framework and don't want to create a class implementation that e.g. contains a Don't initialize the variable at all. @SuppressWarnings(SprWrnValues.OSGI_MANDATORY_SERVICE_REF)
protected @NonNull Object ref; The nullness check framework knows I does not need to care about the missing initialization and also every developer knows "the variable is not initialized because it is a mandatory OSGi service reference". Dependency Injection per bind ... Mostly the same situation as above and the additional situation that you will assign I don't know if there are methods to inform the Eclipse nullness checker about that situations, too. |
As long as there is no option by the official JDT tools, we can use our own workaround to use dependency injection and nullness aware code. Let's have a look at the static method nonNulledNull So instead of writing public class ComponentFoo {
protected ServiceBar serviceBar;
protected void setServiceBar(ServiceBar serviceBar) {
this.serviceBar = serviceBar;
}
protected void unsetServiceBar(ServiceBar serviceBar) {
this.serviceBar = null;
}
} without using nullness checks, we could write @NonNullByDefault
public class ComponentFoo {
protected ServiceBar serviceBar = NullnessUtils.nonNulledNull();
protected void setServiceBar(ServiceBar serviceBar) {
this.serviceBar = serviceBar;
}
protected void unsetServiceBar(ServiceBar serviceBar) {
this.serviceBar = NullnessUtils.nonNulledNull();
}
} and use nullness checks for the whole class (so, all non dependency injected code). If there is sometime a better solution, we could easily look at all locations |
Hi guys,
|
On the transition between 1. and 2. I would like to make a proposal: |
Aren't the most of the warnings caused by the classes that doesn't use nullness annotations at all or correctly? If so, it would be better if we found a agreement about solutions / workaround for problems like dependency injection and fix the warnings. |
@maggu2810 as stated above: for me the workarounds to please the new tooling do not raise the quality of the code, they make it worse. I can see the benefit from null annotations and I would like to make a concrete proposal which incorporates @sjka`s proposal for API documentation:
This way could have the best for external consumers (documented and validated API), external tooling can still check the whole codebase, ESH-only tooling will only check the obvious violation. wdyt? |
Which one of the above topics? We have a class that states that a variable is non-null. I don't think it is only a workaround but we state that we violate the non-null annotation here by intention. |
Regarding the dependency injection I think an injected service should be |
From the compiler (javac) point of view also a I agree that it should be set to null in unbind to let the GC do its job (I hope I have written it above, don't know exactly). I used "workaround" for
For me an annotated API is already a big win. If you prefer to disable nullness checks to reduce warning, okay -- I don't like that decision but if the others agree... I would prefer
|
Regarding eeas, please see #4217 (comment) I agree with @htreu that we should not implement I also agree with him, that now as we have some experience with those null annotations, we should go back to the main idea that we had in mind for them: annotate our public api with what is written in the javadocs. That is because unfortunately it turned out that the tooling with its full static analysis is not able to scan the code as it is (without any workarounds for this tool). |
Don´t get me wrong, I also like the goal and the bigger picture we try to reach with null annotations. Just right now there are imho too many open questions. So lets start by disabling the warnings and concentrate on annotating the API. All other steps (EEAs, fix internal implementation, fix/extend tooling) can then be done step by step. |
My observations:
This will give a warning
These errors go away when putting the nonnull values into local variable and using it instead of accessing multiple fields. |
One more observation, which probably is the same class as above, but it can be fixed differently:
This will give warning for json.states that can be null. |
Setting the severity setting to the values proposed by @htreu helps me to eliminate half of the 60 false-positive warnings I had remaining in my binding.
For the extra variables, sometimes the code looks agreeable with them, but sometimes it looks awkward, for example:
Without the warning:
Do you think such solution for the warnings sake only is acceptable? Or maybe we should change the severity further? |
FMPOV the tooling should add value and not confuse during coding. With the examples @ppieczul provides at least some of the potential null pointer access warnings seem not to be that obvious. I would rather decrease the severity then have warnings that never go away w/o tricks or SuppressWarnings directive. |
@ppieczul Thank you for your observations with the null annotations! I have just had a look on this again so we hopefully find some final settings for these annotations. ExamplesFirst I will go through the examples from @ppieczul:
This is a
This will make the warning disappear and is also IMHO better coding style.
In my IDE this results in an
Again I would say that
I somehow cannot reproduce this example. I have build my own simple one like this:
So even the two logging statements my IDE detects that the null check has been done.
This last example has no warning or error in my IDE, maybe your settings were not correctly applied (see the example above)? OSGI service injectionThis part stays a real issue with the current implementation of the nullness analysis of the Eclipse IDE. Technically correct the definition for a service should look ass follow, i.e. the service should be declared as
If instead we declare the injected service as The Eclipse nullness analysis should offer another annotation, maybe @injected where it knows that this variable will be somehow initialized and disables the "potential null pointer access" for these variables... potential null pointer access warningIf we disable this warning we obviously do not have a problem with it for dependency injections, so let's disable it and everything is fine? Unfortunately not... Consider this example:
With the null annotations we explicitly want to define those cases where we intentionally return a |
After reading the analysis from @triller-telekom I agree we should stay on the "warning" level for potential null pointer access. We gain more then we loose on confusing code style (which is not confusing at all on a second look). |
The problem is how should be identified if a function that access the injected service is called if the service is active only? E.g. a method could be called in the bind function of a service before it is activated and so on. |
One additional thing for #4321 (comment) 1.: We must not use Regarding @triller-telekom´s last comment: we can get around some of these warnings when we actually do annotate fields with |
On another note, I´m also okay with the proposal to only decrease the default scope by |
I have just created PR #4513 which re-enables "Unchecked conversion from non-annotated type to @nonnull type" and adds support for external annotations. In contrast to PR #4217 I am using a zip file for the annotations that will be hosted by an external project (possibly lastnpe.org). I am downloading the zip file via a maven plugin or an eclipse setup task. But we still can use parts of PR #4217, I am not replacing everything! @maggu2810 Can you please have a look on my PR? I left some more details in the commit message. WDYT? |
edit: never mind, works as expected |
Just some feedback after having worked on some "real" code with those settings:
|
Regarding your second point: If we disable the check "Unchecked conversion from non-annotated type to @nonnull type" and the use of external annotations we only get errors if an annotation for the external code exists, but not a warning that we should add an external annotation to the library. In other words there is no warning that the non-annotated library code is used as a return value for a @nonnull method for example. So we are not encouraged to add an external annotation for that library. But IF an external annotation for the library code exists we will still get an error in the IDE id the annotation says @nullable and we want to use it as @nonnull. If we are fine with that we can certainly let this setting disabled. @maggu2810 Since you have worked with annotations (i.e. checker framework in your case) before. Can you estimate briefly what impact it has if we only use annotations for return values and parameters?I.e. reduce their scope to:
Will the analysis results be much less accurate if type parameter, array contents etc. are omitted? I also tried to combine the parameters for the @NonNullByDefault annotation in a static array definition, because @martinvw mentioned that a large definition is ugly. But array variables do not exist at compile time where annotations are evaluated so this is unfortunately not possible. |
Really, that is wired. But perhaps I did not understand that comment correctly. |
@triller-telekom I found out another mechanism to handle injected references or let's say the exclusion of some field. Assume this class: import org.eclipse.jdt.annotation.NonNullByDefault;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
@Component
public class ComponentImpl {
public static interface DummyService {
void foo();
}
private DummyService service;
@Reference
protected void setDummyService(DummyService service) {
this.service = service;
}
protected void unsetDummyService(DummyService service) {
this.service = null;
}
public void foo() {
service.foo();
}
} If you add If you would like to avoid errors for the injected service reference your suggestion has been to exclude every field. My suggestion is: Exclude only the fields that are injected. Annotate the field you want to exclude with @Component
@NonNullByDefault
public class ComponentImpl {
public static interface DummyService {
void foo();
}
@NonNullByDefault({ PARAMETER, RETURN_TYPE, TYPE_BOUND, TYPE_ARGUMENT })
private DummyService service;
@Reference
protected void setDummyService(DummyService service) {
this.service = service;
}
protected void unsetDummyService(DummyService service) {
this.service = null;
}
public void foo() {
service.foo();
}
} WDYT? As we only need the "long annotation" for injected stuff that should be ignored, I assume it is something we could do. If we find another way some time, we can change the lines again. -- edit -- You could also disable the assignment using ` @NonNullByDefault({})
private DummyService service; |
@maggu2810 I appreciate that you invested the time to figure out another way for our dependency injection problem. However I think that writing
or ONCE over the class. I think we already agreed that we would like to have as little annotations as possible? So out of your experience with the checker framework and annotations could you please say something about my question from #4321 (comment) ? I.e. if you could estimate the impact of using only If |
It is not the same, you disable the nullness check for every field, regardless if it is an injected one or not. If no fields are respected, how should a non-annotated field be interpret? Adding |
@maggu2810 this is really nice! I like the method of disabling the assignment the most. With that we could again concentrate on the default we would like to use on all classes. Exceptions to injected or other, later initialised fields can then be handled like you described. |
True, I am totally with you, these are 2 distinct things. However our intention was to annotate the public API, which is certainly not a private field... I am just trying to figure out a configuration that makes it most usable for our goal, even if that means we loose some precision in the null analysis.
We already ignore it because we disable "Unchecked conversion from non-annotated type to @nonnull type". So everything that is not annotated (especially return values from 3rd party libraries) can theoretically go everywhere.
Since @htreu jumped in here while I was still typing... Maybe we should give it a try. So to sum it up:
Warning settings stay as they are at the moment within the IDE. Are you OK with that? |
This is certainly right, I guess @kaikreuzer refers to the behaviour on typed Collections, where the non-nullness also is propagated to the generic type arguments: In an Map<String, String> myMap = new HashMap<>();
myMap.put("key", "value"); // succeeds
myMap.put(null, "value"); // error: "Null type mismatch: required '@NonNull String' but the provided value is null"
myMap.put("key", null); // error: "Null type mismatch: required '@NonNull String' but the provided value is null"
String value = myMap.get("key");
if (value != null) { // warning: "Redundant null check: The variable value cannot be null at this location"
// do smth
} This is not about annotating the method signature but about annotating the generic types used. The definition of the map would look like |
@htreu Ah, okay. But also this topic will be eliminated as soon as external annotations are used. So, to move forward (at least to remove the big points we identified at the moment), we need
|
@htreu Do get more into details: |
@maggu2810 Do you mean that @htreu should explicitly declare his map within a
And by "will be solved by external annotations" you mean that these external annotation will have such a definition with the PS: I just read your email, will have a look on your branch now. |
You guys will sort this out :-) From what I understand the EEAs with an |
@htreu You are right. The method signature win. @triller-telekom I have comment "my obersavations" (they could be wrong) about the EEA 0,1,+ syntax on the respective project already (lastnpe's external annotations). |
Just an update on this "null analysis front" :) Maven does not stop the build on null analysis errors (see #4604) until we have a more recent tycho version. So we intentionally accept the fact that people MIGHT contribute stuff that breaks, but since the IDE checks are still on we assume that this won't be much code and could easily be fixed once we have the new tycho version. I have edited our coding guidelines regarding the null analysis usage, especially the part about excluding injected service fields in PR #4546:
Also I have updated my PR "Corrected null annotations in thing package" #4550 to match these guidelines. |
I'd like to add one thing to this discussion (unless you'd also like me to add situations where a null is marked even though there is no chance of a null) - you simply can't eliminate null checks on method parameters (ie Objects.requireNotNull, etc). Both should be required! Two reasons:
Bottom line - I really like the null annotations but they CANNOT replace runtime checks |
@tmrobert8 Thank you for participating in our discussion. I have created PR #5360 to explain why we introduced those null annotations. Does this answer your concerns? |
Hi! Thanks for having this discussion in the open. It captures my experiences with null annotations well. Is there a meaningful difference between: @NonNullByDefault({})
private DummyService service; @SuppressWarnings( "null" )
private DummyService service; Also, how are the workarounds working out for you? |
Without the |
After being confronted with the new null annotations for the first time now I would like to show some issues I came across and discuss with you how we manage to deal with them. The original PR and discussion was here: #4080.
All my examples assume the
@NonNullByDefault
annotation on class level. This implies all members, return values and method parameters are expected to be non null.Class members of type Map<K,V>
The compiler treads members of type Map<K,V> as
@NonNull Map<@NonNull K, @NonNull V>
. This implies that a call tomyMap.get("invalidKey")
will return a non null value, even if the Map does not contain the key. In the process the following code will be reported as dead code:This behaviour does introduce a NPE which is dangerous.
Implicit null checks
Given a class with constructor
MyThing(String param1)
which is all non null we get the following behaviour during creation:This could very easily lead to a change in programming style which is required to adopt to the null annotations: To express the semantically blank check a
StringUtils.isBlank(str)
will become astr == null || str.isEmpty()
which is technically equal but imho feels wrong.Dependency injection
Class members which are injected by OSGi DeclarativeService (DS) are not supported well: When doing no extra annotation, the often used mechanism to set these members to
null
on unbind will resolve to error. When giving the member a@Nullable
(which semantically is correct) all invocations now require a null check since the compiler warns about potential null pointer access.Both "positions" do not reflect the actual behaviour since we know the framework will take care of the dependency (in most cases) and we can be sure not to have a null pointer at hand.
Some of the above issues where already addressed here but my feeling is we need to have another discussion on how to deal with the new situation. Right now my IDE floods my with warnings which do not seem to resolve magically.
The text was updated successfully, but these errors were encountered: