Skip to content

Commit 5568c56

Browse files
weiznichweiznich
committed
Make #[diagnostic::on_unimplemented] format string parsing more robust
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 rust-lang#122391
1 parent 7de1a1f commit 5568c56

File tree

4 files changed

+382
-54
lines changed

4 files changed

+382
-54
lines changed

‎compiler/rustc_trait_selection/messages.ftl

+9
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ trait_selection_closure_kind_mismatch = expected a closure that implements the `
1919
2020
trait_selection_closure_kind_requirement = the requirement to implement `{$trait_prefix}{$expected}` derives from here
2121
22+
trait_selection_disallowed_positional_argument = positional format arguments are not allowed here
23+
.help = only named format arguments with the name of one of the generic types are allowed in this context
24+
2225
trait_selection_dump_vtable_entries = vtable entries for `{$trait_ref}`: {$entries}
2326
2427
trait_selection_empty_on_clause_in_rustc_on_unimplemented = empty `on`-clause in `#[rustc_on_unimplemented]`
@@ -30,6 +33,9 @@ trait_selection_ignored_diagnostic_option = `{$option_name}` is ignored due to p
3033
3134
trait_selection_inherent_projection_normalization_overflow = overflow evaluating associated type `{$ty}`
3235
36+
trait_selection_invalid_format_specifier = invalid format specifier
37+
.help = no format specifier are supported in this position
38+
3339
trait_selection_invalid_on_clause_in_rustc_on_unimplemented = invalid `on`-clause in `#[rustc_on_unimplemented]`
3440
.label = invalid on-clause here
3541
@@ -60,3 +66,6 @@ trait_selection_unable_to_construct_constant_value = unable to construct a const
6066
6167
trait_selection_unknown_format_parameter_for_on_unimplemented_attr = there is no parameter `{$argument_name}` on trait `{$trait_name}`
6268
.help = expect either a generic argument name or {"`{Self}`"} as format argument
69+
70+
trait_selection_wrapped_parser_error = {$description}
71+
.label = {$label}

‎compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs

+135-54
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,23 @@ pub struct UnknownFormatParameterForOnUnimplementedAttr {
367367
trait_name: Symbol,
368368
}
369369

