From 9e5c9c14c7fb3bc6032fff6e9031e641ed2a104a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Sun, 4 Aug 2024 01:42:14 +0000 Subject: [PATCH 1/2] tests: add regression test for incorrect "builtin attribute is checked" assertion ICE See . --- tests/ui/attributes/check-builtin-attr-ice.rs | 58 +++++++++++++++++++ .../attributes/check-builtin-attr-ice.stderr | 21 +++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/ui/attributes/check-builtin-attr-ice.rs create mode 100644 tests/ui/attributes/check-builtin-attr-ice.stderr diff --git a/tests/ui/attributes/check-builtin-attr-ice.rs b/tests/ui/attributes/check-builtin-attr-ice.rs new file mode 100644 index 0000000000000..9ef5890601f6b --- /dev/null +++ b/tests/ui/attributes/check-builtin-attr-ice.rs @@ -0,0 +1,58 @@ +//! Regression test for #128622. +//! +//! PR #128581 introduced an assertion that all builtin attributes are actually checked via +//! `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes. +//! Unfortunately, the check had correctness problems. +//! +//! The match on attribute path segments looked like +//! +//! ```rs,ignore +//! [sym::should_panic] => /* check is implemented */ +//! match BUILTIN_ATTRIBUTE_MAP.get(name) { +//! // checked below +//! Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {} +//! Some(_) => { +//! if !name.as_str().starts_with("rustc_") { +//! span_bug!( +//! attr.span, +//! "builtin attribute {name:?} not handled by `CheckAttrVisitor`" +//! ) +//! } +//! } +//! None => (), +//! } +//! ``` +//! +//! However, it failed to account for edge cases such as an attribute whose: +//! +//! 1. path segments *starts* with a builtin attribute such as `should_panic` +//! 2. which does not start with `rustc_`, and +//! 3. is also an `AttributeType::Normal` attribute upon registration with the builtin attribute map +//! +//! These conditions when all satisfied cause the span bug to be issued for e.g. +//! `#[should_panic::skip]` because the `[sym::should_panic]` arm is not matched (since it's +//! `[sym::should_panic, sym::skip]`). +//! +//! This test checks that the span bug is not fired for such cases. +//! +//! issue: rust-lang/rust#128622 + +// Notably, `should_panic` is a `AttributeType::Normal` attribute that is checked separately. + +struct Foo { + #[should_panic::skip] + //~^ ERROR failed to resolve + pub field: u8, + + #[should_panic::a::b::c] + //~^ ERROR failed to resolve + pub field2: u8, +} + +fn foo() {} + +fn main() { + #[deny::skip] + //~^ ERROR failed to resolve + foo(); +} diff --git a/tests/ui/attributes/check-builtin-attr-ice.stderr b/tests/ui/attributes/check-builtin-attr-ice.stderr new file mode 100644 index 0000000000000..5a27da565a857 --- /dev/null +++ b/tests/ui/attributes/check-builtin-attr-ice.stderr @@ -0,0 +1,21 @@ +error[E0433]: failed to resolve: use of undeclared crate or module `should_panic` + --> $DIR/check-builtin-attr-ice.rs:43:7 + | +LL | #[should_panic::skip] + | ^^^^^^^^^^^^ use of undeclared crate or module `should_panic` + +error[E0433]: failed to resolve: use of undeclared crate or module `should_panic` + --> $DIR/check-builtin-attr-ice.rs:47:7 + | +LL | #[should_panic::a::b::c] + | ^^^^^^^^^^^^ use of undeclared crate or module `should_panic` + +error[E0433]: failed to resolve: use of undeclared crate or module `deny` + --> $DIR/check-builtin-attr-ice.rs:55:7 + | +LL | #[deny::skip] + | ^^^^ use of undeclared crate or module `deny` + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0433`. From 9d0eaa2ad78ca2995d4f2c4edb61f762ba79846b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Sun, 4 Aug 2024 01:39:05 +0000 Subject: [PATCH 2/2] check_attr: cover multi-segment attributes on specific check arms PR #128581 introduced an assertion that all builtin attributes are actually checked via `CheckAttrVisitor` and aren't accidentally usable on completely unrelated HIR nodes. Unfortunately, the check had correctness problems. The match on attribute path segments looked like ```rust,ignore [sym::should_panic] => /* check is implemented */ match BUILTIN_ATTRIBUTE_MAP.get(name) { // checked below Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {} Some(_) => { if !name.as_str().starts_with("rustc_") { span_bug!( attr.span, "builtin attribute {name:?} not handled by `CheckAttrVisitor`" ) } } None => (), } ``` However, it failed to account for edge cases such as an attribute whose: 1. path segments *starts* with a builtin attribute such as `should_panic` 2. which does not start with `rustc_`, and 3. is also an `AttributeType::Normal` attribute upon registration with the builtin attribute map These conditions when all satisfied cause the span bug to be issued for e.g. `#[should_panic::skip]` because the `[sym::should_panic]` arm is not matched (since it's `[sym::should_panic, sym::skip]`). See . --- compiler/rustc_passes/src/check_attr.rs | 144 ++++++++++++------------ 1 file changed, 73 insertions(+), 71 deletions(-) diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs index 755f67b3f4fe0..a0d0d80b9572e 100644 --- a/compiler/rustc_passes/src/check_attr.rs +++ b/compiler/rustc_passes/src/check_attr.rs @@ -116,130 +116,130 @@ impl<'tcx> CheckAttrVisitor<'tcx> { let attrs = self.tcx.hir().attrs(hir_id); for attr in attrs { match attr.path().as_slice() { - [sym::diagnostic, sym::do_not_recommend] => { + [sym::diagnostic, sym::do_not_recommend, ..] => { self.check_do_not_recommend(attr.span, hir_id, target) } - [sym::diagnostic, sym::on_unimplemented] => { + [sym::diagnostic, sym::on_unimplemented, ..] => { self.check_diagnostic_on_unimplemented(attr.span, hir_id, target) } - [sym::inline] => self.check_inline(hir_id, attr, span, target), - [sym::coverage] => self.check_coverage(attr, span, target), - [sym::optimize] => self.check_optimize(hir_id, attr, target), - [sym::non_exhaustive] => self.check_non_exhaustive(hir_id, attr, span, target), - [sym::marker] => self.check_marker(hir_id, attr, span, target), - [sym::target_feature] => { + [sym::inline, ..] => self.check_inline(hir_id, attr, span, target), + [sym::coverage, ..] => self.check_coverage(attr, span, target), + [sym::optimize, ..] => self.check_optimize(hir_id, attr, target), + [sym::non_exhaustive, ..] => self.check_non_exhaustive(hir_id, attr, span, target), + [sym::marker, ..] => self.check_marker(hir_id, attr, span, target), + [sym::target_feature, ..] => { self.check_target_feature(hir_id, attr, span, target, attrs) } - [sym::thread_local] => self.check_thread_local(attr, span, target), - [sym::track_caller] => { + [sym::thread_local, ..] => self.check_thread_local(attr, span, target), + [sym::track_caller, ..] => { self.check_track_caller(hir_id, attr.span, attrs, span, target) } - [sym::doc] => self.check_doc_attrs( + [sym::doc, ..] => self.check_doc_attrs( attr, hir_id, target, &mut specified_inline, &mut doc_aliases, ), - [sym::no_link] => self.check_no_link(hir_id, attr, span, target), - [sym::export_name] => self.check_export_name(hir_id, attr, span, target), - [sym::rustc_layout_scalar_valid_range_start] - | [sym::rustc_layout_scalar_valid_range_end] => { + [sym::no_link, ..] => self.check_no_link(hir_id, attr, span, target), + [sym::export_name, ..] => self.check_export_name(hir_id, attr, span, target), + [sym::rustc_layout_scalar_valid_range_start, ..] + | [sym::rustc_layout_scalar_valid_range_end, ..] => { self.check_rustc_layout_scalar_valid_range(attr, span, target) } - [sym::allow_internal_unstable] => { + [sym::allow_internal_unstable, ..] => { self.check_allow_internal_unstable(hir_id, attr, span, target, attrs) } - [sym::debugger_visualizer] => self.check_debugger_visualizer(attr, target), - [sym::rustc_allow_const_fn_unstable] => { + [sym::debugger_visualizer, ..] => self.check_debugger_visualizer(attr, target), + [sym::rustc_allow_const_fn_unstable, ..] => { self.check_rustc_allow_const_fn_unstable(hir_id, attr, span, target) } - [sym::rustc_std_internal_symbol] => { + [sym::rustc_std_internal_symbol, ..] => { self.check_rustc_std_internal_symbol(attr, span, target) } - [sym::naked] => self.check_naked(hir_id, attr, span, target, attrs), - [sym::rustc_never_returns_null_ptr] => { + [sym::naked, ..] => self.check_naked(hir_id, attr, span, target, attrs), + [sym::rustc_never_returns_null_ptr, ..] => { self.check_applied_to_fn_or_method(hir_id, attr, span, target) } - [sym::rustc_legacy_const_generics] => { + [sym::rustc_legacy_const_generics, ..] => { self.check_rustc_legacy_const_generics(hir_id, attr, span, target, item) } - [sym::rustc_lint_query_instability] => { + [sym::rustc_lint_query_instability, ..] => { self.check_rustc_lint_query_instability(hir_id, attr, span, target) } - [sym::rustc_lint_diagnostics] => { + [sym::rustc_lint_diagnostics, ..] => { self.check_rustc_lint_diagnostics(hir_id, attr, span, target) } - [sym::rustc_lint_opt_ty] => self.check_rustc_lint_opt_ty(attr, span, target), - [sym::rustc_lint_opt_deny_field_access] => { + [sym::rustc_lint_opt_ty, ..] => self.check_rustc_lint_opt_ty(attr, span, target), + [sym::rustc_lint_opt_deny_field_access, ..] => { self.check_rustc_lint_opt_deny_field_access(attr, span, target) } - [sym::rustc_clean] - | [sym::rustc_dirty] - | [sym::rustc_if_this_changed] - | [sym::rustc_then_this_would_need] => self.check_rustc_dirty_clean(attr), - [sym::rustc_coinductive] - | [sym::rustc_must_implement_one_of] - | [sym::rustc_deny_explicit_impl] - | [sym::const_trait] => self.check_must_be_applied_to_trait(attr, span, target), - [sym::cmse_nonsecure_entry] => { + [sym::rustc_clean, ..] + | [sym::rustc_dirty, ..] + | [sym::rustc_if_this_changed, ..] + | [sym::rustc_then_this_would_need, ..] => self.check_rustc_dirty_clean(attr), + [sym::rustc_coinductive, ..] + | [sym::rustc_must_implement_one_of, ..] + | [sym::rustc_deny_explicit_impl, ..] + | [sym::const_trait, ..] => self.check_must_be_applied_to_trait(attr, span, target), + [sym::cmse_nonsecure_entry, ..] => { self.check_cmse_nonsecure_entry(hir_id, attr, span, target) } - [sym::collapse_debuginfo] => self.check_collapse_debuginfo(attr, span, target), - [sym::must_not_suspend] => self.check_must_not_suspend(attr, span, target), - [sym::must_use] => self.check_must_use(hir_id, attr, target), - [sym::rustc_pass_by_value] => self.check_pass_by_value(attr, span, target), - [sym::rustc_allow_incoherent_impl] => { + [sym::collapse_debuginfo, ..] => self.check_collapse_debuginfo(attr, span, target), + [sym::must_not_suspend, ..] => self.check_must_not_suspend(attr, span, target), + [sym::must_use, ..] => self.check_must_use(hir_id, attr, target), + [sym::rustc_pass_by_value, ..] => self.check_pass_by_value(attr, span, target), + [sym::rustc_allow_incoherent_impl, ..] => { self.check_allow_incoherent_impl(attr, span, target) } - [sym::rustc_has_incoherent_inherent_impls] => { + [sym::rustc_has_incoherent_inherent_impls, ..] => { self.check_has_incoherent_inherent_impls(attr, span, target) } - [sym::ffi_pure] => self.check_ffi_pure(attr.span, attrs, target), - [sym::ffi_const] => self.check_ffi_const(attr.span, target), - [sym::rustc_const_unstable] - | [sym::rustc_const_stable] - | [sym::unstable] - | [sym::stable] - | [sym::rustc_allowed_through_unstable_modules] - | [sym::rustc_promotable] => self.check_stability_promotable(attr, target), - [sym::link_ordinal] => self.check_link_ordinal(attr, span, target), - [sym::rustc_confusables] => self.check_confusables(attr, target), - [sym::rustc_safe_intrinsic] => { + [sym::ffi_pure, ..] => self.check_ffi_pure(attr.span, attrs, target), + [sym::ffi_const, ..] => self.check_ffi_const(attr.span, target), + [sym::rustc_const_unstable, ..] + | [sym::rustc_const_stable, ..] + | [sym::unstable, ..] + | [sym::stable, ..] + | [sym::rustc_allowed_through_unstable_modules, ..] + | [sym::rustc_promotable, ..] => self.check_stability_promotable(attr, target), + [sym::link_ordinal, ..] => self.check_link_ordinal(attr, span, target), + [sym::rustc_confusables, ..] => self.check_confusables(attr, target), + [sym::rustc_safe_intrinsic, ..] => { self.check_rustc_safe_intrinsic(hir_id, attr, span, target) } - [sym::cold] => self.check_cold(hir_id, attr, span, target), - [sym::link] => self.check_link(hir_id, attr, span, target), - [sym::link_name] => self.check_link_name(hir_id, attr, span, target), - [sym::link_section] => self.check_link_section(hir_id, attr, span, target), - [sym::no_mangle] => self.check_no_mangle(hir_id, attr, span, target), - [sym::deprecated] => self.check_deprecated(hir_id, attr, span, target), - [sym::macro_use] | [sym::macro_escape] => { + [sym::cold, ..] => self.check_cold(hir_id, attr, span, target), + [sym::link, ..] => self.check_link(hir_id, attr, span, target), + [sym::link_name, ..] => self.check_link_name(hir_id, attr, span, target), + [sym::link_section, ..] => self.check_link_section(hir_id, attr, span, target), + [sym::no_mangle, ..] => self.check_no_mangle(hir_id, attr, span, target), + [sym::deprecated, ..] => self.check_deprecated(hir_id, attr, span, target), + [sym::macro_use, ..] | [sym::macro_escape, ..] => { self.check_macro_use(hir_id, attr, target) } - [sym::path] => self.check_generic_attr(hir_id, attr, target, Target::Mod), - [sym::macro_export] => self.check_macro_export(hir_id, attr, target), - [sym::ignore] | [sym::should_panic] => { + [sym::path, ..] => self.check_generic_attr(hir_id, attr, target, Target::Mod), + [sym::macro_export, ..] => self.check_macro_export(hir_id, attr, target), + [sym::ignore, ..] | [sym::should_panic, ..] => { self.check_generic_attr(hir_id, attr, target, Target::Fn) } - [sym::automatically_derived] => { + [sym::automatically_derived, ..] => { self.check_generic_attr(hir_id, attr, target, Target::Impl) } - [sym::no_implicit_prelude] => { + [sym::no_implicit_prelude, ..] => { self.check_generic_attr(hir_id, attr, target, Target::Mod) } - [sym::rustc_object_lifetime_default] => self.check_object_lifetime_default(hir_id), - [sym::proc_macro] => { + [sym::rustc_object_lifetime_default, ..] => self.check_object_lifetime_default(hir_id), + [sym::proc_macro, ..] => { self.check_proc_macro(hir_id, target, ProcMacroKind::FunctionLike) } - [sym::proc_macro_attribute] => { + [sym::proc_macro_attribute, ..] => { self.check_proc_macro(hir_id, target, ProcMacroKind::Attribute); } - [sym::proc_macro_derive] => { + [sym::proc_macro_derive, ..] => { self.check_generic_attr(hir_id, attr, target, Target::Fn); self.check_proc_macro(hir_id, target, ProcMacroKind::Derive) } - [sym::coroutine] => { + [sym::coroutine, ..] => { self.check_coroutine(attr, target); } [ @@ -273,14 +273,16 @@ impl<'tcx> CheckAttrVisitor<'tcx> { | sym::default_lib_allocator | sym::start | sym::custom_mir, + .. ] => {} [name, ..] => { match BUILTIN_ATTRIBUTE_MAP.get(name) { // checked below Some(BuiltinAttribute { type_: AttributeType::CrateLevel, .. }) => {} Some(_) => { - // FIXME: differentiate between unstable and internal attributes just like we do with features instead - // of just accepting `rustc_` attributes by name. That should allow trimming the above list, too. + // FIXME: differentiate between unstable and internal attributes just + // like we do with features instead of just accepting `rustc_` + // attributes by name. That should allow trimming the above list, too. if !name.as_str().starts_with("rustc_") { span_bug!( attr.span,