Minimal nightly-only lints

[Jarcho]

This uses is_nightly_build to determine if nightly-only lints should be added to their category, but makes no other changes. This means nightly-only lints are still registered and running, but they will no longer be enabled by default on beta and stable, nor will they be effected by their group name (e.g. #![warn(clippy::pedantic)]).

This accomplishes the main goal of preventing high false-positive rates from leaking out of the nightly release while still allowing the lints to be well tested there. Changes outside clippy are also not required to make this work, which I think any other approach requires.

Any nightly-only lint can still be run on stable, and any ICE's from them will still leak through. I don't think the first one is a problem as the lint has to be specifically named. The latter is an unfortunate problem, but I don't think that's avoidable without a much larger code change.

r? @flip1995

[xFrednet]

Hey 👋, the default lint level of a lint is set by declare_clippy_lint! which is defined in clippy_lints/lib.rs. Just removing the lints from their lint groups helps for allow-by-default groups, but not the others.

This was the blocker I had when implementing it in Clippy. The default lint level has to be static for rustc. And there is no good way to statically check for nightly.

[Jarcho]

Well that ruins my idea.

Edit: Looks like CFG_RELEASE_CHANNEL is passed when building clippy. I'll try using that.

[xFrednet]

That's one option to determine the channel. Though members of the release team advised against using it if I remember correctly.

Could we maybe use a build script to check the nightly flag?

[flip1995]

I think there was also the issue that with removing lints at build time, allow attributes wouldn't work on stable AND on nightly. This would be bad for users that want to allow nightly lints in their code, but still also build with stable. I don't remember where we ended up with the decision on how to deal with that.

But what I remember is that to implement nightly lints properly, we need 1st class support in rustc for nightly lints.

r? @xFrednet since your a bit more in the picture here. (also correct me if I remembered something wrong)

[Jarcho]

Attempt number two. This adds a build script which reads CFG_RELEASE_CHANNEL and sets the nightly feature when compiling clippy_lints.

What was the argument against CFG_RELEASE_CHANNEL? That's how rustfmt implements it's nightly-only config values.

[xFrednet]

What was the argument against CFG_RELEASE_CHANNEL?

From #8444

Some background, this uses the CFG_DISABLE_UNSTABLE_FEATURES environment value to check at compile time if this is a nightly build of Clippy. The same value is used by UnstableFeatures::from_environment(). It was pointed out on Zulip that this works but the actual intended and clean way would be to use UnstableFeatures::from_environment() which is sadly not const.

I suggest reading the linked zulip thread. It explains what I meant with "but the actual intended and clean way would be to use". Also #8444 and #8435 might also be interesting for you.

---

That's how rustfmt implements it's nightly-only config values.

IDK, but rustfmt can in theory just use is_nightly_build since the config values and validation don't need to be known at compile time.

---

This adds a build script which reads CFG_RELEASE_CHANNEL and sets the nightly feature when compiling clippy_lints.

Could you maybe access is_nightly_build from the build script? That sounds like a kind of stable way :)

[Jarcho]

Isn't RUSTC_BOOTSTRAP=1 set when building a stable version of clippy? That would make the build script always think it's a nightly build.

[xFrednet]

Isn't RUSTC_BOOTSTRAP=1 set when building a stable version of clippy? That would make the build script always think it's a nightly build.

I really don't know that 😅 Maybe ask in t-infra about that 🤔

[Jarcho]

One other idea would be to set the lint levels in cargo-clippy via command line args. That would let us use UnstableFeatures::from_environment.

[Jarcho]

Latest commit adds the changes to cargo-clippy needed to use is_nightly_build. Basically nightly-only lints would always be set to Allow, and cargo-clippy will the pass -W for all of them on nightly.

Changes to everywhere else still need to be done, but this gets the basic idea across.

[Jarcho]

Latest version should be working. Nightly lints are now always set to Allow in the lint definition. clippy-driver will insert -W and -D on nightly releases to enable the lints based on their category. Lint directives passed by the user will override those as they appear afterwards in the argument list. Category registration is done based on the same condition as the driver's argument insertion.

This also has an additional --no-unstable-lints argument in cargo-clippy to disable nightly-only lints even when running on a nightly release. This would be used by people who have to use the nightly channel to get unstable rust features, but don't want to get unstable lints from clippy.

Only downside I can see is the lint warning will state that the lint was enabled from the command line. This could be a confusing message to people as they never actually passed the argument.

[flip1995]

