Lint request - avoid_cast_from_null

[natebosch]

In mockito the any and anyNamed utilities allow flexible targeting for stub behavior and required being able to be passed to any argument position. Before null safety this was accomplished by giving them the value of null which could be passed to any argument type. With null safety, non-nullable arguments can't use this technique. With codegen, the Mock* subclass widens all argument types to be nullable. This requires that the type of an expression in a when argument needs to be typed as the Mock* class instead of the mocked type so at the position of the call passing any the argument is statically nullable.

This isn't always obvious, and a potential mistake during a migration is to try to cast the any to another type like when(someMock.someCall(any as int)). This cast is guaranteed to fail when running with sound null safety.

We should consider a lint which warns when an expression with type Null is cast to any non-nullable type, since that cast will always fail when running when sound null safety.

[srawlins]

FWIW we have a Hint for code like Null n = null; n!; (null_check_always_fails). Sounds like we could implement this check as a default-on Hint as well.

[srawlins]

I could implement code for is NonNullableType at the same time. This would be factoring in Null during dead code analysis, I think. As in if (null is int) {}.

[bwilkerson]

I'm guessing that isn't part of flow analysis. The question is: why not? Kind of seems like it would be useful in several ways. Are there false positives that we should be aware of?

[srawlins]

When you say, "that", @bwilkerson , do you mean the original ask? Like why doesn't flow analysis / type analysis determine that the type of null! is Never? I think this is a great question. @stereotype441 did a lot of related investigation in dart-lang/language#1505 and dart-lang/language#1959.

[mit-mit]

I'd like to suggest that we stop encoding the "severity" level (avoid, recommend, prefer, etc.) as part of the lint name. That makes it a bit odd if we later choose to reclassify a lint (e.g. move it from recommended to core).

Couldn't we just name something like this cast_from_null or no_cast_from_null?

[srawlins]

I believe we moved away from including severity level in names.

[bwilkerson]

When you say, "that" ...

I was asking about all of the ideas in this thread related to null values and flow control.

I'd like to suggest that we stop encoding the "severity" level ...

Yes. We made that decision years ago and have been trying to enforce that in all of the newest lints. We generally name diagnostics in a way that indicates what the problem is, so cast_from_null would be better, but cast_from_null_non_nullable might be better.

[stereotype441]

When you say, "that", @bwilkerson , do you mean the original ask? Like why doesn't flow analysis / type analysis determine that the type of null! is Never? I think this is a great question. @stereotype441 did a lot of related investigation in dart-lang/language#1505 and dart-lang/language#1959.

I think some questions are getting confused here. Let me try to clarify with my understanding of the current state of things:

• Why "doesn't" type analysis determine that the type of null! is Never. Actually, it does.
• Issues dart-lang/language#1505 and dart-lang/language#1959 are about a slightly different question: should a type test promote a variable from a type other than Null to the type Null? You can look at my analysis for more details, but the short answer is, that although it seems logical to do so, it would break some pretty common and intuitive coding patterns, so I think we shouldn't.
• Why don't we mark the then-clause of if (null is int) as unreachable code? We totally could do that, we just never thought of it. If we do decide to do this, I would strongly urge us to make the change in the flow analysis engine itself rather than bolting it on as a separate analyzer feature; it's good to keep the analyzer's notion of dead code consistent with flow analysis' notion of dead code, otherwise users will be surprised to see types flowing from dead code into living code and affecting it (perhaps we should call this undesirable phenomenon "zombie types"). My plate is full, but if anyone feels strongly about wanting to add this feature, let me know and I'd be happy to point you to what needs to be changed, do code reviews, and help make sure the appropriate breaking change process is followed.

[srawlins]

Why "doesn't" type analysis determine that the type of null! is Never. Actually, it does.

Sorry, not null!. I meant null as int. This does not appear to be Never today.

[stereotype441]

Why "doesn't" type analysis determine that the type of null! is Never. Actually, it does.

Sorry, not null!. I meant null as int. This does not appear to be Never today.

Ah, ok. The reason for that is because with expressions of the form e as T always have static type T. We definitely could change that, but I worry that it would confuse people.

Another option would be to go ahead and let null as int have static type int, but mark any code that follows it as unreachable. That would be totally doable. As I said above about the if (null is int) case, if we do decide to do this, I would strongly urge us to make the change in the flow analysis engine itself rather than bolting it on as a separate analyzer feature, for consistency reasons. And although my plate is full and can't take on the change myself, I would be happy to assist anyone who wants to work on this.

[srawlins]

I'm seeing cases internally like

void main() {
  [].firstWhere((e) => e==0, orElse: () => null as Never);
  print('yay');
}

Here, the closure is guaranteed to throw, but changing flow analysis to mark code which follows the cast as unreachable would not flag this program. I'll land the Hint which flags the cast.

[natebosch]

FWIW we have a Hint for code like Null n = null; n!; (null_check_always_fails). Sounds like we could implement this check as a default-on Hint as well.

Making this a hint SGTM. This should have zero false positives.