manifold-systems / manifold

Manifold is a Java compiler plugin, its features include Metaprogramming, Properties, Extension Methods, Operator Overloading, Templates, a Preprocessor, and more.
http://manifold.systems/
Apache License 2.0
2.42k stars 125 forks source link

Incorrect IntelliJ warning: Method invocation may produce 'NullPointerException' #527

Open EotT123 opened 1 year ago

EotT123 commented 1 year ago

Describe the bug Extension methods are a great way to deal with nullness, eg see the example here:

public static boolean isNullOrEmpty(@This CharSequence thiz) {
  return thiz == null || thiz.length() == 0;
}

String name = null;
if (name.isNullOrEmpty()) {
  println("empty");
}

This works great. However, when using a value returned by a method annotated with @Nullable, IntelliJ warns me: Method invocation 'xxx' may produce 'NullPointerException'.

Eg:

// a method that returns a @Nullable object
public @Nullable MyObject doSomething() {
  if(...){
    return new MyObject();
  }else{
    return null;
 }
}

// extension method which accepts a @Nullable object
public static boolean validate(@This @Nullable MyObject thiz, Predicate<MyObject> predicate) {
  return this != null && predicate.test(this);
}

// 
doSomething().validate(<predicate>); // --> Method invocation 'validate' may produce 'NullPointerException'

This is clearly not the case: Even if the value is null, it is still correctly handled, and no NPE is thrown.

To Reproduce Steps to reproduce the behavior:

  1. Annotate the return value (eg MyObject) of a method with @Nullable
  2. Write an extension method for the return type (MyObject), which accepts a @Nullable MyObject
  3. Call the method of the extension on a @Nullable value.

Expected behavior IntelliJ should not warn me about a potential NPE

Desktop (please complete the following information):

rsmckinney commented 1 year ago

Yep, these NPEs should be suppressed, great idea!

CC007 commented 8 months ago

While technically true, this would make extension methods behave different from normal methods when it comes to nullability.

I think it would be better to invest into null-safe operators like ?. for null-safe method calls or ?? to provide a default value when the object is null.

In Kotlin you'd have to mark a receiver specifically to be nullable, if you want to be allowed to call an extension method on a variable that contains the null value. This also adds clarity, since now you know if it's expected if you're allowed to use null in the call or if you'd need to use the ?. operator to make it a null-safe call.

As an example where there's a difference between nullable extension methods called with ?. and .:

public static String myToString(@This @Nullable Object thiz) {
  if (thiz == null) return "null value";
  return thiz.toString();
}

public static final main() {
  String foo = "not null value";
  String bar = null;

  String fooString1 = foo.myToString(); // returns "not null value"
  String fooString2 = foo?.myToString(); // returns "not null value"
  String barString1 = bar.myToString(); // returns "null value"
  String barString2 = bar?.myToString(); // returns null
}

So for consistency with normal methods, I think the following would be preferred if the receiver is not specified to be nullable:

public static String myToString(@This Object thiz) { // note the absence of the nullable annotation
  return thiz.toString();
}

public static final main() {
  String foo = "not null value";
  String bar = null;

  String fooString1 = foo.myToString(); // returns "not null value"
  String fooString2 = foo?.myToString(); // returns "not null value"
  String barString1 = bar.myToString(); // throws NullPointerException
  String barString2 = bar?.myToString(); // returns null
}