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

Unstable lints should be considered unknown #469

Closed
3 tasks done
djkoloski opened this issue Nov 4, 2021 · 4 comments
Closed
3 tasks done

Unstable lints should be considered unknown #469

djkoloski opened this issue Nov 4, 2021 · 4 comments
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

@djkoloski
Copy link

djkoloski commented Nov 4, 2021

Proposal

The -Aunknown_lints flag tells the compiler not to warn or error on any unrecognized #[allow(...)] arguments. However, unstable lints are treated differently and will produce an error that a feature needs to be enabled. Here's a quick example of where this can cause problems:

must_not_suspend

rust-lang/rust#89826

The must_not_suspend lint was added as a stable lint and remained that way for about a month. During this time it was discovered that the lint had false positives, and so was recently gated behind a feature. Upgrading a project through this process could go as follows:

  1. Add -Aunknown_lints and mark up any code that will trigger the lint with #[allow(must_not_suspend)]
  2. Upgrade the compiler
  3. Remove -Aunknown_lints since the lints are now known

However, after the lint is destabilized there is no further upgrade path:

  • Removing the #[allow(must_not_suspend)] attributes will trigger the lint on the current version of the compiler.
  • Adding -Amust_not_suspend will allow the removal of the attributes, but trigger an error that must_not_suspend is nightly as soon as the compiler is upgraded.
  • Using -Aunknown_lints does not prevent the error from -Amust_not_suspend after the compiler is upgraded.
  • There is no way for us to make the compiler ignore the must_not_suspend lint.

Escaping this migration trap is more difficult than it needs to be. -Aunknown_lints is a great mechanism for introducing lints to a codebase, but there's no equivalent for removing them. This forces us to perform a compiler upgrade and code change in lockstep, which is not always convenient or possible. We can do better.

Using -Aunknown_lints to ignore nightly lints

An easy way out of this would be to change the behavior of -Aunknown_lints to also ignore any uses of nightly lints. This would allow us to upgrade the compiler gracefully without introducing any sequence points where the build fails. When the lint is re-stabilized, it can be allowed on a case by case basis.

A possible alternative would be to leave the behavior of -Aunknown_lints the same and introduce a new -I flag that takes a lint name and treats the lint as unknown. An example here would be to combine -Imust_not_suspend to act as if the lint does not exist with -Aunknown_lints to ignore any unknown lints. This has the downside that doing so will not prompt users to handle the lint after it is stabilized.

Mentors or Reviewers

I am willing to take on the work to get this done. I'm not sure who would be the best person to review this work, but I can probably tug on @tmandry's sleeve to review.

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.

@djkoloski djkoloski 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 Nov 4, 2021
@Mark-Simulacrum Mark-Simulacrum added major-change A proposal to make a major change to rustc and removed major-change A proposal to make a major change to rustc labels Nov 10, 2021
@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2021

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 Nov 10, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 11, 2021
@tmandry
Copy link
Member

tmandry commented Nov 15, 2021

I'd happily review the code for this since it's affecting Fuchsia, but I don't want to second the MCP because @djkoloski and I work together.

@lcnr
Copy link
Contributor

lcnr commented Nov 18, 2021

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Nov 18, 2021
@apiraino
Copy link
Contributor

apiraino commented Dec 1, 2021

@rustbot label -final-comment-period +major-change-accepted

@apiraino apiraino closed this as completed Dec 1, 2021
@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Dec 1, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 9, 2021
djkoloski pushed a commit to djkoloski/rust that referenced this issue Mar 8, 2022
This change causes unstable lints to be ignored if the `unknown_lints`
lint is allowed. To achieve this, it also changes lints to apply as soon
as they are processed. Previously, lints in the same set were processed
as a batch and then all simultaneously applied.

Implementation of rust-lang/compiler-team#469
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 10, 2022
…=tmandry

Treat unstable lints as unknown

This change causes unstable lints to be ignored if the `unknown_lints`
lint is allowed. To achieve this, it also changes lints to apply as soon
as they are processed. Previously, lints in the same set were processed
as a batch and then all simultaneously applied.

Implementation of rust-lang/compiler-team#469
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

6 participants