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

Make unknown/renamed/removed lints passed via command line respect lint levels #667

Closed
1 of 3 tasks
weihanglo opened this issue Aug 25, 2023 · 2 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@weihanglo
Copy link
Member

weihanglo commented Aug 25, 2023

Proposal

Background

When you specify an unknown/renamed/removed lint name in Rust code, for example #![deny(foo)], the compiler respects the lint system and emits a warning by default1.

However, if it is from the command line, such as -D foo, the compiler simply emits an error and exits2. There is no way to recover from the error, since those errors are emitted directly via Session, not under the control of LintContext or LintLevelsbuilder.

Possible solution

This MCP proposes that those errors should be considered as normal lints and emitted by the lint system. Specifically,

The proposed behavior is more consistent from both source-code level and end-user perspective.

User-facing impact

Compiler invocations that rely on the aforementioned always-error-out behavior will become valid. This is easy to fix by adding -D unknown-lints or -D renamed-and-removed-lints as a command line argument (tbh not as easy as it looks like3).

Motivation

With -Zlints unstable feature in Cargo, you can specify lint rules and levels in Cargo.toml. Under the hood Cargo aggregates those settings and passes via command line to the compiler.

Without this MCP, it is not backward compatible when a user adds a new lint, and then use an old version of the compiler. The compiler will refuse to compile even unknown-lints lint is warn by default not deny.

See rust-lang/cargo#12495.

Compiler version

$ rustc +nightly -vV
rustc 1.74.0-nightly (58eefc33a 2023-08-24)
binary: rustc
commit-hash: 58eefc33adf769a1abe12ad94b3e6811185b4ce5
commit-date: 2023-08-24
host: aarch64-apple-darwin
release: 1.74.0-nightly
LLVM version: 17.0.0

Mentors or Reviewers

Not sure. Perhaps @estebank or @compiler-errors?

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Footnotes

  1. https://github.com/rust-lang/rust/blob/738df13e8a73d6d95ddec81b7393b2d2a64b7e93/compiler/rustc_lint/src/levels.rs#L918-L951

  2. https://github.com/rust-lang/rust/blob/738df13e8a73d6d95ddec81b7393b2d2a64b7e93/compiler/rustc_lint/src/context.rs#L347-L367

  3. This is another command line argument precedence issue. Think about this. Which should emit an unknown lint error? -Dunknown-lints -Dfoo or -Dfoo -Dunknown-lints? Or should we make these lints a special priority like warnings lint group? That may lead to https://github.com/rust-lang/rust/issues/75668 though. Anyway, this belongs to a separate MCP.

@weihanglo weihanglo added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Aug 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 25, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Aug 25, 2023
@davidtwco
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Aug 28, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 1, 2023
@oli-obk oli-obk added to-announce Announce this issue on triage meeting major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Sep 11, 2023
@oli-obk oli-obk closed this as completed Sep 11, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants