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:

sdk/pkg/analyzer/lib/src/lint/options_rule_validator.dart

Lines 277 to 285 in 75d6404

	if (enabledValue is! bool) {
	var warningNode = enabled is YamlNode ? enabled : node;
	reporter.atSourceSpan(
	warningNode.span,
	AnalysisOptionsWarningCode.UNSUPPORTED_VALUE,
	arguments: [value, enabledValue, "'true' or 'false'"],
	);
	return;
	}

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 #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