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

Implement a set of "strict" static analysis rules #33749

Open
8 of 9 tasks
srawlins opened this issue Jul 3, 2018 · 14 comments
Open
8 of 9 tasks

Implement a set of "strict" static analysis rules #33749

srawlins opened this issue Jul 3, 2018 · 14 comments
Assignees
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-enhancement A request for a change that isn't a bug

Comments

@srawlins
Copy link
Member

srawlins commented Jul 3, 2018

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

  • --strict-inference
    • Specify the rule
    • Implement the rules, and support the flag, in the analyzer
  • --strict-raw-types
    • Specify the rule
    • Implement the rules and flag in the analyzer
  • --strict-casts
    • Specify the rule
    • 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 #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.

@srawlins srawlins added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jul 3, 2018
@lrhn
Copy link
Member

lrhn commented Jul 5, 2018

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?

@lrhn lrhn added the type-enhancement A request for a change that isn't a bug label Jul 5, 2018
@srawlins
Copy link
Member Author

srawlins commented Jul 5, 2018

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
Copy link
Contributor

zoechi commented Jul 5, 2018

Related #57364

@bwilkerson
Copy link
Member

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
Copy link
Member

lrhn commented Jul 6, 2018

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.

@a-siva a-siva added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Jul 9, 2018
@munificent munificent removed area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Jul 11, 2018
@mit-mit
Copy link
Member

mit-mit commented Nov 6, 2018

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

@jmesserly
Copy link

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
Copy link
Member Author

srawlins commented Nov 6, 2018

@mit-mit: @matanlurey discusses this in #34304.

@mit-mit
Copy link
Member

mit-mit commented Nov 6, 2018

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
Copy link
Contributor

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

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

@srawlins srawlins self-assigned this Feb 20, 2019
dart-bot pushed a commit that referenced this issue Feb 21, 2019
The first code is HintCode.INFERENCE_FAILURE_ON_UNINITIALIZED_VARIABLE:

```dart
var a;  // Hint: The type of v1 cannot be inferred without a type or initializer
dynamic b;  // OK
var c = 7;  // OK
```

This is currently only enabled via an analysis options file:

```yaml
analyzer:
  language:
    strict-inference: true
```

I could add it as a flag as well, but to start using this internally at Google,
we only need support in the analysis options file.

Bug: #33749
Change-Id: Id2a6afa7c3d724b44c20576c7f48869abcf4255c
Reviewed-on: https://dart-review.googlesource.com/c/93700
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Apr 11, 2019
Bug: #36445 #33749
Change-Id: I127d671840bfcfe3857225932164fa4098963715
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99085
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Apr 12, 2019
…_CREATION

Bug: #33749 #36445
Change-Id: Ic34f0e07b1cc5cbbc265ab2c32cabc61d7901c0f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99264
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Aug 20, 2019
* Untyped function parameters are a strict-inference failure.
* Add tests for strict-inference failures on fields.

This finalizes the "untyped / uninferred variables" cases.

strict-inference spec: https://github.com/dart-lang/language/blob/master/resources/type-system/strict-inference.md

Bug: #33749
Change-Id: I1d01ad15418be1891c95588132a44d478c5905a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112142
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Sep 13, 2019
The tests highlight the change:

    class C<T> {
      C([T t]);
    }

    // This used to be a strict-inference failure, but is no longer.
    var upwardsInfersDynamic = C(42 as dynamic);

Additionally, strict-inference no longer takes @optionalTypeArgs into account.

strict-inference spec: https://github.com/dart-lang/language/blob/master/resources/type-system/strict-inference.md

Bug: #33749
Change-Id: Iac07d9d15ce2146108753cae25890a559288a318
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111740
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@beauxq
Copy link

beauxq commented Jul 14, 2020

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
Copy link
Member

mit-mit commented Aug 3, 2020

cc @leafpetersen

@leafpetersen
Copy link
Member

@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
Copy link

beauxq commented Aug 5, 2020

@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-bot pushed a commit that referenced this issue May 26, 2021
The "inference failure" checks implemented here are long overdue. They
are part of the strict-inference spec [1]. I think I caught most function
invocation cases. All of the work done to determine which error to report
and whether @optionalTypeArgs is annotated is done _after_ the check
for whether strict-inference is enabled, so this should have no effect
on code which does not opt in to that mode.

[1] https://github.com/dart-lang/language/blob/master/resources/type-system/strict-inference.md#function-call

Bug: #33749 and
Change-Id: Ic1d4321fb289acb118e0dbddd48ff917ad39d69a
#45371
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/201321
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
copybara-service bot pushed a commit that referenced this issue Nov 10, 2021
The implementation is extremely simple: wire strict-casts right
alongside implicit-casts in AnalysisOptionsImpl, AnalysisSession, and
TypeSystem. The flag is separate from implicit-casts because it is
_slightly_ stricter. The implicit-casts flag has some gaps in coverage
(like for-each iterables, and yield-each expressions). Filling these
gaps in implicit-casts would be expensive (requiring cleanup), and the
short-term goal is to deprecate the flag, so it is not important to
tidy it up.

A few extra bits are then added which are specific to strict-casts,
which improve the coverage.

Most of this change is the tests: tests are added for (theoretically)
each error code which could be reported as a result of strict-casts. The
test model using WithStrictCasts follows the model of WithStrictCasts,
which couples two tests into one assertion, ensuring the state of each
example code before and after strict-casts.

Bug: #36267
Change-Id: I52b2fc831fac3a5467ec7164df8fc1a1109e42ef
#33749
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/219847
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests