Skip to content

Commit 1a1cd6e

Browse files
weiznichweiznich
committed
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.
1 parent 74fccd0 commit 1a1cd6e

File tree

6 files changed

+523
-61
lines changed

6 files changed

+523
-61
lines changed

‎compiler/rustc_trait_selection/messages.ftl

+3
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,6 @@ trait_selection_trait_has_no_impls = this trait has no implementations, consider
5555
5656
trait_selection_ty_alias_overflow = in case this is a recursive type alias, consider using a struct, enum, or union instead
5757
trait_selection_unable_to_construct_constant_value = unable to construct a constant value for the unevaluated constant {$unevaluated}
58+
59+
trait_selection_unknown_format_parameter_for_on_unimplemented_attr = there is no parameter `{$argument_name}` on trait `{$trait_name}`
60+
.help = expect either a generic argument name or {"`{Self}`"} as format argument

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

+95-52
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,11 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
321321
}
322322

323323
#[derive(Clone, Debug)]
324-
pub struct OnUnimplementedFormatString(Symbol, Span);
324+
pub struct OnUnimplementedFormatString {
325+
symbol: Symbol,
326+
span: Span,
327+
is_diagnostic_namespace_variant: bool,
328+
}
325329

326330
#[derive(Debug)]
327331
pub struct OnUnimplementedDirective {
@@ -401,6 +405,14 @@ impl IgnoredDiagnosticOption {
401405
}
402406
}
403407

