Skip to content
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

Remove unsafe implicit cast warnings in Flutter code base #28588

Closed
kasperl opened this issue Feb 1, 2017 · 19 comments

Comments

@kasperl
Copy link
Contributor

commented Feb 1, 2017

The Flutter analysis options currently contain:

strong_mode_down_cast_composite: ignore

Enabling those warnings leads to >100 warnings in the Flutter code base like these:

[warning] Unsafe implicit cast from 'Map<dynamic, dynamic>' to 'Map<String, dynamic>'. 
[warning] Unsafe implicit cast from '(ByTooltipMessage) → Finder' to '(SerializableFinder) → Finder'.
...

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.

Reproduction steps:

   $ git clone https://github.com/flutter/flutter.git
   $ cd flutter
   $ bin/flutter update-packages
   <<comment out the strong_mode_down_cast_composite in .analysis_options_repo>>
   $ bin/flutter analyze --flutter-repo

@kasperl kasperl added this to the 1.23 milestone Feb 1, 2017

@kasperl kasperl changed the title Remove unsafe implicit cast in Flutter code base Remove unsafe implicit cast warnings in Flutter code base Feb 1, 2017

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

If these warnings are only needed because we don't do the correct runtime strong mode checks, we should think about what state we want this to be in for 1.23.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

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.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

We have reached the decision to turn off the strong_mode_down_cast_composite warning by default, but we will do it in a few steps. First, we will turn it off by default in Flutter projects but keep it on for web projects.

It looks like analysis options are dealt with in a lot of places in the analyzer code base, @pq? It feels hard to manipulate the options map and add the ignore there, so here's a suggestion on how to turn this off for Flutter projects by passing down the hasFlutterDependency flag to the strong mode checker: https://codereview.chromium.org/2703253006/.

Do you have cycles to look at this, Phil, and figure out what the best path forward is?

@devoncarew devoncarew assigned pq and unassigned floitschG Feb 21, 2017

@pq

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

Do you have cycles to look at this, Phil, and figure out what the best path forward is?

👍 Happy to take a look.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

Turned the warning into a hint in fa2a3a1, but had to revert it.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

Finally landed turning this into a lint in 2fe8485.

@pq pq removed their assignment Mar 9, 2017

@mit-mit

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

@kasperl are you still working on this?

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

@mit-mit The work is done, but it hasn't been rolled into Flutter yet. We could turn this into a Flutter issue blocked on rolling the Dart SDK dependency past the next dev channel release -- and close this one. That's probably the best approach.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

@mit-mit This change will be in 1.23.0-dev.6.0. Will go ahead and close this issue.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

Filed flutter/flutter#8783. Closing this one.

@kasperl kasperl closed this Mar 15, 2017

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

What was the final resolution of this? I don't see it surfacing as a hint. Is it now an opt in lint? If so, what are the instructions for enabling it?

@pq

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

I believe it's now only shown if you have strong warnings enabled. I'm not sure how that gets done though! (Not seeing an option in dartanalyzer...)

cc @bwilkerson who may know

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

If by strong warnings, you mean AnalysisOptionsImpl.strongModeHints, I think that's only available programmatically, and is really more of a testing feature for us at this point.

@pq

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

I think that's right -- and was under the impression that was the desired approach... I'll leave it to @kasperl to clarify though!

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

Ok. For context, I'm writing the changelog, and I want to know what to say about this. That is, should I be saying "this is gone" or "this is now opt in via path XXX", or...

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

Can you elaborate more? I've not been able to surface this in any way, even as a hint. Adding

  errors:
    down_cast_composite: error

to an analysis options file still doesn't surface it in any way that I can see.

@kasperl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

Ack, @leafpetersen. I've messed this up. The strong mode checker filters out hints aggressively without consulting the analysis options.

Fix here: https://codereview.chromium.org/2784473004/

@DanTup

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

We have reached the decision to turn off the strong_mode_down_cast_composite warning by default

Is there any more info on the reasons for this decision? To me it seems really strange that "opting in to strong mode" would not give you protection against this by default.

A colleague asked me today why his analysis_options.yaml file wasn't working and it turned out to be that he didn't know about this option and had concluded that strong mode was broken or "not strong". From the outside, based on only the info in this case it seems like the default was changed based on it being easy for some existing code rather than doing what's best for Dart code going forwards. I don't understand why the Flutter code can't be a) changed b) have explicit casts added or c) continue to opt-out if they really want (IMO a/b are best since the decision about whether it's valid can be made on a case-by-case basis).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.