I wish to remove implicit downcasts by default

matanlurey commented 6 years ago

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:

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: https://github.com/dart-lang/sdk/issues/25368
Cascade operators break strong mode typing: https://github.com/dart-lang/sdk/issues/29718
Strong-mode analyzer inconsistent about when types must match: https://github.com/dart-lang/sdk/issues/30385
Stricter strong-mode: https://github.com/dart-lang/sdk/issues/30397
Issues blocking Flutter's use of "implicit-casts: false": https://github.com/dart-lang/sdk/issues/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 commented 6 years ago

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 commented 6 years ago

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

ailabs-software commented 6 years ago

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 commented 6 years ago

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 -> https://github.com/flutter/flutter/issues/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 commented 6 years ago

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 commented 6 years ago

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

https://github.com/dart-lang/sdk/issues/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 commented 6 years ago

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 commented 6 years ago

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

leafpetersen commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

matanlurey commented 6 years ago

How?

DanTup commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

DanTup commented 6 years ago

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

MichaelRFairhurst commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

How?

https://github.com/dart-lang/sdk/issues/31792

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

matanlurey commented 6 years ago

Ah I see, int as String would not work statically. Thanks for clarifying that!

MichaelRFairhurst commented 6 years ago

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.

I guess my poorly made point here is that this is the expected behavior. I don't think developers will be confused that Object can't be assigned to Foo, I think that's what they expect. That's why we're here, right? And if they don't know how to handle it, then how do they handle it when it comes up at runtime?

The confusing branch of the split (if one were to exist) would be the one where implicit downcasts are still allowed. And I can't imagine that having less code in the more confusing branch would increase confusion.

matanlurey commented 6 years ago

I don't think developers will be confused

