Use implicit-cast: false in analysis?

[isoos]

analyzer:
  strong-mode:
    implicit-casts: false

Suggestion from dart-lang/pub-dev#1263 and it was also detailed in flutter/flutter#14921 (comment)

/cc @jakobr-google, @kevmoo, @mraleph

[kevmoo]

This will break many folks, I think. It can help find runtime issues, but folks need to build for it or it'll create a sea of red. I don't think it's a good idea...

[isoos]

As an upside, would it warn people what they need to fix for Dart 2?

[kevmoo]

Dart supports implicit casts in some situations. This would be a BIG change to throw into pana. We should chat w/ the language folks...

…

On Thu, May 10, 2018 at 6:48 AM István Soós ***@***.***> wrote: As an upside, would it warn people what they need to fix for Dart 2? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#312 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCirkpO50o04CaSHN6fIC0WfAstvlAks5txEU-gaJpZM4T5fKQ> .

[isoos]

@kevmoo: as we are moving closer to Dart2, would you consider the introduction of this in the analysis? The lack of it will break folks when they migrate to Dart2.

[kevmoo]

CC @leafpetersen @vsmenon – this is a tricky one.

Implicit casts don't always cause errors – but fixing all implicit casts often fixes errors...

[leafpetersen]

Not sure I understand the context - this is about turning it on for this package? Or somehow turning it on for users of the package?

[isoos]

Not sure I understand the context - this is about turning it on for this package? Or somehow turning it on for users of the package?

The questions is: shall we turn it on for all of the analyzed packages and make it part of the code quality score? (It is already enabled for pana)

[leafpetersen]

Would this be just a separate bit? That is, you get 1 "point" (say) if your code passes the analyzer, and another point if it passes with --no-implicit-casts? If so I think it would be fine. If not though, I think it's a big ask of folks.

[kevmoo]

Would this be just a separate bit? That is, you get 1 "point" (say) if your code passes the analyzer, and another point if it passes with --no-implicit-casts? If so I think it would be fine. If not though, I think it's a big ask of folks.

Right now there are no bonus points. You start w/ a perfect score and then you loose points if you miss things.

I could imagine some "extra love" for being implicit cast clean...but that's adding a new feature

[isoos]

What if we would turn them on, but classify them as hints? That way it would be visible in the list of suggestions, but the penalty would be barely noticeable. I'm not sure if it has a matching key under analyzer: errors: though...

[kevmoo]

I worry about the noise here. I'll leave it to @mit-mit to handle, though.

[mit-mit]

I'm not a big fan of having the score reflect quality for something that is opt-in. If we think this flag is high value, why is it not on by default?

[isoos]

I'm not a big fan of having the score reflect quality for something that is opt-in. If we think this flag is high value, why is it not on by default?

I think there is some space between "opt-in and we don't care about it" and "strong standard that is enforced". We shall consider the questions:

• Do we want people to write (publicly shared) code to match certain standards?
• How much effort is it to rewrite the existing code to match those standards?
• How urgent is that change to happen? In other words: on which schedule and signaling do we want to indicate the urgency of that change?

For example we have the following linter rules turned on for all analysis:

  rules:
    - camel_case_types
    - hash_and_equals
    - iterable_contains_unrelated_type
    - list_remove_unrelated_type
    - unrelated_type_equality_checks
    - valid_regexps

We have even stronger rules for Flutter packages:

    enableStrictCallChecks: true
  strong-mode: true
  errors:
    # treat missing required parameters as a warning (not a hint)
    missing_required_param: warning
    # treat missing returns as a warning (not a hint)
    missing_return: warning

These are opt-in for the average developer, but we felt that not matching these rules indicates a risk of potential error, and we should include it in the code quality score. One thing that happened really early on: developers who cared did fix the issues we have reported, although the penalty on their code quality score was minuscule.

And the plus side is, there is a developer-driven override from all this: if the code has a file-level or an inline suppression of the linter rule, dartanalyzer won't report it, and we would not make it part of the report or the score. I think this is a good balance: the developer looks at a report, and makes a conscious choice and judgement call that the specific issue may not be looked at (because whatever reason it has in the codebase).

[kevmoo]

We should close this issue, me thinks...

[isoos]

agreed