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

Implement --check-cfg option (RFC 3013) #89346

Closed
wants to merge 3 commits into from

Conversation

mwkmwkmwk
Copy link
Contributor

This is my attempt at implementing rust-lang/rfcs#3013. I'm not quite sure I'm doing this right, so I'd appreciate some feedback.

One major problem I hit in implementation is finding a good node_id to attach the early lint to: neither the config condition itself nor (for #[cfg]) the item that it guards is suitable, because they won't survive the expansion. For now I've attached the lint to crate root, but this is clearly suboptimal — I think the best we can do is using the parent item's node_id, but this seems to require a lot of plumbing to get the node_id down to the place where the lint is emitted. Is there some better way to handle this?

Also, I haven't implemented the -Z check-cfg option specified in the RFC — it seems -Z unstable-options already works as a feature gate well enough, so there's not much point?

Other TODOs:

  • add plumbing to pass --check-cfg from rustdoc (do we want this one?)
  • document --check-cfg (or is this something to be done on stabilization?)
  • not in the RFC, but sounds like a good plan: also add a builtin list of well-known values (for stuff like target_os, ...) — perhaps we can even enable the lint by default for those?

cc #82450

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Sep 29, 2021

add plumbing to pass --check-cfg from rustdoc (do we want this one?)

It would be useful, I think, it catches issues like cfg(doctst) or something (and in general I would like expansion to match rustc as closely as possible).

The code lives around

// these are definitely not part of rustdoc, but we want to warn on them anyway.
; you may also have to add an unstable option to `config.rs, I don't remember if rustdoc inherits rustc's options automatically or not.

@Aaron1011
Copy link
Member

One major problem I hit in implementation is finding a good node_id to attach the early lint to: neither the config condition itself nor (for #[cfg]) the item that it guards is suitable, because they won't survive the expansion. For now I've attached the lint to crate root, but this is clearly suboptimal — I think the best we can do is using the parent item's node_id, but this seems to require a lot of plumbing to get the node_id down to the place where the lint is emitted. Is there some better way to handle this?

You can use ExtCtxt.current_expansion.lint_node_id, which is specifically designed for this kind of early linting. To make it available where you need it, you could add a lint_node_id field to StripUnconfigured, and use it in the corresponding call to cfg_matches. For the other calls to cfg_matches, e.g:

Some(ref cfg) => rustc_attr::cfg_matches(cfg, &sess.parse_sess, None),

you could either avoid linting, or continue to use CRATE_NODE_ID. Feel free to message me on Zulip if you have any follow-up questions.

@petrochenkov
Copy link
Contributor

it seems -Z unstable-options already works as a feature gate well enough, so there's not much point?

+1

@mwkmwkmwk
Copy link
Contributor Author

add plumbing to pass --check-cfg from rustdoc (do we want this one?)

It would be useful, I think, it catches issues like cfg(doctst) or something (and in general I would like expansion to match rustc as closely as possible).

This option doesn't affect expansion at all, though — the only effect is that lints are emitted. I also don't see why we need specifically rustdoc to catch cfg(doctst), as this should already be caught by the main rustc run? After all, doctest is already included in the main "well-known conditions" list.

The code lives around

// these are definitely not part of rustdoc, but we want to warn on them anyway.

; you may also have to add an unstable option to `config.rs, I don't remember if rustdoc inherits rustc's options automatically or not.

Wait, this seems like rustdoc specifically disables all lints except for this short list? Are the "unknown condition" lints somehow special enough to warrant adding here?

compiler/rustc_session/src/config.rs Show resolved Hide resolved
compiler/rustc_session/src/config.rs Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved
compiler/rustc_attr/src/builtin.rs Show resolved Hide resolved
compiler/rustc_session/src/parse.rs Show resolved Hide resolved
compiler/rustc_attr/src/builtin.rs Show resolved Hide resolved
compiler/rustc_interface/src/interface.rs Show resolved Hide resolved
compiler/rustc_interface/src/interface.rs Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Could you add one more non-error test as well - making sure that cfg(unknown_key = "value") produces a warning when names checking is enabled.

@petrochenkov petrochenkov 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 Oct 4, 2021
@sivadeilra
Copy link

I actually implemented this, last year, when I wrote the RFC. Let me dust off this old branch and see if there's anything worth rescuing from it...

@sivadeilra
Copy link

Btw, you're welcome to grab anything useful from my old branch: https://github.com/sivadeilra/rust/tree/user/ardavis/check_cfg If you do, just credit me.

I had it mostly working, but had to set it down for some other work. By the time I returned to this work, master had moved on so much that I couldn't just trivially rebase.

@bors
Copy link
Contributor

bors commented Oct 7, 2021

☔ The latest upstream changes (presumably #89629) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2021
@JohnCSimon
Copy link
Member

ping from triage:
@mwkmwkmwk
Please address the merge conflict and adjust the label with @rustbot ready when you're ready for another review. Thank you.

@JohnCSimon
Copy link
Member

Ping from triage:
@mwkmwkmwk
I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thank you.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jan 30, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 30, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2022
Implement --check-cfg option (RFC 3013), take 2

This pull-request implement RFC 3013: Checking conditional compilation at compile time (rust-lang/rfcs#3013) and is based on the previous attempt rust-lang#89346 by `@mwkmwkmwk` that was closed due to inactivity.

I have address all the review comments from the previous attempt and added some more tests.

cc rust-lang#82450
r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 25, 2022
…eGomez

Wire up unstable rustc --check-cfg to rustdoc

This pull-request wire up the new unstable `--check-cfg` option from `rustc` to `rustdoc` as [requested](rust-lang#93915 (comment)) in the [pull-request](rust-lang#93915) that introduce `--check-cfg`.

The motivation was describe in the original PR by `@jyn514` who wrote rust-lang#89346 (comment):
> > add plumbing to pass --check-cfg from rustdoc (do we want this one?)
>
> It would be useful, I think, it catches issues like cfg(doctst) or something (and in general I would like expansion to match rustc as closely as possible).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 25, 2022
…eGomez

Wire up unstable rustc --check-cfg to rustdoc

This pull-request wire up the new unstable `--check-cfg` option from `rustc` to `rustdoc` as [requested](rust-lang#93915 (comment)) in the [pull-request](rust-lang#93915) that introduce `--check-cfg`.

The motivation was describe in the original PR by ``@jyn514`` who wrote rust-lang#89346 (comment):
> > add plumbing to pass --check-cfg from rustdoc (do we want this one?)
>
> It would be useful, I think, it catches issues like cfg(doctst) or something (and in general I would like expansion to match rustc as closely as possible).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.