Nightly lints for Clippy

[xFrednet]

This PR adds support for nightly lints in Clippy. 🎉 Now lints can set the version to nightly with the clippy::version attribute. That ensures that they only emit diagnostics if the run is currently a nightly build. This feature was suggested in issue #6623 to have a longer testing phase for new lints.

This works by adding a nightly mode to Clippy. This mode is usually just a copy of Session::is_nightly_build() but can be influenced by the user with the new CLIPPY_NIGHTLY environment variable. If set to 0 it will disable the mode even on nightly. If set to 1 it will be enabled even on stable. This was requested during a Zulip discussion to give control to users and not split the ecosystem. The mode can change between runs.

This mode currently only effects lints which have defined nighly as their version in the clippy::version attribute. Nightly lints in nightly mode will be handled as all other lints. On stable mode, these nightly lints will not be added to their lint groups and the diagnostics will be suppressed if the lint level was not changed by the user. The user is still free to use lint attributes like #[warn(nightly_lint)] to change the lint level of the nightly lint. The user-defined lint level has precedence over the nightly suppression mechanism.

I at first tried to change the actual lint level based on the current mode. However, the default lint level is defined in the Lint instance and has to be known at compile time. This sadly disables us from changing the lint level based on the current mode. As a solution, I check the lint level source when a diagnostic is being emitted. If the source is Default meaning that it was not overridden by the user, we can suppress the diagnostic. This sadly feels a bit more clumsy but still reuses rustc's infrastructure, which is nice.

I tried to split up the commits in sensible chunks. The additions arise from a change to cargo dev update_lints. It's probably good to review this commit by commit (Or skip the generated files)

---

On Zulip it was suggested to add a new lint that warns if a user enables a nightly lint with a lint attribute like #[warn(nightly_lint)]. This is still an open to-do I intend to implement once this solution is accepted.

---

changelog: New lints can now be restricted to nightly runs to enable better testing

cc: #8211

cc: @camsteffen This adds a new suppression mechanism to the span_lint functions you wanted to work on AFAIK.

cc: @flip1995

And in other news: I've handed in my bachelor theses and took some restful time off. Now I'm slowly getting back into Clippy 🖇️ 🙃

[rust-highfive]

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

[xFrednet]

cargo dev update_lints works fine for me. I'll take a look at the failed test tomorrow. 🙃

[camsteffen]

Why is the suppression in span_lint_* necessary? I thought it would be enough to just not add the lint to the group.

[flip1995]

Why is the suppression in span_lint_* necessary? I thought it would be enough to just not add the lint to the group.

Lint groups are entirely optional. They don't affect the lint, except if they're allow/warn/...

---

Going over this PR, the implementation looks good to me. I wonder if checking if a lint is a nightly lint on every emission has performance impacts. If so, maybe turning some functions into const functions and forcing inlining on the suppress_lint function might limit the performance impact.

[camsteffen]

Why is the suppression in span_lint_* necessary? I thought it would be enough to just not add the lint to the group.

Lint groups are entirely optional. They don't affect the lint, except if they're allow/warn/...

Right. Not adding to a group makes it allow by default. So isn't that enough?

[flip1995]

Right. Not adding to a group makes it allow by default. So isn't that enough?

Not adding it to a group makes it whatever you specify in the lint definition. The declare_clippy_lint macro is just a wrapper around the declare_tool_lint macro provided by rustc, which requires a Allow/Warn/Deny/Forbid lint level. The wrapper macro just projects the specified Clippy lint group to the level.

Actually adding it to the group is done by calling store.register_lint_group("group_name", vec![<LintIds>]) and not by the declare_clippy_lint macro.

[camsteffen]

Ah my mistake.

[xFrednet]

I wonder if checking if a lint is a nightly lint on every emission has performance impacts. If so, maybe turning some functions into const functions and forcing inlining on the suppress_lint function might limit the performance impact.

That was also a concern I had. I'm pretty certain that the overhead is negligible based on everything that happens in rustc after wards. My guess is that the request of the lint level is the most expensive check in the suppression, for that reason, it's also the last.

I thought about just using static values in clippy_utils::nightly without SyncOnceCell<> to avoid some accessing overhead, but that would require unsafe blocks and would have little to no benefit. We can see if the CI time has increases for this PR. That's not really a perf run but a good enough metric for now IMO 🙃

Also thank you for your comments. 🙃

---

