feat: `lints` feature #12148

epage · 2023-05-17T20:44:50Z

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?

Look at this commit by commit to see it gradually build up
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.

rustbot · 2023-05-17T20:44:56Z

r? @weihanglo

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

weihanglo

Thank you for getting this done! Just finished the first round of my review. Haven't started for the test part.

src/doc/src/reference/unstable.md

weihanglo · 2023-05-19T16:18:20Z

src/doc/src/reference/unstable.md

+Example:
+
+```toml
+# [PROJECT_DIR]/Cargo.toml


Might be better?

I know it was from reference/workspaces.md 😆

Suggested change

# [PROJECT_DIR]/Cargo.toml

# [WORKSPACE_DIR]/Cargo.toml

Eh, keeping it consistent for now.

src/cargo/core/compiler/mod.rs

src/doc/src/reference/unstable.md

weihanglo · 2023-05-19T17:34:37Z

src/cargo/util/toml/mod.rs

+    lints.try_into().map(Some).map_err(|err| err.into())
+}
+
+fn warn_for_feature(name: &str, config: &Config) {


Should this be a method of Features struct, like Features::require?

Will the user get a warning from each dependency having lints table?

Should this be a method of Features struct, like Features::require?

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.

Figured it'd be better to understand this usage of the warning before stabilizing

Will the user get a warning from each dependency having lints table?

Yes, and I've switched it to use the existing warning system for manifests. This will hide it for non-workspace members and show the path for workspace members.

Note that we strip the [lints] table on stable so it won't get published

weihanglo

Finished a second pass. This pull request should cover almost all things if I've done the review correctly. Just some minor issues left.

src/cargo/core/compiler/mod.rs

src/cargo/core/compiler/fingerprint/mod.rs

src/cargo/util/toml/mod.rs

weihanglo · 2023-05-22T10:32:23Z

tests/testsuite/lints.rs

@@ -0,0 +1,603 @@
+//! Tests for `[lints]`


I wonder if we should use snapbox for new tests.

There are a couple of missing features (unordered lists) which is why I haven't pushed it since we can't drive towards a new consistency. That doesn't mean I'm against it just unsure how others would take it.

tests/testsuite/lints.rs

weihanglo · 2023-05-22T10:45:56Z

tests/testsuite/lints.rs

+        .build();
+
+    foo.cargo("check")
+        .arg("-v")


Suggested change

.arg("-v")

Not needed and elsewhere ditto.

I've added a comment to clarify why this is here: on failure, it will show the order of the flags which would help with debugging.

tests/testsuite/lints.rs

weihanglo · 2023-05-22T10:52:11Z

tests/testsuite/lints.rs

+}
+
+#[cargo_test]
+fn rustdoc_lint() {


Maybe we can use #[cargo_test(requires_cargo-clippy)] and write a simple test for clippy lint as well?

We can't atm. clippy fails with an error about cargo-features = ["lints"] (lints not existing).

I am looking at a follow up PR that switches us from cargo-features to -Z which will allow us to test with clippy (besides allowing people to experiment!)

tests/testsuite/lints.rs

The implementation doesn't match this yet but this reflects what we'll be working towards.

This will make it easier for users to test this feature

This improves the messaging experience (sometimes showing manifest path) and hiding the warning when not relevant).

…lags This will avoid people reusing it in the future and expanding its use without updating fingerprinting/metadata.

weihanglo · 2023-05-22T16:13:04Z

src/cargo/core/compiler/fingerprint/mod.rs

@@ -75,6 +75,7 @@
 //! [`Lto`] flags                              | ✓           | ✓
 //! config settings[^5]                        | ✓           |
 //! is_std                                     |             | ✓
+//! `[lints]` table                            | ✓           | ✓


No. It is not included in metadata. Metadata is computed here:

cargo/src/cargo/core/compiler/context/compilation_files.rs

Line 569 in 2015080

fn compute_metadata(

The larger question is: Should it be included? RUSTFLAGS is not in metadata whereas cargo rustc extra flags is. I think lints table shouldn't, since it more like an extra layer for checking your code but not for final artifacts. However, at this moment we generalize [lints] table to manifest.rustflags(), which may be reasonable to be in metadata in the future.

My take is: keep it in fingerprint and give it a footnote stating that it is computed as rustflags from mainfest as a whole. What do people think?

weihanglo

Thanks again for the implementation.

Highlight some implementation choices that are unspecified in the RFC:

profile.rustflags takes precedence over [lints] table
rustflags derived from [lints] table are not included in rustc -C metadata hash.

Going to merge it soon.

weihanglo · 2023-05-22T19:54:26Z

@bors r+

bors · 2023-05-22T19:54:28Z

📌 Commit b435eda has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-05-22T19:54:42Z

⌛ Testing commit b435eda with merge e5e68c4...

bors · 2023-05-22T20:57:57Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing e5e68c4 to master...

bors · 2023-05-22T20:57:57Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing e5e68c4 to master...

bors · 2023-05-22T20:58:00Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Update cargo 10 commits in 09276c703a473ab33daaeb94917232e80eefd628..64fb38c97ac4d3a327fc9032c862dd28c8833b17 2023-05-16 21:43:35 +0000 to 2023-05-23 18:53:23 +0000 - Consider rust-version when selecting packages for cargo add (rust-lang/cargo#12078) - fix(lints): Switch to -Zlints so stable projects can experiment (rust-lang/cargo#12168) - Automatically inherit workspace fields when running cargo new/init (rust-lang/cargo#12069) - ci: check if any version bump needed for member crates (rust-lang/cargo#12126) - feat: `lints` feature (rust-lang/cargo#12148) - fix: pass `-C debuginfo` after weakening if explicitly set (rust-lang/cargo#12165) - Tweak build help to clarify role of --bin (rust-lang/cargo#12157) - fix: Pass CI on nightly (rust-lang/cargo#12160) - docs(source): doc comments for Source and its impls (rust-lang/cargo#12159) - docs(source): doc comments for `Source` and friends (rust-lang/cargo#12153) r? `@ghost`

We already discussed non-blocking gates but the language makes it sound like it was limited to `config.toml`. Since I haven't been touching that, I had always overlooked that section. This change brings the blocking / non-blocking decision front and center. To support this, the later sections focus more on mechanisms (the gate) rather than on what is being done (new syntax for `cargo-features`). I also feel this makes the content more scannable. This is adapted from what I did for `[lints]` (see rust-lang#12148).

doc(features): Highlight the non-blocking feature gating technique We already discussed non-blocking gates but the language makes it sound like it was limited to `config.toml`. Since I haven't been touching that, I had always overlooked that section. This change brings the blocking / non-blocking decision front and center. To support this, the later sections focus more on mechanisms (the gate) rather than on what is being done (new syntax for `cargo-features`). I also feel this makes the content more scannable. This is adapted from what I did for `[lints]` (see #12148).

We already discussed non-blocking gates but the language makes it sound like it was limited to `config.toml`. Since I haven't been touching that, I had always overlooked that section. This change brings the blocking / non-blocking decision front and center. To support this, the later sections focus more on mechanisms (the gate) rather than on what is being done (new syntax for `cargo-features`). I also feel this makes the content more scannable. This is adapted from what I did for `[lints]` (see rust-lang#12148).

rustbot assigned weihanglo May 17, 2023

danielhjacobs mentioned this pull request May 18, 2023

core: Implement handling of text control input ruffle-rs/ruffle#11059

Merged

epage mentioned this pull request May 18, 2023

Tracking Issue for [lints] table RFC 3389 #12115

Closed

weihanglo reviewed May 19, 2023

View reviewed changes

epage force-pushed the lints branch 4 times, most recently from 9759d86 to 02f96ea Compare May 19, 2023 21:24

weihanglo reviewed May 22, 2023

View reviewed changes

epage force-pushed the lints branch 3 times, most recently from c52d691 to 9ffa303 Compare May 22, 2023 14:32

epage added 11 commits May 22, 2023 09:51

feat(lints): lints feature skeleton

4d09a6a

docs(ref): Copy over guide-level explanation

228f71f

The implementation doesn't match this yet but this reflects what we'll be working towards.

feat(lints): Parse [lints] table on nightly

8de2529

feat(lints): Error check the lints table

fad2ea5

feat(lints): Expose TomlLints as Manifest::rustflags

25c084a

feat(lints): Pass lints to tools

4279d0d

fix(lints): Only warn when seeing lints on stable

a7555f9

This will make it easier for users to test this feature

test(lints): Show bug with lints on stable

66038fa

fix(lints): Ignore lints table on stable

5072d9b

fix(lints): Clean up warnings about lints feature

a8d7c8a

fix(lints): Tie into manifest warnings

1dfa101

This improves the messaging experience (sometimes showing manifest path) and hiding the warning when not relevant).

epage added 2 commits May 22, 2023 10:06

test(lints): Verify precedence with build.rustflags

be76a55

test(lints): Simplify failure tests

e4b0136

epage force-pushed the lints branch 2 times, most recently from 8d4f929 to ccf5c64 Compare May 22, 2023 15:18

refactor(lints): Restrict new Manifest::rustflags to being lint_rustf…

b435eda

…lags This will avoid people reusing it in the future and expanding its use without updating fingerprinting/metadata.

weihanglo reviewed May 22, 2023

View reviewed changes

epage force-pushed the lints branch from c939cc1 to b435eda Compare May 22, 2023 18:54

weihanglo approved these changes May 22, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023

bors merged commit e5e68c4 into rust-lang:master May 22, 2023

epage mentioned this pull request May 22, 2023

Automatically inherit workspace fields when running cargo new/init #12069

Merged

1 task

epage deleted the lints branch May 22, 2023 21:11

weihanglo mentioned this pull request May 23, 2023

Update cargo rust-lang/rust#111890

Merged

ehuss added this to the 1.71.0 milestone May 30, 2023

weihanglo added the A-lints-table Area: [lints] table label Jun 13, 2023

torokati44 mentioned this pull request Jan 4, 2024

chore: Move lint configuration from .cargo/config.toml to Cargo.toml ruffle-rs/ruffle#14614

Merged

epage mentioned this pull request Jan 16, 2024

doc(features): Highlight the non-blocking feature gating technique #13307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `lints` feature #12148

feat: `lints` feature #12148

epage commented May 17, 2023 •

edited

Loading

rustbot commented May 17, 2023

weihanglo left a comment

weihanglo May 19, 2023

epage May 19, 2023

weihanglo May 19, 2023

epage May 19, 2023

weihanglo left a comment

weihanglo May 22, 2023

epage May 22, 2023

weihanglo May 22, 2023

epage May 22, 2023

weihanglo May 22, 2023

epage May 22, 2023

weihanglo May 22, 2023

weihanglo left a comment

weihanglo commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

feat: lints feature #12148

feat: lints feature #12148

Conversation

epage commented May 17, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented May 17, 2023

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

feat: `lints` feature #12148

feat: `lints` feature #12148

epage commented May 17, 2023 •

edited

Loading