Couple of usability issues with @Nullable/@NotNull

Hello,

There are couple of usability issues I hit while using @Nullable and
@NotNull

I would like to make a disclamer here. Those cases are not errors and behave
logically but cause significant usability issues bu producing lots of
garbage warnings

1. Simple getters marked @Nullable cause warning "Method Invocation
tn.getPriorVersion().getId() may produce NullPointerException"

if (tn.getPriorVersion() != null) {
sb.append(tn.getPriorVersion().getId());
}

I understand that two subsequent calls to tn.getPriorVersion() are not known
to return the same result so null check in first line does not guarantee
execution of second line but never the less working with @Nullable getters
produce tonns of useless warnings.

I could, of course, refactor it to

TemplateNode priorVersion = tn.getPriorVersion();
if (priorVersion != null) {
sb.append(priorVersion.getId();
}

but I would hate to do this evrywhere

I wonder you can propose any solution to this issue? Can code be analyzed to
check if between line 1 and 2 return of getPriorVersion() does not change. I
guess it is not possible in generic case. Or may be some rule based on bean
pattern to suppress this warning for simple getters?

2. Second issue is when using @NotNull for method parameters. If combined
with explicit check for null in method body a warning is issued for every
null check:
"Condition new_Owner == null is always false"

public TemplateContentImpl(long id, @NotNull ComponentNode owner) {
super(id);
if (owner == null) {
throw new IllegalArgumentException("Field 'owner' of class
TemplateContentImpl is required and can not be null");
}
setOwner(owner);
}

Again, while I understand why it is happening I find it very inconvenient as
it produces bunch of useless warnings. As discussed in thread "Handling of
(@NotNul param1, ...) at Runtime" it is not always feesable to not perform
explicit runtime checks but rely on IDEA generated ones

Any solution for this one?

Thank you very much

Alex Roytman


6 comments
Comment actions Permalink

The first issue is rather hard to solve: while theoretically we could analyze the body of each method matching it with "simple getter" pattern, this would slow down highlighting a great deal.

Fixing second issue is simpler to fix, and if you file a request to make an option in constant contions and exceptions inspection not to highlight explicit null checks for @NotNull parameters, this could probably make it even in 6.0.

0
Comment actions Permalink

Hello Eugene,

Not that fast, please. This way we'll throw away useful highlighting detecting
real problems like

void foo(Object o) {
int hc = o.hashCode();
if (o == null) {
throw IllegalArgumentException();
}
}

-


Maxim Shafirov
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

The first issue is rather hard to solve: while theoretically we could
analyze the body of each method matching it with "simple getter"
pattern, this would slow down highlighting a great deal.

Fixing second issue is simpler to fix, and if you file a request to
make an option in constant contions and exceptions inspection not to
highlight explicit null checks for @NotNull parameters, this could
probably make it even in 6.0.



0
Comment actions Permalink

Maxim,

in your example we have a stronger not null witnessed in code.
Seems a reasonable idea to distinguish those weak @NotNull assumptions, and strong evidences obtained with DFA.

0
Comment actions Permalink

Alex Roytman wrote:

1. Simple getters marked @Nullable cause warning "Method Invocation
tn.getPriorVersion().getId() may produce NullPointerException"

if (tn.getPriorVersion() != null) {
sb.append(tn.getPriorVersion().getId());
}


In my projects, I think many of these getters can only return null in an
initial phase. Once an attribute has been set to non-null it can never
again become null. This isn't true for all getters but at least this
part of the problem could be solved using a special annotation for such
fields and methods as suggested in
http://www.jetbrains.net/jira/browse/IDEADEV-3910.

2. Second issue is when using @NotNull for method parameters. If combined
with explicit check for null in method body a warning is issued for every
null check:
"Condition new_Owner == null is always false"


I think there should be a way of configuring this.

Unlike many other checks done by IDEA the @NotNull and @Nullable checks
can't be comprehensive and guaranteed to be correct since these
annotations aren't in the language from the beginning and aren't used in
many libraries. Not only does the javac compiler let violations slip
by, but it's also easy to accidentally pass a @Nullable value through an
unannotated field into a @NotNull parameter, or call
requiresNotNull(libraryMethod()) where libraryMethod() is unannotated
but can return null. IDEA itself doesn't even warn about this, so this
"is always false" warning could be wrong even though the rest of the
code is completely green.

Therefore stating that the condition "is always false" is almost a bug
in my eyes...

0
Comment actions Permalink

Eugene Vigdorchik wrote:

Seems a reasonable idea to distinguish those weak @NotNull assumptions, and strong evidences obtained with DFA.


+10

0
Comment actions Permalink

Eugene Vigdorchik wrote:

The first issue is rather hard to solve: while theoretically we could analyze the body of each method matching it with "simple getter" pattern, this would slow down highlighting a great deal.


Unless this analysis is done during parsing and saved as a flag for the
method, rather than during error highlighting -- then it wouldn't be
that expensive compared to everything else that goes on when a method's
code is parsed, right? But maybe that would be excessive, unless there
are other inspections that would benefit from knowing quickly whether
something is a simple getter or not.

0

Please sign in to leave a comment.