Skip to content

Don't add redundant help for object safety violations #117200

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.

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

Merged
merged 2 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions compiler/rustc_infer/src/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,19 @@ pub fn report_object_safety_error<'tcx>(
to be resolvable dynamically; for more information visit \
<https://doc.rust-lang.org/reference/items/traits.html#object-safety>",
);

// Only provide the help if its a local trait, otherwise it's not actionable.
if trait_span.is_some() {
let mut reported_violations: Vec<_> = reported_violations.into_iter().collect();
reported_violations.sort();
for violation in reported_violations {
// Only provide the help if its a local trait, otherwise it's not actionable.
violation.solution(&mut err);

let mut potential_solutions: Vec<_> =
reported_violations.into_iter().map(|violation| violation.solution()).collect();
potential_solutions.sort();
// Allows us to skip suggesting that the same item should be moved to another trait multiple times.
potential_solutions.dedup();
for solution in potential_solutions {
solution.add_to(&mut err);
}
}

Expand Down
95 changes: 65 additions & 30 deletions compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -843,50 +843,31 @@ impl ObjectSafetyViolation {
}
}

pub fn solution(&self, err: &mut Diagnostic) {
pub fn solution(&self) -> ObjectSafetyViolationSolution {
match self {
ObjectSafetyViolation::SizedSelf(_)
| ObjectSafetyViolation::SupertraitSelf(_)
| ObjectSafetyViolation::SupertraitNonLifetimeBinder(..) => {}
| ObjectSafetyViolation::SupertraitNonLifetimeBinder(..) => {
ObjectSafetyViolationSolution::None
}
ObjectSafetyViolation::Method(
name,
MethodViolationCode::StaticMethod(Some((add_self_sugg, make_sized_sugg))),
_,
) => {
err.span_suggestion(
add_self_sugg.1,
format!(
"consider turning `{name}` into a method by giving it a `&self` argument"
),
add_self_sugg.0.to_string(),
Applicability::MaybeIncorrect,
);
err.span_suggestion(
make_sized_sugg.1,
format!(
"alternatively, consider constraining `{name}` so it does not apply to \
trait objects"
),
make_sized_sugg.0.to_string(),
Applicability::MaybeIncorrect,
);
}
) => ObjectSafetyViolationSolution::AddSelfOrMakeSized {
name: *name,
add_self_sugg: add_self_sugg.clone(),
make_sized_sugg: make_sized_sugg.clone(),
},
ObjectSafetyViolation::Method(
name,
MethodViolationCode::UndispatchableReceiver(Some(span)),
_,
) => {
err.span_suggestion(
*span,
format!("consider changing method `{name}`'s `self` parameter to be `&self`"),
"&Self",
Applicability::MachineApplicable,
);
}
) => ObjectSafetyViolationSolution::ChangeToRefSelf(*name, *span),
ObjectSafetyViolation::AssocConst(name, _)
| ObjectSafetyViolation::GAT(name, _)
| ObjectSafetyViolation::Method(name, ..) => {
err.help(format!("consider moving `{name}` to another trait"));
ObjectSafetyViolationSolution::MoveToAnotherTrait(*name)
}
}
}
Expand All @@ -910,6 +891,60 @@ impl ObjectSafetyViolation {
}
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub enum ObjectSafetyViolationSolution {
None,
AddSelfOrMakeSized {
name: Symbol,
add_self_sugg: (String, Span),
make_sized_sugg: (String, Span),
},
ChangeToRefSelf(Symbol, Span),
MoveToAnotherTrait(Symbol),
}

impl ObjectSafetyViolationSolution {
pub fn add_to(self, err: &mut Diagnostic) {
match self {
ObjectSafetyViolationSolution::None => {}
ObjectSafetyViolationSolution::AddSelfOrMakeSized {
name,
add_self_sugg,
make_sized_sugg,
} => {
err.span_suggestion(
add_self_sugg.1,
format!(
"consider turning `{name}` into a method by giving it a `&self` argument"
),
add_self_sugg.0,
Applicability::MaybeIncorrect,
);
err.span_suggestion(
make_sized_sugg.1,
format!(
"alternatively, consider constraining `{name}` so it does not apply to \
trait objects"
),
make_sized_sugg.0,
Applicability::MaybeIncorrect,
);
}
ObjectSafetyViolationSolution::ChangeToRefSelf(name, span) => {
err.span_suggestion(
span,
format!("consider changing method `{name}`'s `self` parameter to be `&self`"),
"&Self",
Applicability::MachineApplicable,
);
}
ObjectSafetyViolationSolution::MoveToAnotherTrait(name) => {
err.help(format!("consider moving `{name}` to another trait"));
}
}
}
}

/// Reasons a method might not be object-safe.
#[derive(Clone, Debug, PartialEq, Eq, Hash, HashStable, PartialOrd, Ord)]
pub enum MethodViolationCode {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ fn lint_object_unsafe_trait(
);
if node.is_some() {
// Only provide the help if its a local trait, otherwise it's not
violation.solution(err);
violation.solution().add_to(err);
}
err
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ LL | fn test(&self) -> [u8; bar::<Self>()];
| |
| ...because method `test` references the `Self` type in its `where` clause
= help: consider moving `test` to another trait
= help: consider moving `test` to another trait
= help: only type `()` implements the trait, consider using it directly instead

error: aborting due to previous error
Expand Down