Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make #[diagnostic::on_unimplemented] format string parsing more robust #122402

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Make #[diagnostic::on_unimplemented] format string parsing more robust #122402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions compiler/rustc_trait_selection/messages.ftl
View file
Open in desktop
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}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unsure about that "hack". That's here to forward the values of ParserError as lint. I'm not sure if there is a better way of doing that. If that's the case I'm happy to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine until these parsing errors become translatable.

.label = {$label}
189 changes: 135 additions & 54 deletions compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs
View file
Open in desktop
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 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a hack, but I guess it's fine

);
} 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
}
Comment on lines +978 to +982
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems subtle. Please document why we need this.

}
}
45 changes: 45 additions & 0 deletions tests/ui/diagnostic_namespace/on_unimplemented/broken_format.rs
View file
Open in desktop
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
Loading