Taint attributes when validating them causes errors

oli-obk · oli-obk · commit 7f187203ea60 · 2024-07-10T14:27:34.000Z
diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs
@@ -1883,9 +1883,14 @@ pub enum LitKind {
 
 impl LitKind {
     pub fn str(&self) -> Option<Symbol> {
+        self.try_str().ok()?
+    }
+
+    pub fn try_str(&self) -> Result<Option<Symbol>, ErrorGuaranteed> {
         match *self {
-            LitKind::Str(s, _) => Some(s),
-            _ => None,
+            LitKind::Str(s, _) => Ok(Some(s)),
+            LitKind::Err(guar) => Err(guar),
+            _ => Ok(None),
         }
     }
 
@@ -2827,6 +2832,15 @@ pub enum AttrKind {
     DocComment(CommentKind, Symbol),
 }
 
+impl AttrKind {
+    pub fn normal_mut(&mut self) -> &mut AttrItem {
+        match self {
+            AttrKind::Normal(normal) => &mut normal.item,
+            AttrKind::DocComment(..) => panic!("unexpected doc comment"),
+        }
+    }
+}
+
 #[derive(Clone, Encodable, Decodable, Debug)]
 pub struct NormalAttr {
     pub item: AttrItem,
diff --git a/compiler/rustc_ast/src/attr/mod.rs b/compiler/rustc_ast/src/attr/mod.rs
@@ -14,7 +14,7 @@ use crate::util::comments;
 use crate::util::literal::escape_string_symbol;
 use rustc_index::bit_set::GrowableBitSet;
 use rustc_span::symbol::{sym, Ident, Symbol};
-use rustc_span::Span;
+use rustc_span::{ErrorGuaranteed, Span};
 use smallvec::{smallvec, SmallVec};
 use std::iter;
 use std::sync::atomic::{AtomicU32, Ordering};
@@ -144,9 +144,13 @@ impl Attribute {
     }
 
     pub fn value_str(&self) -> Option<Symbol> {
+        self.try_value_str().ok()?
+    }
+
+    pub fn try_value_str(&self) -> Result<Option<Symbol>, ErrorGuaranteed> {
         match &self.kind {
-            AttrKind::Normal(normal) => normal.item.value_str(),
-            AttrKind::DocComment(..) => None,
+            AttrKind::Normal(normal) => normal.item.try_value_str(),
+            AttrKind::DocComment(..) => Ok(None),
         }
     }
 
@@ -236,9 +240,13 @@ impl AttrItem {
     }
 
     fn value_str(&self) -> Option<Symbol> {
+        self.try_value_str().ok()?
+    }
+
+    fn try_value_str(&self) -> Result<Option<Symbol>, ErrorGuaranteed> {
         match &self.args {
             AttrArgs::Eq(_, args) => args.value_str(),
-            AttrArgs::Delimited(_) | AttrArgs::Empty => None,
+            AttrArgs::Delimited(_) | AttrArgs::Empty => Ok(None),
         }
     }
 
@@ -257,15 +265,17 @@ impl AttrItem {
 }
 
 impl AttrArgsEq {
-    fn value_str(&self) -> Option<Symbol> {
+    fn value_str(&self) -> Result<Option<Symbol>, ErrorGuaranteed> {
         match self {
             AttrArgsEq::Ast(expr) => match expr.kind {
-                ExprKind::Lit(token_lit) => {
-                    LitKind::from_token_lit(token_lit).ok().and_then(|lit| lit.str())
-                }
-                _ => None,
+                ExprKind::Lit(token_lit) => LitKind::from_token_lit(token_lit)
+                    .ok()
+                    .and_then(|lit| lit.try_str().transpose())
+                    .transpose(),
+                ExprKind::Err(guar) => Err(guar),
+                _ => Ok(None),
             },
-            AttrArgsEq::Hir(lit) => lit.kind.str(),
+            AttrArgsEq::Hir(lit) => lit.kind.try_str(),
         }
     }
 }
diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs
@@ -949,14 +949,15 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                     && let Ok(lit) = MetaItemLit::from_token_lit(token_lit, expr.span)
                 {
                     lit
-                } else {
-                    let guar = self.dcx().has_errors().unwrap();
+                } else if let ExprKind::Err(guar) = expr.kind {
                     MetaItemLit {
                         symbol: kw::Empty,
                         suffix: None,
                         kind: LitKind::Err(guar),
                         span: DUMMY_SP,
                     }
+                } else {
+                    span_bug!(*eq_span, "eq attrs can only be literals, but got {expr:#?}")
                 };
                 AttrArgs::Eq(*eq_span, AttrArgsEq::Hir(lit))
             }
diff --git a/compiler/rustc_expand/src/config.rs b/compiler/rustc_expand/src/config.rs
@@ -123,7 +123,7 @@ pub fn features(sess: &Session, krate_attrs: &[Attribute], crate_name: Symbol) -
     features
 }
 
-pub fn pre_configure_attrs(sess: &Session, attrs: &[Attribute]) -> ast::AttrVec {
+pub fn pre_configure_attrs(sess: &Session, attrs: &mut [Attribute]) -> ast::AttrVec {
     let strip_unconfigured = StripUnconfigured {
         sess,
         features: None,
@@ -133,7 +133,9 @@ pub fn pre_configure_attrs(sess: &Session, attrs: &[Attribute]) -> ast::AttrVec
     attrs
         .iter()
         .flat_map(|attr| strip_unconfigured.process_cfg_attr(attr))
-        .take_while(|attr| !is_cfg(attr) || strip_unconfigured.cfg_true(attr).0)
+        .map_while(|mut attr| {
+            (!is_cfg(&attr) || strip_unconfigured.cfg_true(&mut attr).0).then_some(attr)
+        })
         .collect()
 }
 
@@ -150,7 +152,9 @@ macro_rules! configure {
 impl<'a> StripUnconfigured<'a> {
     pub fn configure<T: HasAttrs + HasTokens>(&self, mut node: T) -> Option<T> {
         self.process_cfg_attrs(&mut node);
-        self.in_cfg(node.attrs()).then(|| {
+        let mut in_cfg = false;
+        node.visit_attrs(|attrs| in_cfg = self.in_cfg(attrs));
+        in_cfg.then(|| {
             self.try_configure_tokens(&mut node);
             node
         })
@@ -189,7 +193,7 @@ impl<'a> StripUnconfigured<'a> {
                 AttrTokenTree::AttrsTarget(mut target) => {
                     target.attrs.flat_map_in_place(|attr| self.process_cfg_attr(&attr));
 
-                    if self.in_cfg(&target.attrs) {
+                    if self.in_cfg(&mut target.attrs) {
                         target.tokens = LazyAttrTokenStream::new(
                             self.configure_tokens(&target.tokens.to_attr_token_stream()),
                         );
@@ -363,11 +367,11 @@ impl<'a> StripUnconfigured<'a> {
     }
 
     /// Determines if a node with the given attributes should be included in this configuration.
-    fn in_cfg(&self, attrs: &[Attribute]) -> bool {
-        attrs.iter().all(|attr| !is_cfg(attr) || self.cfg_true(attr).0)
+    fn in_cfg(&self, attrs: &mut [Attribute]) -> bool {
+        attrs.iter_mut().all(|attr| !is_cfg(attr) || self.cfg_true(attr).0)
     }
 
-    pub(crate) fn cfg_true(&self, attr: &Attribute) -> (bool, Option<MetaItem>) {
+    pub(crate) fn cfg_true(&self, attr: &mut Attribute) -> (bool, Option<MetaItem>) {
         let meta_item = match validate_attr::parse_meta(&self.sess.psess, attr) {
             Ok(meta_item) => meta_item,
             Err(_guar) => {
diff --git a/compiler/rustc_expand/src/expand.rs b/compiler/rustc_expand/src/expand.rs
@@ -37,7 +37,7 @@ use rustc_span::symbol::{sym, Ident};
 use rustc_span::{ErrorGuaranteed, FileName, LocalExpnId, Span};
 
 use smallvec::SmallVec;
-use std::ops::Deref;
+use std::ops::{Deref, DerefMut};
 use std::path::PathBuf;
 use std::rc::Rc;
 use std::{iter, mem};
@@ -699,7 +699,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                 }
                 _ => unreachable!(),
             },
-            InvocationKind::Attr { attr, pos, mut item, derives } => match ext {
+            InvocationKind::Attr { mut attr, pos, mut item, derives } => match ext {
                 SyntaxExtensionKind::Attr(expander) => {
                     self.gate_proc_macro_input(&item);
                     self.gate_proc_macro_attr_item(span, &item);
@@ -742,7 +742,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                     }
                 }
                 SyntaxExtensionKind::LegacyAttr(expander) => {
-                    match validate_attr::parse_meta(&self.cx.sess.psess, &attr) {
+                    match validate_attr::parse_meta(&self.cx.sess.psess, &mut attr) {
                         Ok(meta) => {
                             let items = match expander.expand(self.cx, span, &meta, item, false) {
                                 ExpandResult::Ready(items) => items,
@@ -1079,7 +1079,7 @@ enum AddSemicolon {
 /// of functionality used by `InvocationCollector`.
 trait InvocationCollectorNode: HasAttrs + HasNodeId + Sized {
     type OutputTy = SmallVec<[Self; 1]>;
-    type AttrsTy: Deref<Target = [ast::Attribute]> = ast::AttrVec;
+    type AttrsTy: Deref<Target = [ast::Attribute]> + DerefMut = ast::AttrVec;
     type ItemKind = ItemKind;
     const KIND: AstFragmentKind;
     fn to_annotatable(self) -> Annotatable;
@@ -1873,9 +1873,9 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
     // Detect use of feature-gated or invalid attributes on macro invocations
     // since they will not be detected after macro expansion.
     #[allow(rustc::untranslatable_diagnostic)] // FIXME: make this translatable
-    fn check_attributes(&self, attrs: &[ast::Attribute], call: &ast::MacCall) {
+    fn check_attributes(&self, attrs: &mut [ast::Attribute], call: &ast::MacCall) {
         let features = self.cx.ecfg.features;
-        let mut attrs = attrs.iter().peekable();
+        let mut attrs = attrs.iter_mut().peekable();
         let mut span: Option<Span> = None;
         while let Some(attr) = attrs.next() {
             rustc_ast_passes::feature_gate::check_attribute(attr, self.cx.sess, features);
@@ -1918,10 +1918,10 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
     fn expand_cfg_true(
         &mut self,
         node: &mut impl HasAttrs,
-        attr: ast::Attribute,
+        mut attr: ast::Attribute,
         pos: usize,
     ) -> (bool, Option<ast::MetaItem>) {
-        let (res, meta_item) = self.cfg().cfg_true(&attr);
+        let (res, meta_item) = self.cfg().cfg_true(&mut attr);
         if res {
             // FIXME: `cfg(TRUE)` attributes do not currently remove themselves during expansion,
             // and some tools like rustdoc and clippy rely on that. Find a way to remove them
@@ -1978,8 +1978,8 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
                     }
                 },
                 None if node.is_mac_call() => {
-                    let (mac, attrs, add_semicolon) = node.take_mac_call();
-                    self.check_attributes(&attrs, &mac);
+                    let (mac, mut attrs, add_semicolon) = node.take_mac_call();
+                    self.check_attributes(&mut attrs, &mac);
                     let mut res = self.collect_bang(mac, Node::KIND).make_ast::<Node>();
                     Node::post_flat_map_node_collect_bang(&mut res, add_semicolon);
                     res
@@ -2058,8 +2058,8 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
                 None if node.is_mac_call() => {
                     visit_clobber(node, |node| {
                         // Do not clobber unless it's actually a macro (uncommon case).
-                        let (mac, attrs, _) = node.take_mac_call();
-                        self.check_attributes(&attrs, &mac);
+                        let (mac, mut attrs, _) = node.take_mac_call();
+                        self.check_attributes(&mut attrs, &mac);
                         self.collect_bang(mac, Node::KIND).make_ast::<Node>()
                     })
                 }
diff --git a/compiler/rustc_expand/src/module.rs b/compiler/rustc_expand/src/module.rs
@@ -4,7 +4,7 @@ use crate::errors::{
 };
 use rustc_ast::ptr::P;
 use rustc_ast::{token, AttrVec, Attribute, Inline, Item, ModSpans};
-use rustc_errors::{Diag, ErrorGuaranteed};
+use rustc_errors::{Diag, ErrorGuaranteed, FatalError};
 use rustc_parse::validate_attr;
 use rustc_parse::{new_parser_from_file, unwrap_or_emit_fatal};
 use rustc_session::parse::ParseSess;
@@ -177,7 +177,7 @@ fn mod_file_path_from_attr(
 ) -> Option<PathBuf> {
     // Extract path string from first `#[path = "path_string"]` attribute.
     let first_path = attrs.iter().find(|at| at.has_name(sym::path))?;
-    let Some(path_sym) = first_path.value_str() else {
+    let Some(path_sym) = first_path.try_value_str().unwrap_or_else(|_| FatalError.raise()) else {
         // This check is here mainly to catch attempting to use a macro,
         // such as #[path = concat!(...)]. This isn't currently supported
         // because otherwise the InvocationCollector would need to defer
diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs
@@ -8,6 +8,7 @@ use rustc_codegen_ssa::traits::CodegenBackend;
 use rustc_data_structures::parallel;
 use rustc_data_structures::steal::Steal;
 use rustc_data_structures::sync::{AppendOnlyIndexVec, FreezeLock, Lrc, OnceLock, WorkerLocal};
+use rustc_errors::FatalError;
 use rustc_expand::base::{ExtCtxt, LintStoreExpand};
 use rustc_feature::Features;
 use rustc_fs_util::try_canonicalize;
@@ -658,7 +659,7 @@ pub(crate) fn create_global_ctxt<'tcx>(
         &sess.opts.unstable_opts.crate_attr,
     );
 
-    let pre_configured_attrs = rustc_expand::config::pre_configure_attrs(sess, &krate.attrs);
+    let pre_configured_attrs = rustc_expand::config::pre_configure_attrs(sess, &mut krate.attrs);
 
     // parse `#[crate_name]` even if `--crate-name` was passed, to make sure it matches.
     let crate_name = find_crate_name(sess, &pre_configured_attrs);
@@ -1045,10 +1046,10 @@ pub(crate) fn start_codegen<'tcx>(
 }
 
 fn get_recursion_limit(krate_attrs: &[ast::Attribute], sess: &Session) -> Limit {
-    if let Some(attr) = krate_attrs
-        .iter()
-        .find(|attr| attr.has_name(sym::recursion_limit) && attr.value_str().is_none())
-    {
+    if let Some(attr) = krate_attrs.iter().find(|attr| {
+        attr.has_name(sym::recursion_limit)
+            && attr.try_value_str().unwrap_or_else(|_| FatalError.raise()).is_none()
+    }) {
         // This is here mainly to check for using a macro, such as
         // #![recursion_limit = foo!()]. That is not supported since that
         // would require expanding this while in the middle of expansion,
diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs
@@ -3,6 +3,7 @@ use rustc_ast as ast;
 use rustc_codegen_ssa::traits::CodegenBackend;
 #[cfg(parallel_compiler)]
 use rustc_data_structures::sync;
+use rustc_errors::FatalError;
 use rustc_metadata::{load_symbol_from_dylib, DylibError};
 use rustc_middle::ty::CurrentGcx;
 use rustc_parse::validate_attr;
@@ -392,7 +393,7 @@ pub(crate) fn check_attr_crate_type(
     // Unconditionally collect crate types from attributes to make them used
     for a in attrs.iter() {
         if a.has_name(sym::crate_type) {
-            if let Some(n) = a.value_str() {
+            if let Some(n) = a.try_value_str().unwrap_or_else(|_| FatalError.raise()) {
                 if categorize_crate_type(n).is_some() {
                     return;
                 }
diff --git a/compiler/rustc_parse/src/validate_attr.rs b/compiler/rustc_parse/src/validate_attr.rs
diff --git a/tests/ui/parser/bad-lit-suffixes.rs b/tests/ui/parser/bad-lit-suffixes.rs
diff --git a/tests/ui/parser/bad-lit-suffixes.stderr b/tests/ui/parser/bad-lit-suffixes.stderr

Original file line number	Diff line number	Diff line change
`@@ -949,14 +949,15 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {`
`949`	`949`	`&& let Ok(lit) = MetaItemLit::from_token_lit(token_lit, expr.span)`
`950`	`950`	`{`
`951`	`951`	`lit`
`952`		`- } else {`
`953`		`- let guar = self.dcx().has_errors().unwrap();`
	`952`	`+ } else if let ExprKind::Err(guar) = expr.kind {`
`954`	`953`	`MetaItemLit {`
`955`	`954`	`symbol: kw::Empty,`
`956`	`955`	`suffix: None,`
`957`	`956`	`kind: LitKind::Err(guar),`
`958`	`957`	`span: DUMMY_SP,`
`959`	`958`	`}`
	`959`	`+ } else {`
	`960`	`+ span_bug!(*eq_span, "eq attrs can only be literals, but got {expr:#?}")`
`960`	`961`	`};`
`961`	`962`	`AttrArgs::Eq(*eq_span, AttrArgsEq::Hir(lit))`
`962`	`963`	`}`