Skip to content

Allow tool lint levels to be set based on the build environment. #109063

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

Closed
wants to merge 2 commits into from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 12, 2023

Changes to allow tool lints to have their default lint level determined by the current build type. Allows using:

declare_tool_lint! {
/// Docs
pub LINT_NAME,
if UnstableFeatures::is_nightly_build_environment() {
    Level::Warn
} else {
    Level::Allow
},
"description"
}

There's still the question of whether this is the best way to go about this.

cc @rust-lang/clippy

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 12, 2023
@jyn514
Copy link
Member

jyn514 commented Mar 13, 2023

Please don't allow this to be set based on arbitrary env variables or cfgs. Add a nightly specifier to the macro if that's what you care about.

@Manishearth
Copy link
Member

I wonder if default level should be something that is set when registering the lint rather than in the static lint definition.

It seems weird to have it work this way, perhaps we should rethink that architecture? We can still have the convenience API and macro.

It does seem like doing arbitrary things based on, e.g. clippy.toml, may be useful here.

@jyn514
Copy link
Member

jyn514 commented Mar 13, 2023

Not opposed to allowing these to be set based on clippy.toml (although maybe it makes sense to have a compiler MCP for that?) but the current PR doesn't allow that, it only allows setting it based on the build configuration, not any file at runtime.

@Mark-Simulacrum
Copy link
Member

To the extent possible, we try to avoid stability affecting anything except whether feature gates can be turned on or off. Doing so means that checking something against nightly (e.g. for bisecting) is more problematic, and also increases the chances that we need to do work during bootstrap bumps (e.g., because lints are now causing errors in tools or similar).

So I think this needs stronger motivation than I'm currently aware of.

@Manishearth
Copy link
Member

@jyn514 Yes, I'm proposing an alternate approach here where lints do not store their default level on the type anymore, it's a part of the process of registering. I think that's a cleaner approach — I do not see a strong reason for the default level to be a part of the &'static Lint anyway, beyond convenience for registering ,which can be handled by making it something that you can do (via a LintWithDefaultLevel trait, or an option field) but don't have to.

I think it's a bit more principled and clean.

@Manishearth
Copy link
Member

Manishearth commented Mar 13, 2023

But I do think any change to this is probably MCP worthy (and then we can properly discuss potential approaches for implementation)

Things to discuss:

  • The way to implement this
  • The bounds within which people are "allowed" to make things conditional in lints (Rust and clippy could come up with different ones! but rust probably wants to tell clippy where it should not overstep)

without nailing down particular Clippy plans

@apiraino
Copy link
Contributor

ok so IIUC here the way to go is opening an MCP, right? @Jarcho do you want to own this? ofc if you're still interested (and have bandwidth) for that

@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023
@Dylan-DPC Dylan-DPC added needs-mcp This change is large enough that it needs a major change proposal before starting work. S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2023
@bors
Copy link
Collaborator

bors commented Sep 14, 2023

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 14, 2023
@xFrednet
Copy link
Member

The group membership should also be controlled as part of this PR. Otherwise, it could happen that #[warn(clippy::style)] enables lints, that should still be Allow-by-default

@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 20, 2023
@Dylan-DPC
Copy link
Member

Closing this as this requires an MCP that hasn't been created and there has been no activity on this.

@Dylan-DPC Dylan-DPC closed this Feb 8, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed needs-mcp This change is large enough that it needs a major change proposal before starting work. S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.