dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
636 stars 170 forks source link

Lint raw types when instantiation to bound yields type argument `dynamic` #2181

Closed eernstg closed 4 years ago

eernstg commented 4 years ago

Cf. https://github.com/dart-lang/sdk/issues/40722#issuecomment-656808700 which is directly aimed at raw type annotations, and https://github.com/dart-lang/linter/issues/1886 which may be interpreted as closely related:

It is a source of potentially unwanted dynamism when a generic type is used with no actual type arguments (that is, when a raw type is used), and instantiation to bound produces a type where dynamic is passed as a type argument, at the top level or in a nested position:

class C<X extends C<X>> {}
class D extends C<D> {}

void main() {
  List xs = [1]; // `xs` has type `List<dynamic>`.
  C c = D(); // `c` has type `C<C<dynamic>>`.
}

The ability to pass --no-implicit-dynamic to the analyzer helps in detecting a number of situations where the type dynamic is introduced implicitly, but it does not cover occurrences of dynamic that are introduced into type annotations or type arguments by instantiation to bound. Apparently, there is no lint on this situation.

This issue is a request for a lint that will flag these occurrences of dynamic in type annotations (that is, in the declared type of a variable or parameter, and in function return types), in type arguments (a type annotation like Set<Set> s = {} is already covered, but there are also expressions like <List, List<List>>{}, new A<Map>(), etc.), in the bounds of formal type parameter declarations, and in clauses like implements, with, on (in a static extension and in a try/catch).

srawlins commented 4 years ago

This should be covered by strict-raw-types: true, in analysis_options.yaml.

bwilkerson commented 4 years ago

A bit more context. As you probably remember, as part of moving to Dart 2.0, work was done to try to determine how much dynamism to remove from the language. There were a number of options added to control the behavior of the analyzer. Because they were being used to help design the language, they were referred to as "language" options and added as sub-options under the strong-mode flag. After 2.0 shipped, Leaf and Sam started putting together a proposal to clean up those language options so that users could control just how dynamic the language could be. That work was postponed until we understood what null safety would do to the language, and the old options still function.

Personally, I'm hoping that the language team will take another look at these options now that null safety is nearing completion. It would be good to

If this work isn't on the language team's radar, perhaps you could add it.

eernstg commented 4 years ago

@srawlins wrote:

This should be covered by strict-raw-types: true

@bwilkerson wrote:

A bit more context. ...

Thanks! That's indeed nearly the same thing. It it doesn't detect all occurrences of dynamic introduced by instantiation to bound, but certainly a lot of them.

class C<X extends C<X>> {}
class D extends C<D> {}

void main() {
  Set s = {}; // Detected.
  C c = D(); // `c` has type `C<C<dynamic>>`, not detected.
}

So I'll close this issue.

I guess the reason why strict-raw-types wasn't mentioned in the obvious locations (analysis_options info, dartanalyzer --help --verbose, lints) is that it hasn't been released officially? Otherwise it would seem useful to make it easier to discover.

It would be good to

  • have a coherent set of options that users can use to control the amount of dynamism in the language,
  • a way to specify them without referring to strong-mode, and
  • a model for whether we want them to be analysis options, lints, or something else.

If this work isn't on the language team's radar, perhaps you could add it.

We have discussed support for enforcing a more strict set of rules along various axes, and @leafpetersen recently mentioned this topic at a language team meeting. I'll make sure it's on the radar.

beauxq commented 4 years ago

What I would really like is a simple way to disable all dynamic typing, unless I explicitly type the word dynamic in the code.

The best I can find right now is using a combination of:

strong-mode:
    implicit-dynamic: false

and

language:
    strict-raw-types: true

The first and less important problem is setting 2 options in 2 different places, instead of 1. I don't see the usefulness in having them separate. Maybe there's a good reason for that. One effect of having them separate is that I am not confident that it will catch all dynamic typing. If it turns out there is a way for a dynamic to sneak in, not covered by these rules, would a 3rd rule be added? and then later a 4th rule?

The more important problem is that one is a warning and the other an error. I would like them to both be errors.

pq commented 4 years ago

/fyi @leafpetersen

bwilkerson commented 4 years ago

The first and less important problem is setting 2 options in 2 different places, instead of 1. I don't see the usefulness in having them separate. Maybe there's a good reason for that.

I don't know whether it's a good reason :-), but the reason is the history. The strong-mode option was introduced when we were exploring the language design space while moving from Dart 1.0 to Dart 2.0. Enough users were using those options that we decided to leave them, but we started an effort to clean them up. That led to the introduction of the language option. Unfortunately, that arc of work was interrupted, but we hope to return to it.

The more important problem is that one is a warning and the other an error. I would like them to both be errors.

It is possible to control the severity of diagnostics in the analysis options file. See https://dart.dev/guides/language/analysis-options#customizing-analysis-rules for more information.