-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
This comment has been minimized.
This comment has been minimized.
e29f24d
to
7d8ee7b
Compare
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1,96 @@ | |||
# Cargo specifics - Checking conditional configurations |
There was a problem hiding this comment.
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 inbuild.rs
(link to the directive's docs) and lint config (show a short example)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
7d8ee7b
to
420833b
Compare
This comment has been minimized.
This comment has been minimized.
63e8473
to
20ccf72
Compare
This comment has been minimized.
This comment has been minimized.
20ccf72
to
d1a4651
Compare
There was a problem hiding this 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.).
d1a4651
to
bc8e034
Compare
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 |
@bors r+ rollup |
…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`
…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``
…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
@bors rollup=iffy |
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 |
@bors p=1 check-cfg is trigger-happy and we should provide up-to-date information |
☀️ Test successful - checks-actions |
Finished benchmarking commit (b92758a): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 672.136s -> 670.311s (-0.27%) |
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) |
Seems like we're likely ok with such regressions when they're in the "unhappy path". @rustbot label: +perf-regression-triaged |
This PR updates the diagnostics output of the
unexpected_cfgs
lint for Cargo newcheck-cfg
config.It's a simple and cost-less alternative to the build-script
cargo::rustc-check-cfg
instruction.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