370+
#[derive(LintDiagnostic)]
371+
#[diag(trait_selection_disallowed_positional_argument)]
372+
#[help]
373+
pub struct DisallowedPositionalArgument;
374+
375+
#[derive(LintDiagnostic)]
376+
#[diag(trait_selection_invalid_format_specifier)]
377+
#[help]
378+
pub struct InvalidFormatSpecifier;
379+
380+
#[derive(LintDiagnostic)]
381+
#[diag(trait_selection_wrapped_parser_error)]
382+
pub struct WrappedParserError {
383+
description: String,
384+
label: String,
385+
}
386+
370387
impl<'tcx> OnUnimplementedDirective {
371388
fn parse(
372389
tcx: TyCtxt<'tcx>,
@@ -758,64 +775,108 @@ impl<'tcx> OnUnimplementedFormatString {
758775
let trait_name = tcx.item_name(trait_def_id);
759776
let generics = tcx.generics_of(item_def_id);
760777
let s = self.symbol.as_str();
761-
let parser = Parser::new(s, None, None, false, ParseMode::Format);
778+
let mut parser = Parser::new(s, None, None, false, ParseMode::Format);
762779
let mut result = Ok(());
763-
for token in parser {
780+
for token in &mut parser {
764781
match token {
765782
Piece::String(_) => (), // Normal string, no need to check it
766-
Piece::NextArgument(a) => match a.position {
767-
Position::ArgumentNamed(s) => {
768-
match Symbol::intern(s) {
769-
// `{ThisTraitsName}` is allowed
770-
s if s == trait_name && !self.is_diagnostic_namespace_variant => (),
771-
s if ALLOWED_FORMAT_SYMBOLS.contains(&s)
772-
&& !self.is_diagnostic_namespace_variant =>
773-
{
774-
()
775-
}
776-
// So is `{A}` if A is a type parameter
777-
s if generics.params.iter().any(|param| param.name == s) => (),
778-
s => {
779-
if self.is_diagnostic_namespace_variant {
780-
tcx.emit_node_span_lint(
781-
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
782-
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
783-
self.span,
784-
UnknownFormatParameterForOnUnimplementedAttr {
785-
argument_name: s,
786-
trait_name,
787-
},
788-
);
789-
} else {
790-
result = Err(struct_span_code_err!(
791-
tcx.dcx(),
792-
self.span,
793-
E0230,
794-
"there is no parameter `{}` on {}",
795-
s,
796-
if trait_def_id == item_def_id {
797-
format!("trait `{trait_name}`")
798-
} else {
799-
"impl".to_string()
800-
}
801-
)
802-
.emit());
783+
Piece::NextArgument(a) => {
784+
let format_spec = a.format;
785+
if self.is_diagnostic_namespace_variant
786+
&& (format_spec.ty_span.is_some()
787+
|| format_spec.width_span.is_some()
788+
|| format_spec.precision_span.is_some()
789+
|| format_spec.fill_span.is_some())
790+
{
791+
tcx.emit_node_span_lint(
792+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
793+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
794+
self.span,
795+
InvalidFormatSpecifier,
796+
);
797+
}
798+
match a.position {
799+
Position::ArgumentNamed(s) => {
800+
match Symbol::intern(s) {
801+
// `{ThisTraitsName}` is allowed
802+
s if s == trait_name && !self.is_diagnostic_namespace_variant => (),
803+
s if ALLOWED_FORMAT_SYMBOLS.contains(&s)
804+
&& !self.is_diagnostic_namespace_variant =>
805+
{
806+
()
807+
}
808+
// So is `{A}` if A is a type parameter
809+
s if generics.params.iter().any(|param| param.name == s) => (),
810+
s => {
811+
if self.is_diagnostic_namespace_variant {
812+
tcx.emit_node_span_lint(
813+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
814+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
815+
self.span,
816+
UnknownFormatParameterForOnUnimplementedAttr {
817+
argument_name: s,
818+
trait_name,
819+
},
820+
);
821+
} else {
822+
result = Err(struct_span_code_err!(
823+
tcx.dcx(),
824+
self.span,
825+
E0230,
826+
"there is no parameter `{}` on {}",
827+
s,
828+
if trait_def_id == item_def_id {
829+
format!("trait `{trait_name}`")
830+
} else {
831+
"impl".to_string()
832+
}
833+
)
834+
.emit());
835+
}
803836
}
804837
}
805838
}
839+
// `{:1}` and `{}` are not to be used
840+
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
841+
if self.is_diagnostic_namespace_variant {
842+
tcx.emit_node_span_lint(
843+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
844+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
845+
self.span,
846+
DisallowedPositionalArgument,
847+
);
848+
} else {
849+
let reported = struct_span_code_err!(
850+
tcx.dcx(),
851+
self.span,
852+
E0231,
853+
"only named generic parameters are allowed"
854+
)
855+
.emit();
856+
result = Err(reported);
857+
}
858+
}
806859
}
807-
// `{:1}` and `{}` are not to be used
808-
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
809-
let reported = struct_span_code_err!(
810-
tcx.dcx(),
811-
self.span,
812-
E0231,
813-
"only named generic parameters are allowed"
814-
)
815-
.emit();
816-
result = Err(reported);
817-
}
818-
},
860+
}
861+
}
862+
}
863+
// we cannot return errors from processing the format string as hard error here
864+
// as the diagnostic namespace gurantees that malformed input cannot cause an error
865+
//
866+
// if we encounter any error while processing we nevertheless want to show it as warning
867+
// so that users are aware that something is not correct
868+
for e in parser.errors {
869+
if self.is_diagnostic_namespace_variant {
870+
tcx.emit_node_span_lint(
871+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
872+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
873+
self.span,
874+
WrappedParserError { description: e.description, label: e.label },
875+
);
876+
} else {
877+
let reported =
878+
struct_span_code_err!(tcx.dcx(), self.span, E0231, "{}", e.description,).emit();
879+
result = Err(reported);
819880
}
820881
}
821882

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

855916
let s = self.symbol.as_str();
856-
let parser = Parser::new(s, None, None, false, ParseMode::Format);
917+
let mut parser = Parser::new(s, None, None, false, ParseMode::Format);
857918
let item_context = (options.get(&sym::ItemContext)).unwrap_or(&empty_string);
858-
parser
919+
let constructed_message = (&mut parser)
859920
.map(|p| match p {
860921
Piece::String(s) => s.to_owned(),
861922
Piece::NextArgument(a) => match a.position {
@@ -895,9 +956,29 @@ impl<'tcx> OnUnimplementedFormatString {
895956
}
896957
}
897958
}
959+
Position::ArgumentImplicitlyIs(_) if self.is_diagnostic_namespace_variant => {
960+
String::from("{}")
961+
}
962+
Position::ArgumentIs(idx) if self.is_diagnostic_namespace_variant => {
963+
format!("{{{idx}}}")
964+
}
898965
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.symbol),
899966
},
900967
})
901-
.collect()
968+
.collect();
969+
// we cannot return errors from processing the format string as hard error here
970+
// as the diagnostic namespace gurantees that malformed input cannot cause an error
971+
//
972+
// if we encounter any error while processing the format string
973+
// we don't want to show the potentially half assembled formated string,
974+
// therefore we fall back to just showing the input string in this case
975+
//
976+
// The actual parser errors are emitted earlier
977+
// as lint warnings in OnUnimplementedFormatString::verify
978+
if self.is_diagnostic_namespace_variant && !parser.errors.is_empty() {
979+
String::from(s)
980+
} else {
981+
constructed_message
982+
}
902983
}
903984
}

