Skip to content

Why are implicit downcasts allowed by default in strong mode? #30392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DanTup opened this issue Aug 10, 2017 · 8 comments
Closed

Why are implicit downcasts allowed by default in strong mode? #30392

DanTup opened this issue Aug 10, 2017 · 8 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-duplicate Closed in favor of an existing report type-enhancement A request for a change that isn't a bug

Comments

@DanTup
Copy link
Collaborator

DanTup commented Aug 10, 2017

In strong mode, this code gives no warnings but crashes at runtime:

Object o = 1;

void main() {
  String s = o;
  print(s.length);
}

I understand this is a deliberate decision and can be "fixed" by using implicit-casts: false but it seems like a crazy default to me for something claiming to be strong. I can't find any info on why this was decided - most language don't seem to allow this and it doesn't seem to be something that people often claim about.

I think this may be discussed here; seems like it was related to the Flutter codebase:

I suspect that a lot of these will require changes to the Flutter code base, but we should make sure that the type system isn't making this unnecessarily hard.

We believe we can get rid of this warning and turn it into a lint that users can enable if they want help finding areas where runtime checks might fail for them.

I'd love this to be reconsidered, but at least, it'd be good to understand why it's this way. It seems like it's just gonna result in runtime errors that could easily be avoided. What's wrong with explicitly casting when required, or disabling this on your entire project if that's really a problem? It seems strange to weaken the type system because of one codebase (which would opt-out of this, or even insert explicit casts which would be an improvement IMO).

@anders-sandholm anders-sandholm added analyzer-strong-mode area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Aug 10, 2017
@bwilkerson bwilkerson added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). and removed analyzer-strong-mode area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Aug 10, 2017
@leafpetersen
Copy link
Member

The linked discussion of downcast-composite is somewhat (but not entirely) unrelated. That was a warning on certain classes of implicit downcasts that were likely to fail in strong mode, but not in regular mode. Hence, it was mostly intended to help users that had code coexisting on strong mode and non-strong mode platforms (DDC and dart2js respectively). It did have a nice side effect of giving warnings on some common downcast errors though (e.g. the ubiquitous List l2 = l1.map(...)) error.

Strong mode was originally an opt-in mode, and so we mostly aimed to minimize the amount of code changes required to get its benefits - hence the decision to keep implicit downcasts as the default. In retrospect, we may have underestimated people's willingness to change their code... or we may not have. :)

In terms of Dart 2.0, there is not consensus on the Dart team that removing implicit casts would be a good change. Implicit casts definitely contribute to the concision of the language, albeit at no small expense to static analyzability.

So currently --no-implicit-casts is the best option available for those preferring stricter checking. For those a bit on the fence, I recently landed an additional flag --[no]-declaration-casts. I'll be advertising these more widely once they land in a stable build, but basically you can use combinations of those flags to independently allow or disallow implicit downcasts at variable declaration sites and at other locations.

void takesInt(int n) {}
void test() {
  num n = 2.0;
  // disallowed with --no-declaration-casts
  // disallowed with --no-implicit-casts unless --declaration-casts is set
  int i = n;
 // disallowed with --no-implicit-casts
  takesInt(n);
}

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 10, 2017

Thanks for the info! I'm still a little confused though; are there examples of where this default is an advantage?

I mentioned it to a few people today, and they all said "WTF?" or "Yeah but this is fixed in strong mode, right?". None of them expected this and would probably be frustrated if they hit a runtime error they would've expected to have been caught at dev time.

I can't come up with any scenarios where I wouldn't expect to put an explicit cast in - I don't know why anybody would say "hey, I wish this just silently did potentially-dangerous downcasts for me". That's not to say I think there aren't any - but if there are, then maybe I can learn something new by understanding them :-)

@matanlurey
Copy link
Contributor

Slightly related: #30397

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 10, 2017

IMO, strong_mode: true should make things as strong as possible - when starting on a new codebase it should be easy to work with the stronger options. If people are upgrading old codebases and want to do it slowly, they can opt-out if required (though I'm still curious who is taking advantage of this and doesn't think they should add a cast).

For v2 people are being told everything is strong, but I really can't help but think that more developers would expect this to be a compile error than not. Since the goal of v2 is to be stronger and breaking changes are already being made; it seems like a perfect opportunity to help improve peoples code and eliminate more potential runtime errors.

@DanTup
Copy link
Collaborator Author

DanTup commented Aug 10, 2017

By coincidence, I just got this runtime error:

00:00 +0 -1: ClosingLabelsComputerTest | test_mixedLineSpanning
  type 'MappedIterable' is not a subtype of type 'List<ClosingLabel>' of 'function result' where
    MappedIterable is from dart:_internal
    List is from dart:core
    ClosingLabel is from package:analysis_server/protocol/protocol_generated.dart

I'd not called .toList() on the end of a map().. Sure, this is a noob mistake but it's exactly the sort of thing I'd expect a type system to catch at dev time. I'd also suggest that to many devs it might not be obvious from that message what the problem is (in JavaScript, Array.map returns an Array.. and in C# you'd get a compile error returning an IEnumerable from a method claiming an Array/List/whatever).

I don't expect to influence this decision; but I am really interested in others opinions since the only pros I can come up with (don't have to type a cast) are outweighed by the cons (intent/expectation not clear in the code; and possible runtime failures). Maybe I'm overlooking some pros :-)

(Or would you accept a PR for really-strong-mode: true that opts-in to anything like this/implicit-dynamic to make things really strong? 😝)

@matanlurey
Copy link
Contributor

FWIW, I don't agree this makes the language concise :)

// No static error, but a common runtime one (in Dart 2.0).
List<T> listOfThing<T>(Iterable<T> iterable) => iterable;

filiph added a commit to filiph/edgehead that referenced this issue Nov 16, 2017
Make the code safer.

Context: dart-lang/sdk#30392
@DanTup
Copy link
Collaborator Author

DanTup commented Jan 2, 2018

I think I may have been bitten by this again!

flutter/flutter#13815

@lrhn lrhn added the type-enhancement A request for a change that isn't a bug label Jun 25, 2018
filiph added a commit to filiph/egamebook that referenced this issue Aug 16, 2018
Make the code safer.

Context: dart-lang/sdk#30392
@lrhn
Copy link
Member

lrhn commented Dec 6, 2018

Duplicate of #31410

@lrhn lrhn marked this as a duplicate of #31410 Dec 6, 2018
@lrhn lrhn closed this as completed Dec 6, 2018
@lrhn lrhn added the closed-duplicate Closed in favor of an existing report label Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-duplicate Closed in favor of an existing report type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants