Analyzer crash when ! is applied to a null-shorting expression

[stereotype441]

The following code is accept without complaint by the VM but crashes the analyzer:

abstract class C {
  int? get x;
}

int f(bool b, C? c) {
  if (b) {
    return c?.x!;
  } else {
    return 0;
  }
}

main() {
  print(f(false, null));
}

I believe what is happening is that the analyzer gets confused about whether the ! should terminate null shorting (it should), and as a result break invariants of flow analysis; this in turn causes a crash when it tries to close the "then" branch of the "if" statement. I'm preparing a fix.

[lrhn]

(What do you mean by ! should terminate null shorting? Something like c?.x!.toRadixString(16) should work and shorten everything if c is null.)

[stereotype441]

@lrhn

(What do you mean by ! should terminate null shorting? Something like c?.x!.toRadixString(16) should work and shorten everything if c is null.)

Hmm, I admit that I didn't read the spec for this; I tested the behavior of the CFE and assumed it was correct. AFAICT, the CFE treats c?.x!.toRadixString(16) as equivalent to (c?.x)!.toRadixString(16); in other words, if c is null, it will fail.

I went back and re-read https://github.com/dart-lang/language/blob/master/accepted/future-releases/nnbd/feature-specification.md#null-aware-operator, and I think this behavior is consistent with it; there's no rule in that section giving special handling of to postfix !. But maybe this is an oversight; I also don't see any rule for ++ and --, and AFAIK we meant for those to participate in null-shorting as well.

My concern if we decide to have ! participate in null shorting is that it would render the code at the top of the bug report incorrect, because c?.x! would have type int? (which is incompatible with the return type of int). I worry about whether users will be able to figure out that the fix is to change it to (c?.x)!. (And I worry that if they did figure that out, the analyzer would get in the way by telling them that the parentheses are unnecessary, but of course that would be an analyzer bug and we could fix it).

[lrhn]

@eernstg We have formally specified null shortening now, right?
I would expect ! to be included in the suffix operators that are shortened, but please check.

I definitely do not want

 x?.foo?.bar;

and

 x?.foo!.bar;

to behave differently when x is null or x.foo is non-null. Users should be able to think that ?. can be replaced by !. when they know that the value will not be null.

[eernstg]

Given that ! is a selector when it is used to check for non-null values, I believe the consistent approach is to treat ! accordingly, by adding a rule along the following lines:

If `e` translates to `F` then `e!` translates to: `PASSTHRU[F, fn[x] => x!]`

[eernstg]

I believe this would ensure that x?.foo!.bar evaluates to null when x is null, and throws when x is non-null but its foo is null.

Also, it seems less useful to let x?.foo!.bar mean (x?.foo)!.bar, because this would imply that we didn't mean ?. in the first place.

[eernstg]

Proposed spec update here.

[stereotype441]

@lrhn @eernstg I'm going to move further discussion of the language change to a language issue, so that I can mark this bug as fixed when I get rid of the analyzer crash.

Moved to dart-lang/language#1163