Skip to content

Commit

Permalink
Make #[diagnostic::on_unimplemented] format string parsing more robust
Browse files Browse the repository at this point in the history 
This commit fixes several issues with the format string parsing of the
`#[diagnostic::on_unimplemented]` attribute that were pointed out by
@ehuss.
In detail it fixes:

* Appearing format specifiers (display, etc). For these we generate a
warning that the specifier is unsupported. Otherwise we ignore them
* Positional arguments. For these we generate a warning that positional
arguments are unsupported in that location and replace them with the
format string equivalent (so `{}` or `{n}` where n is the index of the
positional argument)
* Broken format strings with enclosed }. For these we generate a warning
about the broken format string and set the emitted message literally to
the provided unformatted string
* Unknown format specifiers. For these we generate an additional warning
about the unknown specifier. Otherwise we emit the literal string as
message.

This essentially makes those strings behave like `format!` with the
minor difference that we do not generate hard errors but only warnings.
After that we continue trying to do something unsuprising (mostly either
ignoring the broken parts or falling back to just giving back the
literal string as provided).

Fix #122391
  • Loading branch information
@weiznich
weiznich committed Mar 21, 2024
1 parent 7de1a1f commit 5568c56
Show file tree
Hide file tree
Showing 4 changed files with 382 additions and 54 deletions.
9 changes: 9 additions & 0 deletions compiler/rustc_trait_selection/messages.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ trait_selection_closure_kind_mismatch = expected a closure that implements the `
trait_selection_closure_kind_requirement = the requirement to implement `{$trait_prefix}{$expected}` derives from here
trait_selection_disallowed_positional_argument = positional format arguments are not allowed here
.help = only named format arguments with the name of one of the generic types are allowed in this context
trait_selection_dump_vtable_entries = vtable entries for `{$trait_ref}`: {$entries}
trait_selection_empty_on_clause_in_rustc_on_unimplemented = empty `on`-clause in `#[rustc_on_unimplemented]`
Expand All @@ -30,6 +33,9 @@ trait_selection_ignored_diagnostic_option = `{$option_name}` is ignored due to p
trait_selection_inherent_projection_normalization_overflow = overflow evaluating associated type `{$ty}`
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
Expand Down Expand Up @@ -60,3 +66,6 @@ trait_selection_unable_to_construct_constant_value = unable to construct a const
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
trait_selection_wrapped_parser_error = {$description}
.label = {$label}
189 changes: 135 additions & 54 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 @@ -367,6 +367,23 @@ pub struct UnknownFormatParameterForOnUnimplementedAttr {
trait_name: Symbol,
}

#[derive(LintDiagnostic)]
#[diag(trait_selection_disallowed_positional_argument)]
#[help]
pub struct DisallowedPositionalArgument;

#[derive(LintDiagnostic)]
#[diag(trait_selection_invalid_format_specifier)]
#[help]
pub struct InvalidFormatSpecifier;

#[derive(LintDiagnostic)]
#[diag(trait_selection_wrapped_parser_error)]
pub struct WrappedParserError {
description: String,
label: String,
}

