Skip to content

Commit f8c3c74

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 f8c3c74

File tree

6 files changed

+524
-61
lines changed

6 files changed

+524
-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

+96-52
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use rustc_parse_format::{ParseMode, Parser, Piece, Position};
1515
use rustc_session::lint::builtin::UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES;
1616
use rustc_span::symbol::{kw, sym, Symbol};
1717
use rustc_span::{Span, DUMMY_SP};
18+
use std::borrow::Cow;
1819
use std::iter;
1920

2021
use crate::errors::{
@@ -321,7 +322,11 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
321322
}
322323

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

326331
#[derive(Debug)]
327332
pub struct OnUnimplementedDirective {
@@ -401,6 +406,14 @@ impl IgnoredDiagnosticOption {
401406
}
402407
}
403408

409+
#[derive(LintDiagnostic)]
410+
#[diag(trait_selection_unknown_format_parameter_for_on_unimplemented_attr)]
411+
#[help]
412+
pub struct UnknownFormatParameterForOnUnimplementedAttr {
413+
argument_name: Symbol,
414+
trait_name: Symbol,
415+
}
416+
404417
impl<'tcx> OnUnimplementedDirective {
405418
fn parse(
406419
tcx: TyCtxt<'tcx>,
@@ -414,8 +427,14 @@ impl<'tcx> OnUnimplementedDirective {
414427
let mut item_iter = items.iter();
415428

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

421440
let condition = if is_root {
@@ -552,15 +571,15 @@ impl<'tcx> OnUnimplementedDirective {
552571
IgnoredDiagnosticOption::maybe_emit_warning(
553572
tcx,
554573
item_def_id,
555-
directive.message.as_ref().map(|f| f.1),
556-
aggr.message.as_ref().map(|f| f.1),
574+
directive.message.as_ref().map(|f| f.span),
575+
aggr.message.as_ref().map(|f| f.span),
557576
"message",
558577
);
559578
IgnoredDiagnosticOption::maybe_emit_warning(
560579
tcx,
561580
item_def_id,
562-
directive.label.as_ref().map(|f| f.1),
563-
aggr.label.as_ref().map(|f| f.1),
581+
directive.label.as_ref().map(|f| f.span),
582+
aggr.label.as_ref().map(|f| f.span),
564583
"label",
565584
);
566585
IgnoredDiagnosticOption::maybe_emit_warning(
@@ -573,8 +592,8 @@ impl<'tcx> OnUnimplementedDirective {
573592
IgnoredDiagnosticOption::maybe_emit_warning(
574593
tcx,
575594
item_def_id,
576-
directive.parent_label.as_ref().map(|f| f.1),
577-
aggr.parent_label.as_ref().map(|f| f.1),
595+
directive.parent_label.as_ref().map(|f| f.span),
596+
aggr.parent_label.as_ref().map(|f| f.span),
578597
"parent_label",
579598
);
580599
IgnoredDiagnosticOption::maybe_emit_warning(
@@ -634,7 +653,7 @@ impl<'tcx> OnUnimplementedDirective {
634653
item_def_id,
635654
value,
636655
attr.span,
637-
attr.span,
656+
is_diagnostic_namespace_variant,
638657
)?),
639658
notes: Vec::new(),
640659
parent_label: None,
@@ -712,7 +731,12 @@ impl<'tcx> OnUnimplementedDirective {
712731
// `with_no_visible_paths` is also used when generating the options,
713732
// so we need to match it here.
714733
ty::print::with_no_visible_paths!(
715-
OnUnimplementedFormatString(v, cfg.span).format(
734+
OnUnimplementedFormatString {
735+
symbol: v,
736+
span: cfg.span,
737+
is_diagnostic_namespace_variant: false
738+
}
739+
.format(
716740
tcx,
717741
trait_ref,
718742
&options_map
@@ -760,20 +784,19 @@ impl<'tcx> OnUnimplementedFormatString {
760784
tcx: TyCtxt<'tcx>,
761785
item_def_id: DefId,
762786
from: Symbol,
763-
err_sp: Span,
764787
value_span: Span,
788+
is_diagnostic_namespace_variant: bool,
765789
) -> Result<Self, ErrorGuaranteed> {
766-
let result = OnUnimplementedFormatString(from, value_span);
767-
result.verify(tcx, item_def_id, err_sp)?;
790+
let result = OnUnimplementedFormatString {
791+
symbol: from,
792+
span: value_span,
793+
is_diagnostic_namespace_variant,
794+
};
795+
result.verify(tcx, item_def_id)?;
768796
Ok(result)
769797
}
770798

771-
fn verify(
772-
&self,
773-
tcx: TyCtxt<'tcx>,
774-
item_def_id: DefId,
775-
span: Span,
776-
) -> Result<(), ErrorGuaranteed> {
799+
fn verify(&self, tcx: TyCtxt<'tcx>, item_def_id: DefId) -> Result<(), ErrorGuaranteed> {
777800
let trait_def_id = if tcx.is_trait(item_def_id) {
778801
item_def_id
779802
} else {
@@ -782,7 +805,7 @@ impl<'tcx> OnUnimplementedFormatString {
782805
};
783806
let trait_name = tcx.item_name(trait_def_id);
784807
let generics = tcx.generics_of(item_def_id);
785-
let s = self.0.as_str();
808+
let s = self.symbol.as_str();
786809
let parser = Parser::new(s, None, None, false, ParseMode::Format);
787810
let mut result = Ok(());
788811
for token in parser {
@@ -792,32 +815,48 @@ impl<'tcx> OnUnimplementedFormatString {
792815
Position::ArgumentNamed(s) => {
793816
match Symbol::intern(s) {
794817
// `{ThisTraitsName}` is allowed
795-
s if s == trait_name => (),
796-
s if ALLOWED_FORMAT_SYMBOLS.contains(&s) => (),
818+
s if s == trait_name && !self.is_diagnostic_namespace_variant => (),
819+
s if ALLOWED_FORMAT_SYMBOLS.contains(&s)
820+
&& !self.is_diagnostic_namespace_variant =>
821+
{
822+
()
823+
}
797824
// So is `{A}` if A is a type parameter
798825
s if generics.params.iter().any(|param| param.name == s) => (),
799826
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());
827+
if self.is_diagnostic_namespace_variant {
828+
tcx.emit_spanned_lint(
829+
UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
830+
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
831+
self.span,
832+
UnknownFormatParameterForOnUnimplementedAttr {
833+
argument_name: s,
834+
trait_name,
835+
},
836+
);
837+
} else {
838+
result = Err(struct_span_err!(
839+
tcx.sess,
840+
self.span,
841+
E0230,
842+
"there is no parameter `{}` on {}",
843+
s,
844+
if trait_def_id == item_def_id {
845+
format!("trait `{trait_name}`")
846+
} else {
847+
"impl".to_string()
848+
}
849+
)
850+
.emit());
851+
}
813852
}
814853
}
815854
}
816855
// `{:1}` and `{}` are not to be used
817856
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
818857
let reported = struct_span_err!(
819858
tcx.sess,
820-
span,
859+
self.span,
821860
E0231,
822861
"only named substitution parameters are allowed"
823862
)
@@ -856,45 +895,50 @@ impl<'tcx> OnUnimplementedFormatString {
856895
.collect::<FxHashMap<Symbol, String>>();
857896
let empty_string = String::new();
858897

859-
let s = self.0.as_str();
898+
let s = self.symbol.as_str();
860899
let parser = Parser::new(s, None, None, false, ParseMode::Format);
861900
let item_context = (options.get(&sym::ItemContext)).unwrap_or(&empty_string);
862901
parser
863902
.map(|p| match p {
864-
Piece::String(s) => s,
903+
Piece::String(s) => Cow::Borrowed(s),
865904
Piece::NextArgument(a) => match a.position {
866-
Position::ArgumentNamed(s) => {
867-
let s = Symbol::intern(s);
905+
Position::ArgumentNamed(arg) => {
906+
let s = Symbol::intern(arg);
868907
match generic_map.get(&s) {
869-
Some(val) => val,
870-
None if s == name => &trait_str,
908+
Some(val) => Cow::<str>::Borrowed(val),
909+
None if self.is_diagnostic_namespace_variant => {
910+
Cow::Owned(format!("{{{arg}}}"))
911+
}
912+
None if s == name => Cow::<str>::Borrowed(&trait_str),
871913
None => {
872914
if let Some(val) = options.get(&s) {
873-
val
915+
Cow::<str>::Borrowed(val)
874916
} else if s == sym::from_desugaring {
875917
// don't break messages using these two arguments incorrectly
876-
&empty_string
877-
} else if s == sym::ItemContext {
878-
item_context
918+
Cow::<str>::Borrowed(&empty_string as &str)
919+
} else if s == sym::ItemContext
920+
&& !self.is_diagnostic_namespace_variant
921+
{
922+
Cow::<str>::Borrowed(item_context)
879923
} else if s == sym::integral {
880-
"{integral}"
924+
Cow::<str>::Borrowed("{integral}")
881925
} else if s == sym::integer_ {
882-
"{integer}"
926+
Cow::<str>::Borrowed("{integer}")
883927
} else if s == sym::float {
884-
"{float}"
928+
Cow::<str>::Borrowed("{float}")
885929
} else {
886930
bug!(
887931
"broken on_unimplemented {:?} for {:?}: \
888932
no argument matching {:?}",
889-
self.0,
933+
self.symbol,
890934
trait_ref,
891935
s
892936
)
893937
}
894938
}
895939
}
896940
}
897-
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.0),
941+
_ => bug!("broken on_unimplemented {:?} - bad format arg", self.symbol),
898942
},
899943
})
900944
.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)