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

Announce automatic checking of cfgs at compile-time #1313

Merged
merged 13 commits into from
May 6, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Apr 17, 2024

This blog post announce automatic checking of cfgs at compile-time (rust-lang/rfcs#3013) by the Cargo (and Compiler) team.

To be merged when rust-lang/cargo#13571 (status: merged in Cargo, merged in rustc, is in nightly 🎉) reaches nightly.

Currently scheduled for 2024-05-06.

posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-04-29-check-cfg.md Outdated Show resolved Hide resolved
@weihanglo
Copy link
Member

rust-lang/rust#124684 was just merged, so the next nightly (nightly-2024-05-05 presumably) will include the change.

@Urgau Urgau force-pushed the check-cfg-stabilization branch from 03e1989 to 2682b15 Compare May 4, 2024 14:05
posts/2024-05-05-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-05-05-check-cfg.md Outdated Show resolved Hide resolved
Copy link
Member

@weihanglo weihanglo 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 this!

@Urgau
Copy link
Member Author

Urgau commented May 5, 2024

I've tested the latest nightly and it produces the right warnings. I also updated the diagnostics output to the latest update. This is now ready to be merged.

@Urgau Urgau marked this pull request as ready for review May 5, 2024 08:13
@Urgau Urgau force-pushed the check-cfg-stabilization branch from 7132e74 to 1337ead Compare May 5, 2024 22:26
posts/2024-05-06-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-05-06-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-05-06-check-cfg.md Outdated Show resolved Hide resolved
posts/2024-05-06-check-cfg.md Outdated Show resolved Hide resolved
Co-authored-by: Ed Page <eopage@gmail.com>
@apoelstra
Copy link

This post is emphatic that you can't disable this without a build.rs, but you can, by adding #![allow(unexpected_cfgs] to your lib.rs or main.rs.

I think this is worth saying and would greatly reduce the amount of blowback from this change.

@Urgau
Copy link
Member Author

Urgau commented May 6, 2024

Let's not derail this PR but it is written under "Frequently asked questions":

Can it be disabled?

For Cargo users, the feature is always on and cannot be disabled, but like any other lints it can be controlled: #![warn(unexpected_cfgs)].

@apoelstra
Copy link

@Urgau I read the existing text, and I believe it is misleading. It says "cannot be disabled" with "cannot" emphasized, and shows how to make it a warning. A reader needs to infer that "cannot" is actually wrong and that you can change warn to allow to disable the lint.

Furthermore, later in the post there is a FAQ entry about being able to disable the lint without build.rs, and again the text explicitly says that you can't do it.

BTW, if it is "derailing the PR" to comment on the text of the blog post that this PR adds, then where are comments supposed to be posted?

@Urgau
Copy link
Member Author

Urgau commented May 6, 2024

I see allowing the lint as disabling the effect of the feature but the feature is still there, Cargo is still passing the --check-cfg args to rustc, you can still use cargo::rustc-check-cfg in build.rs, if you do cargo check -v they will still show up.

As for writing warn and not allow in the blog post, there is two reasons, the first is to be consistent with the note emitted by the compiler (as shown in the image) and the second is to avoid users just copy/pasting the example without looking at their warnings first. I understand this is not ideal, but I really want to avoid users allowing the lint instead of fixing the warnings.


BTW, if it is "derailing the PR" to comment on the text of the blog post that this PR adds, then where are comments supposed to be posted?

Wrong wording on my part. I wanted to say not derailing the main thread of the PR, since they can become quite polluted over time, opening a thread would have been better, but it's fine.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit 1a548de into rust-lang:master May 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants