I wish to remove implicit downcasts by default

[matanlurey]

NOTE: This issue is not time-boxed (i.e. it could be a Dart 2.1.0 w/ a flag, a 3.0.0 change, etc)

When I first started using Dart 2.0 semantics/strong-mode, I didn't have a single strong argument for or against allowing implicit downcasts, but after working almost exclusively in Dart 2.0 semantics for the last 6 months (DDC+Analyzer) - and seeing both Googlers and external folks file issues and request help I now strongly (pun intended) believe implicit downcasts by default is a mistake on the order of ever introducing "null" into a language.

Overview

Most of the arguments I've used (and seen) for implicit downcasts are:

• Originally strong-mode was "opt-in", so a goal was minimizing changing existing code.
• It makes the leap from Dart 1.0 -> 2.0 easier.
• It keeps the language "terse" and "concise" for common downcasts.

Most of the arguments against:

• "Why would we not make implicit downcasts an error in general... they're not sound"
• "Why are implicit downcasts allowed by default in strong mode?"

In general, the arguments against are stronger than the arguments for:

Soundness

One of the goals of strong mode is to add a sound type system to the Dart language. Heap soundness has a lot of nice properties - both in developer productivity ("oops, I didn't mean to do that, thanks for catching that IDE!"), and AOT compiler optimizations ("I know for sure, that var x at this point of time only can ever be a String, therefore I don't need a trampoline/de-opt").

Implicit downcasts are still sound (the checking occurs at runtime), but now we've taken something that could have been fixed at code review/development time and shifted to something we need to hope exhaustive test coverage catches before we ship code to production.

Least surprise/most predictable

Dart does not currently have type coercion (as of 2017-11-19), and has a huge limit on the amount of semantic "magic" that occurs at either compile or runtime. Implicit downcasts is one of the areas that is surprising to me and a lot of our users both internally and externally:

• Dart 2.0 runtimes are required to check the type (at runtime), but it's not apparent in the code.
• We require the keyword covariant in signatures, but not in assignments or return values.

It's scary to change framework or library-level APIs

THIS IS MY NUMBER ONE ISSUE SO FAR. HAVE HIT REAL PRODUCTION ISSUES.

Assume I have an API for listing all of the countries a user account has visited:

abstract class Account {
  Set<String> get visitedCountries;
}

I can change it to the following...

abstract class Account {
  // Now computed lazily and not hashed, since that was a rare use-case.
  Iterable<String> get visitedCountries;
}

...and not trigger a single compiler warning or error where users were doing the following:

Iterable<String> warnForCountries(Account account, Set<String> bannedCountries) {
  return account.visitedCountries.intersection(bannedCountries);
}

In fact, in a lot of tests, folks will mock out Account:

class MockAccount extends Mock implements Account {}

void main() {
  final account = new MockAccount();
  when(account.visitedCountries).thenReturn(['New Zealand'].toSet());
  
  test('should trigger a warning', () {
    expect(
      warnForCountries(account, ['New Zealand'].toSet()),
      ['New Zealand'],
    );
  });
}

... and will never hit my subtle re-typing until they get a production issue 👎

This is an area where implicit-downcasts: false can't help - because I need other libraries that haven't opted into the flag to fail, and they of course, won't, so I'll have to assume they have good test coverage and move on.

Accepted language and library features are removing downcasts

• int.clamp instead of num.clamp.
• Adding types T instead of relying on dynamic or Object.

See related issues at the bottom for more context and details.

Examples

The following examples are tested in 2.0.0-dev.6.0.

Null aware operator

void noError(int x) {
  // Implicit downcast: x = <int|String> = <Object>.
  x = x ?? 'Hello';
}

Ternary operator

void noError(int x) {
  // Implicit downcast: x = <int|String> = <Object>.
  x = x == 5 ? 6 : 'Hello';
}

Returning a List

class UserModel {
  final List<User> _users;

  UserModel(this._users);

  // Implicit downcast: Iterable --> List. Will fail at runtime only.
  List<String> toNames() => _users.map((u) => u.name);
}

Cascade operator

void hasError() {
  // Constructor returns type 'Base' that isn't of expected type 'Derived'.
  Derived e = new Base();
}

