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

Tracking Issue for [lints] table RFC 3389 #12115

Closed
ehuss opened this issue May 9, 2023 · 66 comments · Fixed by #12648
Closed

Tracking Issue for [lints] table RFC 3389 #12115

ehuss opened this issue May 9, 2023 · 66 comments · Fixed by #12648
Labels
A-lints-table Area: [lints] table C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo to-announce

Comments

@ehuss
Copy link
Contributor

ehuss commented May 9, 2023

Summary

RFC: #3389
Implementation: #12148, #12168, #12174, #12174
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints
Issues: A-lints-table Area: [lints] table

This feature adds a [lints] table to Cargo.toml to configure reporting levels for rustc and other tool lints.

See also #3591, #5034

Testing instructions

Requires

  • nightly: nightly-2023-05-25 or newer
  • beta
  • stable

Steps:

  1. Switch from attributes to [lints] table per the unstable reference
  • -Dwarnings should likely still be passed in explicitly so local experimentation doesn't get blocked on perfection
  • CI jobs passing in -Adeprecations likely should still do so since users should see deprecation warnings locally to decide whether to address them
  1. Verify lints in CI with a supported compiler by adding the -Zlints flag

Debugging

  • Build with --verbose to see what order [lints] and other RUSTFLAGS are being passed to the underlying tool

Example: #12178

