Skip to content

Commit a52ec87

Browse files
estebankestebank
committed
Use more appropriate spans on object unsafe traits and provide structured suggestions when possible
1 parent 413bfa4 commit a52ec87

22 files changed

+178
-62
lines changed

‎src/librustc/traits/error_reporting/mod.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,9 +1073,15 @@ pub fn report_object_safety_error(
10731073
err.span_label(span, &msg);
10741074
}
10751075
}
1076-
if let (Some(_), Some(note)) = (trait_span, violation.solution()) {
1076+
match (trait_span, violation.solution()) {
1077+
(Some(_), Some((note, None))) => {
1078+
err.help(&note);
1079+
}
1080+
(Some(_), Some((note, Some((sugg, span))))) => {
1081+
err.span_suggestion(span, &note, sugg, Applicability::MachineApplicable);
1082+
}
10771083
// Only provide the help if its a local trait, otherwise it's not actionable.
1078-
err.help(&note);
1084+
_ => {}
10791085
}
10801086
}
10811087
}

‎src/librustc/traits/object_safety.rs‎

Lines changed: 77 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use super::elaborate_predicates;
1313
use crate::traits::{self, Obligation, ObligationCause};
1414
use crate::ty::subst::{InternalSubsts, Subst};
1515
use crate::ty::{self, Predicate, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness};
16+
use rustc_errors::Applicability;
1617
use rustc_hir as hir;
1718
use rustc_hir::def_id::DefId;
1819
use rustc_session::lint::builtin::WHERE_CLAUSES_OBJECT_SAFETY;
@@ -48,14 +49,20 @@ impl ObjectSafetyViolation {
4849
"it cannot use `Self` as a type parameter in the supertraits or `where`-clauses"
4950
.into()
5051
}
51-
ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod, _) => {
52+
ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod(_), _) => {
5253
format!("associated function `{}` has no `self` parameter", name).into()
5354
}
54-
ObjectSafetyViolation::Method(name, MethodViolationCode::ReferencesSelf, _) => format!(
55-
"method `{}` references the `Self` type in its parameters or return type",
55+
ObjectSafetyViolation::Method(
5656
name,
57-
)
58-
.into(),
57+
MethodViolationCode::ReferencesSelfInput(_),
58+
DUMMY_SP,
59+
) => format!("method `{}` references the `Self` type in its parameters", name).into(),
60+
ObjectSafetyViolation::Method(name, MethodViolationCode::ReferencesSelfInput(_), _) => {
61+
format!("method `{}` references the `Self` type in this parameter", name).into()
62+
}
63+
ObjectSafetyViolation::Method(name, MethodViolationCode::ReferencesSelfOutput, _) => {
64+
format!("method `{}` references the `Self` type in its return type", name).into()
65+
}
5966
ObjectSafetyViolation::Method(
6067
name,
6168
MethodViolationCode::WhereClauseReferencesSelf,
@@ -78,23 +85,31 @@ impl ObjectSafetyViolation {
7885
}
7986
}
8087

