Consider `cast_nullable_to_non_nullable` for core lints

[stuartmorgan-g]

See discussion in https://github.com/dart-lang/language/issues/1547. Casting Foo? to Bar (vs Bar?) with as is an easy mistake to miss.

[goderbauer]

For reference, the lint is enabled in the flutter framework code base and has successfully caught some issues there, I believe.

[mit-mit]

Should update GOOD to also include this 2nd case:

class A {}
class B extends A {}

A? a;
var v = a as B?;

cc @pq

[mit-mit]

Would like to undetstand how this affects Google3 before making a choice.

[goderbauer]

@mit-mit Do you know somebody who could drive trying this out in g3?

[mit-mit]

@pq are you looking at that?

[pq]

I can take a look. 👍

[pq]

I did a run and sent results out to a handful of folks for further review. At first blush the results seem valuable to me. If you want access (and are a Googler, sorry), let me know!

[lrhn]

I'm not strongly convinced that this lint is a win. It may help now, during nullability-migration, but I'm less convinced that it's a win in the long run, and that there won't be a significant number of false positives.

I deliberately write T? current; .... => current as T; to remove the extra nullability from something where I don't know if it's nullable or not. If that's not allowed, we have a problem. (The documentation talks about casting a nullable value to non-nullable, it doesn't say anything about potentially nullable or non-nullable. It's not clear what it does.)

If I cast from int? to int, it seems pretty clear-cut that that's what I want (but I agree that I should use ! for that).

If I cast from num? to int, it maybe be because I know something, and doing x! as int will look completely ridiculous.

So, should this be more about unrelated casts or down-casts?

(All in all, not getting my vote for adding to core lints.)

[stuartmorgan-g]

If I cast from num? to int, it maybe be because I know something

But it may also be that that you meant to cast num to int and didn't realize that you had a nullable value. Or that you meant to cast to int? but forgot the ?. That is exactly the sort of case that has caused actual bugs.

and doing x! as int will look completely ridiculous.

Why? Clearly indicating that you know that you are both removing nullability and changing the base type doesn't seem ridiculous to me, since as a reader I can be confident that both casts are intentional, vs the case above where I can't.

[lrhn]

I'm generally concerned about as, since it allows anything. And I'm convinced that nullability is that much of an orthogonal concept in typing that it requires special handling.

That said, if x! as int is guaranteed to be as efficient as x as int, and the lint is properly specificed so I know what it does with potentially nullable types on either or both sides, I think it's a reasonable lint to have for people who want it.

I'd still be against including it in the core lints. To get there, I'd want it to either:

• Catch something which is very likely a mistake. As in >50% of occurrences are mistakes. Not just that a mistake is possible. And not triggering on something that is the obvious thing to write if you happen to know that x does indeed contain an int. Or
• Catch something which is a very dangerous, subtle and hard to find mistake. Not something which will just throw the first time a null value gets there, I'd expect testing to catch that. (Not awaiting a future made it, because it's invisible at the point where it happens, and an unhandled error will be reported somewhere else entirely.) Or
• Something that is a very common mistake. Even if only way less than 50% of occurrences are wrong, there being lots of them numerically makes it worth to avoid the error. I haven't seen enough occurrences of this bug to consider that warranted.

Yes, you can make mistakes with as. Lots of them, it's as unsafe as dynamic, and optimally you should never use as. But as is like an assertion, which should never fail, you only use it when you know you're right, and in those cases, it's the shortest expression which does what you need of it. Writing assert(x is int); ... (x is int ? x : (throw "Not an int")) isn't adding to readability either, and is just as unsafe.

Which raises the question, if num? x = ...; ... x as int is bad, why isn't ... x is int ? ... : someFailure(). Why not require x != null && x is int there too?

[stuartmorgan-g]

I'm generally concerned about as, since it allows anything.

Yes, that's why my preferred solution would be https://github.com/dart-lang/language/issues/1547. That didn't go anywhere though, and if we're going to be stuck with having to use an operator that can do anything, even the things I did not intend to do, a default lint was the next best option.