Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Showcase nightly lints with macro #8444

Closed

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Feb 17, 2022

This is a showcase of how we can use Clippy's declare_clippy_lint! macro to implement nightly lints. The work's macro work is loosely based on #6830 by @camsteffen.

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.

In this version, I was unable to support a second environment value like CLIPPY_NIGHTLY to give the user control as all logic happens at compile time. The result is quite clean, but I would like to give more control to the user.

I tried to dynamically initialize lints with std::lazy::SyncLazy in another branch but that required dereferencing the instance for each usage to access the inner lint.


I can't really say which option I like the most. If we were starting from scratch, I would suggest using std::lazy::SyncLazy and just dereference every lint usage. However, in Clippy that would requite countless changes, even if simple once, and it might have a performance impact. Initializing it at compile time, like this PR does is quite clean in code, but doesn't give any power to the user to opt in or out of Clippy's nightly lints which I would like. The solution from #8435 works quite well, but is also not ideal.

Another option could be to add a new Nightly level to rustc. However, I would guess that that level would require a feature flag to enable emission, which we wanted to avoid. I would like to hear what you think about this and how nightly lints should be managed in general. :)


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

cc: #8211

cc: @camsteffen @flip1995

r? @flip1995 (for now, feel free to reassign)

BTW: I will rebase this and the other PR once we have decided which version we want to keep or if we want an entirely different implementation 🙃

xFrednet and others added 9 commits February 15, 2022 00:44
This was first suggested by `@camsteffen` in rust-lang#6830. Back then, it was decided to avoid this implementation to keep the macro simple. Now it makes sense to again use this macro, as the implementation of the macro will get a bit more complicated to support nightly lints.

Credit where credit is due:

Co-authored-by: camsteffen <cam.steffen94@gmail.com>
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 17, 2022
@camsteffen
Copy link
Contributor

I tried to dynamically initialize lints with std::lazy::SyncLazy in another branch but that required dereferencing the instance for each usage to access the inner lint.

To avoid all the dereferencing, another possibility is to have a new type for declare_clippy_lint!

struct ClippyLint {
    lint: SyncLazy<Lint>,
    category: .. // maybe?
}

and then the lint utils would accept &ClippyLint. This could have a side benefit of simplifying the metadata collection code.

@xFrednet
Copy link
Member Author

xFrednet commented Feb 17, 2022

True, that's also something I suggested for #8291. All rustc functions would still take &'static Lint but those instance would most likely be manageable. That would be a clean solution 👍

That's something I would work. However, I first want to be sure that we want to go that route, as that will still require some work. 🙃

@flip1995
Copy link
Member

Will this version produce unkown_lints warnings, if you try to allow(nightly_lint) on a stable version? If so, that would be quite bad, because there would be no way to disable those.

I think the first version looks better to me.

I'm not opposed to a wrapper struct for Clippy lints, where we may be able to store the documentation, category or other Clippy-specific lint informations.


Another question would be, if we want to add first-class support of nightly lints to rustc, instead of trying to hammer this into Clippy. I could imagine that rustc might be better suited to handle lint emission suppression or deal with allow/warn/... attributes with nightly lints. This will probably require a compiler MCP, but we could first just casually ask on Zulip.

@xFrednet
Copy link
Member Author

I'll create a topic in t-compiler/wg-diagnostics to ask. Otherwise, I think that having a wrapper type for Lints would be the cleanest solution 🙃

@xFrednet xFrednet marked this pull request as draft February 18, 2022 11:19
@xFrednet
Copy link
Member Author

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 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants