Incompatible lint doesnt trigger for included yaml

[FMorschel]

Steps:

1. Create new Dart project
2. Add very_good_analysis to dependencies (can be dev)
3. Swap the content of the analysis_option.yaml with:

include: package:very_good_analysis/analysis_options.7.0.0.yaml

linter:
  rules:
    prefer_relative_imports: true

If you look at https://pub.dev/packages/very_good_analysis they say they prefer the usage of always_use_package_imports. But this triggers nothing, only this way you'll have both lints triggering on a loop. I think we either should trigger incompatible_lint here or prefer the user manually written lint (prefered IMO).

CC @pq

[bwilkerson]

I think we should let users know when they have conflicting lints enabled, and shouldn't silently ignore what's enabled in included options files.

For example, if the included options file enables the lint in a newer version so that the conflict didn't exist until the version of the defining package was updated, then the user should have the opportunity to explicitly decide whether they want to continue to use their previous style or to conform to using the newly enabled style. They can't do that if we ignore the conflict.

[pq]

I'm in full agreement. If a lint you have registered locally conflicts with one you have included, I'd expect a warning.

Thanks for tracking this down, @FMorschel!

[FMorschel]

Edit: here is the CL https://dart-review.googlesource.com/c/sdk/+/419982.

I've asked on Discord for some help. Here is a question from @DanTup:

I don't know if a dependency already exists, but if we produce diagnostics in an analysis_options file based on the content of another one, then changes to that other file will need to recompute the diagnostics for this one, so we don't leave stale diagnostics if you fixed it in the other file.

Also, for the current implementation, I basically copied a part of what pkg\analyzer\lib\src\task\options.dart does for getting the path for the included files, maybe these could be merged into a single place? The implementations differ a bit, but I think making them into the same thing would help us maintain them.

Edit 2: Hey @srawlins, can you take a look at this question since you are reviewing the CL? Thanks!

[FMorschel]

Unfortunately, this CL seems to have broken CI for a customer: https://dart-review.googlesource.com/c/sdk/+/419982/comments/4f71b53e_47e693c4.

This is the problem:

https://github.com/dart-lang/sdk/blob/75d640458b557e2c364c3d02d08c034bd94ff3a0/pkg/analyzer/lib/src/lint/options_rule_validator.dart#L277-L285

Thanks for warning @aam.

What are all the possible options for writing here? I see at Customizing Analysis Rules we have:

• info
• warning
• error
• ignore

But at https://github.com/dart-lang/sdk/issues/57034 we have also suggested:

• disable
• enable

Are these last two also valid as of today, or are they only new suggestions?

[FMorschel]

Here is the new CL for fixing the above-mentioned problem https://dart-review.googlesource.com/c/sdk/+/429240

Edit: I'm not sure how you feel about reviewing these analyzer changes @srawlins, pinging you here so you can take a look at the CL if you are fine with it.

FYI @bwilkerson

[srawlins]

Fix was reverted; @FMorschel has a re-land in progress.

[bwilkerson]

I asked a couple of questions in the CL, but I agree that the issue might be a better place to discuss them, so I'm moving them here. Specifically, I asked:

• Does it catch conflicts within a single file?
• Does it catch conflicts between the current file and an included file? (I know the answer to that one is 'yes'.)
• Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
• Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?

[bwilkerson]

@FMorschel answered:

Does it catch conflicts within a single file?

No, but we may use the previous warning for this case, nice catch!

Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?

It should but no specific lint for this case, we might need a better message.

Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?

Same as the above answer. It would also be a problem with the currently existing lint, it will only warn for a single incompatibility, not sure if it triggers multiple times for that case, we may need to check.

[bwilkerson]

Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?

It should but no specific lint for this case, we might need a better message.

Probably for that I'd say something like "included from {1} files." and then have context messages for everywhere that it's enabled. (Although that could be trick if it's included indirectly, because I'm not sure we remember the full path to every rule.)

Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?

Same as the above answer. It would also be a problem with the currently existing lint, it will only warn for a single incompatibility, not sure if it triggers multiple times for that case, we may need to check.

That's probably a rare enough case that it might not be worth handling. On the other hand it might help a user to know that it's a choice between one rule or three others.

If we handle this case we can probably just insert a list of the rule names into the message with few other changes (making "file" plural, for example. We can then use context messages to point to each of the enablement sites.

[FMorschel]

I think I found a problem here. Can anyone from the team confirm?

I've done the same as the OP and got no diagnostics. But when I added a local other.yaml file with the incompatible rule and added it to the include list it showed me the diagnostic. Can this be a problem with the yaml coming from another package?

[bwilkerson]

Did you remember to run pub after adding the dev dependency on very_good_analysis? (Or confirmed that the package_config.yaml file has a resolution of that name?)

If you use the insights pages (formerly the diagnostic pages), do both rules get listed as being enabled for this package?

[FMorschel]

Yes I verified both rules trigger inside my .dart files. And I tried using a wrong name for the import and it did warn about that too. Also verified that it wasn't because of dev dependencies. Changed to a normal dependency and nothing happened. I'll see if I can get a repro on a test.