408+
#[derive(LintDiagnostic)]
409+
#[diag(trait_selection_unknown_format_parameter_for_on_unimplemented_attr)]
410+
#[help]
411+
pub struct UnknownFormatParameterForOnUnimplementedAttr {
412+
argument_name: Symbol,
413+
trait_name: Symbol,
414+
}
415+
404416
impl<'tcx> OnUnimplementedDirective {
405417
fn parse(
406418
tcx: TyCtxt<'tcx>,
@@ -414,8 +426,14 @@ impl<'tcx> OnUnimplementedDirective {
414426
let mut item_iter = items.iter();
415427

416428
let parse_value = |value_str, value_span| {
417-
OnUnimplementedFormatString::try_parse(tcx, item_def_id, value_str, span, value_span)
418-
.map(Some)
429+
OnUnimplementedFormatString::try_parse(
430+
tcx,
431+
item_def_id,
432+
value_str,
433+
value_span,
434+
is_diagnostic_namespace_variant,
435+
)
436+
.map(Some)
419437
};
420438

421439
let condition = if is_root {
@@ -552,15 +570,15 @@ impl<'tcx> OnUnimplementedDirective {
552570
IgnoredDiagnosticOption::maybe_emit_warning(
553571
tcx,
554572
item_def_id,
555-
directive.message.as_ref().map(|f| f.1),
556-
aggr.message.as_ref().map(|f| f.1),
573+
directive.message.as_ref().map(|f| f.span),
574+
aggr.message.as_ref().map(|f| f.span),
557575
"message",
558576
);
559577
IgnoredDiagnosticOption::maybe_emit_warning(
560578
tcx,
561579
item_def_id,
562-
directive.label.as_ref().map(|f| f.1),
563-
aggr.label.as_ref().map(|f| f.1),
580+
directive.label.as_ref().map(|f| f.span),
581+
aggr.label.as_ref().map(|f| f.span),
564582
"label",
565583
);
566584
IgnoredDiagnosticOption::maybe_emit_warning(
@@ -573,8 +591,8 @@ impl<'tcx> OnUnimplementedDirective {
573591
IgnoredDiagnosticOption::maybe_emit_warning(
574592
tcx,
575593
item_def_id,
576-
directive.parent_label.as_ref().map(|f| f.1),
577-
aggr.parent_label.as_ref().map(|f| f.1),
594+
directive.parent_label.as_ref().map(|f| f.span),
595+
aggr.parent_label.as_ref().map(|f| f.span),
578596
"parent_label",
579597
);
580598
IgnoredDiagnosticOption::maybe_emit_warning(
@@ -634,7 +652,7 @@ impl<'tcx> OnUnimplementedDirective {
634652
item_def_id,
635653
value,
636654
attr.span,
637-
attr.span,
655+
is_diagnostic_namespace_variant,
638656
)?),
639657
notes: Vec::new(),
640658
parent_label: None,
@@ -712,7 +730,12 @@ impl<'tcx> OnUnimplementedDirective {
712730
// `with_no_visible_paths` is also used when generating the options,
713731
// so we need to match it here.
714732
ty::print::with_no_visible_paths!(
715-
OnUnimplementedFormatString(v, cfg.span).format(
733+
OnUnimplementedFormatString {
734+
symbol: v,
735+
span: cfg.span,
736+
is_diagnostic_namespace_variant: false
737+
}
738+
.format(
716739
tcx,
717740
trait_ref,
718741
&options_map
@@ -760,20 +783,19 @@ impl<'tcx> OnUnimplementedFormatString {
760783
tcx: TyCtxt<'tcx>,
761784
item_def_id: DefId,
762785
from: Symbol,
763-
err_sp: Span,
764786
value_span: Span,
787+
is_diagnostic_namespace_variant: bool,
765788
) -> Result<Self, ErrorGuaranteed> {
766-
let result = OnUnimplementedFormatString(from, value_span);
767-
result.verify(tcx, item_def_id, err_sp)?;
789+
let result = OnUnimplementedFormatString {
790+
symbol: from,
791+
span: value_span,
792+
is_diagnostic_namespace_variant,
793+
};
794+
result.verify(tcx, item_def_id)?;
768795
Ok(result)
769796
}
770797

771-
fn verify(
772-
&self,
773-
tcx: TyCtxt<'tcx>,
774-
item_def_id: DefId,
775-
span: Span,
776-
) -> Result<(), ErrorGuaranteed> {
798+
fn verify(&self, tcx: TyCtxt<'tcx>, item_def_id: DefId) -> Result<(), ErrorGuaranteed> {
777799
let trait_def_id = if tcx.is_trait(item_def_id) {
778800
item_def_id
779801
} else {
@@ -782,7 +804,7 @@ impl<'tcx> OnUnimplementedFormatString {
782804
};
783805
let trait_name = tcx.item_name(trait_def_id);
784806
let generics = tcx.generics_of(item_def_id);
785-
let s = self.0.as_str();
807+
let s = self.symbol.as_str();
786808
let parser = Parser::new(s, None, None, false, ParseMode::Format);
787809
let mut result = Ok(());
788810
for token in parser {
@@ -792,32 +814,48 @@ impl<'tcx> OnUnimplementedFormatString {
792814
Position::ArgumentNamed(s) => {
793815
match Symbol::intern(s) {
794816
// `{ThisTraitsName}` is allowed
795-
s if s == trait_name => (),
796-
s if ALLOWED_FORMAT_SYMBOLS.contains(&s) => (),
817+
s if s == trait_name && !self.is_diagnostic_namespace_variant => (),
818+
s if ALLOWED_FORMAT_SYMBOLS.contains(&s)
819+
&& !self.is_diagnostic_namespace_variant =>
820+
{
821+
()
822+
}
797823
// So is `{A}` if A is a type parameter
798824
s if generics.params.iter().any(|param| param.name == s) => (),
799825
s => {
800-
result = Err(struct_span_err!(
801-
tcx.sess,
802-
span,
803-
E0230,
804-
"there is no parameter `{}` on {}",
805-
s,
806-
if trait_def_id == item_def_id {
807-
format!("trait `{trait_name}`")
808-
} else {
809-
"impl".to_string()
810-
}
811-
)
812-
.emit());
826+
if self.is_diagnostic_namespace_variant {
827+
tcx.emit_spanned_lint(
828+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
829+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
830+
self.span,
831+
UnknownFormatParameterForOnUnimplementedAttr {
832+
argument_name: s,
833+
trait_name,
834+
},
835+
);
836+
} else {
837+
result = Err(struct_span_err!(
838+
tcx.sess,
839+
self.span,
840+
E0230,
841+
"there is no parameter `{}` on {}",
842+
s,
843+
if trait_def_id == item_def_id {
844+
format!("trait `{trait_name}`")
845+
} else {
846+
"impl".to_string()
847+
}
848+
)
849+
.emit());
850+
}
813851
}
814852
}
815853
}
816854
// `{:1}` and `{}` are not to be used
817855
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
818856
let reported = struct_span_err!(
819857
tcx.sess,
820-
span,
858+
self.span,
821859
E0231,
822860
"only named substitution parameters are allowed"
823861
)
@@ -856,45 +894,50 @@ impl<'tcx> OnUnimplementedFormatString {
856894
.collect::<FxHashMap<Symbol, String>>();
857895
let empty_string = String::new();
858896

859-
let s = self.0.as_str();
897+
let s = self.symbol.as_str();
860898
let parser = Parser::new(s, None, None, false, ParseMode::Format);
861899
let item_context = (options.get(&sym::ItemContext)).unwrap_or(&empty_string);
862900
parser
863901
.map(|p| match p {
864-
Piece::String(s) => s,
902+
Piece::String(s) => s.to_owned(),
865903
Piece::NextArgument(a) => match a.position {
866-
Position::ArgumentNamed(s) => {
867-
let s = Symbol::intern(s);
904+
Position::ArgumentNamed(arg) => {
905+
let s = Symbol::intern(arg);
868906
match generic_map.get(&s) {
869-
Some(val) => val,
870-
None if s == name => &trait_str,
907+
Some(val) => val.to_string(),
908+
None if self.is_diagnostic_namespace_variant => {
909+
format!("{{{arg}}}")
910+
}
911+
None if s == name => trait_str.clone(),
871912
None => {
872913
if let Some(val) = options.get(&s) {
873-
val
914+
val.clone()
874915
} else if s == sym::from_desugaring {
875916
// don't break messages using these two arguments incorrectly
876-
&empty_string
877-
} else if s == sym::ItemContext {
878-
item_context
917+
String::new()
918+
} else if s == sym::ItemContext
919+
&& !self.is_diagnostic_namespace_variant
920+
{
921+
item_context.clone()
879922
} else if s == sym::integral {
880-
"{integral}"
923+
String::from("{integral}")
881924
} else if s == sym::integer_ {
882-
"{integer}"
925+
String::from("{integer}")
883926
} else if s == sym::float {
884-
"{float}"
927+
String::from("{float}")
885928
} else {
886929
bug!(
887930
"broken on_unimplemented {:?} for {:?}: \
888931
no argument matching {:?}",
889-
self.0,
932+
self.symbol,
890933
trait_ref,
891934
s
892935
)
893936
}
894937
}
895938
}
896939
}
897-
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.0),
940+
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.symbol),
898941
},
899942
})
900943
.collect()

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

+67
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
#![feature(diagnostic_namespace)]
2+
3+
#[diagnostic::on_unimplemented(
4+
on(_Self = "&str"),
5+
//~^WARN malformed `on_unimplemented` attribute
6+
//~|WARN malformed `on_unimplemented` attribute
7+
message = "trait has `{Self}` and `{T}` as params",
8+
label = "trait has `{Self}` and `{T}` as params",
9+
note = "trait has `{Self}` and `{T}` as params",
10+
parent_label = "in this scope",
11+
//~^WARN malformed `on_unimplemented` attribute
12+
//~|WARN malformed `on_unimplemented` attribute
13+
append_const_msg
14+
//~^WARN malformed `on_unimplemented` attribute
15+
//~|WARN malformed `on_unimplemented` attribute
16+
)]
17+
trait Foo<T> {}
18+
19+
#[diagnostic::on_unimplemented = "Message"]
20+
//~^WARN malformed `on_unimplemented` attribute
21+
//~|WARN malformed `on_unimplemented` attribute
22+
trait Bar {}
23+
24+
#[diagnostic::on_unimplemented(message = "Not allowed to apply it on a impl")]
25+
//~^WARN #[diagnostic::on_unimplemented]` can only be applied to trait definitions
26+
impl Bar for i32 {}
27+
28+
// cannot use special rustc_on_unimplement symbols
29+
// in the format string
30+
#[diagnostic::on_unimplemented(
31+
message = "{from_desugaring}{direct}{cause}{integral}{integer}",
32+
//~^WARN there is no parameter `from_desugaring` on trait `Baz`
33+
//~|WARN there is no parameter `from_desugaring` on trait `Baz`
34+
//~|WARN there is no parameter `direct` on trait `Baz`
35+
//~|WARN there is no parameter `direct` on trait `Baz`
36+
//~|WARN there is no parameter `cause` on trait `Baz`
37+
//~|WARN there is no parameter `cause` on trait `Baz`
38+
//~|WARN there is no parameter `integral` on trait `Baz`
39+
//~|WARN there is no parameter `integral` on trait `Baz`
40+
//~|WARN there is no parameter `integer` on trait `Baz`
41+
//~|WARN there is no parameter `integer` on trait `Baz`
42+
label = "{float}{_Self}{crate_local}{Trait}{ItemContext}"
43+
//~^WARN there is no parameter `float` on trait `Baz`
44+
//~|WARN there is no parameter `float` on trait `Baz`
45+
//~|WARN there is no parameter `_Self` on trait `Baz`
46+
//~|WARN there is no parameter `_Self` on trait `Baz`
47+
//~|WARN there is no parameter `crate_local` on trait `Baz`
48+
//~|WARN there is no parameter `crate_local` on trait `Baz`
49+
//~|WARN there is no parameter `Trait` on trait `Baz`
50+
//~|WARN there is no parameter `Trait` on trait `Baz`
51+
//~|WARN there is no parameter `ItemContext` on trait `Baz`
52+
//~|WARN there is no parameter `ItemContext` on trait `Baz`
53+
)]
54+
trait Baz {}
55+
56+
fn takes_foo(_: impl Foo<i32>) {}
57+
fn takes_bar(_: impl Bar) {}
58+
fn takes_baz(_: impl Baz) {}
59+
60+
fn main() {
61+
takes_foo(());
62+
//~^ERROR trait has `()` and `i32` as params
63+
takes_bar(());
64+
//~^ERROR the trait bound `(): Bar` is not satisfied
65+
takes_baz(());
66+
//~^ERROR {from_desugaring}{direct}{cause}{integral}{integer}
67+
}

0 commit comments

Comments
 (0)