void noError() {
  // Implicit downcast: Base --> Derived.
  Derived d = new Base()..value = 'd';
}

class Base {
  String value;
}

class Derived extends Base {}

Related Issues

• Disallow downcasts of ternary operators: Strong mode should disallow implicit downcasts of ternary operators #25368
• Cascade operators break strong mode typing: cascading operators break strong mode type checking #29718
• Strong-mode analyzer inconsistent about when types must match: Strong mode analyzer inconsistent about when types must match #30385
• Stricter strong-mode: Stricter strong-mode (especially around dynamic) #30397
• Issues blocking Flutter's use of "implicit-casts: false": Issues blocking Flutter's use of "implicit-casts: false". #30402

Summary

Don't read this issue as a prescriptive "remove implicit downcasts!", but rather, given the type of errors we hide today (above), and the rationale for-and-against implicit downcasts, consider introducing changes in Dart 2.1+ that allow better static type safety. If assignment terseness is still a user requirement (I'd love to see the UX feedback showing this), then perhaps something like an implicit cast operator:

void futureDart(Object o) {
  int x =~ o; // Same as int x = o as int;
}

I imagine we will get non-nullable types sometime in the future. If we do, you'll never want this:

void futureDart(int? maybeNull) {
  int definitelyNotNull = maybeNull;
}

... so the argument for making this OK is much weaker:

void futureDart(Object maybeInt) {
  int definitelyInt = maybeInt;
}

[MichaelRFairhurst]

If there were a 'gold medal' reaction I would give it to you, for the thorough writeup, inclusion of both sides, convincing argument, sources cited, examples given, affected users, and, for just how important I think the point you're making is.

[frankpepermans]

100% agree, I've hit the List/Iterable problem myself a number of times

[ailabs-software]

This, along with missing public/protected/private, is one of the few things that makes me miss GWT and TypeScript while in Dart.

As this needs to be a breaking change, can it be fixed in Dart 2.0?

By the way, excellent article on Null. The nesting of the Some/None objects to represent no entry vs. no phone number is a really intriguing pattern.

[DanTup]

Me again! ;)

I hit this again... I totally forgot about implicit downcasts and then made this runtime error in the Flutter stocks app which should've been caught by analysis -> flutter/flutter#13815

The decision to allow implicit downcasts by default has always confused me. The benefits seem tiny (saving a few characters on a cast) for crazy issues (pushing a statically detectable errors to runtime - the opposite of what Strong Dart aims for, and removing any clues that the developer believed the cast was safe).

I've mentioned this many times but it's always been met with a wall of silence. Can we at least flip a coin for it? I'll provide a coin.

[robbecker-wf]

We've hit this at Workiva a number of times .. and just found this issue after hitting it today. We're all for it.

[matanlurey]

We hit another issue internally as a result of the current semantics:

#31865

class X {
  List ints = <int>[];
}

main() {
  var x = new X();

  // Static info warning: dynamic is not type of int.
  x.ints.map(formatInt);
}

String formatInt(int x) => '$x';

[leafpetersen]

Something is awry here. That code has no runtime error in 2.0. I just verified on DDC, I see no error. In what context are you seeing a runtime error?

[matanlurey]

@leafpetersen: Updated the example, it is not a runtime error.

[leafpetersen]

Ok, updated example makes sense. We have discussed treating "raw type annotations" like List x as requests for inference to fill in the missing type arguments instead of just using dynamic (or in general the type parameter bound), but that's not the current (2.0) semantics. If you use List as a type (instead of as a constructor), it always means List<dynamic>.

[matanlurey]

Here's an example* I saw lots of folks asking about at DartConf (and the GDE event hit it):

void main() {
  bool someBool = true;
  String thisWillNeverWork = someBool ? 1 : false;
  String thisWillAlsoNotWork = true ?? 1;
}

*simplified example.

These are runtime failures in Dart 2, but of course, look fine in an IDE.

[leafpetersen]

I believe these last examples will be fixed in Dart 2.

[matanlurey]

How?

[DanTup]

Is the only opposition to changing this that it's another breaking change for people to deal with or is there more to it?

