From 8ca47d7ae4e068c94b4ab7b25cc0ccc38d01d52c Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 20 Feb 2022 23:25:18 -0800 Subject: [PATCH 01/18] Stop manually SIMDing in swap_nonoverlapping Like I previously did for `reverse`, this leaves it to LLVM to pick how to vectorize it, since it can know better the chunk size to use, compared to the "32 bytes always" approach we currently have. It does still need logic to type-erase where appropriate, though, as while LLVM is now smart enough to vectorize over slices of things like `[u8; 4]`, it fails to do so over slices of `[u8; 3]`. As a bonus, this also means one no longer gets the spurious `memcpy`(s?) at the end up swapping a slice of `__m256`s: --- library/core/benches/slice.rs | 43 ++++++++- library/core/src/mem/mod.rs | 45 ++++++++- library/core/src/ptr/mod.rs | 132 +++++++++------------------ src/test/codegen/swap-large-types.rs | 64 +++++++++++++ src/test/codegen/swap-simd-types.rs | 32 +++++++ src/test/codegen/swap-small-types.rs | 44 +++++++++ 6 files changed, 263 insertions(+), 97 deletions(-) create mode 100644 src/test/codegen/swap-large-types.rs create mode 100644 src/test/codegen/swap-simd-types.rs diff --git a/library/core/benches/slice.rs b/library/core/benches/slice.rs index 04efa52078778..9b86a0ca97c09 100644 --- a/library/core/benches/slice.rs +++ b/library/core/benches/slice.rs @@ -89,6 +89,15 @@ fn binary_search_l3_worst_case(b: &mut Bencher) { binary_search_worst_case(b, Cache::L3); } +#[derive(Clone)] +struct Rgb(u8, u8, u8); + +impl Rgb { + fn gen(i: usize) -> Self { + Rgb(i as u8, (i as u8).wrapping_add(7), (i as u8).wrapping_add(42)) + } +} + macro_rules! rotate { ($fn:ident, $n:expr, $mapper:expr) => { #[bench] @@ -104,17 +113,43 @@ macro_rules! rotate { }; } -#[derive(Clone)] -struct Rgb(u8, u8, u8); - rotate!(rotate_u8, 32, |i| i as u8); -rotate!(rotate_rgb, 32, |i| Rgb(i as u8, (i as u8).wrapping_add(7), (i as u8).wrapping_add(42))); +rotate!(rotate_rgb, 32, Rgb::gen); rotate!(rotate_usize, 32, |i| i); rotate!(rotate_16_usize_4, 16, |i| [i; 4]); rotate!(rotate_16_usize_5, 16, |i| [i; 5]); rotate!(rotate_64_usize_4, 64, |i| [i; 4]); rotate!(rotate_64_usize_5, 64, |i| [i; 5]); +macro_rules! swap_with_slice { + ($fn:ident, $n:expr, $mapper:expr) => { + #[bench] + fn $fn(b: &mut Bencher) { + let mut x = (0usize..$n).map(&$mapper).collect::>(); + let mut y = ($n..($n * 2)).map(&$mapper).collect::>(); + let mut skip = 0; + b.iter(|| { + for _ in 0..32 { + x[skip..].swap_with_slice(&mut y[..($n - skip)]); + skip = black_box(skip + 1) % 8; + } + black_box((x[$n / 3].clone(), y[$n * 2 / 3].clone())) + }) + } + }; +} + +swap_with_slice!(swap_with_slice_u8_30, 30, |i| i as u8); +swap_with_slice!(swap_with_slice_u8_3000, 3000, |i| i as u8); +swap_with_slice!(swap_with_slice_rgb_30, 30, Rgb::gen); +swap_with_slice!(swap_with_slice_rgb_3000, 3000, Rgb::gen); +swap_with_slice!(swap_with_slice_usize_30, 30, |i| i); +swap_with_slice!(swap_with_slice_usize_3000, 3000, |i| i); +swap_with_slice!(swap_with_slice_4x_usize_30, 30, |i| [i; 4]); +swap_with_slice!(swap_with_slice_4x_usize_3000, 3000, |i| [i; 4]); +swap_with_slice!(swap_with_slice_5x_usize_30, 30, |i| [i; 5]); +swap_with_slice!(swap_with_slice_5x_usize_3000, 3000, |i| [i; 5]); + #[bench] fn fill_byte_sized(b: &mut Bencher) { #[derive(Copy, Clone)] diff --git a/library/core/src/mem/mod.rs b/library/core/src/mem/mod.rs index 989ec0639cd6b..b5c1ae37e5e89 100644 --- a/library/core/src/mem/mod.rs +++ b/library/core/src/mem/mod.rs @@ -700,10 +700,49 @@ pub unsafe fn uninitialized() -> T { #[stable(feature = "rust1", since = "1.0.0")] #[rustc_const_unstable(feature = "const_swap", issue = "83163")] pub const fn swap(x: &mut T, y: &mut T) { - // SAFETY: the raw pointers have been created from safe mutable references satisfying all the - // constraints on `ptr::swap_nonoverlapping_one` + // NOTE(eddyb) SPIR-V's Logical addressing model doesn't allow for arbitrary + // reinterpretation of values as (chunkable) byte arrays, and the loop in the + // block optimization in `swap_slice` is hard to rewrite back + // into the (unoptimized) direct swapping implementation, so we disable it. + // FIXME(eddyb) the block optimization also prevents MIR optimizations from + // understanding `mem::replace`, `Option::take`, etc. - a better overall + // solution might be to make `ptr::swap_nonoverlapping` into an intrinsic, which + // a backend can choose to implement using the block optimization, or not. + #[cfg(not(target_arch = "spirv"))] + { + // For types that are larger multiples of their alignment, the simple way + // tends to copy the whole thing to stack rather than doing it one part + // at a time, so instead treat them as one-element slices and piggy-back + // the slice optimizations that will split up the swaps. + if size_of::() / align_of::() > 4 { + // SAFETY: exclusive references always point to one non-overlapping + // element and are non-null and properly aligned. + return unsafe { ptr::swap_nonoverlapping(x, y, 1) }; + } + } + + // If a scalar consists of just a small number of alignment units, let + // the codegen just swap those pieces directly, as it's likely just a + // few instructions and anything else is probably overcomplicated. + // + // Most importantly, this covers primitives and simd types that tend to + // have size=align where doing anything else can be a pessimization. + // (This will also be used for ZSTs, though any solution works for them.) + swap_simple(x, y); +} + +/// Same as [`swap`] semantically, but always uses the simple implementation. +/// +/// Used elsewhere in `mem` and `ptr` at the bottom layer of calls. +#[rustc_const_unstable(feature = "const_swap", issue = "83163")] +#[inline] +pub(crate) const fn swap_simple(x: &mut T, y: &mut T) { + // SAFETY: exclusive references are always valid to read/write, + // are non-overlapping, and nothing here panics so it's drop-safe. unsafe { - ptr::swap_nonoverlapping_one(x, y); + let z = ptr::read(x); + ptr::copy_nonoverlapping(y, x, 1); + ptr::write(y, z); } } diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index 8ab72e6aeeafa..ff71fadb61418 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -419,106 +419,58 @@ pub const unsafe fn swap(x: *mut T, y: *mut T) { #[stable(feature = "swap_nonoverlapping", since = "1.27.0")] #[rustc_const_unstable(feature = "const_swap", issue = "83163")] pub const unsafe fn swap_nonoverlapping(x: *mut T, y: *mut T, count: usize) { - let x = x as *mut u8; - let y = y as *mut u8; - let len = mem::size_of::() * count; - // SAFETY: the caller must guarantee that `x` and `y` are - // valid for writes and properly aligned. - unsafe { swap_nonoverlapping_bytes(x, y, len) } -} + macro_rules! attempt_swap_as_chunks { + ($ChunkTy:ty) => { + if mem::align_of::() >= mem::align_of::<$ChunkTy>() + && mem::size_of::() % mem::size_of::<$ChunkTy>() == 0 + { + let x: *mut MaybeUninit<$ChunkTy> = x.cast(); + let y: *mut MaybeUninit<$ChunkTy> = y.cast(); + let count = count * (mem::size_of::() / mem::size_of::<$ChunkTy>()); + // SAFETY: these are the same bytes that the caller promised were + // ok, just typed as `MaybeUninit`s instead of as `T`s. + // The `if` condition above ensures that we're not violating + // alignment requirements, and that the division is exact so + // that we don't lose any bytes off the end. + return unsafe { swap_nonoverlapping_simple(x, y, count) }; + } + }; + } -#[inline] -#[rustc_const_unstable(feature = "const_swap", issue = "83163")] -pub(crate) const unsafe fn swap_nonoverlapping_one(x: *mut T, y: *mut T) { - // NOTE(eddyb) SPIR-V's Logical addressing model doesn't allow for arbitrary - // reinterpretation of values as (chunkable) byte arrays, and the loop in the - // block optimization in `swap_nonoverlapping_bytes` is hard to rewrite back - // into the (unoptimized) direct swapping implementation, so we disable it. - // FIXME(eddyb) the block optimization also prevents MIR optimizations from - // understanding `mem::replace`, `Option::take`, etc. - a better overall - // solution might be to make `swap_nonoverlapping` into an intrinsic, which - // a backend can choose to implement using the block optimization, or not. - #[cfg(not(target_arch = "spirv"))] + // Split up the slice into small power-of-two-sized chunks that LLVM is able + // to vectorize (unless it's a special type with more-than-pointer alignment, + // because we don't want to pessimize things like slices of SIMD vectors.) + if mem::align_of::() <= mem::size_of::() + && (!mem::size_of::().is_power_of_two() + || mem::size_of::() > mem::size_of::() * 2) { - // Only apply the block optimization in `swap_nonoverlapping_bytes` for types - // at least as large as the block size, to avoid pessimizing codegen. - if mem::size_of::() >= 32 { - // SAFETY: the caller must uphold the safety contract for `swap_nonoverlapping`. - unsafe { swap_nonoverlapping(x, y, 1) }; - return; - } + attempt_swap_as_chunks!(usize); + attempt_swap_as_chunks!(u8); } - // Direct swapping, for the cases not going through the block optimization. - // SAFETY: the caller must guarantee that `x` and `y` are valid - // for writes, properly aligned, and non-overlapping. - unsafe { - let z = read(x); - copy_nonoverlapping(y, x, 1); - write(y, z); - } + // SAFETY: Same preconditions as this function + unsafe { swap_nonoverlapping_simple(x, y, count) } } +/// Same behaviour and safety conditions as [`swap_nonoverlapping`] +/// +/// LLVM can vectorize this (at least it can for the power-of-two-sized types +/// `swap_nonoverlapping` tries to use) so no need to manually SIMD it. #[inline] #[rustc_const_unstable(feature = "const_swap", issue = "83163")] -const unsafe fn swap_nonoverlapping_bytes(x: *mut u8, y: *mut u8, len: usize) { - // The approach here is to utilize simd to swap x & y efficiently. Testing reveals - // that swapping either 32 bytes or 64 bytes at a time is most efficient for Intel - // Haswell E processors. LLVM is more able to optimize if we give a struct a - // #[repr(simd)], even if we don't actually use this struct directly. - // - // FIXME repr(simd) broken on emscripten and redox - #[cfg_attr(not(any(target_os = "emscripten", target_os = "redox")), repr(simd))] - struct Block(u64, u64, u64, u64); - struct UnalignedBlock(u64, u64, u64, u64); - - let block_size = mem::size_of::(); - - // Loop through x & y, copying them `Block` at a time - // The optimizer should unroll the loop fully for most types - // N.B. We can't use a for loop as the `range` impl calls `mem::swap` recursively +const unsafe fn swap_nonoverlapping_simple(x: *mut T, y: *mut T, count: usize) { let mut i = 0; - while i + block_size <= len { - // Create some uninitialized memory as scratch space - // Declaring `t` here avoids aligning the stack when this loop is unused - let mut t = mem::MaybeUninit::::uninit(); - let t = t.as_mut_ptr() as *mut u8; - - // SAFETY: As `i < len`, and as the caller must guarantee that `x` and `y` are valid - // for `len` bytes, `x + i` and `y + i` must be valid addresses, which fulfills the - // safety contract for `add`. - // - // Also, the caller must guarantee that `x` and `y` are valid for writes, properly aligned, - // and non-overlapping, which fulfills the safety contract for `copy_nonoverlapping`. - unsafe { - let x = x.add(i); - let y = y.add(i); + while i < count { + let x: &mut T = + // SAFETY: By precondition, `i` is in-bounds because it's below `n` + unsafe { &mut *x.add(i) }; + let y: &mut T = + // SAFETY: By precondition, `i` is in-bounds because it's below `n` + // and it's distinct from `x` since the ranges are non-overlapping + unsafe { &mut *y.add(i) }; + mem::swap_simple(x, y); - // Swap a block of bytes of x & y, using t as a temporary buffer - // This should be optimized into efficient SIMD operations where available - copy_nonoverlapping(x, t, block_size); - copy_nonoverlapping(y, x, block_size); - copy_nonoverlapping(t, y, block_size); - } - i += block_size; - } - - if i < len { - // Swap any remaining bytes - let mut t = mem::MaybeUninit::::uninit(); - let rem = len - i; - - let t = t.as_mut_ptr() as *mut u8; - - // SAFETY: see previous safety comment. - unsafe { - let x = x.add(i); - let y = y.add(i); - - copy_nonoverlapping(x, t, rem); - copy_nonoverlapping(y, x, rem); - copy_nonoverlapping(t, y, rem); - } + i += 1; } } diff --git a/src/test/codegen/swap-large-types.rs b/src/test/codegen/swap-large-types.rs new file mode 100644 index 0000000000000..535d301a3d27b --- /dev/null +++ b/src/test/codegen/swap-large-types.rs @@ -0,0 +1,64 @@ +// compile-flags: -O +// only-x86_64 +// ignore-debug: the debug assertions get in the way + +#![crate_type = "lib"] + +use std::mem::swap; +use std::ptr::{read, copy_nonoverlapping, write}; + +type KeccakBuffer = [[u64; 5]; 5]; + +// A basic read+copy+write swap implementation ends up copying one of the values +// to stack for large types, which is completely unnecessary as the lack of +// overlap means we can just do whatever fits in registers at a time. + +// CHECK-LABEL: @swap_basic +#[no_mangle] +pub fn swap_basic(x: &mut KeccakBuffer, y: &mut KeccakBuffer) { +// CHECK: alloca [5 x [5 x i64]] + + // SAFETY: exclusive references are always valid to read/write, + // are non-overlapping, and nothing here panics so it's drop-safe. + unsafe { + let z = read(x); + copy_nonoverlapping(y, x, 1); + write(y, z); + } +} + +// This test verifies that the library does something smarter, and thus +// doesn't need any scratch space on the stack. + +// CHECK-LABEL: @swap_std +#[no_mangle] +pub fn swap_std(x: &mut KeccakBuffer, y: &mut KeccakBuffer) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i64> +// CHECK: store <{{[0-9]+}} x i64> + swap(x, y) +} + +// CHECK-LABEL: @swap_slice +#[no_mangle] +pub fn swap_slice(x: &mut [KeccakBuffer], y: &mut [KeccakBuffer]) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i64> +// CHECK: store <{{[0-9]+}} x i64> + if x.len() == y.len() { + x.swap_with_slice(y); + } +} + +type OneKilobyteBuffer = [u8; 1024]; + +// CHECK-LABEL: @swap_1kb_slices +#[no_mangle] +pub fn swap_1kb_slices(x: &mut [OneKilobyteBuffer], y: &mut [OneKilobyteBuffer]) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i8> +// CHECK: store <{{[0-9]+}} x i8> + if x.len() == y.len() { + x.swap_with_slice(y); + } +} diff --git a/src/test/codegen/swap-simd-types.rs b/src/test/codegen/swap-simd-types.rs new file mode 100644 index 0000000000000..c90b277eb4487 --- /dev/null +++ b/src/test/codegen/swap-simd-types.rs @@ -0,0 +1,32 @@ +// compile-flags: -O -C target-feature=+avx +// only-x86_64 +// ignore-debug: the debug assertions get in the way + +#![crate_type = "lib"] + +use std::mem::swap; + +// SIMD types are highly-aligned already, so make sure the swap code leaves their +// types alone and doesn't pessimize them (such as by swapping them as `usize`s). +extern crate core; +use core::arch::x86_64::__m256; + +// CHECK-LABEL: @swap_single_m256 +#[no_mangle] +pub fn swap_single_m256(x: &mut __m256, y: &mut __m256) { +// CHECK-NOT: alloca +// CHECK: load <8 x float>{{.+}}align 32 +// CHECK: store <8 x float>{{.+}}align 32 + swap(x, y) +} + +// CHECK-LABEL: @swap_m256_slice +#[no_mangle] +pub fn swap_m256_slice(x: &mut [__m256], y: &mut [__m256]) { +// CHECK-NOT: alloca +// CHECK: load <8 x float>{{.+}}align 32 +// CHECK: store <8 x float>{{.+}}align 32 + if x.len() == y.len() { + x.swap_with_slice(y); + } +} diff --git a/src/test/codegen/swap-small-types.rs b/src/test/codegen/swap-small-types.rs index 6205e6a6559c9..2f375844cc716 100644 --- a/src/test/codegen/swap-small-types.rs +++ b/src/test/codegen/swap-small-types.rs @@ -16,3 +16,47 @@ pub fn swap_rgb48(x: &mut RGB48, y: &mut RGB48) { // CHECK: store i48 swap(x, y) } + +// LLVM doesn't vectorize a loop over 3-byte elements, +// so we chunk it down to bytes and loop over those instead. +type RGB24 = [u8; 3]; + +// CHECK-LABEL: @swap_rgb24_slices +#[no_mangle] +pub fn swap_rgb24_slices(x: &mut [RGB24], y: &mut [RGB24]) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i8> +// CHECK: store <{{[0-9]+}} x i8> + if x.len() == y.len() { + x.swap_with_slice(y); + } +} + +// This one has a power-of-two size, so we iterate over it directly +type RGBA32 = [u8; 4]; + +// CHECK-LABEL: @swap_rgba32_slices +#[no_mangle] +pub fn swap_rgba32_slices(x: &mut [RGBA32], y: &mut [RGBA32]) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i32> +// CHECK: store <{{[0-9]+}} x i32> + if x.len() == y.len() { + x.swap_with_slice(y); + } +} + +// Strings have a non-power-of-two size, but have pointer alignment, +// so we swap usizes instead of dropping all the way down to bytes. +const _: () = assert!(!std::mem::size_of::().is_power_of_two()); + +// CHECK-LABEL: @swap_string_slices +#[no_mangle] +pub fn swap_string_slices(x: &mut [String], y: &mut [String]) { +// CHECK-NOT: alloca +// CHECK: load <{{[0-9]+}} x i64> +// CHECK: store <{{[0-9]+}} x i64> + if x.len() == y.len() { + x.swap_with_slice(y); + } +} From da896d35f471a27eb7b9385d6eadf50a5f761265 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20BRANSTETT?= Date: Sat, 19 Feb 2022 23:06:11 +0100 Subject: [PATCH 02/18] Improve CheckCfg internal representation --- compiler/rustc_attr/src/builtin.rs | 31 ++++++++------- compiler/rustc_interface/src/interface.rs | 22 ++++++++--- compiler/rustc_session/src/config.rs | 48 ++++++++++++----------- 3 files changed, 58 insertions(+), 43 deletions(-) diff --git a/compiler/rustc_attr/src/builtin.rs b/compiler/rustc_attr/src/builtin.rs index 49043e9f5f9d6..cd2e150a1907d 100644 --- a/compiler/rustc_attr/src/builtin.rs +++ b/compiler/rustc_attr/src/builtin.rs @@ -463,27 +463,30 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat MetaItemKind::NameValue(..) | MetaItemKind::Word => { let name = cfg.ident().expect("multi-segment cfg predicate").name; let value = cfg.value_str(); - if sess.check_config.names_checked && !sess.check_config.names_valid.contains(&name) - { - sess.buffer_lint( - UNEXPECTED_CFGS, - cfg.span, - CRATE_NODE_ID, - "unexpected `cfg` condition name", - ); - } - if let Some(val) = value { - if sess.check_config.values_checked.contains(&name) - && !sess.check_config.values_valid.contains(&(name, val)) - { + if let Some(names_valid) = &sess.check_config.names_valid { + if !names_valid.contains(&name) { sess.buffer_lint( UNEXPECTED_CFGS, cfg.span, CRATE_NODE_ID, - "unexpected `cfg` condition value", + "unexpected `cfg` condition name", ); } } + if let Some(val) = value { + if let Some(values_valid) = &sess.check_config.values_valid { + if let Some(values) = values_valid.get(&name) { + if !values.contains(&val) { + sess.buffer_lint( + UNEXPECTED_CFGS, + cfg.span, + CRATE_NODE_ID, + "unexpected `cfg` condition value", + ); + } + } + } + } sess.config.contains(&(name, value)) } } diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs index 609fc4b78c0de..5e0d59bf1b136 100644 --- a/compiler/rustc_interface/src/interface.rs +++ b/compiler/rustc_interface/src/interface.rs @@ -169,11 +169,12 @@ pub fn parse_check_cfg(specs: Vec) -> CheckCfg { Ok(meta_item) if parser.token == token::Eof => { if let Some(args) = meta_item.meta_item_list() { if meta_item.has_name(sym::names) { - cfg.names_checked = true; + let names_valid = + cfg.names_valid.get_or_insert_with(|| FxHashSet::default()); for arg in args { if arg.is_word() && arg.ident().is_some() { let ident = arg.ident().expect("multi-segment cfg key"); - cfg.names_valid.insert(ident.name.to_string()); + names_valid.insert(ident.name.to_string()); } else { error!("`names()` arguments must be simple identifers"); } @@ -182,14 +183,19 @@ pub fn parse_check_cfg(specs: Vec) -> CheckCfg { } else if meta_item.has_name(sym::values) { if let Some((name, values)) = args.split_first() { if name.is_word() && name.ident().is_some() { + let values_valid = cfg + .values_valid + .get_or_insert_with(|| FxHashMap::default()); let ident = name.ident().expect("multi-segment cfg key"); - cfg.values_checked.insert(ident.to_string()); + let ident_values = values_valid + .entry(ident.to_string()) + .or_insert_with(|| FxHashSet::default()); + for val in values { if let Some(LitKind::Str(s, _)) = val.literal().map(|lit| &lit.kind) { - cfg.values_valid - .insert((ident.to_string(), s.to_string())); + ident_values.insert(s.to_string()); } else { error!( "`values()` arguments must be string literals" @@ -219,7 +225,11 @@ pub fn parse_check_cfg(specs: Vec) -> CheckCfg { ); } - cfg.names_valid.extend(cfg.values_checked.iter().cloned()); + if let Some(values_valid) = &cfg.values_valid { + if let Some(names_valid) = &mut cfg.names_valid { + names_valid.extend(values_valid.keys().cloned()); + } + } cfg }) } diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index 7a0d9a212c9d9..e0a3cc78b1772 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -8,7 +8,7 @@ use crate::search_paths::SearchPath; use crate::utils::{CanonicalizedPath, NativeLib, NativeLibKind}; use crate::{early_error, early_warn, Session}; -use rustc_data_structures::fx::FxHashSet; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::impl_stable_hash_via_hash; use rustc_target::abi::{Align, TargetDataLayout}; @@ -1023,34 +1023,28 @@ pub fn to_crate_config(cfg: FxHashSet<(String, Option)>) -> CrateConfig /// The parsed `--check-cfg` options pub struct CheckCfg { - /// Set if `names()` checking is enabled - pub names_checked: bool, - /// The union of all `names()` - pub names_valid: FxHashSet, - /// The set of names for which `values()` was used - pub values_checked: FxHashSet, - /// The set of all (name, value) pairs passed in `values()` - pub values_valid: FxHashSet<(T, T)>, + /// The set of all `names()`, if none no names checking is performed + pub names_valid: Option>, + /// The set of all `values()`, if none no values chcking is performed + pub values_valid: Option>>, } impl Default for CheckCfg { fn default() -> Self { - CheckCfg { - names_checked: false, - names_valid: FxHashSet::default(), - values_checked: FxHashSet::default(), - values_valid: FxHashSet::default(), - } + CheckCfg { names_valid: Default::default(), values_valid: Default::default() } } } impl CheckCfg { fn map_data(&self, f: impl Fn(&T) -> O) -> CheckCfg { CheckCfg { - names_checked: self.names_checked, - names_valid: self.names_valid.iter().map(|a| f(a)).collect(), - values_checked: self.values_checked.iter().map(|a| f(a)).collect(), - values_valid: self.values_valid.iter().map(|(a, b)| (f(a), f(b))).collect(), + names_valid: self + .names_valid + .as_ref() + .map(|names_valid| names_valid.iter().map(|a| f(a)).collect()), + values_valid: self.values_valid.as_ref().map(|values_valid| { + values_valid.iter().map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect())).collect() + }), } } } @@ -1090,17 +1084,25 @@ impl CrateCheckConfig { sym::doctest, sym::feature, ]; - for &name in WELL_KNOWN_NAMES { - self.names_valid.insert(name); + if let Some(names_valid) = &mut self.names_valid { + for &name in WELL_KNOWN_NAMES { + names_valid.insert(name); + } } } /// Fills a `CrateCheckConfig` with configuration names and values that are actually active. pub fn fill_actual(&mut self, cfg: &CrateConfig) { for &(k, v) in cfg { - self.names_valid.insert(k); + if let Some(names_valid) = &mut self.names_valid { + names_valid.insert(k); + } if let Some(v) = v { - self.values_valid.insert((k, v)); + if let Some(values_valid) = &mut self.values_valid { + values_valid.entry(k).and_modify(|values| { + values.insert(v); + }); + } } } } From fbe1c153ecc88651a147517a6c1aea3da6aca8fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20BRANSTETT?= Date: Sun, 20 Feb 2022 00:04:10 +0100 Subject: [PATCH 03/18] Add test for well known names defined by rustc --- src/test/ui/check-cfg/well-known-names.rs | 27 +++++++++++++++++++ src/test/ui/check-cfg/well-known-names.stderr | 22 +++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 src/test/ui/check-cfg/well-known-names.rs create mode 100644 src/test/ui/check-cfg/well-known-names.stderr diff --git a/src/test/ui/check-cfg/well-known-names.rs b/src/test/ui/check-cfg/well-known-names.rs new file mode 100644 index 0000000000000..a66568a2ffdc9 --- /dev/null +++ b/src/test/ui/check-cfg/well-known-names.rs @@ -0,0 +1,27 @@ +// This test checks that we lint on non well known names and that we don't lint on well known names +// +// check-pass +// compile-flags: --check-cfg=names() -Z unstable-options + +#[cfg(target_oz = "linux")] +//~^ WARNING unexpected `cfg` condition name +fn target_os_misspell() {} + +#[cfg(target_os = "linux")] +fn target_os() {} + +#[cfg(features = "foo")] +//~^ WARNING unexpected `cfg` condition name +fn feature_misspell() {} + +#[cfg(feature = "foo")] +fn feature() {} + +#[cfg(uniw)] +//~^ WARNING unexpected `cfg` condition name +fn unix_misspell() {} + +#[cfg(unix)] +fn unix() {} + +fn main() {} diff --git a/src/test/ui/check-cfg/well-known-names.stderr b/src/test/ui/check-cfg/well-known-names.stderr new file mode 100644 index 0000000000000..a6b9a77dc8d9a --- /dev/null +++ b/src/test/ui/check-cfg/well-known-names.stderr @@ -0,0 +1,22 @@ +warning: unexpected `cfg` condition name + --> $DIR/well-known-names.rs:6:7 + | +LL | #[cfg(target_oz = "linux")] + | ^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(unexpected_cfgs)]` on by default + +warning: unexpected `cfg` condition name + --> $DIR/well-known-names.rs:13:7 + | +LL | #[cfg(features = "foo")] + | ^^^^^^^^^^^^^^^^ + +warning: unexpected `cfg` condition name + --> $DIR/well-known-names.rs:20:7 + | +LL | #[cfg(uniw)] + | ^^^^ + +warning: 3 warnings emitted + From 3d234770b1c83840d18305e8625da38e97da3174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20BRANSTETT?= Date: Sun, 20 Feb 2022 00:48:10 +0100 Subject: [PATCH 04/18] Improve diagnostic of the unexpected_cfgs lint --- compiler/rustc_attr/src/builtin.rs | 15 ++++++-- compiler/rustc_lint/src/context.rs | 34 ++++++++++++++++++- compiler/rustc_lint/src/lib.rs | 1 + compiler/rustc_lint_defs/src/lib.rs | 1 + src/test/ui/check-cfg/invalid-cfg-name.stderr | 2 +- .../ui/check-cfg/invalid-cfg-value.stderr | 1 + src/test/ui/check-cfg/well-known-names.stderr | 10 ++++-- 7 files changed, 57 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_attr/src/builtin.rs b/compiler/rustc_attr/src/builtin.rs index cd2e150a1907d..503e172b6873f 100644 --- a/compiler/rustc_attr/src/builtin.rs +++ b/compiler/rustc_attr/src/builtin.rs @@ -8,6 +8,7 @@ use rustc_errors::{struct_span_err, Applicability}; use rustc_feature::{find_gated_cfg, is_builtin_attr_name, Features, GatedCfg}; use rustc_macros::HashStable_Generic; use rustc_session::lint::builtin::UNEXPECTED_CFGS; +use rustc_session::lint::BuiltinLintDiagnostics; use rustc_session::parse::{feature_err, ParseSess}; use rustc_session::Session; use rustc_span::hygiene::Transparency; @@ -465,11 +466,16 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat let value = cfg.value_str(); if let Some(names_valid) = &sess.check_config.names_valid { if !names_valid.contains(&name) { - sess.buffer_lint( + sess.buffer_lint_with_diagnostic( UNEXPECTED_CFGS, cfg.span, CRATE_NODE_ID, "unexpected `cfg` condition name", + BuiltinLintDiagnostics::UnexpectedCfg( + cfg.ident().unwrap().span, + name, + None, + ), ); } } @@ -477,11 +483,16 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat if let Some(values_valid) = &sess.check_config.values_valid { if let Some(values) = values_valid.get(&name) { if !values.contains(&val) { - sess.buffer_lint( + sess.buffer_lint_with_diagnostic( UNEXPECTED_CFGS, cfg.span, CRATE_NODE_ID, "unexpected `cfg` condition value", + BuiltinLintDiagnostics::UnexpectedCfg( + cfg.name_value_literal_span().unwrap(), + name, + Some(val), + ), ); } } diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index d2d853efda2d2..72a3f5d5fc9b8 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/compiler/rustc_lint/src/context.rs @@ -766,7 +766,39 @@ pub trait LintContext: Sized { BuiltinLintDiagnostics::NamedAsmLabel(help) => { db.help(&help); db.note("see the asm section of Rust By Example for more information"); - } + }, + BuiltinLintDiagnostics::UnexpectedCfg(span, name, value) => { + let mut possibilities: Vec = if value.is_some() { + let Some(values_valid) = &sess.parse_sess.check_config.values_valid else { + bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable"); + }; + let Some(values) = values_valid.get(&name) else { + bug!("it shouldn't be possible to have a diagnostic on a value whose name is not in values"); + }; + values.iter().map(|&s| s).collect() + } else { + let Some(names_valid) = &sess.parse_sess.check_config.names_valid else { + bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable"); + }; + names_valid.iter().map(|s| *s).collect() + }; + + // Show the full list if all possible values for a given name, but don't do it + // for names as the possibilities could be very long + if value.is_some() { + // Sorting can take some time, so we only do it if required + possibilities.sort(); + + let possibilities = possibilities.iter().map(Symbol::as_str).intersperse(", ").collect::(); + db.note(&format!("possible values for `{name}` are: {possibilities}")); + } + + // Suggest the most probable if we found one + if let Some(best_match) = find_best_match_for_name(&possibilities, value.unwrap_or(name), None) { + let ponctuation = if value.is_some() { "\"" } else { "" }; + db.span_suggestion(span, "did you mean", format!("{ponctuation}{best_match}{ponctuation}"), Applicability::MaybeIncorrect); + } + }, } // Rewrap `db`, and pass control to the user. decorate(LintDiagnosticBuilder::new(db)); diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 69863b5ff827f..7182022d25298 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -31,6 +31,7 @@ #![feature(box_patterns)] #![feature(crate_visibility_modifier)] #![feature(if_let_guard)] +#![feature(iter_intersperse)] #![feature(iter_order_by)] #![feature(let_else)] #![feature(never_type)] diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs index 1f834b7212fe5..e9c62fc400651 100644 --- a/compiler/rustc_lint_defs/src/lib.rs +++ b/compiler/rustc_lint_defs/src/lib.rs @@ -310,6 +310,7 @@ pub enum BuiltinLintDiagnostics { BreakWithLabelAndLoop(Span), NamedAsmLabel(String), UnicodeTextFlow(Span, String), + UnexpectedCfg(Span, Symbol, Option), } /// Lints that are buffered up early on in the `Session` before the diff --git a/src/test/ui/check-cfg/invalid-cfg-name.stderr b/src/test/ui/check-cfg/invalid-cfg-name.stderr index 2587685afa048..2bd1821c9422b 100644 --- a/src/test/ui/check-cfg/invalid-cfg-name.stderr +++ b/src/test/ui/check-cfg/invalid-cfg-name.stderr @@ -2,7 +2,7 @@ warning: unexpected `cfg` condition name --> $DIR/invalid-cfg-name.rs:7:7 | LL | #[cfg(widnows)] - | ^^^^^^^ + | ^^^^^^^ help: did you mean: `windows` | = note: `#[warn(unexpected_cfgs)]` on by default diff --git a/src/test/ui/check-cfg/invalid-cfg-value.stderr b/src/test/ui/check-cfg/invalid-cfg-value.stderr index c591d8474a261..23fd5c8c759cf 100644 --- a/src/test/ui/check-cfg/invalid-cfg-value.stderr +++ b/src/test/ui/check-cfg/invalid-cfg-value.stderr @@ -5,6 +5,7 @@ LL | #[cfg(feature = "sedre")] | ^^^^^^^^^^^^^^^^^ | = note: `#[warn(unexpected_cfgs)]` on by default + = note: possible values for `feature` are: rand, serde, full warning: 1 warning emitted diff --git a/src/test/ui/check-cfg/well-known-names.stderr b/src/test/ui/check-cfg/well-known-names.stderr index a6b9a77dc8d9a..bdbe4d29d30fe 100644 --- a/src/test/ui/check-cfg/well-known-names.stderr +++ b/src/test/ui/check-cfg/well-known-names.stderr @@ -2,7 +2,9 @@ warning: unexpected `cfg` condition name --> $DIR/well-known-names.rs:6:7 | LL | #[cfg(target_oz = "linux")] - | ^^^^^^^^^^^^^^^^^^^ + | ---------^^^^^^^^^^ + | | + | help: did you mean: `target_os` | = note: `#[warn(unexpected_cfgs)]` on by default @@ -10,13 +12,15 @@ warning: unexpected `cfg` condition name --> $DIR/well-known-names.rs:13:7 | LL | #[cfg(features = "foo")] - | ^^^^^^^^^^^^^^^^ + | --------^^^^^^^^ + | | + | help: did you mean: `feature` warning: unexpected `cfg` condition name --> $DIR/well-known-names.rs:20:7 | LL | #[cfg(uniw)] - | ^^^^ + | ^^^^ help: did you mean: `unix` warning: 3 warnings emitted From 8d3de56da1eba68d012977d4c743d5eaaa1baee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20BRANSTETT?= Date: Sun, 20 Feb 2022 01:26:52 +0100 Subject: [PATCH 05/18] Continue improvements on the --check-cfg implementation - Test the combinations of --check-cfg with partial values() and --cfg - Test that we detect unexpected value when none are expected --- compiler/rustc_attr/src/builtin.rs | 39 +++++------ compiler/rustc_interface/src/interface.rs | 14 ++-- compiler/rustc_lint/src/context.rs | 25 +++---- compiler/rustc_session/src/config.rs | 22 +++---- .../ui/check-cfg/invalid-cfg-value.stderr | 2 +- src/test/ui/check-cfg/mix.rs | 50 ++++++++++++++ src/test/ui/check-cfg/mix.stderr | 66 +++++++++++++++++++ src/test/ui/check-cfg/no-values.rs | 10 +++ src/test/ui/check-cfg/no-values.stderr | 11 ++++ 9 files changed, 184 insertions(+), 55 deletions(-) create mode 100644 src/test/ui/check-cfg/mix.rs create mode 100644 src/test/ui/check-cfg/mix.stderr create mode 100644 src/test/ui/check-cfg/no-values.rs create mode 100644 src/test/ui/check-cfg/no-values.stderr diff --git a/compiler/rustc_attr/src/builtin.rs b/compiler/rustc_attr/src/builtin.rs index 503e172b6873f..50eb6b6e5da52 100644 --- a/compiler/rustc_attr/src/builtin.rs +++ b/compiler/rustc_attr/src/builtin.rs @@ -462,7 +462,8 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat true } MetaItemKind::NameValue(..) | MetaItemKind::Word => { - let name = cfg.ident().expect("multi-segment cfg predicate").name; + let ident = cfg.ident().expect("multi-segment cfg predicate"); + let name = ident.name; let value = cfg.value_str(); if let Some(names_valid) = &sess.check_config.names_valid { if !names_valid.contains(&name) { @@ -471,30 +472,24 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat cfg.span, CRATE_NODE_ID, "unexpected `cfg` condition name", - BuiltinLintDiagnostics::UnexpectedCfg( - cfg.ident().unwrap().span, - name, - None, - ), + BuiltinLintDiagnostics::UnexpectedCfg(ident.span, name, None), ); } } - if let Some(val) = value { - if let Some(values_valid) = &sess.check_config.values_valid { - if let Some(values) = values_valid.get(&name) { - if !values.contains(&val) { - sess.buffer_lint_with_diagnostic( - UNEXPECTED_CFGS, - cfg.span, - CRATE_NODE_ID, - "unexpected `cfg` condition value", - BuiltinLintDiagnostics::UnexpectedCfg( - cfg.name_value_literal_span().unwrap(), - name, - Some(val), - ), - ); - } + if let Some(value) = value { + if let Some(values) = &sess.check_config.values_valid.get(&name) { + if !values.contains(&value) { + sess.buffer_lint_with_diagnostic( + UNEXPECTED_CFGS, + cfg.span, + CRATE_NODE_ID, + "unexpected `cfg` condition value", + BuiltinLintDiagnostics::UnexpectedCfg( + cfg.name_value_literal_span().unwrap(), + name, + Some(value), + ), + ); } } } diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs index 5e0d59bf1b136..91ced2a2d90e2 100644 --- a/compiler/rustc_interface/src/interface.rs +++ b/compiler/rustc_interface/src/interface.rs @@ -183,12 +183,10 @@ pub fn parse_check_cfg(specs: Vec) -> CheckCfg { } else if meta_item.has_name(sym::values) { if let Some((name, values)) = args.split_first() { if name.is_word() && name.ident().is_some() { - let values_valid = cfg - .values_valid - .get_or_insert_with(|| FxHashMap::default()); let ident = name.ident().expect("multi-segment cfg key"); - let ident_values = values_valid - .entry(ident.to_string()) + let ident_values = cfg + .values_valid + .entry(ident.name.to_string()) .or_insert_with(|| FxHashSet::default()); for val in values { @@ -225,10 +223,8 @@ pub fn parse_check_cfg(specs: Vec) -> CheckCfg { ); } - if let Some(values_valid) = &cfg.values_valid { - if let Some(names_valid) = &mut cfg.names_valid { - names_valid.extend(values_valid.keys().cloned()); - } + if let Some(names_valid) = &mut cfg.names_valid { + names_valid.extend(cfg.values_valid.keys().cloned()); } cfg }) diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 72a3f5d5fc9b8..5f07cf08c2ea3 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/compiler/rustc_lint/src/context.rs @@ -768,17 +768,14 @@ pub trait LintContext: Sized { db.note("see the asm section of Rust By Example for more information"); }, BuiltinLintDiagnostics::UnexpectedCfg(span, name, value) => { - let mut possibilities: Vec = if value.is_some() { - let Some(values_valid) = &sess.parse_sess.check_config.values_valid else { - bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable"); - }; - let Some(values) = values_valid.get(&name) else { + let possibilities: Vec = if value.is_some() { + let Some(values) = &sess.parse_sess.check_config.values_valid.get(&name) else { bug!("it shouldn't be possible to have a diagnostic on a value whose name is not in values"); }; values.iter().map(|&s| s).collect() } else { let Some(names_valid) = &sess.parse_sess.check_config.names_valid else { - bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable"); + bug!("it shouldn't be possible to have a diagnostic on a name if name checking is not enabled"); }; names_valid.iter().map(|s| *s).collect() }; @@ -786,17 +783,21 @@ pub trait LintContext: Sized { // Show the full list if all possible values for a given name, but don't do it // for names as the possibilities could be very long if value.is_some() { - // Sorting can take some time, so we only do it if required - possibilities.sort(); + if !possibilities.is_empty() { + let mut possibilities = possibilities.iter().map(Symbol::as_str).collect::>(); + possibilities.sort(); - let possibilities = possibilities.iter().map(Symbol::as_str).intersperse(", ").collect::(); - db.note(&format!("possible values for `{name}` are: {possibilities}")); + let possibilities = possibilities.join(", "); + db.note(&format!("expected values for `{name}` are: {possibilities}")); + } else { + db.note(&format!("no expected value for `{name}`")); + } } // Suggest the most probable if we found one if let Some(best_match) = find_best_match_for_name(&possibilities, value.unwrap_or(name), None) { - let ponctuation = if value.is_some() { "\"" } else { "" }; - db.span_suggestion(span, "did you mean", format!("{ponctuation}{best_match}{ponctuation}"), Applicability::MaybeIncorrect); + let punctuation = if value.is_some() { "\"" } else { "" }; + db.span_suggestion(span, "did you mean", format!("{punctuation}{best_match}{punctuation}"), Applicability::MaybeIncorrect); } }, } diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs index e0a3cc78b1772..f9b75690e375f 100644 --- a/compiler/rustc_session/src/config.rs +++ b/compiler/rustc_session/src/config.rs @@ -1023,10 +1023,10 @@ pub fn to_crate_config(cfg: FxHashSet<(String, Option)>) -> CrateConfig /// The parsed `--check-cfg` options pub struct CheckCfg { - /// The set of all `names()`, if none no names checking is performed + /// The set of all `names()`, if None no name checking is performed pub names_valid: Option>, - /// The set of all `values()`, if none no values chcking is performed - pub values_valid: Option>>, + /// The set of all `values()` + pub values_valid: FxHashMap>, } impl Default for CheckCfg { @@ -1042,9 +1042,11 @@ impl CheckCfg { .names_valid .as_ref() .map(|names_valid| names_valid.iter().map(|a| f(a)).collect()), - values_valid: self.values_valid.as_ref().map(|values_valid| { - values_valid.iter().map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect())).collect() - }), + values_valid: self + .values_valid + .iter() + .map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect())) + .collect(), } } } @@ -1098,11 +1100,9 @@ impl CrateCheckConfig { names_valid.insert(k); } if let Some(v) = v { - if let Some(values_valid) = &mut self.values_valid { - values_valid.entry(k).and_modify(|values| { - values.insert(v); - }); - } + self.values_valid.entry(k).and_modify(|values| { + values.insert(v); + }); } } } diff --git a/src/test/ui/check-cfg/invalid-cfg-value.stderr b/src/test/ui/check-cfg/invalid-cfg-value.stderr index 23fd5c8c759cf..bc2c053fed65a 100644 --- a/src/test/ui/check-cfg/invalid-cfg-value.stderr +++ b/src/test/ui/check-cfg/invalid-cfg-value.stderr @@ -5,7 +5,7 @@ LL | #[cfg(feature = "sedre")] | ^^^^^^^^^^^^^^^^^ | = note: `#[warn(unexpected_cfgs)]` on by default - = note: possible values for `feature` are: rand, serde, full + = note: expected values for `feature` are: full, rand, serde warning: 1 warning emitted diff --git a/src/test/ui/check-cfg/mix.rs b/src/test/ui/check-cfg/mix.rs new file mode 100644 index 0000000000000..26c735c4a10bd --- /dev/null +++ b/src/test/ui/check-cfg/mix.rs @@ -0,0 +1,50 @@ +// This test checks the combination of well known names, their activation via names(), the usage of +// partial values() with a --cfg and test that we also correctly lint on the `cfg!` macro and +// `cfg_attr` attribute. +// +// check-pass +// compile-flags: --check-cfg=names() --check-cfg=values(feature,"foo") --cfg feature="bar" -Z unstable-options + +#[cfg(windows)] +fn do_windows_stuff() {} + +#[cfg(widnows)] +//~^ WARNING unexpected `cfg` condition name +fn do_windows_stuff() {} + +#[cfg(feature = "foo")] +fn use_foo() {} + +#[cfg(feature = "bar")] +fn use_bar() {} + +#[cfg(feature = "zebra")] +//~^ WARNING unexpected `cfg` condition value +fn use_zebra() {} + +#[cfg_attr(uu, test)] +//~^ WARNING unexpected `cfg` condition name +fn do_test() {} + +#[cfg_attr(feature = "foo", no_mangle)] +fn do_test_foo() {} + +fn test_cfg_macro() { + cfg!(windows); + cfg!(widnows); + //~^ WARNING unexpected `cfg` condition name + cfg!(feature = "foo"); + cfg!(feature = "bar"); + cfg!(feature = "zebra"); + //~^ WARNING unexpected `cfg` condition value + cfg!(xxx = "foo"); + //~^ WARNING unexpected `cfg` condition name + cfg!(xxx); + //~^ WARNING unexpected `cfg` condition name + cfg!(any(xxx, windows)); + //~^ WARNING unexpected `cfg` condition name + cfg!(any(feature = "bad", windows)); + //~^ WARNING unexpected `cfg` condition value +} + +fn main() {} diff --git a/src/test/ui/check-cfg/mix.stderr b/src/test/ui/check-cfg/mix.stderr new file mode 100644 index 0000000000000..b273be774224d --- /dev/null +++ b/src/test/ui/check-cfg/mix.stderr @@ -0,0 +1,66 @@ +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:11:7 + | +LL | #[cfg(widnows)] + | ^^^^^^^ help: did you mean: `windows` + | + = note: `#[warn(unexpected_cfgs)]` on by default + +warning: unexpected `cfg` condition value + --> $DIR/mix.rs:21:7 + | +LL | #[cfg(feature = "zebra")] + | ^^^^^^^^^^^^^^^^^ + | + = note: expected values for `feature` are: bar, foo + +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:25:12 + | +LL | #[cfg_attr(uu, test)] + | ^^ + +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:34:10 + | +LL | cfg!(widnows); + | ^^^^^^^ help: did you mean: `windows` + +warning: unexpected `cfg` condition value + --> $DIR/mix.rs:38:10 + | +LL | cfg!(feature = "zebra"); + | ^^^^^^^^^^^^^^^^^ + | + = note: expected values for `feature` are: bar, foo + +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:40:10 + | +LL | cfg!(xxx = "foo"); + | ^^^^^^^^^^^ + +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:42:10 + | +LL | cfg!(xxx); + | ^^^ + +warning: unexpected `cfg` condition name + --> $DIR/mix.rs:44:14 + | +LL | cfg!(any(xxx, windows)); + | ^^^ + +warning: unexpected `cfg` condition value + --> $DIR/mix.rs:46:14 + | +LL | cfg!(any(feature = "bad", windows)); + | ^^^^^^^^^^----- + | | + | help: did you mean: `"bar"` + | + = note: expected values for `feature` are: bar, foo + +warning: 9 warnings emitted + diff --git a/src/test/ui/check-cfg/no-values.rs b/src/test/ui/check-cfg/no-values.rs new file mode 100644 index 0000000000000..2440757e52da9 --- /dev/null +++ b/src/test/ui/check-cfg/no-values.rs @@ -0,0 +1,10 @@ +// Check that we detect unexpected value when none are allowed +// +// check-pass +// compile-flags: --check-cfg=values(feature) -Z unstable-options + +#[cfg(feature = "foo")] +//~^ WARNING unexpected `cfg` condition value +fn do_foo() {} + +fn main() {} diff --git a/src/test/ui/check-cfg/no-values.stderr b/src/test/ui/check-cfg/no-values.stderr new file mode 100644 index 0000000000000..ea1c9107d4c2f --- /dev/null +++ b/src/test/ui/check-cfg/no-values.stderr @@ -0,0 +1,11 @@ +warning: unexpected `cfg` condition value + --> $DIR/no-values.rs:6:7 + | +LL | #[cfg(feature = "foo")] + | ^^^^^^^^^^^^^^^ + | + = note: `#[warn(unexpected_cfgs)]` on by default + = note: no expected value for `feature` + +warning: 1 warning emitted + From a556a2a8e60501203f310407b27cf739618c0000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20BRANSTETT?= Date: Mon, 21 Feb 2022 15:29:04 +0100 Subject: [PATCH 06/18] Add compiler flag `--check-cfg` to the unstable book --- .../src/compiler-flags/check-cfg.md | 221 ++++++++++++++++++ 1 file changed, 221 insertions(+) create mode 100644 src/doc/unstable-book/src/compiler-flags/check-cfg.md diff --git a/src/doc/unstable-book/src/compiler-flags/check-cfg.md b/src/doc/unstable-book/src/compiler-flags/check-cfg.md new file mode 100644 index 0000000000000..d7345ad0c33f2 --- /dev/null +++ b/src/doc/unstable-book/src/compiler-flags/check-cfg.md @@ -0,0 +1,221 @@ +# `check-cfg` + +The tracking issue for this feature is: [#82450](https://github.com/rust-lang/rust/issues/82450). + +------------------------ + +This feature allows you to enable complete or partial checking of configuration. + +`rustc` accepts the `--check-cfg` option, which specifies whether to check conditions and how to +check them. The `--check-cfg` option takes a value, called the _check cfg specification_. The +check cfg specification is parsed using the Rust metadata syntax, just as the `--cfg` option is. + +`--check-cfg` option can take one of two forms: + +1. `--check-cfg names(...)` enables checking condition names. +2. `--check-cfg values(...)` enables checking the values within list-valued conditions. + +These two options are independent. `names` checks only the namespace of condition names +while `values` checks only the namespace of the values of list-valued conditions. + +## The `names(...)` form + +The `names(...)` form enables checking the names. This form uses a named list: + +```bash +rustc --check-cfg 'names(name1, name2, ... nameN)' +``` + +where each `name` is a bare identifier (has no quotes). The order of the names is not significant. + +If `--check-cfg names(...)` is specified at least once, then `rustc` will check all references to +condition names. `rustc` will check every `#[cfg]` attribute, `#[cfg_attr]` attribute, `cfg` clause +inside `#[link]` attribute and `cfg!(...)` call against the provided list of expected condition +names. If a name is not present in this list, then `rustc` will report an `unexpected_cfgs` lint +diagnostic. The default diagnostic level for this lint is `Warn`. + +If `--check-cfg names(...)` is not specified, then `rustc` will not check references to condition +names. + +`--check-cfg names(...)` may be specified more than once. The result is that the list of valid +condition names is merged across all options. It is legal for a condition name to be specified +more than once; redundantly specifying a condition name has no effect. + +To enable checking condition names with an empty set of valid condition names, use the following +form. The parentheses are required. + +```bash +rustc --check-cfg 'names()' +``` + +Note that `--check-cfg 'names()'` is _not_ equivalent to omitting the option entirely. +The first form enables checking condition names, while specifying that there are no valid +condition names (outside of the set of well-known names defined by `rustc`). Omitting the +`--check-cfg 'names(...)'` option does not enable checking condition names. + +Conditions that are enabled are implicitly valid; it is unnecessary (but legal) to specify a +condition name as both enabled and valid. For example, the following invocations are equivalent: + +```bash +# condition names will be checked, and 'has_time_travel' is valid +rustc --cfg 'has_time_travel' --check-cfg 'names()' + +# condition names will be checked, and 'has_time_travel' is valid +rustc --cfg 'has_time_travel' --check-cfg 'names(has_time_travel)' +``` + +In contrast, the following two invocations are _not_ equivalent: + +```bash +# condition names will not be checked (because there is no --check-cfg names(...)) +rustc --cfg 'has_time_travel' + +# condition names will be checked, and 'has_time_travel' is both valid and enabled. +rustc --cfg 'has_time_travel' --check-cfg 'names(has_time_travel)' +``` + +## The `values(...)` form + +The `values(...)` form enables checking the values within list-valued conditions. It has this +form: + +```bash +rustc --check-cfg `values(name, "value1", "value2", ... "valueN")' +``` + +where `name` is a bare identifier (has no quotes) and each `"value"` term is a quoted literal +string. `name` specifies the name of the condition, such as `feature` or `target_os`. + +When the `values(...)` option is specified, `rustc` will check every `#[cfg(name = "value")]` +attribute, `#[cfg_attr(name = "value")]` attribute, `#[link(name = "a", cfg(name = "value"))]` +and `cfg!(name = "value")` call. It will check that the `"value"` specified is present in the +list of expected values. If `"value"` is not in it, then `rustc` will report an `unexpected_cfgs` +lint diagnostic. The default diagnostic level for this lint is `Warn`. + +The form `values()` is an error, because it does not specify a condition name. + +To enable checking of values, but to provide an empty set of valid values, use this form: + +```bash +rustc --check-cfg `values(name)` +``` + +The `--check-cfg values(...)` option can be repeated, both for the same condition name and for +different names. If it is repeated for the same condition name, then the sets of values for that +condition are merged together. + +## Examples + +Consider this command line: + +```bash +rustc --check-cfg 'names(feature)' \ + --check-cfg 'values(feature,"lion","zebra")' \ + --cfg 'feature="lion"' -Z unstable-options \ + example.rs +``` + +This command line indicates that this crate has two features: `lion` and `zebra`. The `lion` +feature is enabled, while the `zebra` feature is disabled. Consider compiling this code: + +```rust +// This is expected, and tame_lion() will be compiled +#[cfg(feature = "lion")] +fn tame_lion(lion: Lion) {} + +// This is expected, and ride_zebra() will NOT be compiled. +#[cfg(feature = "zebra")] +fn ride_zebra(zebra: Zebra) {} + +// This is UNEXPECTED, and will cause a compiler warning (by default). +#[cfg(feature = "platypus")] +fn poke_platypus() {} + +// This is UNEXPECTED, because 'feechure' is not a known condition name, +// and will cause a compiler warning (by default). +#[cfg(feechure = "lion")] +fn tame_lion() {} +``` + +> Note: The `--check-cfg names(feature)` option is necessary only to enable checking the condition +> name, as in the last example. `feature` is a well-known (always-expected) condition name, and so +> it is not necessary to specify it in a `--check-cfg 'names(...)'` option. That option can be +> shortened to > `--check-cfg names()` in order to enable checking well-known condition names. + +### Example: Checking condition names, but not values + +```bash +# This turns on checking for condition names, but not values, such as 'feature' values. +rustc --check-cfg 'names(is_embedded, has_feathers)' \ + --cfg has_feathers --cfg 'feature = "zapping"' -Z unstable-options +``` + +```rust +#[cfg(is_embedded)] // This is expected as "is_embedded" was provided in names() +fn do_embedded() {} + +#[cfg(has_feathers)] // This is expected as "has_feathers" was provided in names() +fn do_features() {} + +#[cfg(has_mumble_frotz)] // This is UNEXPECTED because names checking is enable and + // "has_mumble_frotz" was not provided in names() +fn do_mumble_frotz() {} + +#[cfg(feature = "lasers")] // This doesn't raise a warning, because values checking for "feature" + // was never used +fn shoot_lasers() {} +``` + +### Example: Checking feature values, but not condition names + +```bash +# This turns on checking for feature values, but not for condition names. +rustc --check-cfg 'values(feature, "zapping", "lasers")' \ + --cfg 'feature="zapping"' -Z unstable-options +``` + +```rust +#[cfg(is_embedded)] // This is doesn't raise a warning, because names checking was not + // enable (ie not names()) +fn do_embedded() {} + +#[cfg(has_feathers)] // Same as above, --check-cfg names(...) was never used so no name + // checking is performed +fn do_features() {} + + +#[cfg(feature = "lasers")] // This is expected, "lasers" is in the values(feature) list +fn shoot_lasers() {} + +#[cfg(feature = "monkeys")] // This is UNEXPECTED, because "monkeys" is not in the + // --check-cfg values(feature) list +fn write_shakespeare() {} +``` + +### Example: Checking both condition names and feature values + +```bash +# This turns on checking for feature values and for condition names. +rustc --check-cfg 'names(is_embedded, has_feathers)' \ + --check-cfg 'values(feature, "zapping", "lasers")' \ + --cfg has_feathers --cfg 'feature="zapping"' -Z unstable-options +``` + +```rust +#[cfg(is_embedded)] // This is expected because "is_embedded" was provided in names() +fn do_embedded() {} + +#[cfg(has_feathers)] // This is expected because "has_feathers" was provided in names() +fn do_features() {} + +#[cfg(has_mumble_frotz)] // This is UNEXPECTED, because has_mumble_frotz is not in the + // --check-cfg names(...) list +fn do_mumble_frotz() {} + +#[cfg(feature = "lasers")] // This is expected, "lasers" is in the values(feature) list +fn shoot_lasers() {} + +#[cfg(feature = "monkeys")] // This is UNEXPECTED, because "monkeys" is not in + // the values(feature) list +fn write_shakespear() {} +``` From c73a2f8a652134bd7bf00ca61ca65bd7adb8aec7 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 23 Feb 2022 08:11:50 -0800 Subject: [PATCH 07/18] properly handle fat pointers to uninhabitable types --- .../src/debuginfo/metadata.rs | 5 ++- .../rustc_codegen_llvm/src/debuginfo/utils.rs | 33 +++++++++---------- ...uginfo-emit-llvm-ir-and-split-debuginfo.rs | 0 ...fo_with_uninhabitable_field_and_unsized.rs | 29 ++++++++++++++++ 4 files changed, 49 insertions(+), 18 deletions(-) rename src/test/ui/{ => debuginfo}/debuginfo-emit-llvm-ir-and-split-debuginfo.rs (100%) create mode 100644 src/test/ui/debuginfo/debuginfo_with_uninhabitable_field_and_unsized.rs diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs b/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs index ef87b7b1a7e07..517c539b45ac8 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs @@ -449,7 +449,10 @@ fn pointer_or_reference_metadata<'ll, 'tcx>( // This is a thin pointer. Create a regular pointer type and give it the correct name. debug_assert_eq!( (thin_pointer_size, thin_pointer_align), - cx.size_and_align_of(ptr_type) + cx.size_and_align_of(ptr_type), + "ptr_type={}, pointee_type={}", + ptr_type, + pointee_type, ); unsafe { diff --git a/compiler/rustc_codegen_llvm/src/debuginfo/utils.rs b/compiler/rustc_codegen_llvm/src/debuginfo/utils.rs index acd032a7dc6d0..fa75463067f47 100644 --- a/compiler/rustc_codegen_llvm/src/debuginfo/utils.rs +++ b/compiler/rustc_codegen_llvm/src/debuginfo/utils.rs @@ -4,9 +4,9 @@ use super::namespace::item_namespace; use super::CrateDebugContext; use rustc_hir::def_id::DefId; -use rustc_middle::ty::layout::LayoutOf; +use rustc_middle::ty::layout::{HasParamEnv, LayoutOf}; use rustc_middle::ty::{self, DefIdTree, Ty}; -use rustc_target::abi::Variants; +use tracing::trace; use crate::common::CodegenCx; use crate::llvm; @@ -63,30 +63,26 @@ pub(crate) fn fat_pointer_kind<'ll, 'tcx>( cx: &CodegenCx<'ll, 'tcx>, pointee_ty: Ty<'tcx>, ) -> Option { - let layout = cx.layout_of(pointee_ty); + let pointee_tail_ty = cx.tcx.struct_tail_erasing_lifetimes(pointee_ty, cx.param_env()); + let layout = cx.layout_of(pointee_tail_ty); + trace!( + "fat_pointer_kind: {:?} has layout {:?} (is_unsized? {})", + pointee_tail_ty, + layout, + layout.is_unsized() + ); if !layout.is_unsized() { return None; } - match *pointee_ty.kind() { + match *pointee_tail_ty.kind() { ty::Str | ty::Slice(_) => Some(FatPtrKind::Slice), ty::Dynamic(..) => Some(FatPtrKind::Dyn), - ty::Adt(..) | ty::Tuple(..) if matches!(layout.variants, Variants::Single { .. }) => { - let last_field_index = layout.fields.count() - 1; - debug_assert!( - (0..last_field_index) - .all(|field_index| { !layout.field(cx, field_index).is_unsized() }) - ); - - let unsized_field = layout.field(cx, last_field_index); - debug_assert!(unsized_field.is_unsized()); - fat_pointer_kind(cx, unsized_field.ty) - } ty::Foreign(_) => { // Assert that pointers to foreign types really are thin: debug_assert_eq!( - cx.size_of(cx.tcx.mk_imm_ptr(pointee_ty)), + cx.size_of(cx.tcx.mk_imm_ptr(pointee_tail_ty)), cx.size_of(cx.tcx.mk_imm_ptr(cx.tcx.types.u8)) ); None @@ -94,7 +90,10 @@ pub(crate) fn fat_pointer_kind<'ll, 'tcx>( _ => { // For all other pointee types we should already have returned None // at the beginning of the function. - panic!("fat_pointer_kind() - Encountered unexpected `pointee_ty`: {:?}", pointee_ty) + panic!( + "fat_pointer_kind() - Encountered unexpected `pointee_tail_ty`: {:?}", + pointee_tail_ty + ) } } } diff --git a/src/test/ui/debuginfo-emit-llvm-ir-and-split-debuginfo.rs b/src/test/ui/debuginfo/debuginfo-emit-llvm-ir-and-split-debuginfo.rs similarity index 100% rename from src/test/ui/debuginfo-emit-llvm-ir-and-split-debuginfo.rs rename to src/test/ui/debuginfo/debuginfo-emit-llvm-ir-and-split-debuginfo.rs diff --git a/src/test/ui/debuginfo/debuginfo_with_uninhabitable_field_and_unsized.rs b/src/test/ui/debuginfo/debuginfo_with_uninhabitable_field_and_unsized.rs new file mode 100644 index 0000000000000..833a4726acb0f --- /dev/null +++ b/src/test/ui/debuginfo/debuginfo_with_uninhabitable_field_and_unsized.rs @@ -0,0 +1,29 @@ +// check-pass +// compile-flags: -Cdebuginfo=2 +// fixes issue #94149 + +#![allow(dead_code)] + +pub fn main() { + let _ = Foo::::new(); +} + +pub struct Foo { + base: FooBase, + value: T, +} + +impl Foo { + pub fn new() -> Box> { + todo!() + } +} + +pub trait FooTrait {} + +pub struct FooBase { + cls: Bar, +} + +// Bar *must* be a fieldless enum +pub enum Bar {} From f047af24b3b90f19b89fac80822acd69613b89ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Wed, 23 Feb 2022 00:00:00 +0000 Subject: [PATCH 08/18] Normalize main return type during mono item collection & codegen --- compiler/rustc_codegen_cranelift/src/main_shim.rs | 5 ++++- compiler/rustc_codegen_ssa/src/base.rs | 5 ++++- compiler/rustc_monomorphize/src/collector.rs | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/main_shim.rs b/compiler/rustc_codegen_cranelift/src/main_shim.rs index 9ce727279c27c..2f71a70a44946 100644 --- a/compiler/rustc_codegen_cranelift/src/main_shim.rs +++ b/compiler/rustc_codegen_cranelift/src/main_shim.rs @@ -51,7 +51,10 @@ pub(crate) fn maybe_create_entry_wrapper( // late-bound regions, since late-bound // regions must appear in the argument // listing. - let main_ret_ty = tcx.erase_regions(main_ret_ty.no_bound_vars().unwrap()); + let main_ret_ty = tcx.normalize_erasing_regions( + ty::ParamEnv::reveal_all(), + main_ret_ty.no_bound_vars().unwrap(), + ); let cmain_sig = Signature { params: vec![ diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index ed6c156547e29..11f32d92e44cd 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -407,7 +407,10 @@ pub fn maybe_create_entry_wrapper<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( // late-bound regions, since late-bound // regions must appear in the argument // listing. - let main_ret_ty = cx.tcx().erase_regions(main_ret_ty.no_bound_vars().unwrap()); + let main_ret_ty = cx.tcx().normalize_erasing_regions( + ty::ParamEnv::reveal_all(), + main_ret_ty.no_bound_vars().unwrap(), + ); let Some(llfn) = cx.declare_c_main(llfty) else { // FIXME: We should be smart and show a better diagnostic here. diff --git a/compiler/rustc_monomorphize/src/collector.rs b/compiler/rustc_monomorphize/src/collector.rs index a517e4879aafa..2f88c45a2a389 100644 --- a/compiler/rustc_monomorphize/src/collector.rs +++ b/compiler/rustc_monomorphize/src/collector.rs @@ -1270,7 +1270,10 @@ impl<'v> RootCollector<'_, 'v> { // late-bound regions, since late-bound // regions must appear in the argument // listing. - let main_ret_ty = self.tcx.erase_regions(main_ret_ty.no_bound_vars().unwrap()); + let main_ret_ty = self.tcx.normalize_erasing_regions( + ty::ParamEnv::reveal_all(), + main_ret_ty.no_bound_vars().unwrap(), + ); let start_instance = Instance::resolve( self.tcx, From 37d9ea745b358a5b9f48560600841fc6619e545d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 24 Feb 2022 16:10:36 +1100 Subject: [PATCH 09/18] Improve `scan_escape`. `scan_escape` currently has a fast path (for when the first char isn't '\\') and a slow path. This commit changes `scan_escape` so it only handles the slow path, i.e. the actual escaping code. The fast path is inlined into the two call sites. This change makes the code faster, because there is no function call overhead on the fast path. (`scan_escape` is a big function and doesn't get inlined.) This change also improves readability, because it removes a bunch of mode checks on the the fast paths. --- compiler/rustc_lexer/src/unescape.rs | 45 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_lexer/src/unescape.rs b/compiler/rustc_lexer/src/unescape.rs index d789237e692d2..97f9588ae1ef5 100644 --- a/compiler/rustc_lexer/src/unescape.rs +++ b/compiler/rustc_lexer/src/unescape.rs @@ -159,26 +159,8 @@ impl Mode { } } -fn scan_escape(first_char: char, chars: &mut Chars<'_>, mode: Mode) -> Result { - if first_char != '\\' { - // Previous character was not a slash, and we don't expect it to be - // an escape-only character. - return match first_char { - '\t' | '\n' => Err(EscapeError::EscapeOnlyChar), - '\r' => Err(EscapeError::BareCarriageReturn), - '\'' if mode.in_single_quotes() => Err(EscapeError::EscapeOnlyChar), - '"' if mode.in_double_quotes() => Err(EscapeError::EscapeOnlyChar), - _ => { - if mode.is_bytes() && !first_char.is_ascii() { - // Byte literal can't be a non-ascii character. - return Err(EscapeError::NonAsciiCharInByte); - } - Ok(first_char) - } - }; - } - - // Previous character is '\\', try to unescape it. +fn scan_escape(chars: &mut Chars<'_>, mode: Mode) -> Result { + // Previous character was '\\', unescape what follows. let second_char = chars.next().ok_or(EscapeError::LoneSlash)?; @@ -270,9 +252,24 @@ fn scan_escape(first_char: char, chars: &mut Chars<'_>, mode: Mode) -> Result Result { + if mode.is_bytes() && !first_char.is_ascii() { + // Byte literal can't be a non-ascii character. + Err(EscapeError::NonAsciiCharInByte) + } else { + Ok(first_char) + } +} + fn unescape_char_or_byte(chars: &mut Chars<'_>, mode: Mode) -> Result { let first_char = chars.next().ok_or(EscapeError::ZeroChars)?; - let res = scan_escape(first_char, chars, mode)?; + let res = match first_char { + '\\' => scan_escape(chars, mode), + '\n' | '\t' | '\'' => Err(EscapeError::EscapeOnlyChar), + '\r' => Err(EscapeError::BareCarriageReturn), + _ => ascii_check(first_char, mode), + }?; if chars.next().is_some() { return Err(EscapeError::MoreThanOneChar); } @@ -303,12 +300,14 @@ where skip_ascii_whitespace(&mut chars, start, callback); continue; } - _ => scan_escape(first_char, &mut chars, mode), + _ => scan_escape(&mut chars, mode), } } '\n' => Ok('\n'), '\t' => Ok('\t'), - _ => scan_escape(first_char, &mut chars, mode), + '"' => Err(EscapeError::EscapeOnlyChar), + '\r' => Err(EscapeError::BareCarriageReturn), + _ => ascii_check(first_char, mode), }; let end = initial_len - chars.as_str().len(); callback(start..end, unescaped_char); From 44308dc3489e39958b2ce6dd297b895514b6f425 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 24 Feb 2022 16:49:37 +1100 Subject: [PATCH 10/18] Inline a hot closure in `from_lit_token`. The change looks big because `rustfmt` rearranges things, but the only real change is the inlining annotation. --- compiler/rustc_ast/src/lib.rs | 1 + compiler/rustc_ast/src/util/literal.rs | 39 +++++++++++++++----------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_ast/src/lib.rs b/compiler/rustc_ast/src/lib.rs index 84fe9ad26720e..21183121e15a0 100644 --- a/compiler/rustc_ast/src/lib.rs +++ b/compiler/rustc_ast/src/lib.rs @@ -16,6 +16,7 @@ #![feature(min_specialization)] #![recursion_limit = "256"] #![feature(slice_internals)] +#![feature(stmt_expr_attributes)] #[macro_use] extern crate rustc_macros; diff --git a/compiler/rustc_ast/src/util/literal.rs b/compiler/rustc_ast/src/util/literal.rs index 1cc5ddfd8ee29..224afbd553fb8 100644 --- a/compiler/rustc_ast/src/util/literal.rs +++ b/compiler/rustc_ast/src/util/literal.rs @@ -56,25 +56,30 @@ impl LitKind { // new symbol because the string in the LitKind is different to the // string in the token. let s = symbol.as_str(); - let symbol = - if s.contains(&['\\', '\r']) { - let mut buf = String::with_capacity(s.len()); - let mut error = Ok(()); - unescape_literal(&s, Mode::Str, &mut |_, unescaped_char| { - match unescaped_char { - Ok(c) => buf.push(c), - Err(err) => { - if err.is_fatal() { - error = Err(LitError::LexerError); - } + let symbol = if s.contains(&['\\', '\r']) { + let mut buf = String::with_capacity(s.len()); + let mut error = Ok(()); + // Force-inlining here is aggressive but the closure is + // called on every char in the string, so it can be + // hot in programs with many long strings. + unescape_literal( + &s, + Mode::Str, + &mut #[inline(always)] + |_, unescaped_char| match unescaped_char { + Ok(c) => buf.push(c), + Err(err) => { + if err.is_fatal() { + error = Err(LitError::LexerError); } } - }); - error?; - Symbol::intern(&buf) - } else { - symbol - }; + }, + ); + error?; + Symbol::intern(&buf) + } else { + symbol + }; LitKind::Str(symbol, ast::StrStyle::Cooked) } token::StrRaw(n) => { From 70018c19cb34679e6de8b18f84c264abe280f28a Mon Sep 17 00:00:00 2001 From: lcnr Date: Thu, 24 Feb 2022 08:36:29 +0100 Subject: [PATCH 11/18] update auto trait lint --- compiler/rustc_typeck/src/coherence/orphan.rs | 1 + .../ui/auto-traits/suspicious-impls-lint.rs | 10 +++++++ .../auto-traits/suspicious-impls-lint.stderr | 29 ++++++++++++++----- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_typeck/src/coherence/orphan.rs b/compiler/rustc_typeck/src/coherence/orphan.rs index 54fffeb3cdaa6..811136a7eb4ee 100644 --- a/compiler/rustc_typeck/src/coherence/orphan.rs +++ b/compiler/rustc_typeck/src/coherence/orphan.rs @@ -439,6 +439,7 @@ fn fast_reject_auto_impl<'tcx>(tcx: TyCtxt<'tcx>, trait_def_id: DefId, self_ty: } match t.kind() { + ty::Adt(def, substs) if def.is_phantom_data() => substs.super_visit_with(self), ty::Adt(def, substs) => { // @lcnr: This is the only place where cycles can happen. We avoid this // by only visiting each `DefId` once. diff --git a/src/test/ui/auto-traits/suspicious-impls-lint.rs b/src/test/ui/auto-traits/suspicious-impls-lint.rs index 1026a35a455ac..1574a7e02e97f 100644 --- a/src/test/ui/auto-traits/suspicious-impls-lint.rs +++ b/src/test/ui/auto-traits/suspicious-impls-lint.rs @@ -1,5 +1,7 @@ #![deny(suspicious_auto_trait_impls)] +use std::marker::PhantomData; + struct MayImplementSendOk(T); unsafe impl Send for MayImplementSendOk {} // ok @@ -31,4 +33,12 @@ unsafe impl Send for TwoParamsSame {} //~^ ERROR //~| WARNING this will change its meaning +pub struct WithPhantomDataNonSend(PhantomData<*const T>, U); +unsafe impl Send for WithPhantomDataNonSend {} // ok + +pub struct WithPhantomDataSend(PhantomData, U); +unsafe impl Send for WithPhantomDataSend<*const T, i8> {} +//~^ ERROR +//~| WARNING this will change its meaning + fn main() {} diff --git a/src/test/ui/auto-traits/suspicious-impls-lint.stderr b/src/test/ui/auto-traits/suspicious-impls-lint.stderr index f91aa862271d3..084bfef49c029 100644 --- a/src/test/ui/auto-traits/suspicious-impls-lint.stderr +++ b/src/test/ui/auto-traits/suspicious-impls-lint.stderr @@ -1,5 +1,5 @@ error: cross-crate traits with a default impl, like `Send`, should not be specialized - --> $DIR/suspicious-impls-lint.rs:7:1 + --> $DIR/suspicious-impls-lint.rs:9:1 | LL | unsafe impl Send for MayImplementSendErr<&T> {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -12,14 +12,14 @@ LL | #![deny(suspicious_auto_trait_impls)] = warning: this will change its meaning in a future release! = note: for more information, see issue #93367 note: try using the same sequence of generic parameters as the struct definition - --> $DIR/suspicious-impls-lint.rs:6:1 + --> $DIR/suspicious-impls-lint.rs:8:1 | LL | struct MayImplementSendErr(T); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: `&T` is not a generic parameter error: cross-crate traits with a default impl, like `Send`, should not be specialized - --> $DIR/suspicious-impls-lint.rs:19:1 + --> $DIR/suspicious-impls-lint.rs:21:1 | LL | unsafe impl Send for ContainsVec {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -27,14 +27,14 @@ LL | unsafe impl Send for ContainsVec {} = warning: this will change its meaning in a future release! = note: for more information, see issue #93367 note: try using the same sequence of generic parameters as the struct definition - --> $DIR/suspicious-impls-lint.rs:18:1 + --> $DIR/suspicious-impls-lint.rs:20:1 | LL | struct ContainsVec(Vec); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: `i32` is not a generic parameter error: cross-crate traits with a default impl, like `Send`, should not be specialized - --> $DIR/suspicious-impls-lint.rs:30:1 + --> $DIR/suspicious-impls-lint.rs:32:1 | LL | unsafe impl Send for TwoParamsSame {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -42,11 +42,26 @@ LL | unsafe impl Send for TwoParamsSame {} = warning: this will change its meaning in a future release! = note: for more information, see issue #93367 note: try using the same sequence of generic parameters as the struct definition - --> $DIR/suspicious-impls-lint.rs:29:1 + --> $DIR/suspicious-impls-lint.rs:31:1 | LL | struct TwoParamsSame(T, U); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: `T` is mentioned multiple times -error: aborting due to 3 previous errors +error: cross-crate traits with a default impl, like `Send`, should not be specialized + --> $DIR/suspicious-impls-lint.rs:40:1 + | +LL | unsafe impl Send for WithPhantomDataSend<*const T, i8> {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = warning: this will change its meaning in a future release! + = note: for more information, see issue #93367 +note: try using the same sequence of generic parameters as the struct definition + --> $DIR/suspicious-impls-lint.rs:39:1 + | +LL | pub struct WithPhantomDataSend(PhantomData, U); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `*const T` is not a generic parameter + +error: aborting due to 4 previous errors From 34319ff4e1d4b50a2406c03df0befe15362b4227 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Thu, 24 Feb 2022 11:16:45 -0500 Subject: [PATCH 12/18] Avoid emitting full macro body into JSON --- compiler/rustc_errors/src/json.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_errors/src/json.rs b/compiler/rustc_errors/src/json.rs index ff3478073d92b..dc28d1bb45234 100644 --- a/compiler/rustc_errors/src/json.rs +++ b/compiler/rustc_errors/src/json.rs @@ -454,8 +454,14 @@ impl DiagnosticSpan { let end = je.sm.lookup_char_pos(span.hi()); let backtrace_step = backtrace.next().map(|bt| { let call_site = Self::from_span_full(bt.call_site, false, None, None, backtrace, je); - let def_site_span = - Self::from_span_full(bt.def_site, false, None, None, [].into_iter(), je); + let def_site_span = Self::from_span_full( + je.sm.guess_head_span(bt.def_site), + false, + None, + None, + [].into_iter(), + je, + ); Box::new(DiagnosticSpanMacroExpansion { span: call_site, macro_decl_name: bt.kind.descr(), From 8ba74369c210a0cdb1b1440c47f19f2145e1640f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 6 Feb 2022 13:03:28 -0800 Subject: [PATCH 13/18] better ObligationCause for normalization errors in can_type_implement_copy --- compiler/rustc_lint/src/builtin.rs | 11 +++++-- .../rustc_trait_selection/src/traits/misc.rs | 4 +-- .../rustc_typeck/src/coherence/builtin.rs | 3 +- src/test/ui/issues/issue-50480.rs | 4 +-- src/test/ui/issues/issue-50480.stderr | 30 ++++++++++--------- .../ui/traits/copy-impl-cannot-normalize.rs | 25 ++++++++++++++++ .../traits/copy-impl-cannot-normalize.stderr | 14 +++++++++ .../src/needless_pass_by_value.rs | 2 +- 8 files changed, 71 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/traits/copy-impl-cannot-normalize.rs create mode 100644 src/test/ui/traits/copy-impl-cannot-normalize.stderr diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs index 30b5f9b34d099..0a218c2d25584 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -51,7 +51,7 @@ use rustc_span::source_map::Spanned; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{BytePos, InnerSpan, MultiSpan, Span}; use rustc_target::abi::VariantIdx; -use rustc_trait_selection::traits::misc::can_type_implement_copy; +use rustc_trait_selection::traits::{self, misc::can_type_implement_copy}; use crate::nonstandard_style::{method_context, MethodLateContext}; @@ -764,7 +764,14 @@ impl<'tcx> LateLintPass<'tcx> for MissingCopyImplementations { if ty.is_copy_modulo_regions(cx.tcx.at(item.span), param_env) { return; } - if can_type_implement_copy(cx.tcx, param_env, ty).is_ok() { + if can_type_implement_copy( + cx.tcx, + param_env, + ty, + traits::ObligationCause::misc(item.span, item.hir_id()), + ) + .is_ok() + { cx.struct_span_lint(MISSING_COPY_IMPLEMENTATIONS, item.span, |lint| { lint.build( "type could implement `Copy`; consider adding `impl \ diff --git a/compiler/rustc_trait_selection/src/traits/misc.rs b/compiler/rustc_trait_selection/src/traits/misc.rs index b23dce8a58130..dc72945af24e3 100644 --- a/compiler/rustc_trait_selection/src/traits/misc.rs +++ b/compiler/rustc_trait_selection/src/traits/misc.rs @@ -20,6 +20,7 @@ pub fn can_type_implement_copy<'tcx>( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, self_type: Ty<'tcx>, + cause: ObligationCause<'tcx>, ) -> Result<(), CopyImplementationError<'tcx>> { // FIXME: (@jroesch) float this code up tcx.infer_ctxt().enter(|infcx| { @@ -49,9 +50,8 @@ pub fn can_type_implement_copy<'tcx>( continue; } let span = tcx.def_span(field.did); - let cause = ObligationCause::dummy_with_span(span); let ctx = traits::FulfillmentContext::new(); - match traits::fully_normalize(&infcx, ctx, cause, param_env, ty) { + match traits::fully_normalize(&infcx, ctx, cause.clone(), param_env, ty) { Ok(ty) => { if !infcx.type_is_copy_modulo_regions(param_env, ty, span) { infringing.push(field); diff --git a/compiler/rustc_typeck/src/coherence/builtin.rs b/compiler/rustc_typeck/src/coherence/builtin.rs index 401ba188728c1..a43f7f871167e 100644 --- a/compiler/rustc_typeck/src/coherence/builtin.rs +++ b/compiler/rustc_typeck/src/coherence/builtin.rs @@ -74,7 +74,8 @@ fn visit_implementation_of_copy(tcx: TyCtxt<'_>, impl_did: LocalDefId) { debug!("visit_implementation_of_copy: self_type={:?} (free)", self_type); - match can_type_implement_copy(tcx, param_env, self_type) { + let cause = traits::ObligationCause::misc(span, impl_hir_id); + match can_type_implement_copy(tcx, param_env, self_type, cause) { Ok(()) => {} Err(CopyImplementationError::InfrigingFields(fields)) => { let item = tcx.hir().expect_item(impl_did); diff --git a/src/test/ui/issues/issue-50480.rs b/src/test/ui/issues/issue-50480.rs index 10597caf5b2dc..ca81db023ec3b 100644 --- a/src/test/ui/issues/issue-50480.rs +++ b/src/test/ui/issues/issue-50480.rs @@ -1,17 +1,17 @@ #[derive(Clone, Copy)] //~^ ERROR the trait `Copy` may not be implemented for this type +//~| ERROR `i32` is not an iterator struct Foo(N, NotDefined, ::Item, Vec, String); //~^ ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `N` in this scope //~| ERROR cannot find type `N` in this scope -//~| ERROR `i32` is not an iterator #[derive(Clone, Copy)] //~^ ERROR the trait `Copy` may not be implemented for this type +//~| ERROR `i32` is not an iterator struct Bar(T, N, NotDefined, ::Item, Vec, String); //~^ ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `N` in this scope -//~| ERROR `i32` is not an iterator fn main() {} diff --git a/src/test/ui/issues/issue-50480.stderr b/src/test/ui/issues/issue-50480.stderr index 0bb1f9ae03500..48ec4aa434cd7 100644 --- a/src/test/ui/issues/issue-50480.stderr +++ b/src/test/ui/issues/issue-50480.stderr @@ -1,5 +1,5 @@ error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:3:12 + --> $DIR/issue-50480.rs:4:12 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -^ not found in this scope @@ -7,13 +7,13 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:3:15 + --> $DIR/issue-50480.rs:4:15 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | ^^^^^^^^^^ not found in this scope error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:3:12 + --> $DIR/issue-50480.rs:4:12 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -^ not found in this scope @@ -21,7 +21,7 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:3:15 + --> $DIR/issue-50480.rs:4:15 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | - ^^^^^^^^^^ not found in this scope @@ -29,7 +29,7 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:12:18 + --> $DIR/issue-50480.rs:13:18 | LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | - ^ @@ -46,26 +46,27 @@ LL | struct Bar(T, N, NotDefined, ::Item, Vec, Strin | +++ error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:12:21 + --> $DIR/issue-50480.rs:13:21 | LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | ^^^^^^^^^^ not found in this scope error[E0277]: `i32` is not an iterator - --> $DIR/issue-50480.rs:3:27 + --> $DIR/issue-50480.rs:1:17 | -LL | struct Foo(N, NotDefined, ::Item, Vec, String); - | ^^^^^^^^^^^^^^^^^^^^^^^ `i32` is not an iterator +LL | #[derive(Clone, Copy)] + | ^^^^ `i32` is not an iterator | = help: the trait `Iterator` is not implemented for `i32` = note: if you want to iterate between `start` until a value `end`, use the exclusive range syntax `start..end` or the inclusive range syntax `start..=end` + = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0204]: the trait `Copy` may not be implemented for this type --> $DIR/issue-50480.rs:1:17 | LL | #[derive(Clone, Copy)] | ^^^^ -LL | +... LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -------- ------ this field does not implement `Copy` | | @@ -74,20 +75,21 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: `i32` is not an iterator - --> $DIR/issue-50480.rs:12:33 + --> $DIR/issue-50480.rs:10:17 | -LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); - | ^^^^^^^^^^^^^^^^^^^^^^^ `i32` is not an iterator +LL | #[derive(Clone, Copy)] + | ^^^^ `i32` is not an iterator | = help: the trait `Iterator` is not implemented for `i32` = note: if you want to iterate between `start` until a value `end`, use the exclusive range syntax `start..end` or the inclusive range syntax `start..=end` + = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0204]: the trait `Copy` may not be implemented for this type --> $DIR/issue-50480.rs:10:17 | LL | #[derive(Clone, Copy)] | ^^^^ -LL | +... LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | -------- ------ this field does not implement `Copy` | | diff --git a/src/test/ui/traits/copy-impl-cannot-normalize.rs b/src/test/ui/traits/copy-impl-cannot-normalize.rs new file mode 100644 index 0000000000000..a78ff046e97f9 --- /dev/null +++ b/src/test/ui/traits/copy-impl-cannot-normalize.rs @@ -0,0 +1,25 @@ +trait TraitFoo { + type Bar; +} + +struct Foo +where + T: TraitFoo, +{ + inner: T::Bar, +} + +impl Clone for Foo +where + T: TraitFoo, + T::Bar: Clone, +{ + fn clone(&self) -> Self { + Self { inner: self.inner.clone() } + } +} + +impl Copy for Foo {} +//~^ ERROR the trait bound `T: TraitFoo` is not satisfied + +fn main() {} diff --git a/src/test/ui/traits/copy-impl-cannot-normalize.stderr b/src/test/ui/traits/copy-impl-cannot-normalize.stderr new file mode 100644 index 0000000000000..cc540ea905a10 --- /dev/null +++ b/src/test/ui/traits/copy-impl-cannot-normalize.stderr @@ -0,0 +1,14 @@ +error[E0277]: the trait bound `T: TraitFoo` is not satisfied + --> $DIR/copy-impl-cannot-normalize.rs:22:1 + | +LL | impl Copy for Foo {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `TraitFoo` is not implemented for `T` + | +help: consider restricting type parameter `T` + | +LL | impl Copy for Foo {} + | ++++++++++ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index ebd4fb0bf51cc..d27e1383d012b 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -199,7 +199,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { let sugg = |diag: &mut DiagnosticBuilder<'_>| { if let ty::Adt(def, ..) = ty.kind() { if let Some(span) = cx.tcx.hir().span_if_local(def.did) { - if can_type_implement_copy(cx.tcx, cx.param_env, ty).is_ok() { + if can_type_implement_copy(cx.tcx, cx.param_env, ty, traits::ObligationCause::dummy_with_span(span)).is_ok() { diag.span_help(span, "consider marking this type as `Copy`"); } } From ee98dc8b3bdff99591630ca225f4769201100603 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 6 Feb 2022 15:57:29 -0800 Subject: [PATCH 14/18] restore spans for issue-50480 --- .../rustc_trait_selection/src/traits/misc.rs | 15 +++++++++- src/test/ui/issues/issue-50480.rs | 4 +-- src/test/ui/issues/issue-50480.stderr | 30 +++++++++---------- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/misc.rs b/compiler/rustc_trait_selection/src/traits/misc.rs index dc72945af24e3..c293708dcc929 100644 --- a/compiler/rustc_trait_selection/src/traits/misc.rs +++ b/compiler/rustc_trait_selection/src/traits/misc.rs @@ -50,8 +50,21 @@ pub fn can_type_implement_copy<'tcx>( continue; } let span = tcx.def_span(field.did); + // FIXME(compiler-errors): This gives us better spans for bad + // projection types like in issue-50480. + // If the ADT has substs, point to the cause we are given. + // If it does not, then this field probably doesn't normalize + // to begin with, and point to the bad field's span instead. + let cause = if field + .ty(tcx, traits::InternalSubsts::identity_for_item(tcx, adt.did)) + .has_param_types_or_consts() + { + cause.clone() + } else { + ObligationCause::dummy_with_span(span) + }; let ctx = traits::FulfillmentContext::new(); - match traits::fully_normalize(&infcx, ctx, cause.clone(), param_env, ty) { + match traits::fully_normalize(&infcx, ctx, cause, param_env, ty) { Ok(ty) => { if !infcx.type_is_copy_modulo_regions(param_env, ty, span) { infringing.push(field); diff --git a/src/test/ui/issues/issue-50480.rs b/src/test/ui/issues/issue-50480.rs index ca81db023ec3b..10597caf5b2dc 100644 --- a/src/test/ui/issues/issue-50480.rs +++ b/src/test/ui/issues/issue-50480.rs @@ -1,17 +1,17 @@ #[derive(Clone, Copy)] //~^ ERROR the trait `Copy` may not be implemented for this type -//~| ERROR `i32` is not an iterator struct Foo(N, NotDefined, ::Item, Vec, String); //~^ ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `N` in this scope //~| ERROR cannot find type `N` in this scope +//~| ERROR `i32` is not an iterator #[derive(Clone, Copy)] //~^ ERROR the trait `Copy` may not be implemented for this type -//~| ERROR `i32` is not an iterator struct Bar(T, N, NotDefined, ::Item, Vec, String); //~^ ERROR cannot find type `NotDefined` in this scope //~| ERROR cannot find type `N` in this scope +//~| ERROR `i32` is not an iterator fn main() {} diff --git a/src/test/ui/issues/issue-50480.stderr b/src/test/ui/issues/issue-50480.stderr index 48ec4aa434cd7..0bb1f9ae03500 100644 --- a/src/test/ui/issues/issue-50480.stderr +++ b/src/test/ui/issues/issue-50480.stderr @@ -1,5 +1,5 @@ error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:4:12 + --> $DIR/issue-50480.rs:3:12 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -^ not found in this scope @@ -7,13 +7,13 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:4:15 + --> $DIR/issue-50480.rs:3:15 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | ^^^^^^^^^^ not found in this scope error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:4:12 + --> $DIR/issue-50480.rs:3:12 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -^ not found in this scope @@ -21,7 +21,7 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:4:15 + --> $DIR/issue-50480.rs:3:15 | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | - ^^^^^^^^^^ not found in this scope @@ -29,7 +29,7 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); | help: you might be missing a type parameter: `` error[E0412]: cannot find type `N` in this scope - --> $DIR/issue-50480.rs:13:18 + --> $DIR/issue-50480.rs:12:18 | LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | - ^ @@ -46,27 +46,26 @@ LL | struct Bar(T, N, NotDefined, ::Item, Vec, Strin | +++ error[E0412]: cannot find type `NotDefined` in this scope - --> $DIR/issue-50480.rs:13:21 + --> $DIR/issue-50480.rs:12:21 | LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | ^^^^^^^^^^ not found in this scope error[E0277]: `i32` is not an iterator - --> $DIR/issue-50480.rs:1:17 + --> $DIR/issue-50480.rs:3:27 | -LL | #[derive(Clone, Copy)] - | ^^^^ `i32` is not an iterator +LL | struct Foo(N, NotDefined, ::Item, Vec, String); + | ^^^^^^^^^^^^^^^^^^^^^^^ `i32` is not an iterator | = help: the trait `Iterator` is not implemented for `i32` = note: if you want to iterate between `start` until a value `end`, use the exclusive range syntax `start..end` or the inclusive range syntax `start..=end` - = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0204]: the trait `Copy` may not be implemented for this type --> $DIR/issue-50480.rs:1:17 | LL | #[derive(Clone, Copy)] | ^^^^ -... +LL | LL | struct Foo(N, NotDefined, ::Item, Vec, String); | -------- ------ this field does not implement `Copy` | | @@ -75,21 +74,20 @@ LL | struct Foo(N, NotDefined, ::Item, Vec, String); = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: `i32` is not an iterator - --> $DIR/issue-50480.rs:10:17 + --> $DIR/issue-50480.rs:12:33 | -LL | #[derive(Clone, Copy)] - | ^^^^ `i32` is not an iterator +LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); + | ^^^^^^^^^^^^^^^^^^^^^^^ `i32` is not an iterator | = help: the trait `Iterator` is not implemented for `i32` = note: if you want to iterate between `start` until a value `end`, use the exclusive range syntax `start..end` or the inclusive range syntax `start..=end` - = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0204]: the trait `Copy` may not be implemented for this type --> $DIR/issue-50480.rs:10:17 | LL | #[derive(Clone, Copy)] | ^^^^ -... +LL | LL | struct Bar(T, N, NotDefined, ::Item, Vec, String); | -------- ------ this field does not implement `Copy` | | From 17b1afdbb234a1cdf5db92ec8639cbd2909ac629 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 11 Dec 2021 19:52:23 +0800 Subject: [PATCH 15/18] resolve: Fix incorrect results of `opt_def_kind` query for some built-in macros Previously it always returned `MacroKind::Bang` while some of those macros are actually attributes and derives --- compiler/rustc_ast_lowering/src/item.rs | 4 ++-- compiler/rustc_ast_lowering/src/lib.rs | 4 +++- compiler/rustc_hir/src/hir.rs | 3 ++- compiler/rustc_hir/src/intravisit.rs | 2 +- compiler/rustc_hir_pretty/src/lib.rs | 2 +- compiler/rustc_metadata/src/rmeta/encoder.rs | 2 +- compiler/rustc_middle/src/hir/map/mod.rs | 3 +-- compiler/rustc_passes/src/check_attr.rs | 2 +- compiler/rustc_privacy/src/lib.rs | 4 ++-- compiler/rustc_resolve/src/lib.rs | 8 ++++++++ compiler/rustc_resolve/src/macros.rs | 8 +++++++- compiler/rustc_save_analysis/src/sig.rs | 2 +- compiler/rustc_typeck/src/collect.rs | 2 +- src/librustdoc/clean/mod.rs | 2 +- src/librustdoc/doctest.rs | 2 +- src/librustdoc/visit_ast.rs | 2 +- src/tools/clippy/clippy_lints/src/utils/inspector.rs | 2 +- 17 files changed, 35 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs index a86333365128d..3ddc7fce1b770 100644 --- a/compiler/rustc_ast_lowering/src/item.rs +++ b/compiler/rustc_ast_lowering/src/item.rs @@ -444,8 +444,8 @@ impl<'hir> LoweringContext<'_, 'hir> { ), ItemKind::MacroDef(MacroDef { ref body, macro_rules }) => { let body = P(self.lower_mac_args(body)); - - hir::ItemKind::Macro(ast::MacroDef { body, macro_rules }) + let macro_kind = self.resolver.decl_macro_kind(self.resolver.local_def_id(id)); + hir::ItemKind::Macro(ast::MacroDef { body, macro_rules }, macro_kind) } ItemKind::MacCall(..) => { panic!("`TyMac` should have been expanded by now") diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs index 0a59e3c2e3f82..0156c5016acd1 100644 --- a/compiler/rustc_ast_lowering/src/lib.rs +++ b/compiler/rustc_ast_lowering/src/lib.rs @@ -61,7 +61,7 @@ use rustc_session::lint::LintBuffer; use rustc_session::parse::feature_err; use rustc_session::utils::{FlattenNonterminals, NtToTokenstream}; use rustc_session::Session; -use rustc_span::hygiene::ExpnId; +use rustc_span::hygiene::{ExpnId, MacroKind}; use rustc_span::source_map::{respan, DesugaringKind}; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{Span, DUMMY_SP}; @@ -210,6 +210,8 @@ pub trait ResolverAstLowering { expn_id: ExpnId, span: Span, ) -> LocalDefId; + + fn decl_macro_kind(&self, def_id: LocalDefId) -> MacroKind; } /// Context of `impl Trait` in code, which determines whether it is allowed in an HIR subtree, diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index d48c8e81f540f..72c02932945ca 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -15,6 +15,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::sorted_map::SortedMap; use rustc_index::vec::IndexVec; use rustc_macros::HashStable_Generic; +use rustc_span::hygiene::MacroKind; use rustc_span::source_map::Spanned; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{def_id::LocalDefId, BytePos, MultiSpan, Span, DUMMY_SP}; @@ -2803,7 +2804,7 @@ pub enum ItemKind<'hir> { /// A function declaration. Fn(FnSig<'hir>, Generics<'hir>, BodyId), /// A MBE macro definition (`macro_rules!` or `macro`). - Macro(ast::MacroDef), + Macro(ast::MacroDef, MacroKind), /// A module. Mod(Mod<'hir>), /// An external module, e.g. `extern { .. }`. diff --git a/compiler/rustc_hir/src/intravisit.rs b/compiler/rustc_hir/src/intravisit.rs index c55f2a7b03941..1b40f3d390ee5 100644 --- a/compiler/rustc_hir/src/intravisit.rs +++ b/compiler/rustc_hir/src/intravisit.rs @@ -575,7 +575,7 @@ pub fn walk_item<'v, V: Visitor<'v>>(visitor: &mut V, item: &'v Item<'v>) { item.span, item.hir_id(), ), - ItemKind::Macro(_) => { + ItemKind::Macro(..) => { visitor.visit_id(item.hir_id()); } ItemKind::Mod(ref module) => { diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs index 8e45b636f47f7..b3042c61002c4 100644 --- a/compiler/rustc_hir_pretty/src/lib.rs +++ b/compiler/rustc_hir_pretty/src/lib.rs @@ -570,7 +570,7 @@ impl<'a> State<'a> { self.end(); // need to close a box self.ann.nested(self, Nested::Body(body)); } - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { self.print_mac_def(macro_def, &item.ident, item.span, |state| { state.print_visibility(&item.vis) }); diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index da8995df1ac9e..85b1b31ba84b1 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -1406,7 +1406,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> { EntryKind::Fn(self.lazy(data)) } - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { EntryKind::MacroDef(self.lazy(macro_def.clone())) } hir::ItemKind::Mod(ref m) => { diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs index f36847c778109..ec20e888333da 100644 --- a/compiler/rustc_middle/src/hir/map/mod.rs +++ b/compiler/rustc_middle/src/hir/map/mod.rs @@ -14,7 +14,6 @@ use rustc_hir::*; use rustc_index::vec::Idx; use rustc_middle::hir::nested_filter; use rustc_span::def_id::StableCrateId; -use rustc_span::hygiene::MacroKind; use rustc_span::source_map::Spanned; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::Span; @@ -232,7 +231,7 @@ impl<'hir> Map<'hir> { ItemKind::Static(..) => DefKind::Static, ItemKind::Const(..) => DefKind::Const, ItemKind::Fn(..) => DefKind::Fn, - ItemKind::Macro(..) => DefKind::Macro(MacroKind::Bang), + ItemKind::Macro(_, macro_kind) => DefKind::Macro(macro_kind), ItemKind::Mod(..) => DefKind::Mod, ItemKind::OpaqueTy(..) => DefKind::OpaqueTy, ItemKind::TyAlias(..) => DefKind::TyAlias, diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index 3d69e8ba4e430..b545961245ad7 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -1951,7 +1951,7 @@ impl<'tcx> Visitor<'tcx> for CheckAttrVisitor<'tcx> { // Historically we've run more checks on non-exported than exported macros, // so this lets us continue to run them while maintaining backwards compatibility. // In the long run, the checks should be harmonized. - if let ItemKind::Macro(ref macro_def) = item.kind { + if let ItemKind::Macro(ref macro_def, _) = item.kind { let def_id = item.def_id.to_def_id(); if macro_def.macro_rules && !self.tcx.has_attr(def_id, sym::macro_export) { check_non_exported_macro_for_invalid_attrs(self.tcx, item); diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index 48594e73f5b83..3dd9995fa0081 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -564,7 +564,7 @@ impl<'tcx> EmbargoVisitor<'tcx> { // privacy and mark them reachable. DefKind::Macro(_) => { let item = self.tcx.hir().expect_item(def_id); - if let hir::ItemKind::Macro(MacroDef { macro_rules: false, .. }) = item.kind { + if let hir::ItemKind::Macro(MacroDef { macro_rules: false, .. }, _) = item.kind { if vis.is_accessible_from(module.to_def_id(), self.tcx) { self.update(def_id, level); } @@ -686,7 +686,7 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> { } } } - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { self.update_reachability_from_macro(item.def_id, macro_def); } hir::ItemKind::ForeignMod { items, .. } => { diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 04b0a18b12b62..2fb69e438c403 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -990,6 +990,9 @@ pub struct Resolver<'a> { crate_loader: CrateLoader<'a>, macro_names: FxHashSet, builtin_macros: FxHashMap, + /// A small map keeping true kinds of built-in macros that appear to be fn-like on + /// the surface (`macro` items in libcore), but are actually attributes or derives. + builtin_macro_kinds: FxHashMap, registered_attrs: FxHashSet, registered_tools: RegisteredTools, macro_use_prelude: FxHashMap>, @@ -1261,6 +1264,10 @@ impl ResolverAstLowering for Resolver<'_> { def_id } + + fn decl_macro_kind(&self, def_id: LocalDefId) -> MacroKind { + self.builtin_macro_kinds.get(&def_id).copied().unwrap_or(MacroKind::Bang) + } } impl<'a> Resolver<'a> { @@ -1381,6 +1388,7 @@ impl<'a> Resolver<'a> { crate_loader: CrateLoader::new(session, metadata_loader, crate_name), macro_names: FxHashSet::default(), builtin_macros: Default::default(), + builtin_macro_kinds: Default::default(), registered_attrs, registered_tools, macro_use_prelude: FxHashMap::default(), diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 89c2a0c74bd36..e34d3e605ecdf 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -1209,7 +1209,13 @@ impl<'a> Resolver<'a> { // while still taking everything else from the source code. // If we already loaded this builtin macro, give a better error message than 'no such builtin macro'. match mem::replace(builtin_macro, BuiltinMacroState::AlreadySeen(item.span)) { - BuiltinMacroState::NotYetSeen(ext) => result.kind = ext, + BuiltinMacroState::NotYetSeen(ext) => { + result.kind = ext; + if item.id != ast::DUMMY_NODE_ID { + self.builtin_macro_kinds + .insert(self.local_def_id(item.id), result.macro_kind()); + } + } BuiltinMacroState::AlreadySeen(span) => { struct_span_err!( self.session, diff --git a/compiler/rustc_save_analysis/src/sig.rs b/compiler/rustc_save_analysis/src/sig.rs index 3bb1d2ff35730..8f50f44571953 100644 --- a/compiler/rustc_save_analysis/src/sig.rs +++ b/compiler/rustc_save_analysis/src/sig.rs @@ -416,7 +416,7 @@ impl<'hir> Sig for hir::Item<'hir> { Ok(sig) } - hir::ItemKind::Macro(_) => { + hir::ItemKind::Macro(..) => { let mut text = "macro".to_owned(); let name = self.ident.to_string(); text.push_str(&name); diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs index 392144ca7639c..4a25b49eb2dda 100644 --- a/compiler/rustc_typeck/src/collect.rs +++ b/compiler/rustc_typeck/src/collect.rs @@ -730,7 +730,7 @@ fn convert_item(tcx: TyCtxt<'_>, item_id: hir::ItemId) { // These don't define types. hir::ItemKind::ExternCrate(_) | hir::ItemKind::Use(..) - | hir::ItemKind::Macro(_) + | hir::ItemKind::Macro(..) | hir::ItemKind::Mod(_) | hir::ItemKind::GlobalAsm(_) => {} hir::ItemKind::ForeignMod { items, .. } => { diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 76994f2ee1712..1e0c1e8f1f356 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -1855,7 +1855,7 @@ fn clean_maybe_renamed_item( ItemKind::Fn(ref sig, ref generics, body_id) => { clean_fn_or_proc_macro(item, sig, generics, body_id, &mut name, cx) } - ItemKind::Macro(ref macro_def) => { + ItemKind::Macro(ref macro_def, _) => { let ty_vis = cx.tcx.visibility(def_id).clean(cx); MacroItem(Macro { source: display_macro_source(cx, name, macro_def, def_id, ty_vis), diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 696397c5f671b..5ccc3dabe83b7 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -1164,7 +1164,7 @@ impl<'a, 'hir, 'tcx> intravisit::Visitor<'hir> for HirCollector<'a, 'hir, 'tcx> fn visit_item(&mut self, item: &'hir hir::Item<'_>) { let name = match &item.kind { - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { // FIXME(#88038): Non exported macros have historically not been tested, // but we really ought to start testing them. let def_id = item.def_id.to_def_id(); diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index e8b3a0929db61..1693034db0e82 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -325,7 +325,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { om.items.push((item, renamed)) } - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { // `#[macro_export] macro_rules!` items are handled seperately in `visit()`, // above, since they need to be documented at the module top level. Accordingly, // we only want to handle macros if one of three conditions holds: diff --git a/src/tools/clippy/clippy_lints/src/utils/inspector.rs b/src/tools/clippy/clippy_lints/src/utils/inspector.rs index 8691148313702..dc48ea3f4f99d 100644 --- a/src/tools/clippy/clippy_lints/src/utils/inspector.rs +++ b/src/tools/clippy/clippy_lints/src/utils/inspector.rs @@ -373,7 +373,7 @@ fn print_item(cx: &LateContext<'_>, item: &hir::Item<'_>) { let item_ty = cx.tcx.type_of(did); println!("function of type {:#?}", item_ty); }, - hir::ItemKind::Macro(ref macro_def) => { + hir::ItemKind::Macro(ref macro_def, _) => { if macro_def.macro_rules { println!("macro introduced by `macro_rules!`"); } else { From 50568b8ee5665e637e3fe1a481723ea2ec8a6d03 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 24 Jan 2022 15:30:54 +0800 Subject: [PATCH 16/18] metadata: Tweak the way in which declarative macros are encoded To make the `macro_rules` flag more readily available without decoding everything else --- compiler/rustc_metadata/src/rmeta/decoder.rs | 7 +++++-- compiler/rustc_metadata/src/rmeta/encoder.rs | 2 +- compiler/rustc_metadata/src/rmeta/mod.rs | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index aaa44a68dc0c0..71bd449d0c1cd 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -5,6 +5,7 @@ use crate::rmeta::table::{FixedSizeEncoding, Table}; use crate::rmeta::*; use rustc_ast as ast; +use rustc_ast::ptr::P; use rustc_attr as attr; use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxHashMap; @@ -1402,9 +1403,11 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { tcx.arena.alloc_from_iter(self.root.exported_symbols.decode((self, tcx))) } - fn get_macro(self, id: DefIndex, sess: &Session) -> MacroDef { + fn get_macro(self, id: DefIndex, sess: &Session) -> ast::MacroDef { match self.kind(id) { - EntryKind::MacroDef(macro_def) => macro_def.decode((self, sess)), + EntryKind::MacroDef(mac_args, macro_rules) => { + ast::MacroDef { body: P(mac_args.decode((self, sess))), macro_rules } + } _ => bug!(), } } diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index 85b1b31ba84b1..fae76f80c4bde 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -1407,7 +1407,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> { EntryKind::Fn(self.lazy(data)) } hir::ItemKind::Macro(ref macro_def, _) => { - EntryKind::MacroDef(self.lazy(macro_def.clone())) + EntryKind::MacroDef(self.lazy(&*macro_def.body), macro_def.macro_rules) } hir::ItemKind::Mod(ref m) => { return self.encode_info_for_mod(item.def_id, m); diff --git a/compiler/rustc_metadata/src/rmeta/mod.rs b/compiler/rustc_metadata/src/rmeta/mod.rs index da17d9d4c6706..a30cc034c4a96 100644 --- a/compiler/rustc_metadata/src/rmeta/mod.rs +++ b/compiler/rustc_metadata/src/rmeta/mod.rs @@ -2,7 +2,7 @@ use decoder::Metadata; use def_path_hash_map::DefPathHashMapRef; use table::{Table, TableBuilder}; -use rustc_ast::{self as ast, MacroDef}; +use rustc_ast as ast; use rustc_attr as attr; use rustc_data_structures::svh::Svh; use rustc_data_structures::sync::MetadataRef; @@ -350,7 +350,7 @@ enum EntryKind { Fn(Lazy), ForeignFn(Lazy), Mod(Lazy<[ModChild]>), - MacroDef(Lazy), + MacroDef(Lazy, /*macro_rules*/ bool), ProcMacro(MacroKind), Closure, Generator, From 179ce18c5c938417eb73629ebd8d90b3f745bc5c Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 24 Jan 2022 15:41:25 +0800 Subject: [PATCH 17/18] resolve/metadata: Stop encoding macros as reexports --- compiler/rustc_metadata/src/rmeta/decoder.rs | 31 ++++++++++++++----- compiler/rustc_middle/src/metadata.rs | 2 ++ compiler/rustc_resolve/src/access_levels.rs | 5 +-- .../rustc_resolve/src/build_reduced_graph.rs | 6 ++-- compiler/rustc_resolve/src/imports.rs | 22 ++++++++----- compiler/rustc_resolve/src/lib.rs | 4 --- 6 files changed, 47 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 71bd449d0c1cd..e5e0cce198f46 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -1077,6 +1077,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { res, vis: ty::Visibility::Public, span: ident.span, + macro_rules: false, }); } } @@ -1088,17 +1089,19 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { for child_index in children.decode((self, sess)) { if let Some(ident) = self.opt_item_ident(child_index, sess) { let kind = self.def_kind(child_index); - if matches!(kind, DefKind::Macro(..)) { - // FIXME: Macros are currently encoded twice, once as items and once as - // reexports. We ignore the items here and only use the reexports. - continue; - } let def_id = self.local_def_id(child_index); let res = Res::Def(kind, def_id); let vis = self.get_visibility(child_index); let span = self.get_span(child_index, sess); + let macro_rules = match kind { + DefKind::Macro(..) => match self.kind(child_index) { + EntryKind::MacroDef(_, macro_rules) => macro_rules, + _ => unreachable!(), + }, + _ => false, + }; - callback(ModChild { ident, res, vis, span }); + callback(ModChild { ident, res, vis, span, macro_rules }); // For non-re-export structs and variants add their constructors to children. // Re-export lists automatically contain constructors when necessary. @@ -1110,7 +1113,13 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { let ctor_res = Res::Def(DefKind::Ctor(CtorOf::Struct, ctor_kind), ctor_def_id); let vis = self.get_visibility(ctor_def_id.index); - callback(ModChild { ident, res: ctor_res, vis, span }); + callback(ModChild { + ident, + res: ctor_res, + vis, + span, + macro_rules: false, + }); } } DefKind::Variant => { @@ -1135,7 +1144,13 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { vis = ty::Visibility::Restricted(crate_def_id); } } - callback(ModChild { ident, res: ctor_res, vis, span }); + callback(ModChild { + ident, + res: ctor_res, + vis, + span, + macro_rules: false, + }); } _ => {} } diff --git a/compiler/rustc_middle/src/metadata.rs b/compiler/rustc_middle/src/metadata.rs index 6dcdc58c72d82..c8e78747d8e7b 100644 --- a/compiler/rustc_middle/src/metadata.rs +++ b/compiler/rustc_middle/src/metadata.rs @@ -21,4 +21,6 @@ pub struct ModChild { pub vis: ty::Visibility, /// Span of the item. pub span: Span, + /// A proper `macro_rules` item (not a reexport). + pub macro_rules: bool, } diff --git a/compiler/rustc_resolve/src/access_levels.rs b/compiler/rustc_resolve/src/access_levels.rs index 60cc4248edc9d..61a9b644cb8f5 100644 --- a/compiler/rustc_resolve/src/access_levels.rs +++ b/compiler/rustc_resolve/src/access_levels.rs @@ -133,7 +133,7 @@ impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> { ast::ItemKind::Impl(..) => return, // Only exported `macro_rules!` items are public, but they always are - ast::ItemKind::MacroDef(..) => { + ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => { let is_macro_export = item.attrs.iter().any(|attr| attr.has_name(sym::macro_export)); if is_macro_export { Some(AccessLevel::Public) } else { None } @@ -155,7 +155,8 @@ impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> { | ast::ItemKind::Struct(..) | ast::ItemKind::Union(..) | ast::ItemKind::Trait(..) - | ast::ItemKind::TraitAlias(..) => { + | ast::ItemKind::TraitAlias(..) + | ast::ItemKind::MacroDef(..) => { if item.vis.kind.is_pub() { self.prev_level } else { diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 3fa9343c399ad..6b70c98334483 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -940,7 +940,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { /// Builds the reduced graph for a single item in an external crate. fn build_reduced_graph_for_external_crate_res(&mut self, child: ModChild) { let parent = self.parent_scope.module; - let ModChild { ident, res, vis, span } = child; + let ModChild { ident, res, vis, span, macro_rules } = child; let res = res.expect_non_local(); let expansion = self.parent_scope.expansion; // Record primary definitions. @@ -972,7 +972,9 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { _, ) => self.r.define(parent, ident, ValueNS, (res, vis, span, expansion)), Res::Def(DefKind::Macro(..), _) | Res::NonMacroAttr(..) => { - self.r.define(parent, ident, MacroNS, (res, vis, span, expansion)) + if !macro_rules { + self.r.define(parent, ident, MacroNS, (res, vis, span, expansion)) + } } Res::Def( DefKind::TyParam diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 5e21161f2e06a..bf570fb0f80b0 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -1399,14 +1399,22 @@ impl<'a, 'b> ImportResolver<'a, 'b> { let mut reexports = Vec::new(); module.for_each_child(self.r, |_, ident, _, binding| { - // Filter away ambiguous imports and anything that has def-site hygiene. - // FIXME: Implement actual cross-crate hygiene. - let is_good_import = - binding.is_import() && !binding.is_ambiguity() && !ident.span.from_expansion(); - if is_good_import || binding.is_macro_def() { + // FIXME: Consider changing the binding inserted by `#[macro_export] macro_rules` + // into the crate root to actual `NameBindingKind::Import`. + if binding.is_import() + || matches!(binding.kind, NameBindingKind::Res(_, _is_macro_export @ true)) + { let res = binding.res().expect_non_local(); - if res != def::Res::Err { - reexports.push(ModChild { ident, res, vis: binding.vis, span: binding.span }); + // Ambiguous imports are treated as errors at this point and are + // not exposed to other crates (see #36837 for more details). + if res != def::Res::Err && !binding.is_ambiguity() { + reexports.push(ModChild { + ident, + res, + vis: binding.vis, + span: binding.span, + macro_rules: false, + }); } } }); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 2fb69e438c403..eed8aaed4ee09 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -845,10 +845,6 @@ impl<'a> NameBinding<'a> { ) } - fn is_macro_def(&self) -> bool { - matches!(self.kind, NameBindingKind::Res(Res::Def(DefKind::Macro(..), _), _)) - } - fn macro_kind(&self) -> Option { self.res().macro_kind() } From b91ec30159594ed73cabc4c6cb5450996f290577 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 5 Feb 2022 10:07:13 +0800 Subject: [PATCH 18/18] Update clippy tests --- src/tools/clippy/tests/ui/macro_use_imports.fixed | 2 +- src/tools/clippy/tests/ui/macro_use_imports.stderr | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/clippy/tests/ui/macro_use_imports.fixed b/src/tools/clippy/tests/ui/macro_use_imports.fixed index 306ea50258da0..a83c8ba0b6428 100644 --- a/src/tools/clippy/tests/ui/macro_use_imports.fixed +++ b/src/tools/clippy/tests/ui/macro_use_imports.fixed @@ -15,7 +15,7 @@ extern crate macro_use_helper as mac; extern crate proc_macro_derive as mini_mac; mod a { - use mac::{pub_macro, inner_mod_macro, function_macro, ty_macro, pub_in_private_macro}; + use mac::{pub_macro, function_macro, ty_macro, inner_mod_macro, pub_in_private_macro}; use mac; use mini_mac::ClippyMiniMacroTest; use mini_mac; diff --git a/src/tools/clippy/tests/ui/macro_use_imports.stderr b/src/tools/clippy/tests/ui/macro_use_imports.stderr index f8c86c8d9179f..9028a636e7f7a 100644 --- a/src/tools/clippy/tests/ui/macro_use_imports.stderr +++ b/src/tools/clippy/tests/ui/macro_use_imports.stderr @@ -2,7 +2,7 @@ error: `macro_use` attributes are no longer needed in the Rust 2018 edition --> $DIR/macro_use_imports.rs:18:5 | LL | #[macro_use] - | ^^^^^^^^^^^^ help: remove the attribute and import the macro directly, try: `use mac::{pub_macro, inner_mod_macro, function_macro, ty_macro, pub_in_private_macro};` + | ^^^^^^^^^^^^ help: remove the attribute and import the macro directly, try: `use mac::{pub_macro, function_macro, ty_macro, inner_mod_macro, pub_in_private_macro};` | = note: `-D clippy::macro-use-imports` implied by `-D warnings`