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

Enable conditional compilation checking on the Rust codebase #94298

Merged
merged 3 commits into from
Mar 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions compiler/rustc_data_structures/src/sorted_map/index_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,3 @@ impl<I: Idx, K, V> std::ops::Index<I> for SortedIndexMultiMap<I, K, V> {
&self.items[idx].1
}
}

#[cfg(tests)]
mod tests;
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions library/core/src/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,7 @@ impl<T> MaybeUninit<T> {
/// ### Correct usage of this method:
///
/// ```rust
/// # #![allow(unexpected_cfgs)]
/// use std::mem::MaybeUninit;
///
/// # unsafe extern "C" fn initialize_buffer(buf: *mut [u8; 1024]) { *buf = [0; 1024] }
Expand Down
3 changes: 3 additions & 0 deletions library/std/src/os/windows/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ pub trait OpenOptionsExt {
/// # Examples
///
/// ```no_run
/// # #![allow(unexpected_cfgs)]
/// # #[cfg(for_demonstration_only)]
/// extern crate winapi;
/// # mod winapi { pub const FILE_FLAG_DELETE_ON_CLOSE: u32 = 0x04000000; }
Expand Down Expand Up @@ -195,6 +196,7 @@ pub trait OpenOptionsExt {
/// # Examples
///
/// ```no_run
/// # #![allow(unexpected_cfgs)]
/// # #[cfg(for_demonstration_only)]
/// extern crate winapi;
/// # mod winapi { pub const FILE_ATTRIBUTE_HIDDEN: u32 = 2; }
Expand Down Expand Up @@ -236,6 +238,7 @@ pub trait OpenOptionsExt {
/// # Examples
///
/// ```no_run
/// # #![allow(unexpected_cfgs)]
/// # #[cfg(for_demonstration_only)]
/// extern crate winapi;
/// # mod winapi { pub const SECURITY_IDENTIFICATION: u32 = 0; }
Expand Down
28 changes: 28 additions & 0 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use crate::run;
use crate::test;
use crate::tool::{self, SourceType};
use crate::util::{self, add_dylib_path, add_link_lib_path, exe, libdir};
use crate::EXTRA_CHECK_CFGS;
use crate::{Build, CLang, DocTests, GitRepo, Mode};

pub use crate::Compiler;
Expand Down Expand Up @@ -1095,6 +1096,33 @@ impl<'a> Builder<'a> {
rustflags.arg("-Zunstable-options");
}

// #[cfg(not(bootstrap)]
if stage != 0 {
// Enable cfg checking of cargo features
// FIXME: De-comment this when cargo beta get support for it
// cargo.arg("-Zcheck-cfg-features");

// Enable cfg checking of rustc well-known names
rustflags.arg("-Zunstable-options").arg("--check-cfg=names()");

// Add extra cfg not defined in rustc
for (restricted_mode, name, values) in EXTRA_CHECK_CFGS {
if *restricted_mode == None || *restricted_mode == Some(mode) {
// Creating a string of the values by concatenating each value:
// ',"tvos","watchos"' or '' (nothing) when there are no values
let values = match values {
Some(values) => values
.iter()
.map(|val| [",", "\"", val, "\""])
.flatten()
.collect::<String>(),
None => String::new(),
};
rustflags.arg(&format!("--check-cfg=values({name}{values})"));
}
}
}

// FIXME: It might be better to use the same value for both `RUSTFLAGS` and `RUSTDOCFLAGS`,
// but this breaks CI. At the very least, stage0 `rustdoc` needs `--cfg bootstrap`. See
// #71458.
Expand Down
28 changes: 28 additions & 0 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,34 @@ const LLVM_TOOLS: &[&str] = &[

pub const VERSION: usize = 2;

/// Extra --check-cfg to add when building
/// (Mode restriction, config name, config values (if any))
const EXTRA_CHECK_CFGS: &[(Option<Mode>, &'static str, Option<&[&'static str]>)] = &[
(None, "bootstrap", None),
(Some(Mode::Rustc), "parallel_compiler", None),
(Some(Mode::ToolRustc), "parallel_compiler", None),
(Some(Mode::Std), "miri", None),
(Some(Mode::Std), "stdarch_intel_sde", None),
(Some(Mode::Std), "no_fp_fmt_parse", None),
(Some(Mode::Std), "no_global_oom_handling", None),
(Some(Mode::Std), "freebsd12", None),
(Some(Mode::Std), "backtrace_in_libstd", None),
// FIXME: Used by rustfmt is their test but is invalid (neither cargo nor bootstrap ever set
// this config) should probably by removed or use a allow attribute.
(Some(Mode::ToolRustc), "release", None),
// FIXME: Used by stdarch in their test, should use a allow attribute instead.
(Some(Mode::Std), "dont_compile_me", None),
// FIXME: Used by serde_json, but we should not be triggering on external dependencies.
(Some(Mode::Rustc), "no_btreemap_remove_entry", None),
(Some(Mode::ToolRustc), "no_btreemap_remove_entry", None),
// FIXME: Used by crossbeam-utils, but we should not be triggering on external dependencies.
(Some(Mode::Rustc), "crossbeam_loom", None),
(Some(Mode::ToolRustc), "crossbeam_loom", None),
// FIXME: Used by proc-macro2, but we should not be triggering on external dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to document this in the tracking issue for this feature, and consider if there's some kind of crate-level opt-in that can be added -- I suspect in many cases folks will want to have this sort of crate-private cfg without end users needing to opt-in to it.

It's also the case that the proc-macro2 library for example I think intends for this to be used by users, but doesn't expose it as a Cargo feature to avoid accidental use (e.g., by a library that enables that feature), forcing the 'last' user to actually pass the relevant cfg.

I think this is fine for this PR but we should consider this as part of the feature development process, especially if and when stabilization is considered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely, it should be mentioned. I don't have the permissions to edit it but fell free to do so.

I would also mention that were are getting those because we are using RUSTFLAGS which applies to all crates instead of just the one we control. We are currently forced to do it this way because the cargo integration isn't done or even design. It wasn't really discus in the RFC as it was put in the unresolved section with all the cargo stuff. Nevertheless I have some ideas about how we could have a better integration with cargo.

(Some(Mode::Rustc), "span_locations", None),
(Some(Mode::ToolRustc), "span_locations", None),
];

/// A structure representing a Rust compiler.
///
/// Each compiler has a `stage` that it is associated with and a `host` that
Expand Down