To me, is seems like the work in changing the setting back in analysis_options is pretty trivial if people want this behaviour and it seems unexpected enough that opting-in is surely better than opting-out. Dart 2.0 is claiming to be strong and this seems like the biggest hole in that. New Flutter and Dart devs are going to hit this eventually and be confused why they didn't get warnings (for ex "I think analyzer should have warned me that the _RenameOverwriteValidator<_DirectoryNode> callback was being passed a _Node, but it didn't!"). Or worse, they will ship code to customers that crashes unexpectedly in a way they never knew possible.

It's not clear from this case what can be done to move the discussion forwards - many people are asking for it but the responses all seem to focus on the specific examples being given rather than the general request that people think it's a bad default.

If we consider that a lot less Dart has been written in the past than will be in the future, should this default provide a worse Dart experience (as in more runtime errors) for all future Dart code because of some existing code? If you were starting over again today, would you do the same thing? Dart 2.0 is known to be breaking in order to improve safety; I'm struggling to see any good reasons why this one is different to all the other changes. This default just does not fit with the idea of being strong.

[matanlurey]

@DanTup:

Is the only opposition to changing this that it's another breaking change for people to deal with or is there more to it?

I don't think we've been fully transparent here, there was other opposition (i.e. it will make Dart "less terse" to disable by default). I don't want to get into pros/cons completely here, but I think generally speaking now is the cost to flipping this to false by default might not be doable for Dart2.

We'd need to change several millions of lines of code internally, for example. That being said I am still pushing for this for the next breaking release change, hence this issue being open :)

[DanTup]

i.e. it will make Dart "less terse" to disable by default

I don't know if that's still being used as an argument, but it doesn't seem like a good one to me. There are many places the language could be made less terse if we want to throw away dev-time errors and find them at run time. If people wanted shorter code that behaves unpredictably, they'll pick JS ;-)

We'd need to change several millions of lines of code internally, for example

Why can't you change the setting in your analysis_options? This is what always bugged me in discussions about this. Is there some rule that was you have to use the defaults? The default should be set for what's best for the future/most people and if you'd rather the opposite, change your setting (unless I'm misunderstanding something - I must admit I don't know how pub packages you pull in are affected by the switch).

[robbecker-wf]

It sounds like it would be a huge effort to make this happen in time for a Dart 2 release .. or it would push it back. I'd personally like to get Dart 2 shipped sooner than later even if it isn't as perfect as we might like. Perhaps as a compromise there could be good documentation for the cases where devs might get surprising results. Scaffolding or new project generator tooling could also set the appropriate analysis option by default.

[DanTup]

It sounds like it would be a huge effort to make this happen in time for a Dart 2 release .. or it would push it back.

At the risk of sounding like a manager, is there really that much to it? Besides flipping the default (and then setting it back in analysis options for any projects that break) and adding a note to v2 migration guide (if there is one?) is there a lot to do? I'd happily do any of that that I can in my free time if it helps things along =)

It feels to me like something that if it's not done for 2.0 it'll never get done. Breaking changes are really hard to justify and once 2.0 is out I think there will be a reluctance to make any more (and for good reason - frequent breaking changes are super frustrating to people with large projects).

[robbecker-wf]

Oh I see your point now @DanTup. Set the defaults and then let projects opt out when updating to Dart 2 if needed.

We'd need to change several millions of lines of code internally, for example.

What you're saying is ... there will only need to be millions of lines updated to be compliant with the new opt-in default.. but if opting out in analysis options, then carry on.. no updates needed.

What effect would this have on existing packages in the ecosystem? Would it make the transition to Dart2 more challenging in any way?

[matanlurey]

@DanTup:

I don't know if that's still being used as an argument, but it doesn't seem like a good one to me.

Nor me, but it was a reason, though perhaps no longer the main or strongest one.

Why can't you change the setting in your analysis_options?

While you can, it's not feasible inside of Google or a large company. There might be something like 10000 or more packages, and would mean that implicit downcasts would fail or not depending on the directory you are in on a given day.

The default should be set for what's best for the future/most people.

I agree here too, but there are other issues as well

• Are our code-labs updated with this in mind (I bet you they are not)
• Is Flutter in agreement/alignment (Not clear)
• Is our documentation, websites, samples, starter templates updated
• Are there places that implicit-downcasts: false is too strict or not strict enough

