Implement framework so that lints can be enabled in nightly only

[xFrednet]

Hey, I'd like to take a stab at the second point in issue #6623: "Implement framework so that lints can be enabled in nightly only". I have three ideas how this can be implemented, with advantages and drawbacks. Here, I'm hoping to get some early feedback and maybe a few suggestions.

First some background:

• During the compilation with bootstrap, we can determine the targeted release channel with the CFG_RELEASE_CHANNEL environment value. If CFG_RELEASE_CHANNEL is not set, Clippy is most liekly being compiled with Cargo in our respository. Therefore, nightly lints should be enabled. Additionally, we could use UnstableFeatures to determine if unstable features are enabled at runtime.
• In Clippy, we can either use the #[clippy::version] attribute or an additional argument in the declare_clippy_lint macro to indicate is a lint should only be enabled on nightly.
• We need to deal with lint attributes like allow, warn and deny to ensure that they don't trigger the unknown_lints lint when they reference nightly lints while running Clippy stable. This is optional but would be ideal as we otherwise force users to always use nightly.

Now these are the implementation suggestions I have:

1. We add a new Feature to rustc that has to be enabled using a #![feature] attribute to the crate. The nightly lints are only added to the lint store if the feature is enabled, the suppression will be a bit more complicated but should be possible by first checking if the lint is registered.
   • Advantages:
     • Users clearly opt in to Clippys nightly lints
     • Users can use allow, warn and deny attributes with confidence that every user has a nightly Clippy version, since features can only be enabled on nightly versions
   • Disadvantages:
     • We probably get less users testing our nightly lints in comparison to the other options
     • With the feature, users restrict themselves to using nightly.
     • The suppression can't be done at compile time, since features can be enabled and disabled. This makes the emission suppression a bit more expensive in comparison.
     • We would need to enable that feature in all uitests that test nightly lints

2. We register the lints as usual but change the lint level based on the release channel. On stable and beta these lints would be allow-by-default, on nightly they are registered in their corresponding lint groups and with the corresponding lint level.
   • Advantages:
     • Fairly simple to implement, as we reuse rustc's linting infrastructure.
     • Users can switch between stable and nightly as they like, since the lints are also known on stable.
   • Disadvantages
     • Users might unknowingly enable nightly lints on stable.
     • Users don't clearly opt in to nightly lints.

3. We register the lints as usual but suppress the emission for nightly lints based on the release channel in Clippy. I believe this can be fairly easily done by intercepting the lint in our span_lint* methods or later on in span_clippy_lint once Migrate to span_clippy_lint #7797 is implemented.
   • Advantages:
     • Users can switch between stable and nightly as they like, since the lints are also known on stable.
     • Users can't accidentally enable nightly lints on stable.
     • We can implement most of the suppression at compile time
   • Disadvantages
     • Clippy has its own suppression mechanism that has to be maintained, even if it should be simple
     • Users don't clearly opt in to nightly lints.

---

This list might have missed some options, advantages or disadvantages, feel free to comment on this. I can create mockup implementations for one or two options to compare them 🙃

What are your thought's on this @rust-lang/clippy. Also cc: @Jarcho in case you're interested in this as well 🙃

[xFrednet]

My thoughts are that option 2 or 3 are preferable, I'm leaning towards option 3 as it sounds like fun to implement. However, it would require some more work in comparison to option 2.

[flip1995]

I don't like option 1, because I don't want users to do anything to get nightly lints, except for using the nightly toolchain.

Best case would be, if we could just prevent registering the lints. But I'm not sure if or how that's possible. Maybe we could register it for a different LintPass? But that'll probably cause code duplication.

Just not emitting a lint is probably the easiest option, but I'm not sure if this has an performance impact.

[xFrednet]

I thought about not registering the LintPass in the first place, but that would be impossible, with some lint passes that have several lints, like the one we have in our methods module. With the third option, I would like to add a is_enabled function to abort for nightly lints early. With option 3 (How I thought of implementing it), I can also think of a functionality to prevent the registration of lint passes, if all lints are nightly. That would be the advantage of creating our own suppression mechanism 🙃

Just not emitting a lint is probably the easiest option, but I'm not sure if this has an performance impact.

This depends on the option we choose. Option one would be costly, option two would be almost free as we use the normal suppression with the allow lint level. The third option would add one if check, which should almost be free as well.

[camsteffen]

What is the difference semantically between an "unstable" lint and a "nursery" lint?

[xFrednet]

I think the main difference is what they represent, nursery lints are lints which are currently under development or have some false positives which should get fixed. Unstable or nightly lints should already be in the correct category with the correct lint level on nightly. The idea is that they are ready as far as we know, but might have some false positives. We also allow users to use #![warn(clippy:nursery)] which wouldn't be advisable if we add new restriction lints. For me, nightly lints mean that they will be stabilized in one or two releases if we get no negative feedback, while nursery lints will remain in nursery until the issues are fixed. Does that answer your question, or did I misunderstand you? 😅