This is now ready for review. @rustbot ready

[xFrednet]

It looks like the CI doesn't like #[inline(always)]. Now it's just a #[inline] :)

[camsteffen]

I thought about just using static values in clippy_utils::nightly without SyncOnceCell<> to avoid some accessing overhead, but that would require unsafe blocks and would have little to no benefit.

I was also wondering about that. Though in the end I suppose it doesn't matter much either way. But does it really need unsafe? Personally I think it would be okay to construct Lint directly without declare_tool_lint.

[xFrednet]

I was also wondering about that. Though in the end I suppose it doesn't matter much either way. But does it really need unsafe?

Well SyncOnceCell<> has some logic to prevent reinitialization, it basically wraps all values in an Option<>. And it seems hard for the compiler to argue about when this value is initialized and when not. However, with path prediction etc, it most likely doesn't matter (I want to learn more about optimizations at one point, as this is currently just a guess)

Accessing static mutable values always needs unsafe :) (At least from my tests yesterday)

Personally I think it would be okay to construct Lint directly without declare_tool_lint.

Constructing a lint that way is an option. However, then I'm still not sure how the lint level should be defined. We can't modify it afterwards, unless we make the lint instance mutable, which is an option, but sounds like something we should avoid. This means the level has to be known when static items are initialized. Rustc's session instance to check if the current run is a nightly run is only available afterwards AFAIK.

[camsteffen]

For example...

static NIGHTLY_LINT: SyncOnceCell<Lint> = SyncOnceCell::new();

pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &Conf) {
    let nightly_lint = Lint {
        level: if is_nightly { Allow } else { todo!("default level for lint group") }
        ..todo!(),
    };
    let _ = NIGHTLY_LINT.set(nightly_lint);
    store.register_lints(&[NIGHTLY_LINT.get().unwrap()]);
}

[xFrednet]

Wrapping every lint into SyncOnceCell (Or maybe just nightly lints) and then initializing it would be an option. We could potentially also use Lazy<Lint> to automatically initialize the lint once it is used. The only disadvantage I see with this is that we have to dereference every lint usage. That would be a one-time change but a possible one.

Here is an example:

static SOME_LINT: SyncLazy<Lint> = SyncLazy::new(|| 
    Lint {
        name: "SOME_LINT",
        default_level: if is_nightly_run() { Level::Warn } else { Level::Allow },
        desc: "Some description",
        edition_lint_opts: None,
        report_in_external_macro: true,
        future_incompatible: None,
        is_plugin: true,
        feature_gate: None,
        crate_level_only: false,
    }
);

fn some_function() {
    suppress_lint(cx, &SOME_LINT);
    //                ^ This dereference will be needed for all functions
    //                  taking `lint: &'static Lint`. Otherwise, this is fine.
}

I think this is a valid solution, the question is now which is better for our use case. 🤔

[flip1995]

Couldn't we extend the declare_clippy_lint macro to match the clippy::version attribute and if it finds a nightly, adapt the lint level depending on is_nightly_run. It might be a challenge to get a const version of is_nightly_run though.

[flip1995]

I wouldn't just add those attributes. I would first test if they're helping. It might as well be the case, that they just increase code size without improving performance (much)

[xFrednet]

Couldn't we extend the declare_clippy_lint macro to match the clippy::version attribute and if it finds a nightly, adapt the lint level depending on is_nightly_run.

The first part is definitely possible. I got a working version on another branch.

It might be a challenge to get a const version of is_nightly_run though.

This is the "hard" part if we want to check it at build time. When I asked on Zulip's help channel, it was strongly recommended not implement a test myself, but use to just use Session::is_nightly_build(). With that, I don't see a way to have is_nightly_run const.

What we can maybe do, is to check if this is currently a nightly run with UnstableFeatures::from_environment(krate: Option<&str>). It looks to me that Session::is_nightly_build() is doing the same thing under the hood. Except that it knows the crate that is being built. We could just initialize IS_NIGHTLY_RUN based on the result from UnstableFeatures::from_environment and the check of CLIPPY_NIGHTLY to set the lint level by constructing the lint in the macro.

Guess I'll give this a try and create a new PR, so we can compare the two? The code to add nightly lints to groups can remain the same :)

[bors]

☔ The latest upstream changes (presumably #8188) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

I'm going to close this PR as it's likely that we can implement nightly lints in rustc directly. Otherwise, this will also be simple to reopen 🙃