Skip to content

Commit

Permalink
Rollup merge of #118495 - weiznich:more_tests_for_on_unimplemented, r…
Browse files Browse the repository at this point in the history 
…=compiler-errors

Restrict what symbols can be used in `#[diagnostic::on_unimplemented]` format strings

This commit restricts what symbols can be used in a format string for any option of the `diagnostic::on_unimplemented` attribute. We previously allowed all the ad-hoc options supported by the internal `#[rustc_on_unimplemented]` attribute. For the stable attribute we only want to support generic parameter names and `{Self}` as parameters.  For any other parameter an warning is emitted and the parameter is replaced by the literal parameter string, so for example `{integer}` turns into `{integer}`. This follows the general design of attributes in the `#[diagnostic]` attribute namespace, that any syntax "error" is treated as warning and subsequently ignored.

r? `@compiler-errors`
  • Loading branch information
@TaKO8Ki
TaKO8Ki authored Dec 4, 2023
2 parents cf8d812 + 1a1cd6e commit da30882
Show file tree
Hide file tree
Showing 6 changed files with 523 additions and 61 deletions.
3 changes: 3 additions & 0 deletions compiler/rustc_trait_selection/messages.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,6 @@ trait_selection_trait_has_no_impls = this trait has no implementations, consider
trait_selection_ty_alias_overflow = in case this is a recursive type alias, consider using a struct, enum, or union instead
trait_selection_unable_to_construct_constant_value = unable to construct a constant value for the unevaluated constant {$unevaluated}
trait_selection_unknown_format_parameter_for_on_unimplemented_attr = there is no parameter `{$argument_name}` on trait `{$trait_name}`
.help = expect either a generic argument name or {"`{Self}`"} as format argument
147 changes: 95 additions & 52 deletions compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,11 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
}

#[derive(Clone, Debug)]
pub struct OnUnimplementedFormatString(Symbol, Span);
pub struct OnUnimplementedFormatString {
symbol: Symbol,
span: Span,
is_diagnostic_namespace_variant: bool,
}

#[derive(Debug)]
pub struct OnUnimplementedDirective {
Expand Down Expand Up @@ -401,6 +405,14 @@ impl IgnoredDiagnosticOption {
}
}

#[derive(LintDiagnostic)]
#[diag(trait_selection_unknown_format_parameter_for_on_unimplemented_attr)]
#[help]
pub struct UnknownFormatParameterForOnUnimplementedAttr {
argument_name: Symbol,
trait_name: Symbol,
}