I don't think a significant amount of time has been spent asking these questions. If we decided to take action here, we'd need a specific plan, and I just think planning and implementing is definitely out of the scope of Dart 2, perhaps not Dart 3 though :)

@robbecker-wf:

I'd personally like to get Dart 2 shipped sooner than later even if it isn't as perfect as we might like.

👍

Perhaps as a compromise there could be good documentation for the cases where devs might get surprising results.

I'd love to see this too. @leafpetersen @lrhn

Scaffolding or new project generator tooling could also set the appropriate analysis option by default.

I'm afraid that has the same problem as "default should be set for what's best for the future/most people" I've listed above. We're just not ready for that type of change, for better or worse.

What effect would this have on existing packages in the ecosystem? Would it make the transition to Dart2 more challenging in any way?

Yes. Implicit downcasts are used, quite a bit, in existing libraries and packages. It would mean that there would be "strong" packages that violate this, and never knew they were violating this. As you can see by issues such as #31874 there are already soundness issues that we haven't plugged up yet that are causing minor breakages and are taking time to fix.

[matanlurey]

(That being said, please 👍 this issue if this is important to you so we can prioritize post Dart 2)

[DanTup]

@matanlurey Understood; thanks for the extra info! :)

[MichaelRFairhurst]

Great cost analysis Matan.

While you can, it's not feasible inside of Google or a large company...implicit downcasts would fail or not depending on the directory you are in on a given day.

I don't think this is actually much of a usability problem. I'd say that I always code as if implicit downcasts would fail, and that's the problem :) If they do fail statically, I fix them. If they fail at runtime, I fix them.

I mean, yes, I do try to be extra cautious that they don't slip in in my Dart, so maybe at a certain conversion threshold (say, 80%) I would relax at that and get worse in codebases where the checks aren't enabled. But I think this is overall a very minor cost.

Are our code-labs updated with this in mind (I bet you they are not)
Is Flutter in agreement/alignment (Not clear)
Is our documentation, websites, samples, starter templates updated
Are there places that implicit-downcasts: false is too strict or not strict enough

These are definitely a decent chunk of work, unfortunately. Are we going to be going through this stuff for dart 2 anyway? (strong, strong runtime, optional new/const?) Would it be that much work to set a flag too while doing it?

Assuming we can check those other (not trivial) boxes, it would be awesome if we could even simply, make stagehand, flutter init or whatever, start filling in implicit-downcasts: false so that we can instantly solve this problem for new community members and worry about the migration at a later stage.

[matanlurey]

@MichaelRFairhurst:

I don't think this is actually much of a usability problem. I'd say that I always code as if implicit downcasts would fail, and that's the problem :) If they do fail statically, I fix them. If they fail at runtime, I fix them.

I think many people don't even know it is a problem today, so it would be introducing a whole new set of failures they don't understand or know how to fix. There are some performance issues for dart2js w/ --trust-type-annotations as well that I won't get into here.

Would it be that much work to set a flag too while doing it?

That's a question for @leafpetersen and @lrhn respectively, not me.

[leafpetersen]

How?

#31792

Basically, the context type (String in your example) should get "pushed down" into the expression, where it becomes an invalid side cast.

[DanTup]

I guess when I said explicitly I meant in the same scope. I think that example is bad - you shouldn't be able to create a method taking dynamic that will crash if the value isn't a List<String> unless you've written some code that shows you actually believe your value is a List<String>. I think you should have to either fix the type on the signature (preferred for that example) it at least add as List<string> in the call to show that you believe it's a list of string.

Imagine in future you needed to change the type of List<String> there (maybe you'd originally made it Iterable and were now changing it to List), you can't just change it and use analysis errors to make sure you found all the places that needed updating. Find References is no good either because it'll show those you've already fixed (or didn't need fixing) as well as those that are bad.

I think the short-term gains (slightly less typing) are outweighed by the additional risks in maintaining/refactoring :-)

I know I have a preference for much stronger typing than most.. but this one seems really loose to me, and at-odds with the idea of Dart going strong. I think (though have no data to back it up ;-)) that most people would expect that last code block to give them a static warning.

[leafpetersen]

This has been accepted as part of the NNBD release planned for late this year. Closing this issue since it is now tracked as part of that scope of work. Discussion issue here: dart-lang/language#192 .