Implement a set of "strict" static analysis rules

srawlins commented 6 years ago

The new Dart 2 runtime semantics added several changes that have really surprised users. As a prime example, this code:

List<String> list = ['hello'];
List<String> foos = list.where(condition);

will fail at runtime, because List<String>.where returns an Iterable<String>. The frustration is that this really looks like it should be caught at compile-time, or with static analysis.

To help users to detect errors like this early on (instead of relying on really good test coverage), we're proposing a set of "strict" errors that can be enforced in static analysis. We propose that the analyzer supports a --strict- flag for each error.

Todo

[x] --strict-inference
- [x] Specify the rule
- [x] Implement the rules, and support the flag, in the analyzer
[x] --strict-raw-types
- [x] Specify the rule
- [x] Implement the rules and flag in the analyzer
[x] --strict-casts
- [ ] Specify the rule
- [x] Implement the rules and flag in the analyzer

The rules

The first rule we intend to implement is --strict-raw-types, more or less addressing https://github.com/dart-lang/sdk/issues/33119.

Off-hand, @leafpetersen mentioned a few other possible rules might include:

--strict-inference - don't allow inference to produce top types unless top was an input,
--strict-casts - don't allow implicit casts at all,
--strict-raw-types - don't allow "raw" types, missing type arguments, on declarations,
--strict - some default subset of the above that we recommend as a best practice setting.

`implicit-casts` and `implicit-dynamic`

In many cases, these are teasing out individual error codes from the --no-implicit-casts and --no-implicit-dynamic flags. These have been declared more or less "an experiment" (unofficial) as they are today. Additionally, they don't provide good ergonomics; in order to "upgrade" a codebase to be free of "implicit dynamic" code, one needs to update their analysis_options.yaml to look like:

analyzer:
  strong-mode:
    implicit-dynamic: false
  errors:
    strong_mode_implicit_dynamic_list_literal: ignore
    strong_mode_implicit_dynamic_map_literal: ignore
    strong_mode_implicit_dynamic_parameter: ignore
    strong_mode_implicit_dynamic_variable: ignore
    ...

And then work their way through individual codes. In the end, the analysis_options.yaml file is still not expressive. It reads something like "Enforce 'no-implicit-casts' and 'no-implicit-dynamic', except don't enforce 6 of the 8 'implicit-dynamic' codes."

I can see the individual --strict- flags/rules replacing the implicit-casts and implicit-dynamic flags, but that isn't a primary goal of this work.

lrhn commented 6 years ago

This sounds like analysis or linter warnings, which is so far not something that all tools implement.

How does these suggestions differ from other analysis/linter options which are only implemented by the analyzer and/or linter?

srawlins commented 6 years ago

I think these can be implemented just by the analyzer. But I think the "level" at which this warnings would be written is code that will be replaced when the Analyzer switches to CFE someday. i.e. I think the current errors that come from implicit-dynamic and implicit-casts do not come from examining ASTs, but rather come while parsing and building an AST. I've removed some checkboxes.

zoechi commented 6 years ago

Related dart-lang/linter#288

bwilkerson commented 6 years ago

I think the current errors that come from implicit-dynamic and implicit-casts do not come from examining ASTs, but rather come while parsing and building an AST.

Anything that can be done while building ASTs can be done after the fact if enough information is kept around.

We can make them analyzer hints/lints if we make sure that all of the information that analyzer would need to have in order to implement them is being provided by the CFE. It would be a grave mistake (IMHO) to limit analyzer's ability to support users by limiting the amount of information that analyzer can get from the CFE.

lrhn commented 6 years ago

Anything that can be done while building ASTs can be done after the fact if enough information is kept around.

... and that information must be kept around, otherwise dartfmt can't be implemented on top of the front-end, which it will have to be.

mit-mit commented 5 years ago

What is the benefit of changing these to flags, and not just keeping them as analyzer options / lints?

jmesserly commented 5 years ago

What is the benefit of changing these to flags, and not just keeping them as analyzer options / lints?

They haven't been implemented yet, that's mainly what this bug is about.

Perhaps we can implement them only in Analyzer, we'll start with that. But we may need to implement them in CFE as well, at which point we'll need to agree on what the flags should look like.

srawlins commented 5 years ago

@mit-mit: @matanlurey discusses this in https://github.com/dart-lang/sdk/issues/34304.

mit-mit commented 5 years ago

I'm afraid it's a bit hard to understand the reasoning from that bug, as it closes with "We have decided (offline) that this will be a mode, not a set of lints. " with no further details.

Was the language team involved in this decision?

matanlurey commented 5 years ago

@mit-mit Yes, @leafpetersen was intimately involved.

He has a task (internal bug: b/118391875) to write-up his final thoughts.

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.

mit-mit commented 4 years ago

cc @leafpetersen

leafpetersen commented 4 years ago

@beauxq Just as a data point, what do you consider an explicit use of dynamic? For example, if there is a package that you import that provides an API List<dynamic> mkSomething(), then which (if any) of the following would you want to make an error?

  mkSomething[0].arglebargle; // dynamic call
  var x = mkSomething[0];  // x is inferred as 'dynamic'
  x.arglebargle; // dynamic call
  mkSomething.map((x) => null); // x is inferred as dynamic

beauxq commented 4 years ago

@leafpetersen Since there is no keyword dynamic in any of that code, I think I would want an error from all of it.

I'm almost leaning towards an error on the import statement, rather than on any of that code.

But if I decide I want that package with dynamic typing, I would want some way to explicitly say that dynamic from this API is ok, but not from any other API. I'm not sure how I would do that.

dart-lang / sdk