impl<'tcx> OnUnimplementedDirective {
fn parse(
tcx: TyCtxt<'tcx>,
Expand All @@ -414,8 +426,14 @@ impl<'tcx> OnUnimplementedDirective {
let mut item_iter = items.iter();

let parse_value = |value_str, value_span| {
OnUnimplementedFormatString::try_parse(tcx, item_def_id, value_str, span, value_span)
.map(Some)
OnUnimplementedFormatString::try_parse(
tcx,
item_def_id,
value_str,
value_span,
is_diagnostic_namespace_variant,
)
.map(Some)
};

let condition = if is_root {
Expand Down Expand Up @@ -552,15 +570,15 @@ impl<'tcx> OnUnimplementedDirective {
IgnoredDiagnosticOption::maybe_emit_warning(
tcx,
item_def_id,
directive.message.as_ref().map(|f| f.1),
aggr.message.as_ref().map(|f| f.1),
directive.message.as_ref().map(|f| f.span),
aggr.message.as_ref().map(|f| f.span),
"message",
);
IgnoredDiagnosticOption::maybe_emit_warning(
tcx,
item_def_id,
directive.label.as_ref().map(|f| f.1),
aggr.label.as_ref().map(|f| f.1),
directive.label.as_ref().map(|f| f.span),
aggr.label.as_ref().map(|f| f.span),
"label",
);
IgnoredDiagnosticOption::maybe_emit_warning(
Expand All @@ -573,8 +591,8 @@ impl<'tcx> OnUnimplementedDirective {
IgnoredDiagnosticOption::maybe_emit_warning(
tcx,
item_def_id,
directive.parent_label.as_ref().map(|f| f.1),
aggr.parent_label.as_ref().map(|f| f.1),
directive.parent_label.as_ref().map(|f| f.span),
aggr.parent_label.as_ref().map(|f| f.span),
"parent_label",
);
IgnoredDiagnosticOption::maybe_emit_warning(
Expand Down Expand Up @@ -634,7 +652,7 @@ impl<'tcx> OnUnimplementedDirective {
item_def_id,
value,
attr.span,
attr.span,
is_diagnostic_namespace_variant,
)?),
notes: Vec::new(),
parent_label: None,
Expand Down Expand Up @@ -713,7 +731,12 @@ impl<'tcx> OnUnimplementedDirective {
// `with_no_visible_paths` is also used when generating the options,
// so we need to match it here.
ty::print::with_no_visible_paths!(
OnUnimplementedFormatString(v, cfg.span).format(
OnUnimplementedFormatString {
symbol: v,
span: cfg.span,
is_diagnostic_namespace_variant: false
}
.format(
tcx,
trait_ref,
&options_map
Expand Down Expand Up @@ -761,20 +784,19 @@ impl<'tcx> OnUnimplementedFormatString {
tcx: TyCtxt<'tcx>,
item_def_id: DefId,
from: Symbol,
err_sp: Span,
value_span: Span,
is_diagnostic_namespace_variant: bool,
) -> Result<Self, ErrorGuaranteed> {
let result = OnUnimplementedFormatString(from, value_span);
result.verify(tcx, item_def_id, err_sp)?;
let result = OnUnimplementedFormatString {
symbol: from,
span: value_span,
is_diagnostic_namespace_variant,
};
result.verify(tcx, item_def_id)?;
Ok(result)
}

fn verify(
&self,
tcx: TyCtxt<'tcx>,
item_def_id: DefId,
span: Span,
) -> Result<(), ErrorGuaranteed> {
fn verify(&self, tcx: TyCtxt<'tcx>, item_def_id: DefId) -> Result<(), ErrorGuaranteed> {
let trait_def_id = if tcx.is_trait(item_def_id) {
item_def_id
} else {
Expand All @@ -783,7 +805,7 @@ impl<'tcx> OnUnimplementedFormatString {
};
let trait_name = tcx.item_name(trait_def_id);
let generics = tcx.generics_of(item_def_id);
let s = self.0.as_str();
let s = self.symbol.as_str();
let parser = Parser::new(s, None, None, false, ParseMode::Format);
let mut result = Ok(());
for token in parser {
Expand All @@ -793,32 +815,48 @@ impl<'tcx> OnUnimplementedFormatString {
Position::ArgumentNamed(s) => {
match Symbol::intern(s) {
// `{ThisTraitsName}` is allowed
s if s == trait_name => (),
s if ALLOWED_FORMAT_SYMBOLS.contains(&s) => (),
s if s == trait_name && !self.is_diagnostic_namespace_variant => (),
s if ALLOWED_FORMAT_SYMBOLS.contains(&s)
&& !self.is_diagnostic_namespace_variant =>
{
()
}
// So is `{A}` if A is a type parameter
s if generics.params.iter().any(|param| param.name == s) => (),
s => {
result = Err(struct_span_err!(
tcx.sess,
span,
E0230,
"there is no parameter `{}` on {}",
s,
if trait_def_id == item_def_id {
format!("trait `{trait_name}`")
} else {
"impl".to_string()
}
)
.emit());
if self.is_diagnostic_namespace_variant {
tcx.emit_spanned_lint(
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
self.span,
UnknownFormatParameterForOnUnimplementedAttr {
argument_name: s,
trait_name,
},
);
} else {
result = Err(struct_span_err!(
tcx.sess,
self.span,
E0230,
"there is no parameter `{}` on {}",
s,
if trait_def_id == item_def_id {
format!("trait `{trait_name}`")
} else {
"impl".to_string()
}
)
.emit());
}
}
}
}
// `{:1}` and `{}` are not to be used
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
let reported = struct_span_err!(
tcx.sess,
span,
self.span,
E0231,
"only named substitution parameters are allowed"
)
Expand Down Expand Up @@ -857,45 +895,50 @@ impl<'tcx> OnUnimplementedFormatString {
.collect::<FxHashMap<Symbol, String>>();
let empty_string = String::new();

let s = self.0.as_str();
let s = self.symbol.as_str();
let parser = Parser::new(s, None, None, false, ParseMode::Format);
let item_context = (options.get(&sym::ItemContext)).unwrap_or(&empty_string);
parser
.map(|p| match p {
Piece::String(s) => s,
Piece::String(s) => s.to_owned(),
Piece::NextArgument(a) => match a.position {
Position::ArgumentNamed(s) => {
let s = Symbol::intern(s);
Position::ArgumentNamed(arg) => {
let s = Symbol::intern(arg);
match generic_map.get(&s) {
Some(val) => val,
None if s == name => &trait_str,
Some(val) => val.to_string(),
None if self.is_diagnostic_namespace_variant => {
format!("{{{arg}}}")
}
None if s == name => trait_str.clone(),
None => {
if let Some(val) = options.get(&s) {
val
val.clone()
} else if s == sym::from_desugaring {
// don't break messages using these two arguments incorrectly
&empty_string
} else if s == sym::ItemContext {
item_context
String::new()
} else if s == sym::ItemContext
&& !self.is_diagnostic_namespace_variant
{
item_context.clone()
} else if s == sym::integral {
"{integral}"
String::from("{integral}")
} else if s == sym::integer_ {
"{integer}"
String::from("{integer}")
} else if s == sym::float {
"{float}"
String::from("{float}")
} else {
bug!(
"broken on_unimplemented {:?} for {:?}: \
no argument matching {:?}",
self.0,
self.symbol,
trait_ref,
s
)
}
}
}
}
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.0),
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.symbol),
},
})
.collect()
Expand Down
67 changes: 67 additions & 0 deletions ...ostic_namespace/on_unimplemented/do_not_accept_options_of_the_internal_rustc_attribute.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#![feature(diagnostic_namespace)]

#[diagnostic::on_unimplemented(
on(_Self = "&str"),
//~^WARN malformed `on_unimplemented` attribute
//~|WARN malformed `on_unimplemented` attribute
message = "trait has `{Self}` and `{T}` as params",
label = "trait has `{Self}` and `{T}` as params",
note = "trait has `{Self}` and `{T}` as params",
parent_label = "in this scope",
//~^WARN malformed `on_unimplemented` attribute
//~|WARN malformed `on_unimplemented` attribute
append_const_msg
//~^WARN malformed `on_unimplemented` attribute
//~|WARN malformed `on_unimplemented` attribute
)]
trait Foo<T> {}

#[diagnostic::on_unimplemented = "Message"]
//~^WARN malformed `on_unimplemented` attribute
//~|WARN malformed `on_unimplemented` attribute
trait Bar {}

#[diagnostic::on_unimplemented(message = "Not allowed to apply it on a impl")]
//~^WARN #[diagnostic::on_unimplemented]` can only be applied to trait definitions
impl Bar for i32 {}

// cannot use special rustc_on_unimplement symbols
// in the format string
#[diagnostic::on_unimplemented(
message = "{from_desugaring}{direct}{cause}{integral}{integer}",
//~^WARN there is no parameter `from_desugaring` on trait `Baz`
//~|WARN there is no parameter `from_desugaring` on trait `Baz`
//~|WARN there is no parameter `direct` on trait `Baz`
//~|WARN there is no parameter `direct` on trait `Baz`
//~|WARN there is no parameter `cause` on trait `Baz`
//~|WARN there is no parameter `cause` on trait `Baz`
//~|WARN there is no parameter `integral` on trait `Baz`
//~|WARN there is no parameter `integral` on trait `Baz`
//~|WARN there is no parameter `integer` on trait `Baz`
//~|WARN there is no parameter `integer` on trait `Baz`
label = "{float}{_Self}{crate_local}{Trait}{ItemContext}"
//~^WARN there is no parameter `float` on trait `Baz`
//~|WARN there is no parameter `float` on trait `Baz`
//~|WARN there is no parameter `_Self` on trait `Baz`
//~|WARN there is no parameter `_Self` on trait `Baz`
//~|WARN there is no parameter `crate_local` on trait `Baz`
//~|WARN there is no parameter `crate_local` on trait `Baz`
//~|WARN there is no parameter `Trait` on trait `Baz`
//~|WARN there is no parameter `Trait` on trait `Baz`
//~|WARN there is no parameter `ItemContext` on trait `Baz`
//~|WARN there is no parameter `ItemContext` on trait `Baz`
)]
trait Baz {}

fn takes_foo(_: impl Foo<i32>) {}
fn takes_bar(_: impl Bar) {}
fn takes_baz(_: impl Baz) {}

fn main() {
takes_foo(());
//~^ERROR trait has `()` and `i32` as params
takes_bar(());
//~^ERROR the trait bound `(): Bar` is not satisfied
takes_baz(());
//~^ERROR {from_desugaring}{direct}{cause}{integral}{integer}
}
Loading

0 comments on commit da30882

Please sign in to comment.