nnbd: Analyzer not assigning nullability to generic parameter, inferred return type; bang in index-postfix-expression treated wrongly; propagation in collection elements; broken non-nullable promotion of type parameters

[derolf]

This ticket tracks various bugs in the Dart Analyzer regarding NNBD.

PR for fixing: https://github.com/dart-lang/sdk/pull/38614

BUGS FIXED IN THE PR

class ClassT<T> {
  T? nullable() => null;
}

void fooT() {
  final c = ClassT<String>();
  c.nullable()?.length;                  // <-- error here, correct: no error since it can be null
}

void test() {
  // ignore: unnecessary_statements
  () {
    if ('foo' == 'test') {
      return null;                          // <-- error here, correct: return type should be String?
    }
    return 'test';
  };
}

class Class<T> {
  T? operator [](String key) => null;
}

void foo() {
  final x = Class<String>();

  x['X']!.length;                   // <-- error here, correct: bang is fine since [] returns String?
}

class Class {
  final String member;
}

void foo() {
  Class? x;
  x?.member?.length;   // <-- error here, but correct since ? turns `x.member` into `String?` 
}

typedef Callback = void Function(void);
void foo({Callback? foo}) {}  // <-- error MISSING_DEFAULT_VALUE_FOR_PARAMETER

List<int> foo(String? s) {
  return [
    if (s != null) s.length,   // <-- error
   ]
}

NOT FIXED YET IN THE PR

void foo<T>(T? v, void Function(T value) action) {
  if (v == null) {
    return;
  }
  action(v);             // <-- error
}

[derolf]

@scheglov @ stereotype441 @mit-mit I made fixes to all these bugs above in the linked PR. Would you consider the PR?

Especially, the approach I introduced in https://github.com/dart-lang/sdk/pull/38614/commits/410d210b08f6fe1ecd6c5978dbc2df3404dd1835 might be interesting since you will come along the same problem.

[scheglov]

Thank you for these test cases. We fixed all these in analyzer over these weeks.

I don't agree about this test case, x?.member?.length is executed with short-circuiting, and the type of x?.member when it is executed is String. So, the warning on member?.length is valid.

class Class {
  final String member;
}

void foo() {
  Class? x;
  x?.member?.length;   // <-- error here, but correct since ? turns `x.member` into `String?` 
}

[derolf]

@scheglov I am trying to follow the short-circuiting argument.

what about:

class Class {
  Class() : member = "1";
  final String member;
}

void foo(Class? x) {
    x?.member?.length; 
}

Should it compile or fail?

The corresponding TypeScript code does compile:

class Class {
  constructor() {
    this.member = "1";
  }

  readonly member : string;
}

function foo(x: Class | null) {
  x?.member?.length;  
}

[derolf]

Okay, I understand now that short-circuiting bails our after the first ?, still leaving the whole expression nullable, e.g. c?.m.length + 1 fails with nullable error.

Yet, this looks like Dart extremely deviates from the semantics of ?. chaining of any other modern language (TypeScript, Kotlin).

Short-circuiting looks non-obvious to me. This will give lots of people that also code in Kotlin, TypeScript, Swift a headache...

[scheglov]

Well, expressions x?.member?.length is null-safe, and x?.member.length is null-safe per se.

But the whole expression can be null, so any additional invocations to it are not null-safe. And analyzer will highlight x?.member.length in x?.member.length + 1 and report that error: The expression is nullable and must be null-checked before it can be used. You don't have to wait until run time to know this.

As for deviation from other languages, our language designers might know reasons better than me. @leafpetersen @lrhn

[lrhn]

Actually, Dart is now converging with some other languages having a ?. operator in allowing it to short-circuit to the end of the current selector chain.

In C#, x?.member.length is valid and does not cause an error even when x is null.

Example:

String s1 = null;
int? len1 = s1?.Substring(1).Length;
Console.WriteLine("len1: " + (len1 ?? -1));
int? len2 = (s1?.Substring(1)).Length;
Console.WriteLine("len2: " + (len2 ?? -1));

This code prints len1: -1, then it throws when it tries to read Length from null. The difference is that the first s1?. extends to the end of the selector chain. The parentheses in the second computation ends that chain before the .Length operation.

With null-aware shortcircuiting, Dart will behave the same as this.

The advantage of short-circuiting is more precise non-nullable types. Arguably, if we know that every member access after a ?. requires another ?., why do we ask the user to write it? The compiler can insert the ? automatically. However, even that is not as good as short-circuiting because it might still hide a later null.

Imagine a class

abstract class Cloo {
  Cloo? tryCombine(Cloo? other);
  Cloo combine(Cloo other);
  Cloo reverse();
}

If you do not have null-aware short-circuiting and you write

Cloo? cm = ...;
Cloo cd = ...;
Cloo? cc = cm?.combine(cd)?.reverse();

you won't notice that the second ?. is not necessary. Dart will tell you that the second ?. is unnecessary because if it gets there at all, then the receiver is not null. Or if you wrote:

Cloo? cc = cm?.tryCombine(cd).reverse();

you would be warned that it might not work. If you actually intended it to work (and intended to use combine), this error would be hidden from you if you had to write ?. in both places.

So, short-circuiting here gives more precise warnings, and it's consistent with some other languages that have a static nullability anaysis.

The only issue I'd worry about here is that we can only extend this to selector chains, and not, for example, a following + 1. That's mainly a matter of making the short-circuiting predictable and understandable.