dart-lang / language

Design of the Dart language
Other
2.66k stars 205 forks source link

Provide a safer-for-nullability version of `as` #1547

Open stuartmorgan opened 3 years ago

stuartmorgan commented 3 years ago

https://github.com/dart-lang/sdk/issues/45473 is an example of a general foot-gun of as in an NNBD world: as doesn't care at compile time if you are casting away nullability even if doing so is invalid. Ideally, as should simply not allow removing nullability, requiring ! for that, in much the same way that most C++ cast operators won't let you remove const, so you have to use const_cast for that—making as's current behavior analogous to the much more dangerous C-style cast.

Obviously the ship has sailed on that (and maybe there's some specific language design reason it wasn't viable in the first place), but hopefully it's not to late to add a new cast operator that's like as, but will fail to compile if it would remove nullability. That could then be used preferentially, and any use of as could be treated with extreme scrutiny (and linted). This would allow for writing safer versions of code that needs to downcast Object->Foo and Object?->Foo? without letting people accidentally compile code that's doing Object?->Foo (as in the issue linked above).

jakemac53 commented 3 years ago

Just as a note here this was the root cause of a bug in the sound null safety release of the test package, https://github.com/dart-lang/test/pull/1455/files#diff-dd263a0546610e8df9a78ad7fe70f8afb0c0784b15b0039c6c87da503308fe7eR66.

mnordine commented 3 years ago

@stuartmorgan I think this lint rule is relevant: https://dart-lang.github.io/linter/lints/cast_nullable_to_non_nullable.html

stuartmorgan commented 3 years ago

Yes, that lint time helps; someone mentioned it offline after I filed this.

