Should the migration to NNBD include eliminating implicit downcasts?

[leafpetersen]

Dart 2.0 assignability allows an assignment from something of type A to something of type B if A is a subtype of B or if B is a subtype of A. With non-nullable types, the implication would be that assignment of a nullable variable to a non-nullable variable would be silently accepted. This seems very contrary to the spirit of the NNBD change, and is inconsistent with what we expect from other languages.

One possible solution is to special case non-nullability with respect to assignability. For example, add a side condition that B must be nullable if A is.

An alternative is to take this opportunity to remove implicit downcasts from the language entirely as part of the NNBD opt in. Implicit downcasts seem to be a common source of confusion since they push a surprising amount of type checking to runtime. See background issues below for more discussion.

Topic specific discussion issues:

• Implicit cast operator syntax
• Casts during iteration

Background issues:

dart-lang/sdk#33749
dart-lang/sdk#31410
dart-lang/sdk#30402
dart-lang/sdk#32661
dart-lang/sdk#25368
dart-lang/sdk#29718
dart-lang/sdk#30385
dart-lang/sdk#29547
dart-lang/sdk#29548

[munificent]

YES PLEASE.

[matanlurey]

I am already planning the Piñata to ship to SEA upon this change.

[leafpetersen]

A key question is, how intrusive would this be to existing code bases. That is, would eliminating implicit casts add large amounts of syntactic noise to existing code bases. Here are a few examples of packages that have done this migration that I've found:

• analyzer cli
• analyzer server and plugin
• watcher (part of a larger set of changes)
• test (part of a larger set of changes, but mostly implicit cast related)
• file (lots of unrelated cleanup in that commit)
• markdown (some unrelated lints enabled in the same commit)
• mime (lots of unrelated changes)
• logging (lots of unrelated changes)
• process (some unrelated changes)

cc @mit-mit @munificent @lrhn

[munificent]

This isn't a super clean commit because it also includes a bunch of --no-implicit-dynamic fixes, but here is the commit where I fixed most implicit casts in my game. This file in particular shows casts I needed to handle processing some JSON. Overall, there weren't too many changes for casts and some bugs were discovered.

Here is the change in the SDK where I enabled --no-implicit-casts for test.dart. It also includes a bunch of --no-implicit-dynamic changes too. It's sort of a worst case example because, at the time, test.dart heavily used a giant map as its core data structure. This change was in fact intended to find the places where that map was used so I could replace it with an actual typed object.

[leafpetersen]

I filed an issue for discussing implicit casts during iteration here: #264 .

[eernstg]

Note that it could damage readability if we give up the distinction between arbitrary casts and downcasts:

// Assume that we have some unrelated classes `A` and `B`,
// and a class that implements both, `C`.
class A {}
class B {}
class C implements A, B {}

Here's some code as we could write it today (when accepting implicit casts):

main() {
  A a = C();
  B b = a as B; // Cast required, this is an "arbitrary" cast.
  C c = a; // Downcast, permitted.
}

If we just require all casts to be explicit then we'd have the following, which drops the hint that some casts are more risky than others:

main() {
  // All casts must be explicit.
  A a = C();
  B b = a as B;
  C c = a as C;
}

The point is that we could have explicit casts, and still avoid some of the verbosity, and still maintain the distinction between "less surprising" and "more surprising" casts (using !! for 'cast to context type if assignable, otherwise error'):

main() {
  // All casts must be explicit, but downcasts can be `!!`.
  A a = C();
  B b = a as B;
  C c = a!!;
}

[leafpetersen]

This is accepted.

[leafpetersen]

cc @natebosch

[natebosch]

Reopening to see if we can take this further.

The implementations currently allow implicit casts from dynamic, but not any other type. We are losing an opportunity to bring even more safety and disallow casts from dynamic as well. It's unfortunately very easy to accidentally fall into an implicit cast and be unaware of it. I was very eager to be able to drop the extra analyzer config for this after null safety - and sad to learn that I'd have to keep the extra config, and that new Dart users unaware of the config will continue to have this trap to fall into.

My understanding is that the main argument for allowing implicit casts from dynamic is the noise at the boundary of things like working with JSON. We have had really positive experiences with implicit-casts: false, and the noise around JSON has not been overwhelming.

Can we reconsider the strength of this change?

[lrhn]

I suggested that you should be able to do implicit downcasts from dynamic.

Any dynamic value is inherently unsafe!

You can call any method on it without a static type check. If (you think that) you know that a value is a String, nothing prevents you from calling string methods on the value. Then it seems weird and inconsistent that you have to go through hoops to assign the same value to a String variable.
If anything, that makes it more likely that the user will keep the dynamic type for longer and catch errors later in practice.

I'd be much happier working towards fewer implicit dynamic types. For example, we could consider changing instantiate-to-bounds of raw types to use Object? instead of dynamic as the top type. I think that's one of the primary causes of implicit dynamiccs. That would avoid the actual problem, which is an expression being dynamic and unsafe unnecessarily, rather than complicate the code where something is dynamic deliberately.
When a value is intended to be dynamic, the right thing is to trust the user to use the value correctly. Trusting them to call methods, but not assign, is not consistent.