impl<'tcx> OnUnimplementedDirective {
fn parse(
tcx: TyCtxt<'tcx>,
Expand Down Expand Up @@ -758,64 +775,108 @@ impl<'tcx> OnUnimplementedFormatString {
let trait_name = tcx.item_name(trait_def_id);
let generics = tcx.generics_of(item_def_id);
let s = self.symbol.as_str();
let parser = Parser::new(s, None, None, false, ParseMode::Format);
let mut parser = Parser::new(s, None, None, false, ParseMode::Format);
let mut result = Ok(());
for token in parser {
for token in &mut parser {
match token {
Piece::String(_) => (), // Normal string, no need to check it
Piece::NextArgument(a) => match a.position {
Position::ArgumentNamed(s) => {
match Symbol::intern(s) {
// `{ThisTraitsName}` is allowed
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 => {
if self.is_diagnostic_namespace_variant {
tcx.emit_node_span_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_code_err!(
tcx.dcx(),
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());
Piece::NextArgument(a) => {
let format_spec = a.format;
if self.is_diagnostic_namespace_variant
&& (format_spec.ty_span.is_some()
|| format_spec.width_span.is_some()
|| format_spec.precision_span.is_some()
|| format_spec.fill_span.is_some())
{
tcx.emit_node_span_lint(
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
self.span,
InvalidFormatSpecifier,
);
}
match a.position {
Position::ArgumentNamed(s) => {
match Symbol::intern(s) {
// `{ThisTraitsName}` is allowed
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 => {
if self.is_diagnostic_namespace_variant {
tcx.emit_node_span_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_code_err!(
tcx.dcx(),
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(_) => {
if self.is_diagnostic_namespace_variant {
tcx.emit_node_span_lint(
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
self.span,
DisallowedPositionalArgument,
);
} else {
let reported = struct_span_code_err!(
tcx.dcx(),
self.span,
E0231,
"only named generic parameters are allowed"
)
.emit();
result = Err(reported);
}
}
}
// `{:1}` and `{}` are not to be used
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
let reported = struct_span_code_err!(
tcx.dcx(),
self.span,
E0231,
"only named generic parameters are allowed"
)
.emit();
result = Err(reported);
}
},
}
}
}
// we cannot return errors from processing the format string as hard error here
// as the diagnostic namespace gurantees that malformed input cannot cause an error
//
// if we encounter any error while processing we nevertheless want to show it as warning
// so that users are aware that something is not correct
for e in parser.errors {
if self.is_diagnostic_namespace_variant {
tcx.emit_node_span_lint(
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
self.span,
WrappedParserError { description: e.description, label: e.label },
);
} else {
let reported =
struct_span_code_err!(tcx.dcx(), self.span, E0231, "{}", e.description,).emit();
result = Err(reported);
}
}

Expand Down Expand Up @@ -853,9 +914,9 @@ impl<'tcx> OnUnimplementedFormatString {
let empty_string = String::new();

let s = self.symbol.as_str();
let parser = Parser::new(s, None, None, false, ParseMode::Format);
let mut parser = Parser::new(s, None, None, false, ParseMode::Format);
let item_context = (options.get(&sym::ItemContext)).unwrap_or(&empty_string);
parser
let constructed_message = (&mut parser)
.map(|p| match p {
Piece::String(s) => s.to_owned(),
Piece::NextArgument(a) => match a.position {
Expand Down Expand Up @@ -895,9 +956,29 @@ impl<'tcx> OnUnimplementedFormatString {
}
}
}
Position::ArgumentImplicitlyIs(_) if self.is_diagnostic_namespace_variant => {
String::from("{}")
}
Position::ArgumentIs(idx) if self.is_diagnostic_namespace_variant => {
format!("{{{idx}}}")
}
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.symbol),
},
})
.collect()
.collect();
// we cannot return errors from processing the format string as hard error here
// as the diagnostic namespace gurantees that malformed input cannot cause an error
//
// if we encounter any error while processing the format string
// we don't want to show the potentially half assembled formated string,
// therefore we fall back to just showing the input string in this case
//
// The actual parser errors are emitted earlier
// as lint warnings in OnUnimplementedFormatString::verify
if self.is_diagnostic_namespace_variant && !parser.errors.is_empty() {
String::from(s)
} else {
constructed_message
}
}
}
45 changes: 45 additions & 0 deletions tests/ui/diagnostic_namespace/on_unimplemented/broken_format.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#[diagnostic::on_unimplemented(message = "{{Test } thing")]
//~^WARN unmatched `}` found
//~|WARN unmatched `}` found
trait ImportantTrait1 {}

#[diagnostic::on_unimplemented(message = "Test {}")]
//~^WARN positional format arguments are not allowed here
//~|WARN positional format arguments are not allowed here
trait ImportantTrait2 {}

#[diagnostic::on_unimplemented(message = "Test {1:}")]
//~^WARN positional format arguments are not allowed here
//~|WARN positional format arguments are not allowed here
trait ImportantTrait3 {}

#[diagnostic::on_unimplemented(message = "Test {Self:123}")]
//~^WARN invalid format specifier
//~|WARN invalid format specifier
trait ImportantTrait4 {}

#[diagnostic::on_unimplemented(message = "Test {Self:!}")]
//~^WARN expected `'}'`, found `'!'`
//~|WARN expected `'}'`, found `'!'`
//~|WARN unmatched `}` found
//~|WARN unmatched `}` found
trait ImportantTrait5 {}

fn check_1(_: impl ImportantTrait1) {}
fn check_2(_: impl ImportantTrait2) {}
fn check_3(_: impl ImportantTrait3) {}
fn check_4(_: impl ImportantTrait4) {}
fn check_5(_: impl ImportantTrait5) {}

fn main() {
check_1(());
//~^ERROR {{Test } thing
check_2(());
//~^ERROR Test {}
check_3(());
//~^ERROR Test {1}
check_4(());
//~^ERROR Test ()
check_5(());
//~^ERROR Test {Self:!}
}
Loading

0 comments on commit 5568c56

Please sign in to comment.