Flow analysis. No promotion from `Object?` to `Null`

[sgrekhov]

The following test fails.

test1() {
  Object? s = "x";
  s as String;
  if (s is Null) {} // ignore: unnecessary_type_check
  s = null;
  s.expectStaticType<Exactly<Null>>(); // COMPILE_TIME_ERROR.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS
                                       //  'Exactly<Null>' doesn't conform to the bound 'Exactly<Object?>' of the type parameter 'R'.
}

Null is a proper subtype of Object? and the type of interest for s, so there should be a promotion.

cc @stereotype441

[lrhn]

I believe Null is explicitly excluded from assignment promotion, just like if (x == null) doesn't promote x to Null.

(Not sure we're consistent. I think if (x is! Object) and if (x is Null) do promote x from Object? to Null, but at least the use-case for double? v = maybeDouble(); if (v == null) v = 0; gives 0 a useful context type instead of just Null.)

[eernstg]

I believe Null is explicitly excluded from assignment promotion

I looked for that exact kind of rule in the feature specification, but did not find anything. It is true that the v == null and v != null tests do not promote v to type Null, but the type test ... is Null does not seem to have any special exceptions. Or maybe you have spotted it somewhere?

[eernstg]

Here is something!: https://github.com/dart-lang/language/blob/main/accepted/2.12/nnbd/feature-specification.md#null-promotion.

[eernstg]

Fun fact: This is actually the opposite, and clearly not what is implemented:

A check of the form e == null or of the form e is Null where e has static type T promotes the type of e to Null in the true continuation, and to NonNull(T) in the false continuation.

The flow-analysis spec clearly specifies that the == and != tests do not promote anything to null, and I'm convinced that this is working as intended. So the rule in the NNBD feature specification should be updated to agree with that.

I still haven't found anything that contradicts the assumption that Null is treated like any other type. This could also be the reason why it is recommended that null tests are expressed using ... == null or ... != null rather than ... is Null or ... is! Null: Standardization on the simplest possible approach is a good thing, and you don't actually want the promotions that Null gives rise to.

[lrhn]

Maybe == null and != null should promote. They didn't before Null Safety, but with null safety, we can safely distinguish Object and Null from Object?.
(Unless we still think having Null as context type is bad, but then we should start using entire promotion chains as context type for promoted variables, not just the last value.)

Also null == e should work the same as e == null, which the quote doesn't actually say.

[stereotype441]

@lrhn

Maybe == null and != null should promote. They didn't before Null Safety, but with null safety, we can safely distinguish Object and Null from Object?. (Unless we still think having Null as context type is bad, but then we should start using entire promotion chains as context type for promoted variables, not just the last value.)

Also null == e should work the same as e == null, which the quote doesn't actually say.

We have discussed having == null and != null promote to Null before, and I did some investigation into the consequences of it. My conclusion was that it wasn't worth it: dart-lang/language#1505 (comment)

This question gets raised often enough that I should probably add some informative text to the flow analysis spec explaining the rationale for the behavior we currently have. I'll add it to my list to do that.

[lrhn]

Proposal: do promote to Null, but special-case the context type provided by a promoted variable:

• if promoted to Null from a nullable type (there is a nullable type in the promotion chain), use the next type in the chain (which is a nullable type).
• if promoted from a FutureOr<S> type to T (T subtype of S, but not Future<S>) or to Future<T> (T subtype of S), use FutureOr<T> as context type.

The problems from promotion described here all come from losing information about the other branch of a union type, that would be useful to what would otherwise be a demoting assignment.
It's not just null, you can lose information with FutureOr too.

(And Never, I guess, but that's unreachable code anyway.)

[stereotype441]

FWIW, here's what I found last time I experimented with allowing promotion to Null: dart-lang/language#1505 (comment). In addition to the problems noted above with context types, I found that promoting to Null sometimes caused dead code to fail to type check (see the link for more details).

@lrhn

Proposal: do promote to Null, but special-case the context type provided by a promoted variable:

• if promoted to Null from a nullable type (there is a nullable type in the promotion chain), use the next type in the chain (which is a nullable type).
• if promoted from a FutureOr<S> type to T (T subtype of S, but not Future<S>) or to Future<T> (T subtype of S), use FutureOr<T> as context type.

The problems from promotion described here all come from losing information about the other branch of a union type, that would be useful to what would otherwise be a demoting assignment. It's not just null, you can lose information with FutureOr too.

(And Never, I guess, but that's unreachable code anyway.)

Interesting idea. I could certainly experiment with this. But to be honest, I'm having trouble coming up with a compelling use case for it. In a code path where a variable is known to be null, what's the benefit of flow analysis treating that variable as having type Null rather than some type T?? So someone can pass it to a function expecting a parameter of type Null? They could just pass null. And besides, nobody writes functions like that anyway.

I suppose we could detect some dead code paths, e.g. if (x == null) { if (x != null) { /* DEAD */ } }. But if that's the goal, there are other ways we could accomplish that. For example, flow analysis could have a "definitely null" flag for each variable (similar to how it has "definitely assigned" and "definitely unassigned" flags); that would allow us to identify this particular class of dead code without doing a promotion that loses type information.

[lrhn]

It's true that knowing that a value is definitely null is unlikely to be useful. It was promoted from a nullable type, so it's not like the type used to be useful and now isn't.

I don't remember if I have hit the issue for FutureOr, so I probably haven't. If I have figured out that a value is not a future, I'm usually happy and don't want to introduce a future again immediately.

So the benefits are theoretical.

[eernstg]

So what's the situation at this time? The original topic was promotion to Null via assignment, and it was reported that it does not take place in the particular example (perhaps is Null does not add Null to the set of types of interest?).

Here are a couple of declarations which are used repeatedly, so I'll just mention them once here:

typedef Exactly<X> = X Function(X);

extension<X> on X { 
  X expectStaticType<Y extends Exactly<X>>() => this; 
}

The following shows that the promotion via assignment does take place in a very similar example:

void test1() {
  Object? s = "x";
  s as String;
  if (s is int) {}
  s = 42;
  s.expectStaticType<Exactly<int>>(); // OK.
}

We're again declaring s to have type Object? and promoting it to Object in the declaration, and then to String based on the type check s as String. Next, we're testing for int (used to be Null, that is, another non-subtype of the current type of s which is a subtype of the declared type of s), and it is (apparently) added to the set of types of interest for s, because s = 42 does promote s to have type int (but s = null did not promote s to have type Null).

I think we would improve the overall consistency by adding Null to the types of interest for s in the original example:

test1() {
  Object? s = "x";
  s as String;
  if (s is Null) {} // ignore: unnecessary_type_check
  s = null;
  s.expectStaticType<Exactly<Null>>(); // This would then be OK.
}

@lrhn's context type enhancement is interesting (in particular, if we can come up with some use cases where it is really helpful), but I'd recommend that that proposal is handled separately, and this issue is used to improve consistency in the simpler way mentioned above.

[stereotype441]

In our discussion on Monday, @eernstg and I decided that our long term plan is to change flow analysis so that it never promotes to the types Never or Null. It's going to take a while to get to an implementation that's fully consistent in this regard, but I've already started moving in that direction. So for this particular issue, which is about flow analysis not promoting to Null in a certain circumstance, I'm going to say that the implementation is working as intended.

There is still some work to do to update the flow analysis spec to say that we don't promote to Null. So I'm going to leave this issue open (and assigned to me) until I've had a chance to do that.