Warn when the type on the right of `is` is nullable but the type on the left is not

[DanTup]

Edit: This issue has changed a little from what was originally reported, and the consensus is that the example code should warn that the ? in is String? should be removed since c is not nullable. With that removed, the static type is as expected.

--

In the following code, the variable c is considered Object even though it seems like the when clause should ensure that is is String?:

void f(Object c) {
  switch (true) {
    case true when c is String?:
      // A value of type 'Object' can't be assigned to a variable of type 'String'.
      String a = c;
  }
}

I would expect it to be String? here (or maybe even String since it was never nullable in the first place). I'm not sure if this is a bug, a missing feature, or behaves like this for a reason I'm overlooking.

[bwilkerson]

Thanks for simplifying the example. I think I understand now what's going on. I suspect that inference isn't happening because String? isn't a subtype of Object. In the original example I hadn't (yet) noticed that inference will have made the variable non-nullable before getting to the cast.

It does raise the interesting question of whether there's ever a good reason to cast a non-nullable type to a nullable type, and if not whether we should have a warning for that case.

@lrhn @eernstg

[DanTup]

Oh, you're right - I didn't realise it didn't occur if I used Object? or just String.

I think a warning on as String? showing that the ? is bogus would be the best fix (but without that, it'd be nice if it was inferred that it must be String).

[srawlins]

To clarify the warning idea: I think checking a is B, where a has static type A, given unrelated types A and B, is generally fine. So maybe there are just some special union-like cases we could warn on:

• checking a is B? where a already has static type A which is non-nullable.
• checking a is FutureOr<X> where a already has static type A which is Future<Y>, for some X and some Y.
• checking a is FutureOr<X> where a already has static type A which is ... not Future?, for some X.

The FutureOr cases are making my head spin... maybe FutureOr is used so little, we could just special case the nullable case as an "unnecessary type comparison".

(You wrote "a warning on as String?", but I don't think that should ever be warned. For example, var x = 'hi' as String?; allows you to later assign null to x. But I think we could warn on is checks like this.)

[FMorschel]

Somewhat related to:

• Support static constraints on type casts and type tests language#4074
• [linter request] Don't use a non-nullable casting type when combined with conditional member access #59805

[bwilkerson]

The FutureOr cases are making my head spin... maybe FutureOr is used so little, we could just special case the nullable case as an "unnecessary type comparison".

I agree, I wouldn't touch FutureOr (although it's an interestingly similar). I think the nullability case is really the only one we should handle specially, and only the type on the right of is is nullable when the static type of the expression on the left isn't.

[lrhn]

In general, promotion is an intersection type, a type that is both the original type and the tested your. Dart doesn't have intersection types (other than static promotions of type variables), so you're only allowed to promote to a subtype, because the intersection of a type and a subtype is the subtype.

With union types it's tempting to do the point-wise intersection, (A | B) & (C | D) = (A & C) | (B & C) | (A & D) | (B & D).

When some of those smaller intersections are empty (which we can only really say for nullable types intersecting non-nullable types), they can be omitted, and if they're Null the result can be made nullable, If there is only one non-empty and non-Null intersection left, and that one is of a type and a subtype, then the results can still be represented.

That works for Object & String?.

It likely won't work for... anything but A & B? where B is a subtype of A.

On the other hand, it also only works when the ? on the tested type is unnecessary and can be removed. I don't think we need to change the language, if we give a warning that the ? is unnecessary.

[DanTup]

sgtm - I've updated the title of this about showing a warning (feel free to tweak if it can be worded better).

[FMorschel]

Just to point out, this is similar to my request (I understand the ordering is different; this is just a link for people who want to find that issue):

• unnecessary_null_check_pattern false negative #60975

Edit:

We also have another similar lint but for the other way around: cast_nullable_to_non_nullable.