I would still argue that it's worth considering a new operator though, since having the cast operator be dangerous by default and require opting in to a lint rule to become safe isn't ideal. (There are also secondary effects of it being a lint, such as not being able to easily tell in reviews, or when reading code in other repositories, whether it's in a context where its safety is being checked.)

lrhn commented 3 years ago

We have the problem that as is inherently completely unsafe, so it's not more unsafe to use it on a nullable type. It always allows you to cast from different type than you thought it was. I'd also always prefer promotion to casting if at all possible. If not possible, it's likely because I derive a property of the cast value from another value, like sharing a single data field based on the state of some object. (if (_state == _uninitialized) _stateData = (_stateData as stream).listen(...);).

An as which doesn't allow null, let's call it as!, would be a cast which only worked on subtypes of Object, where the plain as works on subtypes of Object?. Definitely possible, but also very easy to forget to use, since as works everywhere without any possible warning.

It doesn't need to be a language feature. It's easy to introduce your own cast operation as:

extension CastExt on Object {
  T as<T>() => this as T;
}

then you can do nonNull.as<int>(); and not be allowed to do it on a nullable type. A simple static method like this should be easy to inline, so I wouldn't worry about performance.

mateusfccp commented 3 years ago

@lrhn I also prefer promotion over as, and agree that an as! is not viable. We had a linter rule avoid_as that hinted to prefer promotion instead. Why it has been deprecated? I think it would be a nice alternative to have a linter rule for this instead of as!.

stuartmorgan commented 3 years ago

Thanks for the discussion; feel free to close if it's not something you feel is worthwhile. I do want to address a couple of small points though:

We have the problem that as is inherently completely unsafe, so it's not more unsafe to use it on a nullable type.

While I understand that this is technically true from the standpoint of the language mechanics, I think it ignores that nullability adds a significant new category of error that can happen in actual usage. In practice, there are now two conceptually—from a developer standpoint—different kinds of casts: fundamentally changing type (treat this Object as a Foo) and removing nullability, whereas there used to be only one. An operator that can do two different things when you only meant to do one is, in practice, more dangerous than an operator that only did one thing.

(Again, see the comparison to C vs C++ casts above.)

It always allows you to cast from different type than you thought it was.

Absolutely, but it didn't allow me to intentionally do one thing (cast an Object to a String) while accidentally doing a different thing that I didn't mean to do (remove nullability by casting an Object? to a String).

Coming at this from the other direction: why does ! exist instead of just telling people to use as to cast from String? to String? I'm not able to come up with an argument for that which isn't equally applicable as an argument for an operator that can cast Object to String and Object? to String? but not Object to String?.

Definitely possible, but also very easy to forget to use, since as works everywhere without any possible warning.

The same is true of using as instead of !, but have you ever forgotten to write ! instead of as when your intention was purely to remove nullability?

It doesn't need to be a language feature. It's easy to introduce your own cast operation

While true, this doesn't do a very good job of addressing common use cases I mentioned above cases like reviewing code and looking at code in other people's repositories. If everyone has to go out of their way to make a custom safer thing and create a local style to prefer it, then code becomes more siloed and less easily understood by others.

As a concrete example: nobody is ever going to answer a StackOverflow question with code that does .as<Foo>() in a place that happens to needs a cast, because they would have to add the extension, an explanation of the extension and why they think it's a good idea, etc., all of which is irrelevant noise in the answer. They're just going to use as. And everyone seeing that, and countless other examples, will simply absorb that as is a reasonable way to cast. (This can be extrapolated to, e.g., all official Flutter sample code, for the same reason.)

If there were a safer operator, many people would use it everywhere, including samples, and people would (rightly) call out samples that didn't use it.

jakemac53 commented 3 years ago

This is also problematic for null safety conversions, which is where I have seen it cause real issues (in multiple places now).

It is very common to cast values out of a Map or some other object (myMap['obj'] as String). None of these had ? on the type previously because that syntax didn't exist, and when opting in you get no static errors about these casts. That makes this type of code very error prone to convert to null safety (really this applies to any existing cast, but it seems to come up a lot when pulling values out of things like maps or arg parsers).

natebosch commented 3 years ago

I don't understand how adding a new operator makes the migrations any more safe if it doesn't stop you from using the old one.

Is this a tooling issue? Should we change dart migrate to change instances of as String to as String? when the left side is nullable or dynamic?

jakemac53 commented 3 years ago

Ya I agree that any new mechanism is not likely to see much usage given the prevalence of as today. Maybe we could roll out a breaking change where you have to use as!, in some newer language version where we introduced that. Or maybe just make it a warning to use the old as where it also removes nullability.

Either of those are pretty scorched earth approaches though... while I agree as is very easy to misuse now I also am not sure it is so bad as to take such a strong approach to the problem.

Another use case to consider here are the cast methods which really face the same type of problem, and maybe even whereType as well?

mnordine commented 3 years ago

Hold up, you had me wondering.

print([1, null, 3].whereType<int>();

Output: (1, 3)

print([1, null, 3].whereType<int?>();

Output: (1, null, 3)

Works as expected.

jakemac53 commented 3 years ago

Works as expected.

Phew! lol

I guess that there was this weird asymmetry in pre-nnbd Dart where null is int is false but null as int was ok :D

stuartmorgan commented 3 years ago

I don't understand how adding a new operator makes the migrations any more safe if it doesn't stop you from using the old one.

I agree it doesn't help with migrations; I filed this as a general concern, not a migration concern.

Is this a tooling issue?

No; I want to write (manually) code that doesn't use as, and tell people in reviews not to use as, and file issues about docs that should stop using as, but there's no replacement available to do any of those things.

Ya I agree that any new mechanism is not likely to see much usage given the prevalence of as today.

I'm confused by this argument. While there's no better replacement available for as, then of course people are using as. That doesn't mean that people wouldn't switch to a better thing if it were available. (Non-null-safe Dart code was ubiquitous not that long ago, and many people are adopting null safety without being forced to.)

And again I would point to !. If the argument that everyone would still keep using as even if a better option were available, why are people using ! instead of as to remove nullability?

natebosch commented 3 years ago

I want to write (manually) code that doesn't use as

We could consider a lint that disallows as casts from nullable to non-nullable. Then instead of expression as String the lint would force you to use expression! as String.

jakemac53 commented 3 years ago

We could consider a lint that disallows as casts from nullable to non-nullable. Then instead of expression as String the lint would force you to use expression! as String.

That seems like a pretty reasonable solution to me

bwilkerson commented 3 years ago

That lint has been suggested before. The question I have is whether that should be a lint or whether that ought to be part of the same feature that @leafpetersen and @srawlins were working on to provide an additional level of type safety. I don't know what the current status of that work is, but at one point it also included disallowing implicit downcasts from dynamic.

stuartmorgan commented 3 years ago

There is a lint, but that doesn't do a very good job of addressing some use cases (like example code); see https://github.com/dart-lang/language/issues/1547#issuecomment-812515811 and my reply.

lrhn commented 1 year ago

There is the option of using:

extension AsCast<T> on T {
  R as<R extends T>() => this as R;
}

which only allows downcasts, and

extension SafeAsCast<T extends Object> on T {
  R safeAs<R extends T>() => this as R;
}

which doesn't allow casting a nullable value.

void main() {
  num? x = 1 as dynamic;
  print(x.as<int>()); // Works.
  print(x!.safeAs<int>()); // Works.
  print(x.safeAs<int>()); // Error.
}

(Not sure I'd recommend using the name as in this way, because it might block introducing .as<int> as a special selector that works like the current cast.)

stuartmorgan commented 1 year ago

Yes, we had this conversation already above; this earlier comment explains why I don't think "everyone makes their own cast operator" is a good solution to the general problem.

lrhn commented 1 year ago

And if we keep this open for another two years, I'm sure I'll suggest it again :)

lrhn commented 6 months ago

This feels like C++ casts. Should we have different casts for different things?

// Only allows downcast, and not removing `?`
downcast<T>(o)
// Only removes `?` from type, like current `!`
nullcast(o)
// Requires giving a common supertype 
sidecast<Object, int>(o)
// Like current `as`
unsafecast<T>(o)

No reinterpret cast.