Conditionals and "cast_nullable_to_non_nullable"

[RohitSaily]

Dart SDK version: 3.8.0-24.0.dev (dev) (Wed Jan 22 12:05:43 2025 -0800) on "macos_x64"

Details for cast_nullable_to_non_nullable indicate the intention of the rule is to not have implicit casting from a nullable type to one that is not nullable. Shouldn't this also apply to conditional checks? This aligns with its stated motivation

DON'T cast a nullable value to a non nullable type. This hides a null check and most of the time it is not what is expected.

The rest of this post shows multiple ways the nullability can be made more explicit, although for simplicity, standard style(s) may need to be selected. Also, making the nullability more explicit does result in slightly more code, but that seems to be the point of the rule.

Case Patterns

BAD

void function(final Object? object)
{	if (object case final String s)
		{}
}

GOOD
Something that directly indicates the value was nullable.

void function(final Object? object)
{	if (object case final String s?)
		{}
}

void function(final Object? object)
{	if (object! case final String s)
		{}
}

void function(final Object? object)
{	if (object case final String? s when s!=null)
		{}
}

Type Checks

BAD

void function(final Object? object)
{	if (object is String)
		{}
}

GOOD
Again, something that directly indicates the value was nullable.

void function(final Object? object)
{	if (object!=null&&(object is String))
		{}
}

void function(final Object? object)
{	if ((object is String?)&&object!=null)
		{}
}

[RohitSaily]

For case patterns, I believe the first two GOOD examples are preferable because they are both concise and clear. I'm not fond of the GOOD examples for type checks. It would be nice to have syntax like object? is String or object is? String.

[bwilkerson]

The case pattern isn't doing an unconditional type cast. It's doing the equivalent of an is test. The condition will only be true, and the variable s will only be assigned a value, if object is String.

The type checks are the same. Testing before casting isn't hiding the fact that a runtime exception might occur.

[RohitSaily]

So if I understand correctly, the purpose of the linter rule is strictly to avoid accidental formal runtime errors? Wouldn't it also be beneficial for it to serve as a safeguard to ensure nullable values are being handled as such, thus helping avoid informal errors?

[bwilkerson]

IIUC, the purpose of the rule is to flag casts (uses of as) that might produce a runtime exception because the type of the left-hand operand (the thing being cast) is nullable but the right-hand operand (the type to which it is being cast) is non-nullable. It's very narrow, but I don't think there's any intention to flag cases where a value of null is being ignored. Instead, the rule is concerned with making the possibility of an exception explicit in the code. It's fairly explicit in the code when a non-null value is being operated on but there's nothing happening for the non-null case.

[lrhn]

To be clear, there is absolutely nothing wrong with:

void function(final Object? object) {
  if (object case final String s) {
    // ...
  }
  // ...
}

which isn't also wrong with

void function(final Object? object) {
  if (object is String) {
    String s = object;
    // ...
  }
  // ...
}

Which I guess is why you list both.

There is nothing wrong with the latter at all, it's completely idiomatic.

There is no requirement that you do an if (object != null && object is String) because that would be redundant.
Doing something unnecessary is more confusing to the reader, because then they start looking for reasons to do taht, and there are none.

If you want to make extra redundant checks, then I won't stop you, and if someone wants to write the lint, they can, but I will definitely not want this as a recommended lint, or to ever suggest anyone use it.

[RohitSaily]

@bwilkerson I see, that makes sense. It may just be me, but that wasn’t clear from the linter rule description. Perhaps it should explicitly mention its goal is to mitigate errors and is not for readability or style.

@lrhn I agree that object != null && object is String is less readable and I don’t like it either, it was just the only way I could think of to make it explicit.

I still believe that having some form of indication that the nullability was intentionally acknowledged and is being explicitly handled can be useful. It’s more akin to exhaustiveness checking of cases and, not the same as, but more inline with using type annotations even if the type is inferable (which there are linter rules for).

Nullable types are very common and this can help ensure they are handled exhaustively. It can also be beneficial to a reader to be able to see at a glance that a value which is not type annotated is nullable and was intentionally handled as such. For example

Object lookup()
{	if (key case final String key)
	{//Do something with non-null key.
	}
	//Nonnull key is not handled! This is causing nonexpected
	//behavior. Reader has to probe to figure out why.
}



Compare this to



Object lookup()
{	if (key case final String key?)
	{//...
	}
	//Reader can immediately infer that key is nullable and there
	//is a null case that is not handled.
}