Well, for one example, Flutter's own repo doesn't allow the use of the as notation, so they would be forced to label every single implicit downcast (I'm not sure how many there are) with an // ignore, largely defeating the point?:

(I don't know why, maybe @yjbanov or @Hixie do)

DanTup commented 6 years ago

Flutter's own repo doesn't allow the use of the as notation, so they would be forced to label every single implicit downcast (I'm not sure how many there are) with an // ignore, largely defeating the point?:

Could they use explicit casts instead of ignore?

IMO if a developer is certain that a downcast is safe, putting an explicit cast in is a great way to convey that. It also makes it more obvious to the person reviewing the code there's a risk there (implicit downcasts aren't obvious when reviewing). It may also cause them to think twice - whether they're actually certain the code can't be called in a way that this cast would fail. Maybe they'll even see if they can tweak the code so the types flow better and the cast isn't needed at all.

If you can't easily prove to the compiler/type system that the thing really is that type, maybe either it's not guaranteed to be or you have code that could easily be changed in the future in a way that breaks this assumption and could go unnoticed? One of the great benefits of a good type system is changing code in future and being confident that you're not breaking something - that's less of a guarantee here.

Anyway, my opinion is well documented here, I won't keep going on - I'm least qualified to influence the decision and trust your judgements!

Hixie commented 6 years ago

FWIW, in Flutter we want this, and will be turning on all the relevant lint flags in the coming months. We only disallow as because today it's more expensive in release builds than assignment. In the future all assignments will be as expensive as using as (except those the compiler can prove are safe, and those will be cheaper), at which point using as will be fine, and we'll move over to using as instead of assignment. As of today there's a number of problems with turning on the lints which is why we haven't yet (and this isn't a high enough priority for us to deal with that right now, though patches would be very welcome).

matanlurey commented 6 years ago

@Hixie:

As of today there's a number of problems with turning on the lints which is why we haven't yet (and this isn't a high enough priority for us to deal with that right now, though patches would be very welcome).

By lint, do you mean implicit-downcasts: false, and if so, do you have a list of issues?

Hixie commented 6 years ago

There are several downcast-related lints/flags/whatever; I forget what they all are and what they do exactly. @leafpetersen probably knows. I don't have a list of issues off-hand.

matanlurey commented 6 years ago

Thanks!

lrhn commented 6 years ago

This has been filed as a language issue and a request to remove implicit downcast. Currently, the language allows implicit downcasts. There are anaylyzer flags that people can use to restrict the their dialect of Dart to not contain some, otherwise valid, language constructs, but that's unrelated to the language.

If we remove implicit downcasts from the language, there is nothing an analyzer flag can do to enable it again. We can't just flip a flag to allow existing code to run programs that are disallowed by the language because they will be rejected by the compilers. The only thing the analyzer can do is restrict you to a (self-imposed) subset of the valid programs.

It sounds like what some here are asking for is to flip the default in the analyzer. I wouldn't recommend that - the analyzer should follow the language semantics out of the box, and any restrictions is something that you opt-in to individually, or per project. The analyzer lints are not the language, you can write perfectly good Dart without any lints or annotations, and you should be allowed to.

So, back to the original request: Removing implicit downcasts from the language. It's not planned for Dart 2. It's unlikely to be reconsidered for Dart 2 at this point.

We should make the analysis server provide information about implicit down-casts so they can be high-lighted, and we can even make a quick-fix for turning it into an explicit cast.

Hixie commented 6 years ago

As far as Flutter is concerned we have no opinion on whether this should be a language or linter change; there's no practical difference for us. What matters is just that we be able to catch bugs.

matanlurey commented 6 years ago

@lrhn:

Currently, the language allows implicit downcasts

Er, it doesn't in lots of places; for example, requiring covariant for overriding methods. So the decision to allow implicit downcasts for assignments and invoking methods is bizarre to me, though I respect that changing it before Dart 2 ships is a no-go, and I'm definitely not arguing that.

If we remove implicit downcasts from the language, there is nothing an analyzer flag can do to enable it again.

That's what I'd prefer (i.e. no option to allow it).

It sounds like what some here are asking for is to flip the default in the analyzer.

We already ship lints that are not in the language with every template app that is created, FWIW.

We should make the analysis server provide information about implicit down-casts so they can be high-lighted, and we can even make a quick-fix for turning it into an explicit cast.

This was the old language team's stance, and I hope we don't follow their path - the number 1 use of Dart is in code reviews (i.e. not in an IDE), and unless code reviews can block bad code "highlighting it" doesn't help us at all.

@Hixie:

As far as Flutter is concerned we have no opinion on whether this should be a language or linter change; there's no practical difference for us. What matters is just that we be able to catch bugs.

👍 - I don't want an implicit downcast in my code or any of the code I depend on (i.e. not my own). How this is accomplished (language change, linter change/enforcement) doesn't matter to me either.

MichaelRFairhurst commented 6 years ago

any restrictions is something that you opt-in to individually, or per project.

I think we could learn something here from the field of Behavioral Economics. Behavioral Economists have shown that requiring people to opt-out of, rather than opt-in to, being an organ donor increases signups and in turn saves lives.

Behavioral economists have tackled poverty problems by making direct deposit opt-out instead of opt-in and believe that the solutions there are applicable worldwide.

The organ donor link I provided says it has the most impact "where there is inertia or uncertainty." Stricter checks in the analyzer (whether they be lints, hints, or downcasts or implicit dynamic) suffer greatly from these things. Uncertainty, in the face of "implicit-downcasts" should be obvious, but inertia as well, as by the time you've been bitten by something where a lint would have helped, you have to take an action and clean up all your existing code too. Note that turning off strict checks that you've been hitting has less inertia and uncertainty. This indicates to me that our defaults are clearly backwards.

So in response to "any restrictions is something that you opt-in to individually, or per project" I have to respond with a big resounding "disagree" and I'd like to think that I'm basing that on sound research & reasoning.

We should make the analysis server provide information about implicit down-casts so they can be high-lighted

This has continually been the suggestion which is continually met with the same resistance: it won't show up in code review, not everyone uses editors that do it, and its a hard UX problem. Should we color it in yellow, how will users know what that means? Should we give it a popup on hover? Should you have access to a list of all places it occurs available? The answer is yes to all questions, with no changes to IDE tooling required, immediate command line support, etc.....by adding a warning/error/hint.

leafpetersen commented 6 years ago

There are several downcast-related lints/flags/whatever; I forget what they all are and what they do exactly. @leafpetersen probably knows. I don't have a list of issues off-hand.

I added a flag to allow/disallow declaration casts (casts of the form int x = somethingOfTypeObject) independently from other casts in hopes that this would allow flutter to disable everything except declaration casts in the interim. I think @Hixie tried this out, but felt that there were still too many required changes.

If someone wants to try it out, you can specify any combination:

--no-implicit-casts // no implicit casts at all
--no-implicit-casts --declaration-casts  // no implicit casts except in declarations
--implicit-casts --no-declaration-casts // implicit casts except in declarations

with the corresponding syntax allowed in analysis_options.yaml of course.

MichaelRFairhurst commented 6 years ago

Just another quick thought; I think we can all agree that anything being surprising in the language is bad for our users.

The argument for implicit downcasts is that they look nice. Here's a straw-man syntax that looks nice, and isn't surprising:

List<Object> parseResult = ...;
TableElement t = from document.getElementById(from parseResult[0])

in this case, from x simply means "x may (or must?) be implicitly downcast". Therefore, getElementById accepts parseResult[0] even though its of type Object, and the result is assignable to t even though its of type Element.

I think this actually looks really nice (though in general I prefer postfix unary operators) and handles that annoying case where you have to do some kind of silly cast to Some<Complicated, Type<And, Stuff>>.

Its based partly on a nice feature in rust, where you get to implement like From and To to do data conversions, and you can write things to the effect of json result = from(string) or something like that, its been a while but its something I remember thinking was really really cool.

My overall point is, the surprise factor here should definitely outweigh any ergonomic benefit -- and the ergonomics we do get from implicit downcasts should be acheivable through other less surprising means.

DanTup commented 6 years ago

TableElement t = from document.getElementById(from parseResult[0]);

I don't want to flog a dead horse but I'm curious why everybody is so against explicit casts:

TableElement t = (TableElement)document.getElementById((String)parseResult[0]);
// could even use final/var to remove the duplicate type?

IMO it's really clear what the intention is, it's familiar to people and it doesn't involve a language change. These casts are (hopefully) an exception and if the dev is telling the type system they know better, making them put that in writing seems line a reasonable thing to do. I don't think I've ever seen anybody complain that they had to put explicit casts anywhere before. It's trivial to do yet the little effort should make them consider whether it's actually safe. It's also obvious in a code review (I don't think the from example is - particularly for the string argument where the type isn't written).

yjbanov commented 6 years ago

Given non-nullability is on the roadmap, can we reuse ! as a general-purpose downcast operator? After all, going from String? to String is also a downcast. With that, the aforementioned example would look like this:

TableElement t = document.getElementById(parseResult[0]!)!;

(conversely, if we decide to introduce a keyword such as from, I think we should reuse it for NNBD)

matanlurey commented 6 years ago

@MichaelRFairhurst @DanTup:

Honestly, this is fine to me:

var t = document.getElementById(result[0] as String) as TableElement

It's also more clear, if there is a runtime error, why (I see the as). It is not more writing either.

FWIW, I chatted offline with @leafpetersen (one of the language leads), and there is some support for implicit-casts: false after Dart2 (doing it before or during is just a no-go right now, there is way too much going on and we really can't afford to lose focus).

I could imagine something like the following (but I don't speak for the language team):

Dart 2.1.0: --no-implicit-casts now fully supported (i.e. tested, documented) and is recommended in all new Dart projects and packages. Documentation and examples now start to use this flag, and internally, we let teams starting opting in. (NOT A BREAKING CHANGE)
Dart 2.2.0: --no-implicit-casts is now the default internally, and teams have to opt-out in order to use the older behavior (i.e. NOT A BREAKING CHANGE). We document that in Dart 3.0, this will become the default, and required, so teams wishing to migrate early can start.
Dart 3.0.0: There are no more implicit casts, internally or externally.

Again, all of the above is entirely hypothetical. Please if you are on the Dart or Flutter team, continue to file examples of where you are hitting pain points above, but let's make sure we know there will not be any traction on this (for better or worse) until Dart2 is shipped.

rfuhrer commented 6 years ago

I'm a new Dart user and figured I'd chime in with some anecdotal evidence for this change. I tried Flutter about 6 months ago for a personal project and then got too busy at work and didn't touch it again. About 5 days ago I finally sat down and really started on it again. Updated flutter, got the new VSCode extension, started hammering away. It was all 🌈s and 🍭s. I was coming from C# and TypeScript as my main languages so Dart was really easy to pickup.

About 2 days ago I noticed that there was the Dart 2 preview available for Flutter and figured, hey I just started this project, might as well start the migration now rather than once I've got 15 screens done. Upgrade path was super simple, analyzer found nothing, project starts up great, I see my home page, I've now got diabetes from how sweet this all is.

Fast forward to Sqflite library blowing up, Shared Preferences blowing up, My code blowing up. No errors in my analyzer.

Literally 100% of my issues were caused by implicit downcasts. I believe the 2 libraries were also the same thing. It's a real trap when you're learning a language if it lets you do things you shouldn't, and only lets you know once you get a runtime error. I didn't even realize I was implicitly downcasting all over the place because the analyzer didn't tell me, and my code wasn't crashing in Dart 1 (because the implicit casts happened to succeed).

Now I'm using all of the analyzer settings and it's much more of the experience I expect. Maybe when I'm a Dart expert I won't need it, but for learning I need big red squiggles telling me "you're doing a dumb!" before I find it runtime.

MichaelRFairhurst commented 6 years ago

I think @rfuhrer's experience is going to be a very common one for users during the dart 2 upgrade.

In dart 1, this would succeed: List x = [].map(id); in the analyzer and at runtime, unless you passed in --checked.

In dart 2, this will never succeed at runtime and still somehow succeed at analysis time :(

This means that users who don't run --checked all the time, will suddenly hit this issue like crazy. And I don't think many people will have the opinion "this should have worked originally!"

We're working on it, though, as you can see in this thread!! We just have a lot of fish to fry with stabilizing dart 2 and not much time. I do hope that it becomes our next priority.

matanlurey commented 6 years ago

Oh by the way, if we get invariant generics, I might not care about this anymore :)

DanTup commented 6 years ago

It's a missing .toList() after a .where() that bugs me most. With JS/TypeScript filter() returning an array it's really easy to write and feels really annoying to hit the runtime error (especially if the refresh cycle is long) which most other languages would've highlighted as you typed it :(

Emily + Emily showed this error in their demo last night (I think it was deliberate) but I suspect many people watching would've wondered why they hadn't had a squiggle for it.

matanlurey commented 6 years ago

I may have overlooked at least one place where implicit casts are very helpful: type inference.

This is a small example from an internally filed issue:

import 'library.dart' show User;

void parseEntity(Map<String, dynamic> entity) {
  final users = entity['users'] ?? [];
  ...
  displayUsers(users);
}

void displayUsers(List<User> users) { ... }

See the issue? If 'users' is missing, the ?? [] is hit, creating a List<dynamic> instead. DOH!

So only in the missing case, a runtime error occurs. This is because of the LUB is dynamic.

There are a few ways to fix this, the most elegant, in Dart 2, is to specific the LHS type:

final List<User> users = entity['users'] ?? [];

I could even rely on the type parameters of displayUsers:

displayUsers(entity['users'] ?? []);

This does the right thing, but now I've violated my own request to remove implicit casts to achieve this. Imagine I had to write this without implicit casts:

final users = entity['users'] as List<User> ?? [];

Doesn't nearly read as good. I don't know how we could structure this, though. Maybe this is rare enough in Dart not to worry about though, or just focus efforts on code generation/JSON parsing instead.

DanTup commented 6 years ago

See the issue? If 'users' is missing, the ?? [] is hit, creating a List<dynamic> instead

Normally, wouldn't being typed List<dynamic> be considered better than just being typed dynamic here?

To me, this doesn't seem any different to any other case. Taking something out of a Map<String, dynamic> and letting it implicitly cast to anything is fraught with danger for all the discussed reasons. If the developer is certain it's a specific type they should be forced to write that.

I think you should do this:

final List<User> users = entity['users'] ?? [];

I don't think this should work:

displayUsers(entity['users'] ?? []);

What if entity['users'] was not a List<User> in the future? It should fail on an explicit cast where you told it it was this type and it turned out not to be.

(I don't want to get into discussions about JSON, but I'd say you shouldn't be using maps like this - deserialise into a type when you get the JSON and then be strong everywhere! Using maps (and implicit downcasts) seems at odds with the idea of Dart going strong!)

matanlurey commented 6 years ago

Normally, wouldn't being typed List<dynamic> be considered better than just being typed dynamic here?

Depends what you mean. Not if the API expects List<T>.

To me, this doesn't seem any different to any other case. Taking something out of a Map<String, dynamic> and letting it implicitly cast to anything is fraught with danger for all the discussed reasons.

It does make type inference a little worse if you want default values (like above).

It should fail on an explicit cast where you told it it was this type and it turned out not to be.

FWIW in Dart 2 implicit casts are explicit casts, meaning it will fail in either case.

Anyway, I still think implicit casts should be disabled by default. I'm just logging a usability issue with my suggestion, though I don't think entity['users'] as List<User> ?? [] is too bad. I could see folks "not trusting" inference and writing this though:

final List<User> users = entity['users'] as List<User> ?? [];

... which looks plain ugly.

DanTup commented 6 years ago

Depends what you mean. Not if the API expects List<T>.

I meant just in terms of having the most information for the type. It seems weird to say typing something that we know is a List is better typed as dynamic than List<dynamic> (whatever the reason).

I guess my point was that it seems to me that it's the same trade-off as everywhere else. Why do you want the types to be loose here, but strong elsewhere? I think it should be consistent - either the compiler shouldn't trust you and should force you to be explicit when there are casts that could fail at runtime, or it should allow it. I don't think your latest example is much different to the others.

FWIW in Dart 2 implicit casts are explicit casts, meaning it will fail in either case.

I'm not sure what this means; but I meant that there should be an explicit cast the user wrote in order for a cast to fail. It's too easily to accidentally write something that "compiles" but fails at runtime. It would be nice to know that if you didn't write an explicit cast you can't have a runtime cast error.

matanlurey commented 6 years ago

It's too easily to accidentally write something that "compiles" but fails at runtime

Right, I am on board (that's why I filed this issue). I just meant that

object as List<String>

and

List<String> x = object

... are identical in Dart2 in terms of semantics.

dart-lang / sdk