So,. I do think that you should consider removing the "no implicit downcast" lint as long as you keep the 'no implicit dynamic" lint. The type dynamic is still a part of Dart and it fills a role, so we should not break its usability when it is being used in that role.

Would you recommend that we disallow dynamic invocations as well? Because that would be consistent with disallowing the downcasts. You would always have to cast a dynamic value to a known interface before using it, either as s receiver or as an argument.
(There wouldn't be any need for the dynamic type then, other as a tag on Object? suggesting that it's more complicated than just accepting any Object? value).

[natebosch]

Any dynamic value is inherently unsafe!

I agree. We have the opportunity to make them slightly more safe relatively cheaply.

You can call any method on it without a static type check.

In practice I believe this is a very rare use case for dynamic.

I'd be much happier working towards fewer implicit dynamic types. For example, we could consider changing instantiate-to-bounds of raw types to use Object? instead of dynamic as the top type.

I strongly agree that we should work on this.

That would avoid the actual problem, which is an expression being dynamic and unsafe unnecessarily, rather than complicate the code where something is dynamic deliberately.

I think they are both problems. We have an established, well received solution to one of these problems. We are intentionally applying part of that popular solution to the core SDK so that everyone can benefit. I don't think we should avoid making things safer than they are today because of a potential solution that has no schedule or plan.

I do think that you should consider removing the "no implicit downcast" lint as long as you keep the 'no implicit dynamic" lint.

In practice implicit-dynamic: false is nearly unusable. I know we have discussions elsewhere about other "strict" solutions that may alleviate some of those downsides. implicit-casts: false is very usable and does exactly what, I think, a lot of users want.

Would you recommend that we disallow dynamic invocations as well?

Yes, if I thought that I could convince anyone I'd definitely recommend removing dynamic invocations, or at least making them stand out like an explicit as does. If we could do something like dynamicVariable!!.thisMethodMayNotExist() I'd highly support it.

[matanlurey]

I would be happy to help make this decision easier in any way possible, including doing user studies, meeting with internal teams to gather feedback, running global presubmits, etc - this is probably the #1 issue internal Dart users are confused by (with the exception of the broader implicit-casts).

I feel frustrated that I think we have tried to make a case we do not like the current dynamic dispatch behavior in Dart 2.0 for a number of years now with seemingly little progress (outside of "well, you can write a lint" or "you can have IntelliJ highlight it"). Please please let us know if there is anything we can do to help facilitate progress here.

I know @srawlins is OOO right now, but he had a number of plans around tightening up the language (strict dynamic?) that I worked with him and @leafpetersen on, I do not know if that is still planned and/or available, but I consider that near mandatory right now to use the language properly.

[mit-mit]

@matanlurey can you give us a few real-world (i.e. google3) example of where implicit downcasts from dynamic are problematic?

[matanlurey]

Sure, I guess I could find some specific examples. The simplest one I can think of is something like:

void readMap(Map<String, dynamic> someBlob) {
  doAThing(someBlob['campaign']); // No indication this is inserting a runtime check
}

void doAThing(AdsCampaign campaign) {}

My main point is this seems to be walking back "strict" checks that I thought were already agreed to and we spent time developing and discussing for a few quarters. If that isn't feasible than I think disabling implicit-downcasts entirely is in the best interest of our users.

[natebosch]

I've seen cases with incorrect defaulting. Something like:

set entity(Map<String, dynamic> entity) {
  _foo = entity['foo'] ?? 0;
 //... more fields
}

Int64 _foo;

The author in that case had forgotten to write a test where 'foo' wasn't set, so wasn't getting the runtime failure in tests with DDC. With dart2js --omit-implicit-checks which is the default internally this doesn't fail until much later. The correct code, which is more obvious if we have an explicit cast is:

set entity(Map<String, dynamic> entity) {
  _foo = entity['foo'] as Int64 ?? Int64.zero;
 //... more fields
}

Int64 _foo;

[jakemac53]

If you follow the dart sdk gitter channel there are multiple instances of people being confused about some runtime failure, somebody telling them to disable implicit casts, and them replying with some form of "oh my god why isn't this the default I just fixed X bugs".

Most codebases I have migrated to it have also had bugs fixed as a result.

We have a lot of concrete evidence that the existing feature as shipped is exactly what users want, and relaxing that for dynamic is not something I have heard a single request for. It seems to be in fact the exact opposite of what users want/expect.

[natebosch]

Here is another real world (and still existing) bug that would be more visible with explicit casts from dynamic:
dart-archive/csslib#100 (comment)

In this case we surfaced the bug with a different lint. Disabling implicit casts in this package shows a bunch of stuff, including from dynamic, but I haven't dug in to see how many of those are bugs vs casts that just need to be made explicit.

[leafpetersen]

Ok, after further discussion, there's no plan to change this at this point. We will move forward with optional strict checking separately.