[Null analysis] Object.isNull causes dead code warnings when previously it did not

[agentgt]

@stephan-herrmann

Prior to 3.37 ECJ doing the following with null analysis would NOT yield a dead code:

void someMethod(@NonNull s) {
  if (Objects.isNull(s)) {
    // usually throw NPE
    // this block is now reported as dead code
  }
}

Basically ECJ is now equating the above as:

void someMethod(@NonNull s) {
  if (s == null) {
    // this is correctly reported as dead code
  }
}

However the reason Objects.isNull is even used in the first place is defensive programming. While one could use Objects.requireNonNull in some cases that is not desired to have the stack one level deep from the actually method call and or a different exception is desired.

Furthermore I thought it was even documented (cannot seem to find the doc at the moment) that Objects.isNull is the way to avoid the dead code but maybe I'm misremembering.

Is there a configuration for that that I'm just missing?

[agentgt]

This looks like the change that broke it: https://github.com/eclipse-jdt/eclipse.jdt.core/pull/1469

@snjeza was that your intent to make Objects.isNull do that?

[agentgt]

I guess the reasoning is to support lambda stream filtering.

I was trying to avoid creating a custom util method for this as I would need it everywhere (that is the obvious workaround).

[agentgt]

I will likely close this bug in a little while as I think the stream filtering is more desirable than my use case and there is an easy albeit tedious workaround.

[snjeza]

@agentgt

void someMethod(@NonNull s) {
  if (Objects.isNull(s)) {
    // this block is now reported as dead code
  }
}

It is correct. s can't be null here. It is the same as

void someMethod(@NonNull s) {
  if (s == null) {
    // this is correctly reported as dead code
  }
}

[agentgt]

@snjeza

Just so we are clear. It is not the same. You made it the same. There is no language construct that says Object.isNull guarantees something is not null like == null. You picked a method in the JDK and made it special just like Objects.requiresNonNull is special in that it will cast without assignment.

For example Checkerframework which provides similar null analysis will not say that is dead code.

However am I correct the impetus for the change was to support stream filtering?

[agentgt]

@snjeza just to give you an idea of the impact of your change. Across all my projects I had to change close to 30 places where I was using Object.isNull for fail fast validation of something that would never be null internally but could be passed null by a consumer of the API.

Now I could have used Eclipse's annotations isNull or Checker frameworks version but I do not want the runtime coupling with those libraries (that is I only use org.eclipse.jd.annotation annotations ... in fact one of the major reason I use Eclipses annotation is because they are retention CLASS and not RUNTIME).

If the change is because someone is doing Object.isNull in normal imperative code... I don't agree. Like just do o == null.

Even for Stream programming it barely makes sense. Object.nonNull makes since for flow analysis but not Object.isNull.

My question is how did you ever run across that use case of wanting to use Object.isNull with flow analysis?

[snjeza]

@agentgt You may want to take a look at java.util.Objects.isNull(Object)

public static boolean isNull(Object obj) {
        return obj == null;
    }

[agentgt]

@snjeza I don't know if you are just being difficult. Do you honestly think I don't know what Objects.isNull does.

How do you recommend I check something is null when it can never be null?

How would you do:

public void someApiMethod(@NonNull String s) { // I don't normally use the NonNull but rely on package annotation
  if (s == null) {
     throw someException // usually NPE but sometimes can vary
  }
}

Now I could suppress the warning but the easier way that I did and I think many others is:

public void someApiMethod(@NonNull String s) { // I don't normally use the NonNull but rely on package annotation
  if (Object.isNull(s)) {
     throw someException // usually NPE but sometimes can vary
  }
}

That is I was using it exactly like Eclipses annotations Checks.isNull.

[snjeza]

My question is how did you ever run across that use case of wanting to use Object.isNull with flow analysis?

See MessageSend.detectAssertionUtility(int)

[agentgt]

But it is not an assertion method. Assertion methods fail and throw exceptions.

In great irony you use it write assertion methods precisely to avoid the null or dead code warnings.

[agentgt]

You still didn't really answer the question. Where are you using Objects.isNull to do flow inference? Like I thought it was for Stream but I can't even come up with an example for that like Objects.nonNull where one wants to do

Stream<@Nullable Object> sNullable; 
Stream<@NonNull Object> sNonNull = sNullable.filter(Objects::nonNull); 

I'm arguing and maybe I'm wrong that my use case is far more often. It is being used for defensive programming in an environment where null analysis is happening and you need to check if it is null even though the type is NonNull.

I am willing to live with the change and write my own isNull but I would love to see a use case for this over o == null.

[snjeza]

I don't know if you are just being difficult. Do you honestly think I don't know what Objects.isNull does.

Sorry!

[agentgt]

FWIW ultimately I think the change you did is probably the right change for consistency with all the other methods in Objects and at this point I am the one being difficult.

Perhaps I am one of the very few (ab)using Object.isNull in this way.

[snjeza]

@agentgt You may want to take a look at the original issue

[agentgt]

See the fundamental issue is that Eclipse probably needs another annotation like Checker and Intellij otherwise the list of methods that can do this flow inference is never ending (e.g. you saw the complaints of AssertJ).

For example Checker its like

    @EnsuresNonNullIf(expression={"#1"}, result=false)
    @Pure
    public static boolean isNull(@GuardSatisfied @Nullable @UnknownSignedness Object obj) {
        return obj == null;
    }

I suppose if IntelliJ shows Object.isNull as dead code than it is probably worth it but I wonder how many more of add this method to an unmodifiable/unconfigurable whitelist are going to keep happening (that then break downstream folks like myself).

[stephan-herrmann]

Some more comments:

ecj understands two kinds of "dead" code: For some situations even JLS requires that the compiler detects dead code. In these cases it's a "hard" error that cannot be suppressed. If null analysis (which is not specified in JLS) detects dead code, this is a different problem, which can be configured to different severity or suppressed using @SuppressWarnings

As per javadoc, the intention of Objects.isNull is indeed the use as a predicate, e.g., in stream operations like Stream.dropWhile(). In fact to fully support that idiom we probably still need to extend #1952. Anyway, the authors of this method obviously did not think of defensive programming.

Yes, JDT's Checks.isNull() is a better fit for defensive programming, as you already found. In all the years, I never understood why people consider a dependency on o.e.j.annotation as a problematic thing. It's hard to find a smaller library on the net, so size is obviously not the reason. Availability is also guaranteed, either from p2 repos, or from maven central. Why this reluctance?

As for hard coding knowledge about null-related methods: Users tend to expect that ecj "understands" relevant methods in core libraries, in particular in java.lang.*, java.lang.util.*. To make such thinks configurable via annotations is trickier. In particular the design space for such annotations leaves many more options than the core null annotations. At the time when I started implementing support for null annotations, JDT leadership was quite strict to admit only those annotations that are quasi standard, obvious or common sense. As a result implementing specific analysis for well known methods had a much lower barrier than adding more general purpose annotations. Even though it's obviously difficult to draw a line which ones to support ...

1461 indeed shows a situation where ecj didn't understand the possible real flows. And, yes, consistency with similar methods is a good reason to include isNull() in our special analysis.

@agentgt do you still think there is a bug needing a fix?

[stephan-herrmann]

Another thing that had been discussed: in theory a compiler could automatically add the "defensive programming" idiom when it sees @NonNull parameters in API methods (how to detect API?), so there wouldn't even be a need to spell it out in source code. This idea was rejected, however, because the compiler should always adhere to JLS / JVMS in what byte codes it emits, and not add "inventions" of its own.

Perhaps if we take a fresh look at this idea, such a feature request may be regarded more positively nowadays?

[agentgt]

@agentgt do you still think there is a bug needing a fix?

@stephan-herrmann I think @snjeza implementation is correct and will close the bug. I greatly appreciate both of your patience!

Ultimately I'm more annoyed with myself relying on I have to say sort of loophole (Object.isNull). As for why I don't depend on Eclipse annotations runtime is more for marketing also because many of my libraries are used in annotation processors themselves where classpath issues get complicated (e.g. processor-module-path vs process-path as one example). So no dependency is more desirable.

Also I had some issues with the annotations jar and gradle in the past where a few users asked why I even used eclipse annotations: https://github.com/jstachio/jstachio/issues/236

Of course the JSpecify guys have their own problems (javadoc link issues to come) so don't feel bad. I love the eclipse annotations and it is small and great!

I have been using all the null analysis tools as well as Eclipse and by far Eclipse is my favorite. It is so fast. I think it is the closest to JSpecify even if it is not the reference checker. So I am ashamed if I caused any lost time on working on null analysis because of this bug.

As for the Object.isNull and why I guess I went so much back and forth is the overall external annotation problem (even if those annotations don't exist yet). Ideally the library authors do the annotation. That is the annotation is kind of a contract!. Obviously that is not possible especially with the JDK. In this case and why I'm closing the bug I believe Object.isNull really is equal to o == null and in actuality it really is after few passes of the JIT inlining as that method is probably called way more than defensive programming methods. Luckily I agree with that basically "externally" applied annotation.

Because there are scenarios where it has become exceedingly annoying when others pick the annotations for libs that are not their own. For example the checkerframework thinks JUnit's assertNotNull is assertNotNull(@NonNull o). Luckily you can change that. They also make Objects.requireNonNull require nonnull arguments. Again you can change it. (BTW Checker can't detect dead code like Eclipse but if it could it probably would show the same error. Again why Eclipse IMHO is superior for null analysis.)

So while I agree with Object.isNull having the flow logic I am worried how many other methods with more debatable contracts will be added to the unconfigurable list.

Perhaps if we take a fresh look at this idea, such a feature request may be regarded more positively nowadays?

Yes I will revisit this tomorrow morning when my mind is fresh. I can't stress how much appreciate you guys taking the time to respond and the work in general!

[agentgt]

@stephan-herrmann

Sorry for the late reply in regards to:

Another thing that had been discussed: in theory a compiler could automatically add the "defensive programming" idiom when it sees @NonNull parameters in API methods (how to detect API?), so there wouldn't even be a need to spell it out in source code. This idea was rejected, however, because the compiler should always adhere to JLS / JVMS in what byte codes it emits, and not add "inventions" of its own. Perhaps if we take a fresh look at this idea, such a feature request may be regarded more positively nowadays?

I think I prefer how it currently is and am not sure I like the idea of Eclipse generating different code than javac but I have a feeling I'm misunderstanding you on that feature.

The defensive programming IMO is mostly a minor problem. A bigger problem is polynull. I don't want a checker polynull annotation but just a whitelist similar to the inferred null flow list.

For other libraries I'm willing to accept the equating @PolyNull to @Nullable but with the JDK methods particularly orElse it is very tedious.

And the worse part is the JDK authors just keep adding more PolyNull: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ScopedValue.html#orElse(T)

In case others see this and don't know why its painful is you have to do:

@Nullable Object result = o.orElse(null); // you cannot put anything else other than null because the result will always be nullable

if (result == null) {
  // do the actual orElse
  result = ...;
}

Even if you make static helper methods it is annoying to have to wrap an entire fluent stream with some static method.

I'm sure its been brought up a million times but it is my number one pet peeve with Eclipse null analysis such that I'm willing to even do some serious dev work or help in anyway to add just some bare minimum polynull of jdk methods.