This becomes more valuable when the code involved is more distributed in the code base or there is simply a lot of code to consider.

I would not make this a recommended linter rule at the moment, but would still suggest it to be an available rule.

[lrhn]

I'm not seeing the confusion here.
There is an if, so you can assume that it doesn't handle all cases.
It's very clear about what it handles, String values, and says absolutely nothing about what it doesn't handle.

Having an unnecessary ? on the pattern makes no difference in behavior (and that makes it's presence confusing in itself). It suggests that the type of key is nullable, but that doesn't tell us whether the type is String?, Object? or a FutureOr<String>?.

I can see how it does give the information that key case String _ was intentional and didn't just forget the ? of it key case String? _, but if the branch uses the String value for anything, then it clearly intended it to be a String.

If the goal is to give a local hint about the type of key, just write the type.
Either String? key = this.key; or key as String?; or whatever makes sense.
Adding ? to the pattern doesn't say anything about which types are left after the if, other than that it probably includes Null. (Otherwise you'd get a warning about the ?.)

this can help ensure they are handled exhaustively

Don't see how it helps. Not in an if.
If you want help with exhaustion, use a switch. Otherwise you still need to look up the type of key to know what is left.
Now, if the type of key was actually String?, I'd recommend a pattern of _? instead of writing String _, so the String _ pattern itself tells you that you must look at the type of key. You can't assume anything about what's left from the pattern alone.

some form of indication that the nullability was intentionally acknowledged and is being explicitly handled

Adding ? doesn't handle nullability, so it's not a good sign.

[RohitSaily]

To clarify, I have no problem now after being informed by yourself
and @bwilkerson. I suppose this has become more of a
idea/suggestion.

There is an if, so you can assume that it doesn't handle all
cases.

I agree. That is completely sensible and expected.

Having an unnecessary ? on the pattern makes no difference in
behavior (and that makes it's presence confusing in itself). It
suggests that the type of key is nullable, but that doesn't
tell us whether the type is String?, Object? or a
FutureOr<String>?.

That is true. I suppose the reason I still hold my view point is
that I don't understand how that is different than the ?.
operator. It isn't necessary but by design that a?.b?.c makes
the nullability explicit. It very well could have been implicit or
optional to specify it that way. It also does not convey
the specific types but simply indicates nullability. The ?.
doesn't provide complete information but nonetheless it
provides useful information, especially to a reader who may
not have the full context of the code they are looking at.

I can see how it does give the information that key case
String _ was intentional and didn't just forget the ? of it key
case String? _, but if the branch uses the String value for
anything, then it clearly intended it to be a String.

The idea was to conveniently and minimally indicate that the value
is nullable much like the ?. operator. I suppose this can get
into more of a language suggestion than a linter suggestion
depending how it's approached e.g. another approach might be
if (value? case Type t), but that would make this a language
change.

If the goal is to give a local hint about the type of key,
just write the type. Either String? key = this.key; or key
as String?; or whatever makes sense. Adding ? to the pattern
doesn't say anything about which types are left after the if,
other than that it probably includes Null. (Otherwise you'd
get a warning about the ?.)

The goal was to do that but to express it locally where it's being
used and minimally, without adding lines of code or comments.
Also, to do it conditionally.

this can help ensure they are handled exhaustively

Don't see how it helps. Not in an if. If you want help with
exhaustion, use a switch. Otherwise you still need to look up
the type of key to know what is left.

The switch is definitely sensible when there are multiple cases
but compare the following

if (value case final String s?)
{//Do something.
}

This code communicates that the value is nullable and is more
concise than a switch statement

switch (value)
{	case final String s:
		//Do something.
	default:
		break;
}

I suppose what I view as the main utility of the suggestion is that it
makes something explicit for a reader that they otherwise would
have to rely on prior knowledge or dig into code to determine, just
like the ?. operator. It's very convenient to provide this information
so the reader immediately gets that information at a glance.

final value=somethingThatGivesAValue();
if (value case final String s)
{//Do something.
}

I may not necessarily remember value is nullable. Practically,
readers are not necessarily working on complete
knowledge. They may be jumping in to a block of code to
figure something out or be making use of partial knowledge.

some form of indication that the nullability was intentionally
acknowledged and is being explicitly handled

Adding ? doesn't handle nullability, so it's not a good sign.

I stated that poorly, I meant that it is intentionally not being
handled at that point of the code.