[camsteffen]

Makes sense!

How about on stable the lint is just not registered with any lint group.

[xFrednet]

That would be the second suggestion and definitely the easiest 🙃

[Jarcho]

Option 1 looks unusable if I understand it correctly. It requires a code change in the crate being linted (which prevents it from compiling on stable) just to be able to test nightly lints. That's basically not going to happen in practice.

Option 3 matches how other nightly things work, but option 2 is probably good enough for all practical purposes. There's already no guarantee lint detection won't change from version to version (either adding or removing cases), so nightly lints changing isn't an issue. If a nightly lint is explicitly enabled then you get exactly what you asked for if the lint runs on stable.

Basically option 2 unless you want to put in the effort for option 3 (though it looks like it will be fairly simple).

[xFrednet]

Alright, thank you for the feedback so far! I'm currently working on a prototype for option 3. and I'll create an implementation for 2. as well. Option 3 appears to be more hacky than I hoped, but we'll see how my mockup turns out. It might take some time until I post an update 🙃

[camsteffen]

So the benefit of option 3 is to disallow the lints completely on stable? Can we get the right behavior by just not registering the lints or by using register_removed with an appropriate message?

[camsteffen]

Another option is to add something like if unstable_enabled() to the lint implementation and somehow make our tests fail if we forget to do so. But I suppose we'd rather not.

[Jarcho]

For option 2 would setting clippy::version to nightly and then using a cargo feature to determine whether those are added to lint groups or not accomplish the job? Also has the benefit of having a good default version as well.

[xFrednet]

Can we get the right behavior by just not registering the lints or by using register_removed with an appropriate message?

I would like to avoid using register_removed or something similar, as users should be free to switch from nightly to stable. Something like this would trigger a warning if a user adds a lint attribute on nightly and switches to stable (where the lint is currently also registered). In practice, this will most likely never happen, but it would be nice to have this. One advantage of option 3 would be that we could ignore lint passes, which only implement nightly lints.

Another option is to add something like if unstable_enabled() to the lint implementation and somehow make our tests fail if we forget to do so. But I suppose we'd rather not.

I'm currently trying to implement a kind of automatic mechanism to have this, but I have to figure out a nice way to handle test and so on. Otherwise, this is something which should be avoided.

For option 2 would setting clippy::version to nightly and then using a cargo feature to determine whether those are added to lint groups or not accomplish the job? Also has the benefit of having a good default version as well.

Clippy stable, beta and nightly are being compiled in the rust repo without cargo. But that's the basic plan for option two. I'm also more in favor of it, the more I work on my idea for a custom suppression mechanism.

[camsteffen]

I would like to avoid using register_removed or something similar, as users should be free to switch from nightly to stable.

That is fine and I agree that is probably what we want. But FWIW I think this is different from nightly features:

`#![feature]` may not be used on the stable release channel

If we want to allow unstable lints in the stable channel, then Option 2 seems perfect.

One advantage of option 3 would be that we could ignore lint passes, which only implement nightly lints.

I don't think so because someone could #[warn] on a function.

[Jarcho]

I don't think so because someone could #[warn] on a function.

If the lint pass was never registered the detection code would never run as rustc doesn't know anything about it. I think the intent is to not call register_*_pass if the lint pass is only associated with nightly lints.

Clippy stable, beta and nightly are being compiled in the rust repo without cargo.

So pass --cfg feature=nightly or whatever cargo does when it compiles. It accomplishes the same goal.

[xFrednet]

If the lint pass was never registered the detection code would never run as rustc doesn't know anything about it. I think the intent is to not call register_*_pass if the lint pass is only associated with nightly lints.

That was an idea I had in case a lint implementation contains some ICEs. However, that is not super important. There might also be ways to still implement something like this. For now, I'll focus on changing the lint level and group membership 🙃

[camsteffen]

I think that #[warn] should either enable a lint or trigger a warning if the lint is not available. Doing nothing is not an option IMO as that would be surprising behavior. I'm not sure we've agreed on the desired behavior, which is separate from implementation.

[xFrednet]

This topic has been discussed in our meeting today on Zulip. I will take a bit until I can create a PR for this, as I'm currently writing my bachelor thesis. I'll still claim this issue until then 🙃

@rustbot claim

[xFrednet]

In several discussions, we determined that this feature should be implemented in rustc directly. @Jarcho created a PR to implement this in rustc: rust-lang/rust#109063.

The compiler team requested us to create a MCP first. That's the latest status I'm aware.

I'll close this issue, as it was directed towards implementing this in Clippy.