Hmm... I don't really like that this requires driver changes and complex lint definition macros. I thought we already came up with a plan on how to implement this in rustc to avoid all of this hackery. I'm sure this that also technically works, but I would really prefer a clean rustc solution, even if it will take a little longer to get there.

[xFrednet]

I haven't looked at the changes, but also believe that it would be simpler to implement in rustc. The implementation itself should be pretty simple, it could be suppressed in struct_lint_level. There you have access to the lint reference and can suppress the lint. That's also how it's done for #[allow].

The actual work with this implementation is finding a way that's also okay for the compiler maintainers. They mentioned in the linked Zulip that they want to avoid different behavior between nightly and beta/stable builds (especially related to tests). One option might be to just have nightly lints as allow by default on stable/beta and have an additional check that every test file in Clippy has #[deny(lint_name)] for all lints tested in that file. Then the tests would work the same on every version. The lint would also not be added to the lint group in this case 🙃

[Jarcho]

The easiest way to make sure the tests work would be to have to have compile-test allow all the lints by default. I don't know how easy that would be though.

The lack of lint group registration would be annoying. Allowing all style lints and then all of a sudden getting warning for one does not make for a great experience.

[xFrednet]

The easiest way to make sure the tests work would be to have to have compile-test allow all the lints by default. I don't know how easy that would be though.

The lint level is evaluated with the lint_levels query. There you would only need to adjust the case where the default lint level of a lint is read. The simplest way would be to add a is_nightly field to Lint and then checking it, when the default lint level is requested.

The lack of lint group registration would be annoying. Allowing all style lints and then all of a sudden getting warning for one does not make for a great experience.

The lint group registration could be adjusted on startup. This would mean on nightly, the lint is added as usual but not on stable/beta. This should be fine, since they're allow by default on stable/beta.

In such a case, we should avoid #[warn(clippy::<group_name>)] or other group attributes besides #[allow] in our tests. Just to make sure that they behave the same on stable/beta. But that should be fine, at least I don't know a case where it would be a good idea to have that in our tests.

Does this help, or did I misunderstand you? 🙃

[Jarcho]

The lint level is evaluated with the lint_levels query. There you would only need to adjust the case where the default lint level of a lint is read. The simplest way would be to add a is_nightly field to Lint and then checking it, when the default lint level is requested.

I was talking specifically about the behaviour differences for our tests between nightly and stable builds. If compile test did the equivalent of passing -A clippy::all then any test would require explicitly setting the lint level.

The lint group registration could be adjusted on startup. This would mean on nightly, the lint is added as usual but not on stable/beta. This should be fine, since they're allow by default on stable/beta.

Ok. That's what I have right now. For some reason I interpreted your comment as saying avoid doing any stable/nightly differences on clippy's side. Re-reading it doesn't give me the same interpretation.

[xFrednet]

I was talking specifically about the behaviour differences for our tests between nightly and stable builds. If compile test did the equivalent of passing -A clippy::all then any test would require explicitly setting the lint level.

Ahh, that's true, but I feel like we don't want to generally pass -A clippy::all to detect when lints overlap. 🙃

[Jarcho]

I don't think I've seen that happen where the solution wasn't just to add yet another allowed lint to a test. It's possible it's caught something before though.

[xFrednet]

I've seen it a few rare times. clippy::branches_sharing_code and clippy::if_same_then_else for instance. The #[allow] lints are also often related to the test code and not the specific pattern that triggers the tested lint itself. Though it might be less common than I think 🤔

[flip1995]

I don't think I've seen that happen where the solution wasn't just to add yet another allowed lint to a test.

It happened before multiple times. For examples, look at the methods module and for things like this (N.B. this is not unique to the methods module):

rust-clippy/clippy_lints/src/methods/mod.rs

Lines 2702 to 2706 in 868dba9

	let biom_option_linted = bind_instead_of_map::OptionAndThenSome::check(cx, expr, recv, arg);
	let biom_result_linted = bind_instead_of_map::ResultAndThenOk::check(cx, expr, recv, arg);
	if !biom_option_linted && !biom_result_linted {
	unnecessary_lazy_eval::check(cx, expr, recv, arg, "and");
	}

Those structures exist, because the lint overlapping happened. We definitely want to continue test for this for all non-allow lints.

[xFrednet]

Hey @Jarcho, I'm closing this PR, as it seems like you're starting to focus on these changes in rustc (And I want to triage my PRs). Feel free to reopen it, if you ever want to continue this branch :)