Fixes #1398. Fix tests that use unreachable code after Never

[sgrekhov]

No description provided.

[stereotype441]

LGTM

[eernstg]

Looks good, except that I don't understand why we would stop reporting a warning for the use of ?. when the receiver has type Never.

[eernstg]

Why would we stop reporting a warning for ?. because x cannot be null?

[eernstg]

Same for all cases below. Have the analyzer based tools actually stopped emitting this warning?

[sgrekhov]

I think warnings are not reported for the unreachable code. @stereotype441 could you please confirm?

[stereotype441]

Aha, I see what happened. An unintentional consequence of Flag additional code as unreachable due to types Null and Never. is that if (x is Never) no longer promotes the type of x to Never, so these errors no longer appear.

Personally I think that's a good change, so I'm inclined to leave things as they are and just update the CHANGELOG to clarify what happened. My rationale is that promoting a variable's type to Never is unhelpful to users for the same reason that promoting a type to Null is unhelpful--it effectively hides the previous (unpromoted) type, making it impossible for the compiler to help detect errors like misspelled methods and passing the wrong number and type of arguments to methods. The new behavior of simply marking the code unreachable, but leaving the type of the variable alone, is better.

I'll speak with some folks today to confirm whether others think this is a good idea.

[stereotype441]

Oh, and @sgrekhov, I should mention: if you want to continue testing behaviors that result from an expression having type Never, you can still achieve that by having a function whose return type is Never.

For example instead of this:

void test(var x) {
  if (x is Never) {
    x?.toString();
    ...

You could do this:

Never f() => throw '';
void test(var x) {
  f()?.toString();
  ...

[stereotype441]

@sgrekhov, I've discussed this behavior change with @leafpetersen and it looks like we're going to stick with the current behavior. So I think you should feel free to either (a) land this change as is, or (b) change this test to use the Never f() => throw ''; approach I described above, so that it continues exercising the behavior of Never, as intended.

[eernstg]

I'd recommend using Never f() => throw ''; in order to ensure that the tests actually test the treatment of member accesses on a receiver of type Never.

Another thing is that when x is not promoted to have type Never then lines 27-29 in LanguageFeatures/nnbd/expression_typing_A02_t01.dart are accepted (that is, there is no error there) because x has type dynamic. Surely that's not what the test is intended to test.

[sgrekhov]

Changed the tests to use Never foo() => throw Exception();. Please review

[eernstg]

I don't understand why we would have this update. Is it from a spec document?

[eernstg]

Same comment as last time we had the same description, and same comment below.

[eernstg]

Ditto.

[eernstg]

I think these tests should be changed from using if (x is Never) { ...Never n = x.something ...} to ...Never n = f().something ... where Never f() => throw '';, such that the description of the test matches the actual behavior of the test.

[eernstg]

I'd recommend using Never f() => throw ''; in order to ensure that the tests actually test the treatment of member accesses on a receiver of type Never.

Another thing is that when x is not promoted to have type Never then lines 27-29 in LanguageFeatures/nnbd/expression_typing_A02_t01.dart are accepted (that is, there is no error there) because x has type dynamic. Surely that's not what the test is intended to test.

[eernstg]

LGTM