New YAML configuration for diagnostics analysis options

[srawlins]

New YAML configuration for diagnostics analysis options

Addresses dart-lang/linter#3319

Problem: confusing way to specify diagnostics options

Let's look at an example:

analyzer:
  errors:
    unused_element: ignore
    unused_import: warning
    dead_code: error
  language:
    - strict-casts
linter:
  rules:
    double_quotes: false
    single_quotes: true 

Section by section:

• analyzer/errors: In this section, we can set the severity of individual diagnostic codes. In the example, the unused_element Warning's severity is set to "ignore", the unused_import Warning's severity is set to "warning" (the diagnostic type, "Warning," is distinct from the severity, "warning"), and the dead_code Warning's severity is set to "error."
• analyzer/language: In this section, we can enable three different "strict analysis modes": strict-casts, strict-inference, and strict-raw-types.
• linter/rules: In this section, we can specify a list of lint rules to enable (distinct from the names of the diagnostics they produce), or a map, where each key is a lint rule name, and each value is true or false, enabling or disabling the lint rule.

There can be confusion around these three sections, as they seem to each serve approximately the same function: enabling and disabling diagnostics that are produced by the analyzer.

Proposal: a single section

I believe we can combine these three sections into a single one, placed at analyzer/diagnostics (or alternatively just a top-level diagnostics section). The above example would be written as:

analyzer:
  diagnostics:
    unused_element: false|disable
    unused_import: warning
    dead_code: error
    strict-casts: warning|enable
    double_quotes: false|disable
    single_quotes: warning|enable

Here's how it works:

• The analyzer/diagnostics section must be a YAML map.
• Each key is just a diagnostic name. In the case of Warning diagnostics, this can be a "shared name" (e.g. unnecessary_null_comparison or a "unique name" (e.g. unnecessary_null_comparison_always_null_false). In the case of Lint diagnostics, this can be a lint rule name, which acts as a shared name (e.g. invalid_runtime_check_with_js_interop_types), or can be a unique diagnostic name (e.g. invalid_runtime_check_with_js_interop_types_dart_as_js). The three "strict modes" will have to be changed to report diagnostics that have a shared name like strict_casts (or they could be refactored in a bigger way to just be like lint rules...).
• There are four possible values: false, info, warning, and error.
  • false means "don't report this."
  • info, warning, and error mean, "do enable this, and report violations at this severity."
• Alternative possible values: disable, enable, info, warning, and error.
  • disable means "don't report this."
  • enable means "do enable this, and report violations at the default severity."
  • info, warning, and error mean "do enable this, and report violations at this severity."

Alternatives

Using a YAML list

The analyzer/diagnostics section could be a list. This would allow specifying lint rules without a true/false/warning/whatever value:

analyzer:
  diagnostics:
    - unused_element: false
    - unused_import: warning
    - dead_code: error
    - strict-casts
    - double_quotes: false
    - single_quotes

I think this probably makes the syntax more confusing, and not much more terse. One benefit is that developers don't have to "choose" what severity lint rules are reported at (e.g. "warning").

Save the strict modes for later

It can be an incremental migration. Refactoring the strict modes may be a bigger effort. We could leave them in place, for a first phase of migrating to analyzer/diagnostics. The two sections which are most important to combine are analyzer/errors and linter/rules.

Migration

The migration would be incremental.

1. In one release, Dart 3.N, we introduce the analyzer/diagnostics section. It cannot be specified side-by-side with analyzer/errors or linter/rules, but the values can be merged with included files.

   • If a file with analyzer/diagnostics includes one-or-more files with analyzer/errors or linter/rules, the options are merged, not at the YAML level, but at the internal-representation level.
   • If a file with analyzer/errors or linter/rules includes one-or-more files with analyzer/diagnostics, the options are merged, not at the YAML level, but at the internal-representation level.
   • In this release, we also offer an assist to migrate the contents of an analysis options file from the old format to the new. (I guess we'd offer it for any file named *.yaml which has either an analyzer/errors section or a linter/rules section?

2. In the next release, Dart 3.N+1, we deprecate the old options. There is no change in the behavior of what options are parsed; only new warnings are reported in analysis options files.

   We delay deprecating them for CI purposes. (Maybe this can be done between dev releases or flutter rolls... but tricky with the customer tests.)

3. In the next release, Dart 3.N+2, we stop respecting the old sections (and report the old sections as "unknown"?).

CC @dart-lang/analyzer-team @DanTup @parlough

[matanlurey]

No strong opinion but I love that we're revisiting making the configuration easier to use!

[bwilkerson]

Overall I really like this, thanks!

Just a couple of comments / preferences.

In the case of Warning diagnostics, this can be a "shared name" ... or a "unique name" ...

We don't currently expose the unique name anywhere and I'd like to preserve that. The existence of a unique name is an implementation detail that allows us to tailor the message for a diagnostic to be more contextual, nothing more.

Alternative possible values: disable, enable, info, warning, and error.

I have a preference for this option because (a) I like that users can enable without specifying the severity and (b) I prefer 'disable' to 'false' as the way to disable a diagnostic.

Using a YAML list

If I understand YAML correctly, the value of the 'diagnostics' entry is a list of single entry maps. That seems less than ideal.

Also, I like that the simple map means that the structure can't have multiple equal keys.

Save the strict modes for later

I like the idea of making these diagnostics, but I'm also fine with making that migration later.

[parlough]

Alternative possible values: disable, enable, info, warning, and error.

+1 I think disable seems more understandable and explicit than false

The three "strict modes" will have to be changed to report diagnostics that have a shared name like strict_casts (or they could be refactored in a bigger way to just be like lint rules...).

Save the strict modes for later

+1 I love these modes but putting them in this new category in their current form does feel a little awkward. I think getting them in an ideal position, including one that the lints package might be happy including, is best left to more focused work later on.

Using a YAML list

I agree that mixing the two options is confusing, and just a map is likely better. I like requiring the specific enable or severity. It is a bit more typing, but I think completion makes it not so bad. Maybe a quick fix could be offered to add enable if a value isn't specified. Anyway, #52394 (or similar) would be my preferred way to edit the map.

In the next release, Dart 3.N+2, we stop respecting the old sections (and report the old sections as "unknown"?).

I think it might be worth 3.N+2 instead increasing the severity of the specific warning while still providing the assist/fix. I imagine some apps skip releases and developers going from no warning at all to just "unknown" would be confusing.

---

1. What about includes from packages?

You mention that they will be merged at the internal representation level, but will that support stay? There are many lint/analysis config packages out there with an upper SDK bound of Dart 4, so developers can continue to pull the outdated configs in after 3.N+2. If those included configs just stop working with no indication it might be confusing. Maybe a special diagnostic will be needed if an included file uses the old syntax?

2. Completion for diagnostic names:

Just a note, but one very small downside of this is losing the more specific completion suggestions for the rules property. I think lints are more likely to be configured, but there are a lot of non-lint diagnostics that might drown them out. Maybe they can be ranked higher?

[DanTup]

I like the idea, though I do worry this might make it less clear what are lints (if users mostly have lints here, they might incorrectly assume some of the other values are lints and look for documentation about them on the lints website?). Perhaps we should ensure we have good hovers on all of the possible values here? (we already show the lint docs in completion for lint names, though I don't think on hover - but we probably should).

Alternative possible values: disable, enable, info, warning, and error

I also prefer this.. Using false and some non-boolean options feels a bit weird.

In the case of Lint diagnostics, this can be a lint rule name, which acts as a shared name (e.g. invalid_runtime_check_with_js_interop_types), or can be a unique diagnostic name (e.g. invalid_runtime_check_with_js_interop_types_dart_as_js

What happens if I specify both of these with different values? Would they both appear in code completion? (if so, we might want to ensure they are annotated in some way so it's clear one is a group of the others?).

[FMorschel]

Here is the existing issue for hover there #55592.

[bwilkerson]

I like the idea, though I do worry this might make it less clear what are lints ...

I hadn't really thought about that, but the point is well taken.

On the other hand, the aspects that users can control about warnings and lints is the same: whether it's enabled and if so what the severity is. It does seem like the affordance for controlling those aspects should be the same.

One possibility would be to have separate maps for the two, something like

analyzer:
  diagnostics:
    warnings:
      unused_element: false|disable
      unused_import: warning
      dead_code: error
    lints:
      strict-casts: warning|enable
      double_quotes: false|disable
      single_quotes: warning|enable

(I didn't go look to see which were warnings and which are lints, I just wanted to show the structure I have in mind.)

I haven't decided whether I think that's better or worse, but it would make the distinction clear.

Perhaps we should ensure we have good hovers on all of the possible values here? (we already show the lint docs in completion for lint names, though I don't think on hover - but we probably should).

That would be good. We have been working for a while to have two separate pieces of documentation for lints: one that explains what it does and why you might want to enable it (which is presumably what we'd show here), and another that explains what's wrong at the location where a diagnostic was produced. We only have the second form of documentation for warnings. We hadn't really thought about explaining what the warning does and why you shouldn't disable it, but maybe we should?

if so, we might want to ensure they are annotated in some way so it's clear one is a group of the others?

That's exactly the kind of confusion I don't want to cause users, which is why I'm strongly opposed to allowing unique names to be used at all. The "shared" name isn't the name of a group or category of diagnostics, it's the name of a single diagnostic. The existence of the unique name just allows us to have messages that are better tailored to the specifics of the problem being reported. Let me give three examples.

We sometimes have multiple messages in order to provide more context:

• Extending 'Function' is deprecated.
• Mixing in 'Function' is deprecated.

The problem is the same, the class Function is being used in a way that is no longer allowed. The different messages just indicate how it's being used.

Sometimes there is optional information associated with a diagnostic, so we end up with multiple messages:

• '{0}' is deprecated and shouldn't be used.
• '{0}' is deprecated and shouldn't be used. {1}

The second is used when the constructor is used and a message is included (@Deprecated('Use bar instead.')). The problem isn't different. In both cases the issue is that a declaration that is deprecated is being used in some way when it shouldn't be.

Sometimes we use multiple messages when the amount of information to be communicated is different:

• The '{0}' directive is missing a '{1}' argument.
• The '{0}' directive is missing a '{1}' and a '{2}' argument.
• The '{0}' directive is missing a '{1}', a '{2}', and a '{3}' argument.

Again, the problem is the same: a doc directive is missing some required information. The different messages just allow us to correctly express the case when the number of pieces of information is different.

In none of these cases would it make sense for a user to enable one message and not another. It's a single diagnostic with multiple messages.

If there are cases where it would make sense for one message to be enabled and another not then it's an indication that they are different problems and should be different diagnostics with different (shared) names.

[DanTup]

On the other hand, the aspects that users can control about warnings and lints is the same: whether it's enabled and if so what the severity is. It does seem like the affordance for controlling those aspects should be the same.

Do we intend/expect the distinction between lints/built-in-warnings to remain or be something users care about? It feels a little odd to put two different things into the same list, but if the differences between those is ultimately just an implementation detail (where the code lives), then perhaps the documentation for the two should also be combined?

For example, if I open a project I've not worked on before and I see an interesting looking rule name I've not seen before, how do I know where to go and look for the documentation (assuming no IDE hover)? It feels odd to have to search two places even if it's not a lot of effort.

That's exactly the kind of confusion I don't want to cause users, which is why I'm strongly opposed to allowing unique names to be used at all.
...
If there are cases where it would make sense for one message to be enabled and another not then it's an indication that they are different problems and should be different diagnostics with different (shared) names.

This sounds better to me too.

[srawlins]

@DanTup @bwilkerson Do we currently still have no auto-complete in analysis_options.yaml, because we don't have good "broken" / "in-progress" YAML file support?

I think some usability issues can really be aided by some IDE tooling like:

• auto-complete lint diagnostic and warning diagnostic names
• auto-complete values (enable, warning, etc.)
• ctrl-click support on lint diagnostic and warning diagnostic names; either to some code file like lint_codes.g.dart, or opening a link externally in the browser.

[bwilkerson]

Do we intend/expect the distinction between lints/built-in-warnings to remain or be something users care about?

It's my belief that, from a user's point of view, the only difference between a warning and a lint is that the former is enabled by default and the latter is disabled by default. Don't misunderstand, I do think that the difference is important, but I think that's the only difference that users care about.

When we moved the code for lints from a separate package to being inside the SDK repo no user noticed the change.

Do we currently still have no auto-complete in analysis_options.yaml, because we don't have good "broken" / "in-progress" YAML file support?

We have auto-completion support in all of our YAML files: analysis options, pubspec, and even fix data. Sometimes it fails to work because of the lack of recovery in the YAML parser we use, but if you go to a linter/rules list, enter a new element by typing a -, then you can get suggestions even if you haven't started typing a lint name.

I think some usability issues can really be aided by some IDE tooling ...

I agree. I haven't tried implementing the ability to navigate from some location in a YAML file to an external web site, but it would be a nice feature if you could. Having the same information in a hover would be a second-best option.

[FMorschel]

For the opening of links about the lints, there is this issue Dart-Code/Dart-Code#2785. Maybe we could move it here to the SDK?

[DanTup]

ctrl-click support on lint diagnostic and warning diagnostic names; either to some code file like lint_codes.g.dart, or opening a link externally in the browser.

I don't know if Ctrl+Click to a website would feel good in VS Code, but we can include links in the hover (although if we also show all the information from the website in the hover, that might be redundant).

It's my belief that, from a user's point of view, the only difference between a warning and a lint is that the former is enabled by default and the latter is disabled by default. Don't misunderstand, I do think that the difference is important, but I think that's the only difference that users care about.

That distinction is probably also a bit blurry given the lint packages.. If you use the project templates that reference these packages, what is "enabled by default" and "on the diagnostic page instead of the lint page" probably don't seem to match up.

For the opening of links about the lints, there is this issue Dart-Code/Dart-Code#2785. Maybe we could move it here to the SDK?

Some of the options for implementing that could be done entirely from the server (for ex. links in hovers), but something like a context menu would probably also (or instead) require work in the VS Code extension. I feel like hovers might be the better solution though (a single implementation could work across more editors), so it could be worth having an issue here to discuss/agree.