Rollup merge of rust-lang#157215 - JonathanBrouwer:repr-target-checking3, r=mejrs

Implement argument-dependent target checking for the `#[repr]` parser

Fixes some of rust-lang#156499
Fixes some of rust-lang#153101
Alternative approach to rust-lang#156569, which was suggested in the comments here: rust-lang#156569 (review)

How do you feel about this approach?
r? @mejrs

Author: JonathanBrouwer

compiler/rustc_attr_parsing/src/attributes/repr.rs

@@ -37,6 +37,16 @@ impl CombineAttributeParser for ReprParser {
         };
 
         if list.is_empty() {
+            cx.check_target(
+                "()",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Enum),
+                    Allow(Target::Union),
+                    Warn(Target::MacroCall),
+                ]),
+            );
+
             let attr_span = cx.attr_span;
             cx.adcx().warn_empty_attribute(attr_span);
             return vec![];
@@ -53,51 +63,120 @@ impl CombineAttributeParser for ReprParser {
         reprs
     }
 
-    //FIXME Still checked fully in `check_attr.rs`
-    //This one is slightly more complicated because the allowed targets depend on the arguments
-    const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::AllowList(ALL_TARGETS);
+    const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::ManuallyChecked;
     const STABILITY: AttributeStability = AttributeStability::Stable;
 }
 
 fn parse_repr(cx: &mut AcceptContext<'_, '_>, param: &MetaItemParser) -> Option<ReprAttr> {
     use ReprAttr::*;
 
-    macro_rules! no_args {
-        ($constructor: expr) => {{
+    macro_rules! repr_int {
+        ($arg: ident, $constructor: expr) => {{
+            cx.check_target(
+                concat!("(", stringify!($arg), ")"),
+                &AllowedTargets::AllowList(&[Allow(Target::Enum), Warn(Target::MacroCall)]),
+            );
             cx.expect_no_args(param.args())?;
             Some($constructor)
         }};
     }
 
     match param.path().word_sym() {
         Some(sym::align) => {
+            cx.check_target(
+                "(align(...))",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Enum),
+                    Allow(Target::Union),
+                    Warn(Target::MacroCall),
+                ]),
+            );
             let l = cx.expect_list(param.args(), param.span())?;
             parse_repr_align(cx, l, AlignKind::Align)
         }
-        Some(sym::packed) => match param.args() {
-            ArgParser::NoArgs => Some(ReprPacked(Align::ONE)),
-            ArgParser::List(l) => parse_repr_align(cx, l, AlignKind::Packed),
-            ArgParser::NameValue(_) => {
-                cx.adcx().expected_list_or_no_args(param.span());
-                None
+        Some(sym::packed) => {
+            cx.check_target(
+                "(packed)",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Union),
+                    Warn(Target::MacroCall),
+                ]),
+            );
+            match param.args() {
+                ArgParser::NoArgs => Some(ReprPacked(Align::ONE)),
+                ArgParser::List(l) => parse_repr_align(cx, l, AlignKind::Packed),
+                ArgParser::NameValue(_) => {
+                    cx.adcx().expected_list_or_no_args(param.span());
+                    None
+                }
             }
-        },
-        Some(sym::Rust) => no_args!(ReprRust),
-        Some(sym::C) => no_args!(ReprC),
-        Some(sym::simd) => no_args!(ReprSimd),
-        Some(sym::transparent) => no_args!(ReprTransparent),
-        Some(sym::i8) => no_args!(ReprInt(SignedInt(IntTy::I8))),
-        Some(sym::u8) => no_args!(ReprInt(UnsignedInt(UintTy::U8))),
-        Some(sym::i16) => no_args!(ReprInt(SignedInt(IntTy::I16))),
-        Some(sym::u16) => no_args!(ReprInt(UnsignedInt(UintTy::U16))),
-        Some(sym::i32) => no_args!(ReprInt(SignedInt(IntTy::I32))),
-        Some(sym::u32) => no_args!(ReprInt(UnsignedInt(UintTy::U32))),
-        Some(sym::i64) => no_args!(ReprInt(SignedInt(IntTy::I64))),
-        Some(sym::u64) => no_args!(ReprInt(UnsignedInt(UintTy::U64))),
-        Some(sym::i128) => no_args!(ReprInt(SignedInt(IntTy::I128))),
-        Some(sym::u128) => no_args!(ReprInt(UnsignedInt(UintTy::U128))),
-        Some(sym::isize) => no_args!(ReprInt(SignedInt(IntTy::Isize))),
-        Some(sym::usize) => no_args!(ReprInt(UnsignedInt(UintTy::Usize))),
+        }
+
+        Some(sym::Rust) => {
+            cx.check_target(
+                "(Rust)",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Enum),
+                    Allow(Target::Union),
+                    Warn(Target::MacroCall),
+                ]),
+            );
+            cx.expect_no_args(param.args())?;
+            Some(ReprRust)
+        }
+        Some(sym::C) => {
+            cx.check_target(
+                "(C)",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Enum),
+                    Allow(Target::Union),
+                    Warn(Target::MacroCall),
+                ]),
+            );
+            cx.expect_no_args(param.args())?;
+            Some(ReprC)
+        }
+        Some(sym::simd) => {
+            cx.check_target(
+                "(simd)",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),   // Feature gated in `rustc_ast_passes`
+                    Warn(Target::MacroCall), // FIXME: This is not feature gated (!!)
+                ]),
+            );
+            cx.expect_no_args(param.args())?;
+            Some(ReprSimd)
+        }
+        Some(sym::transparent) => {
+            cx.check_target(
+                "(transparent)",
+                &AllowedTargets::AllowList(&[
+                    Allow(Target::Struct),
+                    Allow(Target::Enum),
+                    Allow(Target::Union), // Feature gated in `rustc_hir_analysis`
+                    Warn(Target::MacroCall),
+                ]),
+            );
+            cx.expect_no_args(param.args())?;
+            Some(ReprTransparent)
+        }
+
+        Some(sym::i8) => repr_int!(i8, ReprInt(SignedInt(IntTy::I8))),
+        Some(sym::u8) => repr_int!(u8, ReprInt(UnsignedInt(UintTy::U8))),
+        Some(sym::i16) => repr_int!(i16, ReprInt(SignedInt(IntTy::I16))),
+        Some(sym::u16) => repr_int!(u16, ReprInt(UnsignedInt(UintTy::U16))),
+        Some(sym::i32) => repr_int!(i32, ReprInt(SignedInt(IntTy::I32))),
+        Some(sym::u32) => repr_int!(u32, ReprInt(UnsignedInt(UintTy::U32))),
+        Some(sym::i64) => repr_int!(i64, ReprInt(SignedInt(IntTy::I64))),
+        Some(sym::u64) => repr_int!(u64, ReprInt(UnsignedInt(UintTy::U64))),
+        Some(sym::i128) => repr_int!(i128, ReprInt(SignedInt(IntTy::I128))),
+        Some(sym::u128) => repr_int!(u128, ReprInt(UnsignedInt(UintTy::U128))),
+        Some(sym::isize) => repr_int!(isize, ReprInt(SignedInt(IntTy::Isize))),
+        Some(sym::usize) => repr_int!(usize, ReprInt(UnsignedInt(UintTy::Usize))),
         _ => {
             cx.adcx().expected_specific_argument(
                 param.span(),

compiler/rustc_attr_parsing/src/context.rs

@@ -366,6 +366,10 @@ pub struct AcceptContext<'f, 'sess> {
 
     /// The name of the attribute we're currently accepting.
     pub(crate) attr_path: AttrPath,
+
+    /// Used for `AllowedTargets::ManuallyChecked`, to assert that the manual target check has been done
+    #[cfg(debug_assertions)]
+    pub(crate) has_target_been_checked: bool,
 }
 
 impl<'f, 'sess: 'f> SharedContext<'f, 'sess> {

compiler/rustc_attr_parsing/src/interface.rs

@@ -240,6 +240,8 @@ impl<'sess> AttributeParser<'sess> {
             template,
             attr_safety: attr_safety.unwrap_or(Safety::Default),
             attr_path,
+            #[cfg(debug_assertions)]
+            has_target_been_checked: false,
         };
         parse_fn(&mut cx, args)
     }
@@ -410,12 +412,14 @@ impl<'sess> AttributeParser<'sess> {
                             template: &accept.template,
                             attr_safety: n.item.unsafety,
                             attr_path: attr_path.clone(),
+                            #[cfg(debug_assertions)]
+                            has_target_been_checked: false,
                         };
 
                         (accept.accept_fn)(&mut cx, &args);
                         finalizers.push(accept.finalizer);
 
-                        Self::check_target(&accept.allowed_targets, &mut cx);
+                        Self::check_target(&accept.allowed_targets, "", &mut cx);
                         #[cfg(debug_assertions)]
                         if !cx.shared.has_lint_been_emitted.load(Ordering::Relaxed) {
                             cx.shared.cx.check_args_used(&attr, &args)

compiler/rustc_attr_parsing/src/session_diagnostics.rs

@@ -331,8 +331,8 @@ pub(crate) struct EmptyConfusables {
 }
 
 #[derive(Diagnostic)]
-#[help("`#[{$name}]` can {$only}be applied to {$applied}")]
-#[diag("`#[{$name}]` attribute cannot be used on {$target}")]
+#[help("`#[{$name}{$attribute_args}]` can {$only}be applied to {$applied}")]
+#[diag("`#[{$name}{$attribute_args}]` attribute cannot be used on {$target}")]
 pub(crate) struct InvalidTarget {
     #[primary_span]
     #[suggestion(
@@ -346,12 +346,23 @@ pub(crate) struct InvalidTarget {
     pub target: &'static str,
     pub applied: DiagArgValue,
     pub only: &'static str,
+    pub attribute_args: &'static str,
+    #[subdiagnostic]
+    pub help: Option<InvalidTargetHelp>,
     #[warning(
         "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!"
     )]
     pub previously_accepted: bool,
 }
 
+#[derive(Subdiagnostic)]
+pub(crate) enum InvalidTargetHelp {
+    #[help("use `#[rustc_align(...)]` instead")]
+    UseRustcAlign,
+    #[help("use `#[rustc_align_static(...)]` instead")]
+    UseRustcAlignStatic,
+}
+
 #[derive(Diagnostic)]
 #[diag("invalid alignment value: {$error_part}", code = E0589)]
 pub(crate) struct InvalidAlignmentValue {

compiler/rustc_attr_parsing/src/target_checking.rs

@@ -11,14 +11,18 @@ use crate::context::AcceptContext;
 use crate::errors::{
     InvalidAttrAtCrateLevel, ItemFollowingInnerAttr, UnsupportedAttributesInWhere,
 };
-use crate::session_diagnostics::InvalidTarget;
+use crate::session_diagnostics::{InvalidTarget, InvalidTargetHelp};
 use crate::target_checking::Policy::Allow;
 use crate::{AttributeParser, ShouldEmit};
 
 #[derive(Debug)]
 pub(crate) enum AllowedTargets {
     AllowList(&'static [Policy]),
     AllowListWarnRest(&'static [Policy]),
+    /// This is useful for argument-dependent target checking.
+    /// If debug assertions are enabled,
+    /// this emits a delayed bug if the `cx.check_target(...)` method is not called during attribute parsing.
+    ManuallyChecked,
 }
 
 pub(crate) enum AllowedResult {
@@ -52,13 +56,15 @@ impl AllowedTargets {
                     AllowedResult::Warn
                 }
             }
+            AllowedTargets::ManuallyChecked => unreachable!(),
         }
     }
 
     pub(crate) fn allowed_targets(&self) -> Vec<Target> {
         match self {
             AllowedTargets::AllowList(list) => list,
             AllowedTargets::AllowListWarnRest(list) => list,
+            AllowedTargets::ManuallyChecked => unreachable!(),
         }
         .iter()
         .filter_map(|target| match target {
@@ -89,38 +95,29 @@ pub(crate) enum Policy {
 impl<'sess> AttributeParser<'sess> {
     pub(crate) fn check_target(
         allowed_targets: &AllowedTargets,
+        attribute_args: &'static str,
         cx: &mut AcceptContext<'_, 'sess>,
     ) {
         if matches!(cx.should_emit, ShouldEmit::Nothing) {
             return;
         }
 
+        if let AllowedTargets::ManuallyChecked = allowed_targets {
+            #[cfg(debug_assertions)]
+            if !cx.has_target_been_checked {
+                cx.dcx().delayed_bug("Attribute target has not been checked");
+            }
+
+            return;
+        }
+
         // For crate-level attributes we emit a specific set of lints to warn
         // people about accidentally not using them on the crate.
         if let &AllowedTargets::AllowList(&[Allow(Target::Crate)]) = allowed_targets {
             Self::check_crate_level(cx);
             return;
         }
 
-        if matches!(cx.attr_path.segments.as_ref(), [sym::repr]) && cx.target == Target::Crate {
-            // The allowed targets of `repr` depend on its arguments. They can't be checked using
-            // the `AttributeParser` code.
-            let span = cx.attr_span;
-            let item =
-                cx.cx.first_line_of_next_item(span).map(|span| ItemFollowingInnerAttr { span });
-
-            let pound_to_opening_bracket = cx.attr_span.until(cx.inner_span);
-
-            cx.dcx()
-                .create_err(InvalidAttrAtCrateLevel {
-                    span,
-                    pound_to_opening_bracket,
-                    name: sym::repr,
-                    item,
-                })
-                .emit();
-        }
-
         let result = allowed_targets.is_allowed(cx.target);
         if matches!(result, AllowedResult::Allowed) {
             return;
@@ -134,6 +131,8 @@ impl<'sess> AttributeParser<'sess> {
             target: cx.target.plural_name(),
             only: if only { "only " } else { "" },
             applied: DiagArgValue::StrListSepByAnd(applied.into_iter().map(Cow::Owned).collect()),
+            attribute_args,
+            help: Self::target_checking_help(attribute_args, cx),
             previously_accepted: matches!(result, AllowedResult::Warn),
         };
 
@@ -164,6 +163,24 @@ impl<'sess> AttributeParser<'sess> {
         }
     }
 
+    fn target_checking_help(
+        attribute_args: &'static str,
+        cx: &AcceptContext<'_, '_>,
+    ) -> Option<InvalidTargetHelp> {
+        match &*cx.attr_path.segments {
+            [sym::repr] if attribute_args == "(align(...))" => match cx.target {
+                Target::Fn | Target::Method(..) if cx.features().fn_align() => {
+                    Some(InvalidTargetHelp::UseRustcAlign)
+                }
+                Target::Static if cx.features().static_align() => {
+                    Some(InvalidTargetHelp::UseRustcAlignStatic)
+                }
+                _ => None,
+            },
+            _ => None,
+        }
+    }
+
     pub(crate) fn check_crate_level(cx: &mut AcceptContext<'_, 'sess>) {
         if cx.target == Target::Crate {
             return;
@@ -399,6 +416,20 @@ fn filter_targets(
     added_fake_targets.push(target_group_name);
 }
 
+impl<'f, 'sess> AcceptContext<'f, 'sess> {
+    pub(crate) fn check_target(
+        &mut self,
+        attribute_args: &'static str,
+        allowed_targets: &AllowedTargets,
+    ) {
+        #[cfg(debug_assertions)]
+        {
+            self.has_target_been_checked = true;
+        }
+        AttributeParser::check_target(allowed_targets, attribute_args, self);
+    }
+}
+
 /// This is the list of all targets to which a attribute can be applied
 /// This is used for:
 /// - `rustc_dummy`, which can be applied to all targets

compiler/rustc_error_codes/src/error_codes/E0517.md

@@ -1,8 +1,12 @@
+#### Note: this error code is no longer emitted by the compiler.
+This error code was replaced with the
+`attribute cannot be used on...` diagnostic that does not have an error code.
+
 A `#[repr(..)]` attribute was placed on an unsupported item.
 
 Examples of erroneous code:
 
-```compile_fail,E0517
+```compile_fail
 #[repr(C)]
 type Foo = u8;