Refactor rustc_on_unimplemented's filter parser

8 files changed, 448 insertions, 165 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 97a90a44a39..a5b41b6edd5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -4520,7 +4520,6 @@ dependencies = [
  "itertools",
  "rustc_abi",
  "rustc_ast",
- "rustc_attr_parsing",
  "rustc_data_structures",
  "rustc_errors",
  "rustc_fluent_macro",
diff --git a/compiler/rustc_trait_selection/Cargo.toml b/compiler/rustc_trait_selection/Cargo.toml
index 1c61e23362a..e6de2a3978d 100644
--- a/compiler/rustc_trait_selection/Cargo.toml
+++ b/compiler/rustc_trait_selection/Cargo.toml
@@ -8,7 +8,6 @@ edition = "2024"
 itertools = "0.12"
 rustc_abi = { path = "../rustc_abi" }
 rustc_ast = { path = "../rustc_ast" }
-rustc_attr_parsing = { path = "../rustc_attr_parsing" }
 rustc_data_structures = { path = "../rustc_data_structures" }
 rustc_errors = { path = "../rustc_errors" }
 rustc_fluent_macro = { path = "../rustc_fluent_macro" }
diff --git a/compiler/rustc_trait_selection/messages.ftl b/compiler/rustc_trait_selection/messages.ftl
index 74e38f525c8..cf6dd40718b 100644
--- a/compiler/rustc_trait_selection/messages.ftl
+++ b/compiler/rustc_trait_selection/messages.ftl
@@ -148,9 +148,6 @@ trait_selection_dtcs_has_req_note = the used `impl` has a `'static` requirement
 trait_selection_dtcs_introduces_requirement = calling this method introduces the `impl`'s `'static` requirement
 trait_selection_dtcs_suggestion = consider relaxing the implicit `'static` requirement
 
-trait_selection_empty_on_clause_in_rustc_on_unimplemented = empty `on`-clause in `#[rustc_on_unimplemented]`
-    .label = empty on-clause here
-
 trait_selection_explicit_lifetime_required_sugg_with_ident = add explicit lifetime `{$named}` to the type of `{$simple_ident}`
 
 trait_selection_explicit_lifetime_required_sugg_with_param_type = add explicit lifetime `{$named}` to type
@@ -187,9 +184,6 @@ trait_selection_inherent_projection_normalization_overflow = overflow evaluating
 trait_selection_invalid_format_specifier = invalid format specifier
     .help = no format specifier are supported in this position
 
-trait_selection_invalid_on_clause_in_rustc_on_unimplemented = invalid `on`-clause in `#[rustc_on_unimplemented]`
-    .label = invalid on-clause here
-
 trait_selection_label_bad = {$bad_kind ->
     *[other] cannot infer type
     [more_info] cannot infer {$prefix_kind ->
@@ -237,10 +231,6 @@ trait_selection_negative_positive_conflict = found both positive and negative im
     .positive_implementation_here = positive implementation here
     .positive_implementation_in_crate = positive implementation in crate `{$positive_impl_cname}`
 
-trait_selection_no_value_in_rustc_on_unimplemented = this attribute must have a valid value
-    .label = expected value here
-    .note = eg `#[rustc_on_unimplemented(message="foo")]`
-
 trait_selection_nothing = {""}
 
 trait_selection_oc_cant_coerce_force_inline =
@@ -339,6 +329,22 @@ trait_selection_ril_introduced_by = requirement introduced by this return type
 trait_selection_ril_introduced_here = `'static` requirement introduced here
 trait_selection_ril_static_introduced_by = "`'static` lifetime requirement introduced by the return type
 
+trait_selection_rustc_on_unimplemented_empty_on_clause = empty `on`-clause in `#[rustc_on_unimplemented]`
+    .label = empty `on`-clause here
+trait_selection_rustc_on_unimplemented_expected_identifier = expected an identifier inside this `on`-clause
+    .label = expected an identifier here, not `{$path}`
+trait_selection_rustc_on_unimplemented_expected_one_predicate_in_not = expected a single predicate in `not(..)`
+    .label = unexpected quantity of predicates here
+trait_selection_rustc_on_unimplemented_invalid_flag = invalid flag in `on`-clause
+    .label = expected one of the `crate_local`, `direct` or `from_desugaring` flags, not `{$invalid_flag}`
+trait_selection_rustc_on_unimplemented_invalid_predicate = this predicate is invalid
+    .label = expected one of `any`, `all` or `not` here, not `{$invalid_pred}`
+trait_selection_rustc_on_unimplemented_missing_value = this attribute must have a value
+    .label = expected value here
+    .note = e.g. `#[rustc_on_unimplemented(message="foo")]`
+trait_selection_rustc_on_unimplemented_unsupported_literal_in_on = literals inside `on`-clauses are not supported
+    .label = unexpected literal here
+
 trait_selection_source_kind_closure_return =
     try giving this closure an explicit return type
 
diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented.rs
index 4c4491269b7..d5ee6e2123a 100644
--- a/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented.rs
+++ b/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented.rs
@@ -4,6 +4,7 @@ use std::path::PathBuf;
 use rustc_ast::{LitKind, MetaItem, MetaItemInner, MetaItemKind, MetaItemLit};
 use rustc_errors::codes::*;
 use rustc_errors::{ErrorGuaranteed, struct_span_code_err};
+use rustc_hir as hir;
 use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::{AttrArgs, Attribute};
 use rustc_macros::LintDiagnostic;
@@ -13,17 +14,16 @@ use rustc_middle::ty::{self, GenericArgsRef, GenericParamDef, GenericParamDefKin
 use rustc_session::lint::builtin::UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES;
 use rustc_span::{Span, Symbol, sym};
 use tracing::{debug, info};
-use {rustc_attr_parsing as attr, rustc_hir as hir};
 
 use super::{ObligationCauseCode, PredicateObligation};
 use crate::error_reporting::TypeErrCtxt;
-use crate::error_reporting::traits::on_unimplemented_condition::{Condition, ConditionOptions};
+use crate::error_reporting::traits::on_unimplemented_condition::{
+    ConditionOptions, OnUnimplementedCondition,
+};
 use crate::error_reporting::traits::on_unimplemented_format::{
     Ctx, FormatArgs, FormatString, FormatWarning,
 };
-use crate::errors::{
-    EmptyOnClauseInOnUnimplemented, InvalidOnClauseInOnUnimplemented, NoValueInOnUnimplemented,
-};
+use crate::errors::{InvalidOnClause, NoValueInOnUnimplemented};
 use crate::infer::InferCtxtExt;
 
 impl<'tcx> TypeErrCtxt<'_, 'tcx> {
@@ -306,21 +306,21 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
 #[derive(Clone, Debug)]
 pub struct OnUnimplementedFormatString {
     /// Symbol of the format string, i.e. `"content"`
-    pub symbol: Symbol,
+    symbol: Symbol,
     ///The span of the format string, i.e. `"content"`
-    pub span: Span,
-    pub is_diagnostic_namespace_variant: bool,
+    span: Span,
+    is_diagnostic_namespace_variant: bool,
 }
 
 #[derive(Debug)]
 pub struct OnUnimplementedDirective {
-    pub condition: Option<Condition>,
-    pub subcommands: Vec<OnUnimplementedDirective>,
-    pub message: Option<(Span, OnUnimplementedFormatString)>,
-    pub label: Option<(Span, OnUnimplementedFormatString)>,
-    pub notes: Vec<OnUnimplementedFormatString>,
-    pub parent_label: Option<OnUnimplementedFormatString>,
-    pub append_const_msg: Option<AppendConstMessage>,
+    condition: Option<OnUnimplementedCondition>,
+    subcommands: Vec<OnUnimplementedDirective>,
+    message: Option<(Span, OnUnimplementedFormatString)>,
+    label: Option<(Span, OnUnimplementedFormatString)>,
+    notes: Vec<OnUnimplementedFormatString>,
+    parent_label: Option<OnUnimplementedFormatString>,
+    append_const_msg: Option<AppendConstMessage>,
 }
 
 /// For the `#[rustc_on_unimplemented]` attribute
@@ -427,18 +427,12 @@ impl<'tcx> OnUnimplementedDirective {
         } else {
             let cond = item_iter
                 .next()
-                .ok_or_else(|| tcx.dcx().emit_err(EmptyOnClauseInOnUnimplemented { span }))?
-                .meta_item_or_bool()
-                .ok_or_else(|| tcx.dcx().emit_err(InvalidOnClauseInOnUnimplemented { span }))?;
-            attr::eval_condition(cond, &tcx.sess, Some(tcx.features()), &mut |cfg| {
-                if let Some(value) = cfg.value
-                    && let Err(guar) = parse_value(value, cfg.span)
-                {
-                    errored = Some(guar);
-                }
-                true
-            });
-            Some(Condition { inner: cond.clone() })
+                .ok_or_else(|| tcx.dcx().emit_err(InvalidOnClause::Empty { span }))?;
+
+            match OnUnimplementedCondition::parse(cond) {
+                Ok(condition) => Some(condition),
+                Err(e) => return Err(tcx.dcx().emit_err(e)),
+            }
         };
 
         let mut message = None;
@@ -724,7 +718,7 @@ impl<'tcx> OnUnimplementedDirective {
         result
     }
 
-    pub fn evaluate(
+    pub(crate) fn evaluate(
         &self,
         tcx: TyCtxt<'tcx>,
         trait_ref: ty::TraitRef<'tcx>,
@@ -744,7 +738,7 @@ impl<'tcx> OnUnimplementedDirective {
         for command in self.subcommands.iter().chain(Some(self)).rev() {
             debug!(?command);
             if let Some(ref condition) = command.condition
-                && !condition.matches_predicate(tcx, condition_options)
+                && !condition.matches_predicate(condition_options)
             {
                 debug!("evaluate: skipping {:?} due to condition", command);
                 continue;
diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs
index 116cfb01cb6..13753761f09 100644
--- a/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs
+++ b/compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs
@@ -1,52 +1,251 @@
-use rustc_ast::MetaItemInner;
-use rustc_attr_parsing as attr;
-use rustc_middle::ty::{self, TyCtxt};
+use rustc_ast::{MetaItemInner, MetaItemKind, MetaItemLit};
 use rustc_parse_format::{ParseMode, Parser, Piece, Position};
-use rustc_span::{DesugaringKind, Span, Symbol, kw, sym};
+use rustc_span::{DesugaringKind, Ident, Span, Symbol, kw, sym};
 
-/// A predicate in an attribute using on, all, any,
-/// similar to a cfg predicate.
+use crate::errors::InvalidOnClause;
+
+/// Represents the `on` filter in `#[rustc_on_unimplemented]`.
 #[derive(Debug)]
-pub struct Condition {
-    pub inner: MetaItemInner,
+pub(crate) struct OnUnimplementedCondition {
+    span: Span,
+    pred: Predicate,
 }
 
-impl Condition {
-    pub fn span(&self) -> Span {
-        self.inner.span()
+impl OnUnimplementedCondition {
+    pub(crate) fn span(&self) -> Span {
+        self.span
+    }
+
+    pub(crate) fn matches_predicate(&self, options: &ConditionOptions) -> bool {
+        self.pred.eval(&mut |p| match p {
+            FlagOrNv::Flag(b) => options.has_flag(*b),
+            FlagOrNv::NameValue(NameValue { name, value }) => {
+                let value = value.format(&options.generic_args);
+                options.contains(*name, value)
+            }
+        })
+    }
+
+    pub(crate) fn parse(input: &MetaItemInner) -> Result<Self, InvalidOnClause> {
+        let span = input.span();
+        let pred = Predicate::parse(input)?;
+        Ok(OnUnimplementedCondition { span, pred })
     }
+}
 
-    pub fn matches_predicate<'tcx>(&self, tcx: TyCtxt<'tcx>, options: &ConditionOptions) -> bool {
-        attr::eval_condition(&self.inner, tcx.sess, Some(tcx.features()), &mut |cfg| {
-            let value = cfg.value.map(|v| {
-                // `with_no_visible_paths` is also used when generating the options,
-                // so we need to match it here.
-                ty::print::with_no_visible_paths!({
-                    Parser::new(v.as_str(), None, None, false, ParseMode::Format)
-                        .map(|p| match p {
-                            Piece::Lit(s) => s.to_owned(),
-                            Piece::NextArgument(a) => match a.position {
-                                Position::ArgumentNamed(arg) => {
-                                    let s = Symbol::intern(arg);
-                                    match options.generic_args.iter().find(|(k, _)| *k == s) {
-                                        Some((_, val)) => val.to_string(),
-                                        None => format!("{{{arg}}}"),
-                                    }
-                                }
-                                Position::ArgumentImplicitlyIs(_) => String::from("{}"),
-                                Position::ArgumentIs(idx) => format!("{{{idx}}}"),
-                            },
-                        })
-                        .collect()
-                })
+/// Predicate(s) in `#[rustc_on_unimplemented]`'s `on` filter. See [`OnUnimplementedCondition`].
+///
+/// It is similar to the predicate in the `cfg` attribute,
+/// and may contain nested predicates.
+#[derive(Debug)]
+enum Predicate {
+    /// A condition like `on(crate_local)`.
+    Flag(Flag),
+    /// A match, like `on(Rhs = "Whatever")`.
+    Match(NameValue),
+    /// Negation, like `on(not($pred))`.
+    Not(Box<Predicate>),
+    /// True if all predicates are true, like `on(all($a, $b, $c))`.
+    All(Vec<Predicate>),
+    /// True if any predicate is true, like `on(any($a, $b, $c))`.
+    Any(Vec<Predicate>),
+}
+
+impl Predicate {
+    fn parse(input: &MetaItemInner) -> Result<Self, InvalidOnClause> {
+        let meta_item = match input {
+            MetaItemInner::MetaItem(meta_item) => meta_item,
+            MetaItemInner::Lit(lit) => {
+                return Err(InvalidOnClause::UnsupportedLiteral { span: lit.span });
+            }
+        };
+
+        let Some(predicate) = meta_item.ident() else {
+            return Err(InvalidOnClause::ExpectedIdentifier {
+                span: meta_item.path.span,
+                path: meta_item.path.clone(),
             });
+        };
 
-            options.contains(cfg.name, &value)
-        })
+        match meta_item.kind {
+            MetaItemKind::List(ref mis) => match predicate.name {
+                sym::any => Ok(Predicate::Any(Predicate::parse_sequence(mis)?)),
+                sym::all => Ok(Predicate::All(Predicate::parse_sequence(mis)?)),
+                sym::not => match &**mis {
+                    [one] => Ok(Predicate::Not(Box::new(Predicate::parse(one)?))),
+                    [first, .., last] => Err(InvalidOnClause::ExpectedOnePredInNot {
+                        span: first.span().to(last.span()),
+                    }),
+                    [] => Err(InvalidOnClause::ExpectedOnePredInNot { span: meta_item.span }),
+                },
+                invalid_pred => {
+                    Err(InvalidOnClause::InvalidPredicate { span: predicate.span, invalid_pred })
+                }
+            },
+            MetaItemKind::NameValue(MetaItemLit { symbol, .. }) => {
+                let name = Name::parse(predicate);
+                let value = FilterFormatString::parse(symbol);
+                let kv = NameValue { name, value };
+                Ok(Predicate::Match(kv))
+            }
+            MetaItemKind::Word => {
+                let flag = Flag::parse(predicate)?;
+                Ok(Predicate::Flag(flag))
+            }
+        }
+    }
+
+    fn parse_sequence(sequence: &[MetaItemInner]) -> Result<Vec<Self>, InvalidOnClause> {
+        sequence.iter().map(Predicate::parse).collect()
+    }
+
+    fn eval(&self, eval: &mut impl FnMut(FlagOrNv<'_>) -> bool) -> bool {
+        match self {
+            Predicate::Flag(flag) => eval(FlagOrNv::Flag(flag)),
+            Predicate::Match(nv) => eval(FlagOrNv::NameValue(nv)),
+            Predicate::Not(not) => !not.eval(eval),
+            Predicate::All(preds) => preds.into_iter().all(|pred| pred.eval(eval)),
+            Predicate::Any(preds) => preds.into_iter().any(|pred| pred.eval(eval)),
+        }
     }
 }
 
-/// Used with `Condition::matches_predicate` to test whether the condition applies
+/// Represents a `MetaWord` in an `on`-filter.
+#[derive(Debug, Clone, Copy)]
+enum Flag {
+    /// Whether the code causing the trait bound to not be fulfilled
+    /// is part of the user's crate.
+    CrateLocal,
+    /// Whether the obligation is user-specified rather than derived.
+    Direct,
+    /// Whether we are in some kind of desugaring like
+    /// `?` or `try { .. }`.
+    FromDesugaring,
+}
+
+impl Flag {
+    fn parse(Ident { name, span }: Ident) -> Result<Self, InvalidOnClause> {
+        match name {
+            sym::crate_local => Ok(Flag::CrateLocal),
+            sym::direct => Ok(Flag::Direct),
+            sym::from_desugaring => Ok(Flag::FromDesugaring),
+            invalid_flag => Err(InvalidOnClause::InvalidFlag { invalid_flag, span }),
+        }
+    }
+}
+
+/// A `MetaNameValueStr` in an `on`-filter.
+///
+/// For example, `#[rustc_on_unimplemented(on(name = "value", message = "hello"))]`.
+#[derive(Debug, Clone)]
+struct NameValue {
+    name: Name,
+    /// Something like `"&str"` or `"alloc::string::String"`,
+    /// in which case it just contains a single string piece.
+    /// But if it is something like `"&[{A}]"` then it must be formatted later.
+    value: FilterFormatString,
+}
+
+/// The valid names of the `on` filter.
+#[derive(Debug, Clone, Copy)]
+enum Name {
+    Cause,
+    FromDesugaring,
+    SelfUpper,
+    GenericArg(Symbol),
+}
+
+impl Name {
+    fn parse(Ident { name, .. }: Ident) -> Self {
+        match name {
+            sym::_Self | kw::SelfUpper => Name::SelfUpper,
+            sym::from_desugaring => Name::FromDesugaring,
+            sym::cause => Name::Cause,
+            // FIXME(mejrs) Perhaps we should start checking that
+            // this actually is a valid generic parameter?
+            generic => Name::GenericArg(generic),
+        }
+    }
+}
+
+#[derive(Debug, Clone)]
+enum FlagOrNv<'p> {
+    Flag(&'p Flag),
+    NameValue(&'p NameValue),
+}
+
+/// Represents a value inside an `on` filter.
+///
+/// For example, `#[rustc_on_unimplemented(on(name = "value", message = "hello"))]`.
+/// If it is a simple literal like this then `pieces` will be `[LitOrArg::Lit("value")]`.
+/// The `Arg` variant is used when it contains formatting like
+/// `#[rustc_on_unimplemented(on(Self = "&[{A}]", message = "hello"))]`.
+#[derive(Debug, Clone)]
+struct FilterFormatString {
+    pieces: Vec<LitOrArg>,
+}
+
+#[derive(Debug, Clone)]
+enum LitOrArg {
+    Lit(String),
+    Arg(String),
+}
+
+impl FilterFormatString {
+    fn parse(input: Symbol) -> Self {
+        let pieces = Parser::new(input.as_str(), None, None, false, ParseMode::Format)
+            .map(|p| match p {
+                Piece::Lit(s) => LitOrArg::Lit(s.to_owned()),
+                // We just ignore formatspecs here
+                Piece::NextArgument(a) => match a.position {
+                    // In `TypeErrCtxt::on_unimplemented_note` we substitute `"{integral}"` even
+                    // if the integer type has been resolved, to allow targeting all integers.
+                    // `"{integer}"` and `"{float}"` come from numerics that haven't been inferred yet,
+                    // from the `Display` impl of `InferTy` to be precise.
+                    //
+                    // Don't try to format these later!
+                    Position::ArgumentNamed(arg @ "integer" | arg @ "integral" | arg @ "float") => {
+                        LitOrArg::Lit(format!("{{{arg}}}"))
+                    }
+
+                    // FIXME(mejrs) We should check if these correspond to a generic of the trait.
+                    Position::ArgumentNamed(arg) => LitOrArg::Arg(arg.to_owned()),
+
+                    // FIXME(mejrs) These should really be warnings/errors
+                    Position::ArgumentImplicitlyIs(_) => LitOrArg::Lit(String::from("{}")),
+                    Position::ArgumentIs(idx) => LitOrArg::Lit(format!("{{{idx}}}")),
+                },
+            })
+            .collect();
+        Self { pieces }
+    }
+
+    fn format(&self, generic_args: &[(Symbol, String)]) -> String {
+        let mut ret = String::new();
+
+        for piece in &self.pieces {
+            match piece {
+                LitOrArg::Lit(s) => ret.push_str(s),
+                LitOrArg::Arg(arg) => {
+                    let s = Symbol::intern(arg);
+                    match generic_args.iter().find(|(k, _)| *k == s) {
+                        Some((_, val)) => ret.push_str(val),
+                        None => {
+                            // FIXME(mejrs) If we start checking as mentioned in
+                            // FilterFormatString::parse then this shouldn't happen
+                            let _ = std::fmt::write(&mut ret, format_args!("{{{s}}}"));
+                        }
+                    }
+                }
+            }
+        }
+
+        ret
+    }
+}
+
+/// Used with `OnUnimplementedCondition::matches_predicate` to evaluate the
+/// [`OnUnimplementedCondition`].
 ///
 /// For example, given a
 /// ```rust,ignore (just an example)
@@ -85,36 +284,34 @@ impl Condition {
 /// }
 /// ```
 #[derive(Debug)]
-pub struct ConditionOptions {
+pub(crate) struct ConditionOptions {
     /// All the self types that may apply.
-    /// for example
-    pub self_types: Vec<String>,
+    pub(crate) self_types: Vec<String>,
     // The kind of compiler desugaring.
-    pub from_desugaring: Option<DesugaringKind>,
-    /// Match on a variant of [rustc_infer::traits::ObligationCauseCode]
-    pub cause: Option<String>,
-    pub crate_local: bool,
+    pub(crate) from_desugaring: Option<DesugaringKind>,
+    /// Match on a variant of [rustc_infer::traits::ObligationCauseCode].
+    pub(crate) cause: Option<String>,
+    pub(crate) crate_local: bool,
     /// Is the obligation "directly" user-specified, rather than derived?
-    pub direct: bool,
-    // A list of the generic arguments and their reified types
-    pub generic_args: Vec<(Symbol, String)>,
+    pub(crate) direct: bool,
+    // A list of the generic arguments and their reified types.
+    pub(crate) generic_args: Vec<(Symbol, String)>,
 }
 
 impl ConditionOptions {
-    pub fn contains(&self, key: Symbol, value: &Option<String>) -> bool {
-        match (key, value) {
-            (sym::_Self | kw::SelfUpper, Some(value)) => self.self_types.contains(&value),
-            // from_desugaring as a flag
-            (sym::from_desugaring, None) => self.from_desugaring.is_some(),
-            // from_desugaring as key == value
-            (sym::from_desugaring, Some(v)) if let Some(ds) = self.from_desugaring => ds.matches(v),
-            (sym::cause, Some(value)) => self.cause.as_deref() == Some(value),
-            (sym::crate_local, None) => self.crate_local,
-            (sym::direct, None) => self.direct,
-            (other, Some(value)) => {
-                self.generic_args.iter().any(|(k, v)| *k == other && v == value)
-            }
-            _ => false,
+    fn has_flag(&self, name: Flag) -> bool {
+        match name {
+            Flag::CrateLocal => self.crate_local,
+            Flag::Direct => self.direct,
+            Flag::FromDesugaring => self.from_desugaring.is_some(),
+        }
+    }
+    fn contains(&self, name: Name, value: String) -> bool {
+        match name {
+            Name::SelfUpper => self.self_types.contains(&value),
+            Name::FromDesugaring => self.from_desugaring.is_some_and(|ds| ds.matches(&value)),
+            Name::Cause => self.cause == Some(value),
+            Name::GenericArg(arg) => self.generic_args.contains(&(arg, value)),
         }
     }
 }
diff --git a/compiler/rustc_trait_selection/src/errors.rs b/compiler/rustc_trait_selection/src/errors.rs
index 756d9a57b93..21d05312481 100644
--- a/compiler/rustc_trait_selection/src/errors.rs
+++ b/compiler/rustc_trait_selection/src/errors.rs
@@ -1,5 +1,6 @@
 use std::path::PathBuf;
 
+use rustc_ast::Path;
 use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
 use rustc_errors::codes::*;
 use rustc_errors::{
@@ -31,23 +32,50 @@ pub struct UnableToConstructConstantValue<'a> {
 }
 
 #[derive(Diagnostic)]
-#[diag(trait_selection_empty_on_clause_in_rustc_on_unimplemented, code = E0232)]
-pub struct EmptyOnClauseInOnUnimplemented {
-    #[primary_span]
-    #[label]
-    pub span: Span,
-}
-
-#[derive(Diagnostic)]
-#[diag(trait_selection_invalid_on_clause_in_rustc_on_unimplemented, code = E0232)]
-pub struct InvalidOnClauseInOnUnimplemented {
-    #[primary_span]
-    #[label]
-    pub span: Span,
+pub enum InvalidOnClause {
+    #[diag(trait_selection_rustc_on_unimplemented_empty_on_clause, code = E0232)]
+    Empty {
+        #[primary_span]
+        #[label]
+        span: Span,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_expected_one_predicate_in_not, code = E0232)]
+    ExpectedOnePredInNot {
+        #[primary_span]
+        #[label]
+        span: Span,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_unsupported_literal_in_on, code = E0232)]
+    UnsupportedLiteral {
+        #[primary_span]
+        #[label]
+        span: Span,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_expected_identifier, code = E0232)]
+    ExpectedIdentifier {
+        #[primary_span]
+        #[label]
+        span: Span,
+        path: Path,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_invalid_predicate, code = E0232)]
+    InvalidPredicate {
+        #[primary_span]
+        #[label]
+        span: Span,
+        invalid_pred: Symbol,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_invalid_flag, code = E0232)]
+    InvalidFlag {
+        #[primary_span]
+        #[label]
+        span: Span,
+        invalid_flag: Symbol,
+    },
 }
 
 #[derive(Diagnostic)]
-#[diag(trait_selection_no_value_in_rustc_on_unimplemented, code = E0232)]
+#[diag(trait_selection_rustc_on_unimplemented_missing_value, code = E0232)]
 #[note]
 pub struct NoValueInOnUnimplemented {
     #[primary_span]
diff --git a/tests/ui/on-unimplemented/bad-annotation.rs b/tests/ui/on-unimplemented/bad-annotation.rs
index 32b825ee3cc..25de5978110 100644
--- a/tests/ui/on-unimplemented/bad-annotation.rs
+++ b/tests/ui/on-unimplemented/bad-annotation.rs
@@ -25,49 +25,85 @@ trait ParameterNotPresent<A, B> {}
 trait NoPositionalArgs<A, B> {}
 
 #[rustc_on_unimplemented(lorem = "")]
-//~^ ERROR this attribute must have a valid
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
 trait EmptyMessage {}
 
 #[rustc_on_unimplemented(lorem(ipsum(dolor)))]
-//~^ ERROR this attribute must have a valid
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
 trait Invalid {}
 
 #[rustc_on_unimplemented(message = "x", message = "y")]
-//~^ ERROR this attribute must have a valid
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
 trait DuplicateMessage {}
 
 #[rustc_on_unimplemented(message = "x", on(desugared, message = "y"))]
-//~^ ERROR this attribute must have a valid
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
 trait OnInWrongPosition {}
 
 #[rustc_on_unimplemented(on(), message = "y")]
 //~^ ERROR empty `on`-clause
-trait NoEmptyOn {}
+//~^^ NOTE empty `on`-clause here
+trait EmptyOn {}
 
 #[rustc_on_unimplemented(on = "x", message = "y")]
-//~^ ERROR this attribute must have a valid
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
 trait ExpectedPredicateInOn {}
 
 #[rustc_on_unimplemented(on(x = "y"), message = "y")]
 trait OnWithoutDirectives {}
 
-#[rustc_on_unimplemented(on(desugared, on(desugared, message = "x")), message = "y")]
-//~^ ERROR this attribute must have a valid
-trait NoNestedOn {}
+#[rustc_on_unimplemented(on(from_desugaring, on(from_desugaring, message = "x")), message = "y")]
+//~^ ERROR this attribute must have a value
+//~^^ NOTE e.g. `#[rustc_on_unimplemented(message="foo")]`
+//~^^^ NOTE expected value here
+trait NestedOn {}
 
-// caught by `OnUnimplementedDirective::parse`, *not* `eval_condition`
 #[rustc_on_unimplemented(on("y", message = "y"))]
-//~^ ERROR invalid `on`-clause
+//~^ ERROR literals inside `on`-clauses are not supported
+//~^^ NOTE unexpected literal here
 trait UnsupportedLiteral {}
 
+#[rustc_on_unimplemented(on(42, message = "y"))]
+//~^ ERROR literals inside `on`-clauses are not supported
+//~^^ NOTE unexpected literal here
+trait UnsupportedLiteral2 {}
+
 #[rustc_on_unimplemented(on(not(a, b), message = "y"))]
-//~^ ERROR expected 1 cfg-pattern
+//~^ ERROR expected a single predicate in `not(..)` [E0232]
+//~^^ NOTE unexpected quantity of predicates here
 trait ExpectedOnePattern {}
 
+#[rustc_on_unimplemented(on(not(), message = "y"))]
+//~^ ERROR expected a single predicate in `not(..)` [E0232]
+//~^^ NOTE unexpected quantity of predicates here
+trait ExpectedOnePattern2 {}
+
 #[rustc_on_unimplemented(on(thing::What, message = "y"))]
-//~^ ERROR `cfg` predicate key must be an identifier
+//~^ ERROR expected an identifier inside this `on`-clause
+//~^^ NOTE expected an identifier here, not `thing::What`
 trait KeyMustBeIdentifier {}
 
 #[rustc_on_unimplemented(on(thing::What = "value", message = "y"))]
-//~^ ERROR `cfg` predicate key must be an identifier
+//~^ ERROR  expected an identifier inside this `on`-clause
+//~^^ NOTE expected an identifier here, not `thing::What`
 trait KeyMustBeIdentifier2 {}
+
+#[rustc_on_unimplemented(on(aaaaaaaaaaaaaa(a, b), message = "y"))]
+//~^ ERROR this predicate is invalid
+//~^^ NOTE expected one of `any`, `all` or `not` here, not `aaaaaaaaaaaaaa`
+trait InvalidPredicate {}
+
+#[rustc_on_unimplemented(on(something, message = "y"))]
+//~^ ERROR invalid flag in `on`-clause
+//~^^ NOTE expected one of the `crate_local`, `direct` or `from_desugaring` flags, not `something`
+trait InvalidFlag {}
diff --git a/tests/ui/on-unimplemented/bad-annotation.stderr b/tests/ui/on-unimplemented/bad-annotation.stderr
index 817d3c42cf6..35b919c7b78 100644
--- a/tests/ui/on-unimplemented/bad-annotation.stderr
+++ b/tests/ui/on-unimplemented/bad-annotation.stderr
@@ -23,85 +23,109 @@ error[E0231]: positional format arguments are not allowed here
 LL | #[rustc_on_unimplemented = "Unimplemented trait error on `{Self}` with params `<{A},{B},{}>`"]
    |                                                                                          ^
 
-error[E0232]: this attribute must have a valid value
+error[E0232]: this attribute must have a value
   --> $DIR/bad-annotation.rs:27:26
    |
 LL | #[rustc_on_unimplemented(lorem = "")]
    |                          ^^^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
-error[E0232]: this attribute must have a valid value
-  --> $DIR/bad-annotation.rs:31:26
+error[E0232]: this attribute must have a value
+  --> $DIR/bad-annotation.rs:33:26
    |
 LL | #[rustc_on_unimplemented(lorem(ipsum(dolor)))]
    |                          ^^^^^^^^^^^^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
-error[E0232]: this attribute must have a valid value
-  --> $DIR/bad-annotation.rs:35:41
+error[E0232]: this attribute must have a value
+  --> $DIR/bad-annotation.rs:39:41
    |
 LL | #[rustc_on_unimplemented(message = "x", message = "y")]
    |                                         ^^^^^^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
-error[E0232]: this attribute must have a valid value
-  --> $DIR/bad-annotation.rs:39:41
+error[E0232]: this attribute must have a value
+  --> $DIR/bad-annotation.rs:45:41
    |
 LL | #[rustc_on_unimplemented(message = "x", on(desugared, message = "y"))]
    |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
 error[E0232]: empty `on`-clause in `#[rustc_on_unimplemented]`
-  --> $DIR/bad-annotation.rs:43:26
+  --> $DIR/bad-annotation.rs:51:26
    |
 LL | #[rustc_on_unimplemented(on(), message = "y")]
-   |                          ^^^^ empty on-clause here
+   |                          ^^^^ empty `on`-clause here
 
-error[E0232]: this attribute must have a valid value
-  --> $DIR/bad-annotation.rs:47:26
+error[E0232]: this attribute must have a value
+  --> $DIR/bad-annotation.rs:56:26
    |
 LL | #[rustc_on_unimplemented(on = "x", message = "y")]
    |                          ^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
-error[E0232]: this attribute must have a valid value
-  --> $DIR/bad-annotation.rs:54:40
+error[E0232]: this attribute must have a value
+  --> $DIR/bad-annotation.rs:65:46
    |
-LL | #[rustc_on_unimplemented(on(desugared, on(desugared, message = "x")), message = "y")]
-   |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected value here
+LL | #[rustc_on_unimplemented(on(from_desugaring, on(from_desugaring, message = "x")), message = "y")]
+   |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected value here
    |
-   = note: eg `#[rustc_on_unimplemented(message="foo")]`
+   = note: e.g. `#[rustc_on_unimplemented(message="foo")]`
 
-error[E0232]: invalid `on`-clause in `#[rustc_on_unimplemented]`
-  --> $DIR/bad-annotation.rs:59:26
+error[E0232]: literals inside `on`-clauses are not supported
+  --> $DIR/bad-annotation.rs:71:29
    |
 LL | #[rustc_on_unimplemented(on("y", message = "y"))]
-   |                          ^^^^^^^^^^^^^^^^^^^^^^ invalid on-clause here
+   |                             ^^^ unexpected literal here
 
-error[E0536]: expected 1 cfg-pattern
-  --> $DIR/bad-annotation.rs:63:29
+error[E0232]: literals inside `on`-clauses are not supported
+  --> $DIR/bad-annotation.rs:76:29
+   |
+LL | #[rustc_on_unimplemented(on(42, message = "y"))]
+   |                             ^^ unexpected literal here
+
+error[E0232]: expected a single predicate in `not(..)`
+  --> $DIR/bad-annotation.rs:81:33
    |
 LL | #[rustc_on_unimplemented(on(not(a, b), message = "y"))]
-   |                             ^^^^^^^^^
+   |                                 ^^^^ unexpected quantity of predicates here
+
+error[E0232]: expected a single predicate in `not(..)`
+  --> $DIR/bad-annotation.rs:86:29
+   |
+LL | #[rustc_on_unimplemented(on(not(), message = "y"))]
+   |                             ^^^^^ unexpected quantity of predicates here
 
-error: `cfg` predicate key must be an identifier
-  --> $DIR/bad-annotation.rs:67:29
+error[E0232]: expected an identifier inside this `on`-clause
+  --> $DIR/bad-annotation.rs:91:29
    |
 LL | #[rustc_on_unimplemented(on(thing::What, message = "y"))]
-   |                             ^^^^^^^^^^^
+   |                             ^^^^^^^^^^^ expected an identifier here, not `thing::What`
 
-error: `cfg` predicate key must be an identifier
-  --> $DIR/bad-annotation.rs:71:29
+error[E0232]: expected an identifier inside this `on`-clause
+  --> $DIR/bad-annotation.rs:96:29
    |
 LL | #[rustc_on_unimplemented(on(thing::What = "value", message = "y"))]
-   |                             ^^^^^^^^^^^
+   |                             ^^^^^^^^^^^ expected an identifier here, not `thing::What`
+
+error[E0232]: this predicate is invalid
+  --> $DIR/bad-annotation.rs:101:29
+   |
+LL | #[rustc_on_unimplemented(on(aaaaaaaaaaaaaa(a, b), message = "y"))]
+   |                             ^^^^^^^^^^^^^^ expected one of `any`, `all` or `not` here, not `aaaaaaaaaaaaaa`
+
+error[E0232]: invalid flag in `on`-clause
+  --> $DIR/bad-annotation.rs:106:29
+   |
+LL | #[rustc_on_unimplemented(on(something, message = "y"))]
+   |                             ^^^^^^^^^ expected one of the `crate_local`, `direct` or `from_desugaring` flags, not `something`
 
-error: aborting due to 14 previous errors
+error: aborting due to 18 previous errors
 
-Some errors have detailed explanations: E0230, E0231, E0232, E0536.
+Some errors have detailed explanations: E0230, E0231, E0232.
 For more information about an error, try `rustc --explain E0230`.