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

Update unexpected_cfgs lint for Cargo new check-cfg config #125219

Merged
merged 6 commits into from
May 20, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented May 17, 2024

This PR updates the diagnostics output of the unexpected_cfgs lint for Cargo new check-cfg config.

It's a simple and cost-less alternative to the build-script cargo::rustc-check-cfg instruction.

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(foo, values("bar"))'] }

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the check-cfg lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

Not to be merged before rust-lang/cargo#13913 reaches master! (EDIT: merged in #125237)

@rustbot label +F-check-cfg
r? @fmease (feel free to roll)
Fixes #124800
cc @epage @weihanglo

@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. F-check-cfg --check-cfg labels May 17, 2024
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the check-cfg-cargo-config branch from e29f24d to 7d8ee7b Compare May 17, 2024 18:41
@rust-log-analyzer

This comment has been minimized.

@@ -0,0 +1,96 @@
# Cargo specifics - Checking conditional configurations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I suggested this go into the check-cfg docs, it was with the assumption that it was to be an additional note that built on existing knowledge. I also didn't realize there was a separate section for this and thought it would be in the lint docs.

Personally, what I recommend is

  • Update the lint docs to point to the check-cfg docs so users can discover how to customize things
  • Soon after It has this basic form: in the check-cfg docs, have an addendum that this can also be set in build.rs (link to the directive's docs) and lint config (show a short example)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the lint docs to point to the check-cfg docs so users can discover how to customize things

That is indeed something we should do. I will do that in this PR.

Soon after It has this basic form: in the check-cfg docs, have an addendum that this can also be set in build.rs (link to the directive's docs) and lint config (show a short example)

I would rather prefer if we not do that. That section is specifically for the --check-cfg flag and adding in the middle of it some cargo knowledge seems out place, in particular since Cargo is not the build system that uses --check-cfg.

In general I would really prefer to keep the current --check-cfg doc as is, that's why I propose to add a small "sub-page" dedicated to those Cargo specificity so that 1. we can easily link to it and 2. so that users of Cargo are not overwhelm with all the technicalities of the --check-cfg flag when they only care about Cargo (at first at least).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the lint documentation to refer to the check-cfg docs.

@Urgau Urgau force-pushed the check-cfg-cargo-config branch from 7d8ee7b to 420833b Compare May 17, 2024 22:28
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the check-cfg-cargo-config branch 2 times, most recently from 63e8473 to 20ccf72 Compare May 17, 2024 22:52
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Personally speaking – not on behalf of T-compiler-contributors —, I'm quite wary / skeptical about adding such a Cargo-specific chapter to the rustc book. While it's very nicely written, one could be fooled into believing it was a page from the Cargo book.

I see where T-cargo is coming from in regards to “it's an implementation detail” I agree with T-cargo that the format of check-cfg is an implementation detail from Cargo's POV but the way the docs are split up right now in this PR seems less than ideal to me.

I'm willing to approve this PR with its current overall structure (i.e., after the smaller nits I picked have been addressed) because we want to promote Cargo's new way to configure the cfgs as soon as possible I suppose.

However, I'd rather keep Cargo-specific explainers that are as long as this one out of the rustc book. It should be possible to rephrase the relevant chapters in the Cargo book (build scripts & lint tables) in such a way that it's obvious what is part of Cargo's API and what is rustc-specific without degrading the quality of docs (by linking to the rustc book where necessary, by disclaiming for example that any examples that are provided are not guaranteed to be valid according to rustc and merely serve as an example, etc.).

compiler/rustc_lint/src/context/diagnostics/check_cfg.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/context/diagnostics/check_cfg.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/context/diagnostics/check_cfg.rs Outdated Show resolved Hide resolved
src/doc/rustc/src/SUMMARY.md Outdated Show resolved Hide resolved
src/doc/rustc/src/check-cfg/cargo-specifics.md Outdated Show resolved Hide resolved
src/doc/rustc/src/check-cfg/cargo-specifics.md Outdated Show resolved Hide resolved
src/doc/rustc/src/check-cfg/cargo-specifics.md Outdated Show resolved Hide resolved
src/doc/rustc/src/check-cfg/cargo-specifics.md Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
@fmease fmease 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 19, 2024
@Urgau Urgau force-pushed the check-cfg-cargo-config branch from d1a4651 to bc8e034 Compare May 19, 2024 18:15
@Urgau
Copy link
Member Author

Urgau commented May 19, 2024

I fixed the nits and reduced the doc changes to the minimal, with only a small description, a link to relevant page and a small example for the each of the 3 ways.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2024
@fmease
Copy link
Member

fmease commented May 20, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 20, 2024

📌 Commit ccd3e99 has been approved by fmease

It is now in the queue for this repository.

@bors 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 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 20, 2024
…ease

Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

```toml
[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(foo, values("bar"))'] }
```

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang#125237)

`@rustbot` label +F-check-cfg
r? `@fmease` *(feel free to roll)*
Fixes rust-lang#124800
cc `@epage` `@weihanglo`
fmease added a commit to fmease/rust that referenced this pull request May 20, 2024
…ease

Update `unexpected_cfgs` lint for Cargo new `check-cfg` config

This PR updates the diagnostics output of the `unexpected_cfgs` lint for Cargo new `check-cfg` config.

It's a simple and cost-less alternative to the build-script `cargo::rustc-check-cfg` instruction.

```toml
[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(foo, values("bar"))'] }
```

This PR also adds a Cargo specific section regarding check-cfg and Cargo inside rustc's book (motivation is described inside the file, but mainly check-cfg is a rustc feature not a Cargo one, Cargo only enabled the feature, it does not own it; T-cargo even considers the `check-cfg` lint config to be an implementation detail).

This PR also updates the links to refer to that sub-page when using Cargo from rustc.

As well as updating the lint doc to refer to the check-cfg docs.

~**Not to be merged before rust-lang/cargo#13913 reaches master!**~ (EDIT: merged in rust-lang#125237)

``@rustbot`` label +F-check-cfg
r? ``@fmease`` *(feel free to roll)*
Fixes rust-lang#124800
cc ``@epage`` ``@weihanglo``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#125219 (Update `unexpected_cfgs` lint for Cargo new `check-cfg` config)
 - rust-lang#125255 (Make `EvalCtxt` generic over `InferCtxtLike`)
 - rust-lang#125283 (Use a single static for all default slice Arcs.)
 - rust-lang#125300 (rustdoc: Don't strip items with inherited visibility in `AliasedNonLocalStripper`)
 - rust-lang#125309 (Fix `tests/debuginfo/strings-and-strs`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors rollup=iffy

@epage
Copy link
Contributor

epage commented May 20, 2024

Personally speaking – not on behalf of T-compiler-contributors —, I'm quite wary / skeptical about adding such a Cargo-specific chapter to the rustc book. While it's very nicely written, one could be fooled into believing it was a page from the Cargo book.

I agree that the framing of this feels out of place in rustc. When I suggested putting content here, the intent was to be very limited, to only say that the --check-cfg flag can also be set through lints and build.rs and leave it at that. See #125219 (comment)

@fmease
Copy link
Member

fmease commented May 20, 2024

@bors p=1 check-cfg is trigger-happy and we should provide up-to-date information

@bors
Copy link
Contributor

bors commented May 20, 2024

⌛ Testing commit ccd3e99 with merge b92758a...

@bors
Copy link
Contributor

bors commented May 20, 2024

☀️ Test successful - checks-actions
Approved by: fmease
Pushing b92758a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2024
@bors bors merged commit b92758a into rust-lang:master May 20, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 20, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b92758a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.2%, 2.5%] 21
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.2%, 2.5%] 21

Max RSS (memory usage)

Results (primary -1.8%, secondary 4.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)
4.5% [4.5%, 4.5%] 1
Improvements ✅
(primary)
-6.5% [-6.5%, -6.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.8% [-6.5%, 2.9%] 2

Cycles

Results (primary 1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [1.4%, 2.2%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.8% [1.4%, 2.2%] 4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.136s -> 670.311s (-0.27%)
Artifact size: 316.17 MiB -> 315.45 MiB (-0.23%)

@rustbot rustbot added the perf-regression Performance regression. label May 20, 2024
@Urgau
Copy link
Member Author

Urgau commented May 21, 2024

I think the perf regressions are the result of adding one more note in the diagnostic output, which is trigger happy on many compile benchmark from rustc-perf. cc #124684 (comment)

@rylev
Copy link
Member

rylev commented May 21, 2024

Seems like we're likely ok with such regressions when they're in the "unhappy path".

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-check-cfg --check-cfg merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

New rustc nightly suggests adding a build.rs to use conditional compilation
10 participants