81-
pub fn solution(&self) -> Option<String> {
88+
pub fn solution(&self) -> Option<(String, Option<(String, Span)>)> {
8289
Some(match *self {
8390
ObjectSafetyViolation::SizedSelf(_) | ObjectSafetyViolation::SupertraitSelf => {
8491
return None;
8592
}
86-
ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod, _) => format!(
87-
"consider turning `{}` into a method by giving it a `&self` argument or \
88-
constraining it with `where Self: Sized`",
89-
name
93+
ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod(sugg), _) => (
94+
format!(
95+
"consider turning `{}` into a method by giving it a `&self` argument or \
96+
constraining it so it does not apply to trait objects",
97+
name
98+
),
99+
sugg.map(|(sugg, sp)| (sugg.to_string(), sp)),
90100
),
91-
ObjectSafetyViolation::Method(name, MethodViolationCode::UndispatchableReceiver, _) => {
101+
ObjectSafetyViolation::Method(
102+
name,
103+
MethodViolationCode::UndispatchableReceiver,
104+
span,
105+
) => (
92106
format!("consider changing method `{}`'s `self` parameter to be `&self`", name)
93-
.into()
94-
}
107+
.into(),
108+
Some(("&Self".to_string(), span)),
109+
),
95110
ObjectSafetyViolation::AssocConst(name, _)
96111
| ObjectSafetyViolation::Method(name, ..) => {
97-
format!("consider moving `{}` to another trait", name)
112+
(format!("consider moving `{}` to another trait", name), None)
98113
}
99114
})
100115
}
@@ -119,10 +134,13 @@ impl ObjectSafetyViolation {
119134
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
120135
pub enum MethodViolationCode {
121136
/// e.g., `fn foo()`
122-
StaticMethod,
137+
StaticMethod(Option<(&'static str, Span)>),
138+
139+
/// e.g., `fn foo(&self, x: Self)`
140+
ReferencesSelfInput(usize),
123141

124-
/// e.g., `fn foo(&self, x: Self)` or `fn foo(&self) -> Self`
125-
ReferencesSelf,
142+
/// e.g., `fn foo(&self) -> Self`
143+
ReferencesSelfOutput,
126144

127145
/// e.g., `fn foo(&self) where Self: Clone`
128146
WhereClauseReferencesSelf,
@@ -193,7 +211,7 @@ fn object_safety_violations_for_trait(
193211
.filter(|item| item.kind == ty::AssocKind::Method)
194212
.filter_map(|item| {
195213
object_safety_violation_for_method(tcx, trait_def_id, &item)
196-
.map(|code| ObjectSafetyViolation::Method(item.ident.name, code, item.ident.span))
214+
.map(|(code, span)| ObjectSafetyViolation::Method(item.ident.name, code, span))
197215
})
198216
.filter(|violation| {
199217
if let ObjectSafetyViolation::Method(
@@ -224,9 +242,15 @@ fn object_safety_violations_for_trait(
224242
)
225243
};
226244
err.span_label(*span, &msg);
227-
if let (Some(_), Some(note)) = (node, violation.solution()) {
245+
match (node, violation.solution()) {
246+
(Some(_), Some((note, None))) => {
247+
err.help(&note);
248+
}
249+
(Some(_), Some((note, Some((sugg, span))))) => {
250+
err.span_suggestion(span, &note, sugg, Applicability::MachineApplicable);
251+
}
228252
// Only provide the help if its a local trait, otherwise it's not actionable.
229-
err.help(&note);
253+
_ => {}
230254
}
231255
err.emit();
232256
false
@@ -398,15 +422,34 @@ fn object_safety_violation_for_method(
398422
tcx: TyCtxt<'_>,
399423
trait_def_id: DefId,
400424
method: &ty::AssocItem,
401-
) -> Option<MethodViolationCode> {
425+
) -> Option<(MethodViolationCode, Span)> {
402426
debug!("object_safety_violation_for_method({:?}, {:?})", trait_def_id, method);
403427
// Any method that has a `Self : Sized` requisite is otherwise
404428
// exempt from the regulations.
405429
if generics_require_sized_self(tcx, method.def_id) {
406430
return None;
407431
}
408432

409-
virtual_call_violation_for_method(tcx, trait_def_id, method)
433+
let violation = virtual_call_violation_for_method(tcx, trait_def_id, method);
434+
// Get an accurate span depending on the violation.
435+
violation.map(|v| {
436+
let node = tcx.hir().get_if_local(method.def_id);
437+
let span = match (v, node) {
438+
(MethodViolationCode::ReferencesSelfInput(arg), Some(node)) => node
439+
.fn_decl()
440+
.and_then(|decl| decl.inputs.get(arg + 1))
441+
.map_or(method.ident.span, |arg| arg.span),
442+
(MethodViolationCode::UndispatchableReceiver, Some(node)) => node
443+
.fn_decl()
444+
.and_then(|decl| decl.inputs.get(0))
445+
.map_or(method.ident.span, |arg| arg.span),
446+
(MethodViolationCode::ReferencesSelfOutput, Some(node)) => {
447+
node.fn_decl().map_or(method.ident.span, |decl| decl.output.span())
448+
}
449+
_ => method.ident.span,
450+
};
451+
(v, span)
452+
})
410453
}
411454

412455
/// Returns `Some(_)` if this method cannot be called on a trait
@@ -420,18 +463,26 @@ fn virtual_call_violation_for_method<'tcx>(
420463
) -> Option<MethodViolationCode> {
421464
// The method's first parameter must be named `self`
422465
if !method.method_has_self_argument {
423-
return Some(MethodViolationCode::StaticMethod);
466+
// We'll attempt to provide a structured suggestion for `Self: Sized`.
467+
let sugg =
468+
tcx.hir().get_if_local(method.def_id).as_ref().and_then(|node| node.generics()).map(
469+
|generics| match generics.where_clause.predicates {
470+
[] => (" where Self: Sized", generics.where_clause.span),
471+
[.., pred] => (", Self: Sized", pred.span().shrink_to_hi()),
472+
},
473+
);
474+
return Some(MethodViolationCode::StaticMethod(sugg));
424475
}
425476

426477
let sig = tcx.fn_sig(method.def_id);
427478

428-
for input_ty in &sig.skip_binder().inputs()[1..] {
479+
for (i, input_ty) in sig.skip_binder().inputs()[1..].iter().enumerate() {
429480
if contains_illegal_self_type_reference(tcx, trait_def_id, input_ty) {
430-
return Some(MethodViolationCode::ReferencesSelf);
481+
return Some(MethodViolationCode::ReferencesSelfInput(i));
431482
}
432483
}
433484
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.output().skip_binder()) {
434-
return Some(MethodViolationCode::ReferencesSelf);
485+
return Some(MethodViolationCode::ReferencesSelfOutput);
435486
}
436487

437488
// We can't monomorphize things like `fn foo<A>(...)`.

‎src/librustc_hir/hir.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2631,4 +2631,25 @@ impl Node<'_> {
26312631
_ => None,
26322632
}
26332633
}
2634+
2635+
pub fn fn_decl(&self) -> Option<&FnDecl<'_>> {
2636+
match self {
2637+
Node::TraitItem(TraitItem { kind: TraitItemKind::Method(fn_sig, _), .. })
2638+
| Node::ImplItem(ImplItem { kind: ImplItemKind::Method(fn_sig, _), .. })
2639+
| Node::Item(Item { kind: ItemKind::Fn(fn_sig, _, _), .. }) => Some(fn_sig.decl),
2640+
Node::ForeignItem(ForeignItem { kind: ForeignItemKind::Fn(fn_decl, _, _), .. }) => {
2641+
Some(fn_decl)
2642+
}
2643+
_ => None,
2644+
}
2645+
}
2646+
2647+
pub fn generics(&self) -> Option<&Generics<'_>> {
2648+
match self {
2649+
Node::TraitItem(TraitItem { generics, .. })
2650+
| Node::ImplItem(ImplItem { generics, .. })
2651+
| Node::Item(Item { kind: ItemKind::Fn(_, generics, _), .. }) => Some(generics),
2652+
_ => None,
2653+
}
2654+
}
26342655
}

‎src/librustc_parse/parser/generics.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ impl<'a> Parser<'a> {
172172
/// ```
173173
pub(super) fn parse_where_clause(&mut self) -> PResult<'a, WhereClause> {
174174
let mut where_clause =
175-
WhereClause { predicates: Vec::new(), span: self.prev_span.to(self.prev_span) };
175+
WhereClause { predicates: Vec::new(), span: self.prev_span.shrink_to_hi() };
176176

177177
if !self.eat_keyword(kw::Where) {
178178
return Ok(where_clause);

‎src/test/ui/coherence/coherence-impl-trait-for-trait-object-safe.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ error[E0038]: the trait `NotObjectSafe` cannot be made into an object
22
--> $DIR/coherence-impl-trait-for-trait-object-safe.rs:7:6
33
|
44
LL | trait NotObjectSafe { fn eq(&self, other: Self); }
5-
| ------------- -- ...because method `eq` references the `Self` type in its parameters or return type
5+
| ------------- ---- ...because method `eq` references the `Self` type in this parameter
66
| |
77
| this trait cannot be made into an object...
88
LL | impl NotObjectSafe for dyn NotObjectSafe { }

‎src/test/ui/error-codes/E0033-teach.stderr‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ LL | fn foo();
1515
LL | let trait_obj: &dyn SomeTrait = SomeTrait;
1616
| ^^^^^^^^^^^^^^ the trait `SomeTrait` cannot be made into an object
1717
|
18-
= help: consider turning `foo` into a method by giving it a `&self` argument or constraining it with `where Self: Sized`
18+
help: consider turning `foo` into a method by giving it a `&self` argument or constraining it so it does not apply to trait objects
19+
|
20+
LL | fn foo() where Self: Sized;
21+
| ^^^^^^^^^^^^^^^^^
1922

2023
error[E0033]: type `&dyn SomeTrait` cannot be dereferenced
2124
--> $DIR/E0033-teach.rs:12:9

‎src/test/ui/error-codes/E0033.stderr‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ LL | fn foo();
1515
LL | let trait_obj: &dyn SomeTrait = SomeTrait;
1616
| ^^^^^^^^^^^^^^ the trait `SomeTrait` cannot be made into an object
1717
|
18-
= help: consider turning `foo` into a method by giving it a `&self` argument or constraining it with `where Self: Sized`
18+
help: consider turning `foo` into a method by giving it a `&self` argument or constraining it so it does not apply to trait objects
19+
|
20+
LL | fn foo() where Self: Sized;
21+
| ^^^^^^^^^^^^^^^^^
1922

2023
error[E0033]: type `&dyn SomeTrait` cannot be dereferenced
2124
--> $DIR/E0033.rs:10:9

‎src/test/ui/error-codes/E0038.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ error[E0038]: the trait `Trait` cannot be made into an object
44
LL | trait Trait {
55
| ----- this trait cannot be made into an object...
66
LL | fn foo(&self) -> Self;
7-
| --- ...because method `foo` references the `Self` type in its parameters or return type
7+
| ---- ...because method `foo` references the `Self` type in its return type
88
...
99
LL | fn call_foo(x: Box<dyn Trait>) {
1010
| ^^^^^^^^^^^^^^ the trait `Trait` cannot be made into an object

‎src/test/ui/feature-gates/feature-gate-object_safe_for_dispatch.stderr‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ LL | fn static_fn() {}
2020
LL | fn return_non_object_safe_ref() -> &'static dyn NonObjectSafe2 {
2121
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NonObjectSafe2` cannot be made into an object
2222
|
23-
= help: consider turning `static_fn` into a method by giving it a `&self` argument or constraining it with `where Self: Sized`
23+
help: consider turning `static_fn` into a method by giving it a `&self` argument or constraining it so it does not apply to trait objects
24+
|
25+
LL | fn static_fn() where Self: Sized {}
26+
| ^^^^^^^^^^^^^^^^^
2427

2528
error[E0038]: the trait `NonObjectSafe3` cannot be made into an object
2629
--> $DIR/feature-gate-object_safe_for_dispatch.rs:27:35
@@ -41,7 +44,7 @@ error[E0038]: the trait `NonObjectSafe4` cannot be made into an object
4144
LL | trait NonObjectSafe4 {
4245
| -------------- this trait cannot be made into an object...
4346
LL | fn foo(&self, &Self);
44-
| --- ...because method `foo` references the `Self` type in its parameters or return type
47+
| ----- ...because method `foo` references the `Self` type in this parameter
4548
...
4649
LL | fn return_non_object_safe_rc() -> std::rc::Rc<dyn NonObjectSafe4> {
4750
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NonObjectSafe4` cannot be made into an object

‎src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.stderr‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ LL | fn foo() -> Self;
99
LL | fn car() -> dyn NotObjectSafe {
1010
| ^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object
1111
|
12-
= help: consider turning `foo` into a method by giving it a `&self` argument or constraining it with `where Self: Sized`
12+
help: consider turning `foo` into a method by giving it a `&self` argument or constraining it so it does not apply to trait objects
13+
|
14+
LL | fn foo() -> Self where Self: Sized;
15+
| ^^^^^^^^^^^^^^^^^
1316

1417
error[E0038]: the trait `NotObjectSafe` cannot be made into an object
1518
--> $DIR/object-unsafe-trait-in-return-position-dyn-trait.rs:28:13
@@ -22,7 +25,10 @@ LL | fn foo() -> Self;
2225
LL | fn cat() -> Box<dyn NotObjectSafe> {
2326
| ^^^^^^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object
2427
|
25-
= help: consider turning `foo` into a method by giving it a `&self` argument or constraining it with `where Self: Sized`
28+
help: consider turning `foo` into a method by giving it a `&self` argument or constraining it so it does not apply to trait objects
29+
|
30+
LL | fn foo() -> Self where Self: Sized;
31+
| ^^^^^^^^^^^^^^^^^
2632

2733
error: aborting due to 2 previous errors
2834

0 commit comments

Comments
 (0)