Linter rules such as `enable_null_safety` don't show diagnostic anymore

[FMorschel]

My pubspec.yaml:

environment:
  sdk: ^3.6.0

I'm creating a new flutter project with the latest stable.

I saw the following doesn't have any warnings:

linter:
  rules:
    - enable_null_safety

I'd expect this to give a warning as is not needed. This is on the lint docs:

NOTE: This rule is removed in Dart 3.0.0; it is no longer functional.

Some other rules such as always_require_non_null_named_parameters are also not showing any diagnostics.

Tested by changing my pubspec.yaml to:

environment:
  sdk: ^3.3.0

And the diagnostic for always_require_non_null_named_parameters did show up. So I guess something is not working as intended.

[FMorschel]

I also feel like these rules could show up as deprecated on auto-complete in these cases or something similar.

WDYT @bwilkerson and @DanTup? I'm unsure if that is possible, to show deprecation only when dependencies are above a certain version.

[DanTup]

It looks like there is some existing code that handles removed lints:

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

Lines 114 to 124 in 31a354f

	} else if (state is RemovedState) {
	var since = state.since.toString();
	var replacedBy = state.replacedBy;
	if (replacedBy != null) {
	reporter.reportErrorForSpan(AnalysisOptionsWarningCode.REPLACED_LINT,
	node.span, [value, since, replacedBy]);
	} else {
	reporter.reportErrorForSpan(AnalysisOptionsWarningCode.REMOVED_LINT,
	node.span, [value, since]);
	}
	}

And that lint is marked removed since 3.0:

sdk/pkg/linter/lib/src/rules/enable_null_safety.dart

Line 14 in 9b631bf

state: State.removed(since: dart3),

So perhaps there is a bug/regression here somewhere (I'm assuming by the word "anymore" in the title, that this worked in the past but doesn't now?).

[bwilkerson]

Yes, there should be a diagnostic displayed in the analysis_options.yaml file if there is a removed lint being enabled. The fact that it isn't there is a regression and a bug.

As for code completion, the intention was to not display removed lints at all (rather than marking them as deprecated). If removed lints are being suggested in code completion then that's also a bug (and might have a different cause).

[FMorschel]

I'm assuming by the word "anymore" in the title, that this worked in the past but doesn't now?

That's what I assumed. I'm not 100% sure. I've not tested with other versions of the SDK yet but that is what I would expect.

As for code completion, the intention was to not display removed lints at all (rather than marking them as deprecated). If removed lints are being suggested in code completion then that's also a bug (and might have a different cause).

I'll open a second issue about it then. Thanks.

[FMorschel]

I've just tested both with the latest master. The auto-completion (for removed lints) is not happening on master (so I won't open an issue on it) but there is no diagnostic on the lints when adding them manually.

[DanTup]

I had a quick look at this and can reproduce. There is a test test_removed_rule which tests that something removed in the current SDK produces the lint, but not something removed in an earlier SDK. A test that does that fails:

  @soloTest
  test_removed_rule_inEarlier() {
    assertErrors(
      '''
linter:
  rules:
    - removed_in_2_12_lint
''',
      [AnalysisOptionsWarningCode.REMOVED_LINT],
      sdk: dart3,
    );
  }

I think the issue is the way the versions are compared by isRemovedInCurrentSdk. It's not clear to me from the name if it really means "in the current SDK" or it means "in the current or a previous SDK" (if the former, it's working as intended - but that behaviour seems questionable):

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

Lines 43 to 46 in 3f30223

	bool isRemovedInCurrentSdk(State state) {
	if (state is! RemovedState) return false;
	return currentSdkAllows(state.since);
	}

In this case, sdk is the current SDK as a constraint like ^3.6.0 and since is a Version of 3.0.0. Since a constraint of ^3.6.0 does not allow 3.0.0, currentSdkAllows returns false and therefore isRemovedInCurrentSdk also returns false.

My feeling is that probably isRemovedInCurrentSdk should be isRemovedInCurrentOrEarlierSdk and the comparison should be just whether since is less than or equal to the minimum version allowed by sdkVersionConstraint(?)? I'm not certain and not sure if the classes have a way to express this simply though. There are other similar functions here (like isDeprecatedInCurrentSdk) that may need similar treatment.

(cc @pq)

[pq]

Oh yeah. That does seem wrong and on the surface you're idea of a fix sounds exactly right.

I'll bump the priority on this too since I think it's a legitimate regression that we should address.

Thanks for digging in @DanTup and for the report @FMorschel!