Updates Since RFC

  • cargo new will automatically inherit the lints table (Automatically inherit workspace lints when running cargo new/init #12174)
  • profile.rustflags precedence with [lints] was unspecified. We have put made it a higher precedence.
  • rustflags derived from [lints] table are not included in rustc -C metadata hash.
  • [lints] should only apply to doctests as much as a diagnostic attributes (#[warn(...)]) in an .rs file applies to doctests
  • Per-target or per-target-kind (bin vs lib) [lints] tables is deferred (not in original RFC)
    • Workaround: using diagnostic attributes (e.g. #[warn] in the target)
    • This is helpful when production code (a subset of [lib], [bin]s in a workspace) want to be held to a higher standard than development code (especially tests)
      • e.g. clippy has an unwrap_used lint but people might want that in tests. clippy works around it today by having a allow-unwrap-in-tests setting
    • Since workspaces can have non-production libs and bins, it almost seems like this is more of user-defined lint settings groups and inheriting specific groups. Or put another way, more design work is needed.
  • Workspace-wide lints are deferred until they are needed. See Tracking Issue for [lints] table RFC 3389 #12115 (comment) for ideas
  • We fingerprint all lints; we can optimize the fingerprinting later

Unresolved Issues

Feedback

Cases that don't work well:

  • Some lints are crate specific, like only wanting missing_docs and missing_debug_implementations for [lib]s (comment)
  • Some lints are package and maybe crate specific, like disallowed_methods in chore: dogfood Cargo -Zlints table feature #12178
  • Explicit inheritance doesn't work well when you want to catch lints for example packages but want the example package to standalone from the workspace (comment)

Future Extensions

No response

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

@ehuss ehuss added the C-tracking-issue Category: A tracking issue for something unstable. label May 9, 2023
@ehuss ehuss added S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. A-lints-table Area: [lints] table labels May 9, 2023
@epage

This comment was marked as resolved.

@flip1995

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@thomaseizinger

This comment was marked as resolved.

@oxalica

This comment was marked as resolved.

@roblabla

This comment was marked as resolved.

@zblach

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@Nemo157

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@Nemo157

This comment was marked as resolved.

@kornelski
Copy link
Contributor

Currently there's no mechanism for controlling Cargo's warnings. Could the [lints] be used to silence Cargo's own warnings too? Like this one?

@epage
Copy link
Contributor

epage commented May 12, 2023

Yes, one of the future possibilities listed in the RFC is for cargo to have configurable lints.

@ehuss

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@Nemo157

This comment was marked as resolved.

@epage

This comment was marked as resolved.

@ehuss

This comment was marked as resolved.

bors added a commit that referenced this issue May 22, 2023
feat: `lints` feature

### What does this PR try to resolve?

Implement rust-lang/rfcs#3389 which shifts a subset of `.cargo/config.toml` functionality to `Cargo.toml` by adding a `[lints]` table.

This **should** cover all of the user-facing aspects of the RFC
- This doesn't reduce what flags we fingerprint
- This will fail if any lint name as `::` in it.  What to do in this case was in the RFC discussion but I couldn't find the thread to see what all was involved in that discussion
- This does not fail if a `[lints]` table is present or malformed unless nightly with the `lints` feature enabled
  - The idea is this will act like a `[lints]` table is present in an existing version of rust, ignore it
  - The intent is to not force an MSRV bump to use it.
  - When disabled, it will be a warning
  - When disabled, it will be stripped so we don't publish it

Tracking issue for this is #12115.

### How should we test and review this PR?

1. Look at this commit by commit to see it gradually build up
2. Look through the final set of test cases to make sure everything in the RFC is covered

I tried to write this in a way that will make it easy to strip out the special handling of this unstable feature, both in code and commit history

### Additional information

I'd love to bypass the need for `cargo-features = ["lints"]` so users today can test it on their existing projects but hesitated for now.  We can re-evaluate that later.

I broke out the `warn_for_feature` as an experiment towards us systemitizing this stabilization approach which we also used with #9732.  This works well when we can ignore the new information which isn't too often but sometimes happens.

This does involve a subtle change to `profile.rustflags` precedence but
its nightly and most likely people won't notice it?  The benefit is its
in a location more like the rest of the rustflags.
epage added a commit to epage/cargo that referenced this issue May 22, 2023
In rust-lang#12115, we explored how we can let stable projects
experiment with `[lints]` to provide feedback.  What we settled on is
switching from the `cargo-features` manifest key to the `-Z` flag as
`cargo-features` always requires nightly while `-Z` only requires it
when being passed in.  This means a project can have a `[lints]` table
and have CI / contributors run `cargo +nightly check -Zlints` when they
care about warnings.
bors added a commit that referenced this issue May 23, 2023
fix(lints): Switch to -Zlints so stable projects can experiment

### What does this PR try to resolve?

In #12115, we explored how we can let stable projects
experiment with `[lints]` to provide feedback.  What we settled on is
switching from the `cargo-features` manifest key to the `-Z` flag as
`cargo-features` always requires nightly while `-Z` only requires it
when being passed in.  This means a project can have a `[lints]` table
and have CI / contributors run `cargo +nightly check -Zlints` when they
care about warnings.

### How should we test and review this PR?

Demonstrate how you test this change and guide reviewers through your PR.
With a smooth review process, a pull request usually gets reviewed quicker.

If you don't know how to write and run your tests, please read the guide:
https://doc.crates.io/contrib/tests

### Additional information

I considered reworking the code to show the user the errors they would encounter once the feature is stable but held off.  I wasn't quite sure what language to use and most likely a user would have something doing error reporting, like CI, so it should be fine.
@weihanglo
Copy link
Member

This is now stabilized on nightly from nightly-2023-09-10 and later!

@extrawurst
Copy link

@weihanglo what is the expected timeline to stabilize it?

@jhpratt
Copy link
Member

jhpratt commented Oct 18, 2023

what is the expected timeline to stabilize it?

It is stable on nightly. It will reach the stable compiler in 1.75.

@mcandre
Copy link

mcandre commented Oct 22, 2023

Dumb question, is the lints section in a Rust crate inherited by downstream projects?

I love linting, but I am wary of accidentally forcing a particular lint configuration onto my users.

@weihanglo
Copy link
Member

If my memory hasn't rusty, then yes. Dependencies will compile with their lint configurations. However, users are not expected to see any compilation failure since Cargo uses --cap-lints to stop dependency warnings from propogating to users. See relevant code below:

fn add_cap_lints(bcx: &BuildContext<'_, '_>, unit: &Unit, cmd: &mut ProcessBuilder) {
// If this is an upstream dep we don't want warnings from, turn off all
// lints.
if !unit.show_warnings(bcx.config) {
cmd.arg("--cap-lints").arg("allow");
// If this is an upstream dep but we *do* want warnings, make sure that they
// don't fail compilation.
} else if !unit.is_local() {
cmd.arg("--cap-lints").arg("warn");
}
}

dignifiedquire added a commit to n0-computer/iroh that referenced this issue Oct 26, 2023
Depends on a newly stabilized feature which will be in 1.75 and available on nightly today

rust-lang/cargo#12115 (comment)
dignifiedquire added a commit to n0-computer/iroh that referenced this issue Oct 26, 2023
Depends on a newly stabilized feature which will be in 1.75 and available on nightly today

rust-lang/cargo#12115 (comment)
@jwodder
Copy link

jwodder commented Nov 1, 2023

(I'm not sure if this issue is the best place to ask this; if there's a better location, please let me know.)

Does the syntax for the [lints] tables only allow per-lint settings, or can it also be used to, e.g., deny all warn-by-default lints — perhaps something like lints.clippy.warn = "deny"?

@epage
Copy link
Contributor

epage commented Nov 1, 2023

warn is a lint group and lint groups are supported. You need to make sure the priority is set correctly so it is correctly applied.

However, I would recommend against it. In general, #![deny(warnings)] (and by extension doing it in the lints table) is generally frowned upon because

  • whether you see warnings is dependent on the rust version used so someone using a different Rust version than you might not be able to build your project
  • it means your code always has to be "perfect" even when you are experimenting on something

I would instead recommend looking at #8424.

@flip1995
Copy link
Member

flip1995 commented Nov 2, 2023

warn is a lint group and lint groups are supported.

Small correction: warn is not a lint group in Clippy. But all is, which contains all warn-by-default lints. What epage wrote still applies though: #![deny(warnings)] and by extension #![deny(clippy::all)] is an anti-pattern. So to deny warn-by-default Clippy lints, you should use the same alternatives as listed in the linked documentation.

@jwodder
Copy link

jwodder commented Nov 16, 2023

  • whether you see warnings is dependent on the rust version used so someone using a different Rust version than you might not be able to build your project

Just to be clear on this point: Does the [lints] table have an effect when another user builds a crate that uses mine or installs my binary crate? Or does it only take effect when building my crate directly (e.g., in a clone of the source repository)? Does this differ between the rustc lints and the clippy lints?

@MarijnS95
Copy link

  • cargo new will automatically inherit the lints table

Is there (going to be) a way to force workspace lints on all member crates? Having to add [lints] workspace = true to every Cargo.toml in my workspace is not only verbose, it also turns this into an opt-in (that we'd have to watch out for very carefully in PRs that add new crates) rather than being a set of rules that are unconditionally enforced on all crates in our repo / workspace.

@epage
Copy link
Contributor

epage commented Nov 16, 2023

#12976 was just merged which added the following to the documentation

Generally, these will only affect local development of the current package.
Cargo only applies these to the current package and not to dependencies.
As for dependents, Cargo suppresses lints from non-path dependencies with features like
--cap-lints.

@epage
Copy link
Contributor

epage commented Nov 16, 2023

  • cargo new will automatically inherit the lints table

Is there (going to be) a way to force workspace lints on all member crates? Having to add [lints] workspace = true to every Cargo.toml in my workspace is not only verbose, it also turns this into an opt-in (that we'd have to watch out for very carefully in PRs that add new crates) rather than being a set of rules that are unconditionally enforced on all crates in our repo / workspace.

#12208 is our general issue for considering implicit inheritance. If we should consider piece-meal implicit inheritance, I think that should also be discussed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints-table Area: [lints] table C-tracking-issue Category: A tracking issue for something unstable. disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. T-cargo Team: Cargo to-announce
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.