‎tests/ui/diagnostic_namespace/on_unimplemented/broken_format.rs

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#[diagnostic::on_unimplemented(message = "{{Test } thing")]
2+
//~^WARN unmatched `}` found
3+
//~|WARN unmatched `}` found
4+
trait ImportantTrait1 {}
5+
6+
#[diagnostic::on_unimplemented(message = "Test {}")]
7+
//~^WARN positional format arguments are not allowed here
8+
//~|WARN positional format arguments are not allowed here
9+
trait ImportantTrait2 {}
10+
11+
#[diagnostic::on_unimplemented(message = "Test {1:}")]
12+
//~^WARN positional format arguments are not allowed here
13+
//~|WARN positional format arguments are not allowed here
14+
trait ImportantTrait3 {}
15+
16+
#[diagnostic::on_unimplemented(message = "Test {Self:123}")]
17+
//~^WARN invalid format specifier
18+
//~|WARN invalid format specifier
19+
trait ImportantTrait4 {}
20+
21+
#[diagnostic::on_unimplemented(message = "Test {Self:!}")]
22+
//~^WARN expected `'}'`, found `'!'`
23+
//~|WARN expected `'}'`, found `'!'`
24+
//~|WARN unmatched `}` found
25+
//~|WARN unmatched `}` found
26+
trait ImportantTrait5 {}
27+
28+
fn check_1(_: impl ImportantTrait1) {}
29+
fn check_2(_: impl ImportantTrait2) {}
30+
fn check_3(_: impl ImportantTrait3) {}
31+
fn check_4(_: impl ImportantTrait4) {}
32+
fn check_5(_: impl ImportantTrait5) {}
33+
34+
fn main() {
35+
check_1(());
36+
//~^ERROR {{Test } thing
37+
check_2(());
38+
//~^ERROR Test {}
39+
check_3(());
40+
//~^ERROR Test {1}
41+
check_4(());
42+
//~^ERROR Test ()
43+
check_5(());
44+
//~^ERROR Test {Self:!}
45+
}

0 commit comments

Comments
 (0)