From 403c8c2fd6205ca0687abde55fbe5af7b325b7d9 Mon Sep 17 00:00:00 2001 From: dianne Date: Thu, 21 Nov 2024 03:23:56 -0800 Subject: [PATCH] E0277: suggest dereferencing function arguments in more cases --- .../src/error_reporting/traits/suggestions.rs | 198 ++++++++---------- tests/ui/no_send-rc.stderr | 4 + .../suggestions/issue-84973-blacklist.stderr | 4 + .../suggest-dereferences/deref-argument.fixed | 37 ++++ .../suggest-dereferences/deref-argument.rs | 37 ++++ .../deref-argument.stderr | 39 ++++ 6 files changed, 206 insertions(+), 113 deletions(-) create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.fixed create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.rs create mode 100644 tests/ui/traits/suggest-dereferences/deref-argument.stderr diff --git a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs index a5e364d49f7cf..f9971e5e2d5a8 100644 --- a/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs +++ b/compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs @@ -443,9 +443,8 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { } } - /// When after several dereferencing, the reference satisfies the trait - /// bound. This function provides dereference suggestion for this - /// specific situation. + /// Provide a suggestion to dereference arguments to functions and binary operators, if that + /// would satisfy trait bounds. pub(super) fn suggest_dereferences( &self, obligation: &PredicateObligation<'tcx>, @@ -459,127 +458,100 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> { && let Some(arg_ty) = typeck_results.expr_ty_adjusted_opt(expr) { // Suggest dereferencing the argument to a function/method call if possible + + // Get the root obligation, since the leaf obligation we have may be unhelpful (#87437) let mut real_trait_pred = trait_pred; while let Some((parent_code, parent_trait_pred)) = code.parent() { code = parent_code; if let Some(parent_trait_pred) = parent_trait_pred { real_trait_pred = parent_trait_pred; } + } - // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle - // `ReBound`, and we don't particularly care about the regions. - let real_ty = - self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty()); + // We `instantiate_bound_regions_with_erased` here because `make_subregion` does not handle + // `ReBound`, and we don't particularly care about the regions. + let real_ty = self.tcx.instantiate_bound_regions_with_erased(real_trait_pred.self_ty()); + if !self.can_eq(obligation.param_env, real_ty, arg_ty) { + return false; + } - if self.can_eq(obligation.param_env, real_ty, arg_ty) - && let ty::Ref(region, base_ty, mutbl) = *real_ty.kind() + // Potentially, we'll want to place our dereferences under a `&`. We don't try this for + // `&mut`, since we can't be sure users will get the side-effects they want from it. + // If this doesn't work, we'll try removing the `&` in `suggest_remove_reference`. + // FIXME(dianne): this misses the case where users need both to deref and remove `&`s. + // This method could be combined with `TypeErrCtxt::suggest_remove_reference` to handle + // that, similar to what `FnCtxt::suggest_deref_or_ref` does. + let (is_under_ref, base_ty, span) = match expr.kind { + hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Not, subexpr) + if let &ty::Ref(region, base_ty, hir::Mutability::Not) = real_ty.kind() => { - let autoderef = (self.autoderef_steps)(base_ty); - if let Some(steps) = - autoderef.into_iter().enumerate().find_map(|(steps, (ty, obligations))| { - // Re-add the `&` - let ty = Ty::new_ref(self.tcx, region, ty, mutbl); - - // Remapping bound vars here - let real_trait_pred_and_ty = real_trait_pred - .map_bound(|inner_trait_pred| (inner_trait_pred, ty)); - let obligation = self.mk_trait_obligation_with_new_self_ty( - obligation.param_env, - real_trait_pred_and_ty, - ); - let may_hold = obligations - .iter() - .chain([&obligation]) - .all(|obligation| self.predicate_may_hold(obligation)) - .then_some(steps); + (Some(region), base_ty, subexpr.span) + } + // Don't suggest `*&mut`, etc. + hir::ExprKind::AddrOf(..) => return false, + _ => (None, real_ty, obligation.cause.span), + }; - may_hold - }) - { - if steps > 0 { - // Don't care about `&mut` because `DerefMut` is used less - // often and user will not expect that an autoderef happens. - if let hir::Node::Expr(hir::Expr { - kind: - hir::ExprKind::AddrOf( - hir::BorrowKind::Ref, - hir::Mutability::Not, - expr, - ), - .. - }) = self.tcx.hir_node(*arg_hir_id) - { - let derefs = "*".repeat(steps); - err.span_suggestion_verbose( - expr.span.shrink_to_lo(), - "consider dereferencing here", - derefs, - Applicability::MachineApplicable, - ); - return true; - } - } - } else if real_trait_pred != trait_pred { - // This branch addresses #87437. - - let span = obligation.cause.span; - // Remapping bound vars here - let real_trait_pred_and_base_ty = real_trait_pred - .map_bound(|inner_trait_pred| (inner_trait_pred, base_ty)); - let obligation = self.mk_trait_obligation_with_new_self_ty( - obligation.param_env, - real_trait_pred_and_base_ty, - ); - let sized_obligation = Obligation::new( - self.tcx, - obligation.cause.clone(), - obligation.param_env, - ty::TraitRef::new( - self.tcx, - self.tcx.require_lang_item( - hir::LangItem::Sized, - Some(obligation.cause.span), - ), - [base_ty], - ), - ); - if self.predicate_may_hold(&obligation) - && self.predicate_must_hold_modulo_regions(&sized_obligation) - // Do not suggest * if it is already a reference, - // will suggest removing the borrow instead in that case. - && !matches!(expr.kind, hir::ExprKind::AddrOf(..)) - { - let call_node = self.tcx.hir_node(*call_hir_id); - let msg = "consider dereferencing here"; - let is_receiver = matches!( - call_node, - Node::Expr(hir::Expr { - kind: hir::ExprKind::MethodCall(_, receiver_expr, ..), - .. - }) - if receiver_expr.hir_id == *arg_hir_id - ); - if is_receiver { - err.multipart_suggestion_verbose( - msg, - vec![ - (span.shrink_to_lo(), "(*".to_string()), - (span.shrink_to_hi(), ")".to_string()), - ], - Applicability::MachineApplicable, - ) - } else { - err.span_suggestion_verbose( - span.shrink_to_lo(), - msg, - '*', - Applicability::MachineApplicable, - ) - }; - return true; - } - } + let autoderef = (self.autoderef_steps)(base_ty); + let mut is_boxed = base_ty.is_box(); + if let Some(steps) = autoderef.into_iter().position(|(mut ty, obligations)| { + // Ensure one of the following for dereferencing to be valid: we're passing by + // reference, `ty` is `Copy`, or we're moving out of a (potentially nested) `Box`. + let can_deref = is_under_ref.is_some() + || self.type_is_copy_modulo_regions(obligation.param_env, ty) + || ty.is_numeric() // for inference vars (presumably but not provably `Copy`) + || is_boxed && self.type_is_sized_modulo_regions(obligation.param_env, ty); + is_boxed &= ty.is_box(); + + // Re-add the `&` if necessary + if let Some(region) = is_under_ref { + ty = Ty::new_ref(self.tcx, region, ty, hir::Mutability::Not); } + + // Remapping bound vars here + let real_trait_pred_and_ty = + real_trait_pred.map_bound(|inner_trait_pred| (inner_trait_pred, ty)); + let obligation = self.mk_trait_obligation_with_new_self_ty( + obligation.param_env, + real_trait_pred_and_ty, + ); + + can_deref + && obligations + .iter() + .chain([&obligation]) + .all(|obligation| self.predicate_may_hold(obligation)) + }) && steps > 0 + { + let derefs = "*".repeat(steps); + let msg = "consider dereferencing here"; + let call_node = self.tcx.hir_node(*call_hir_id); + let is_receiver = matches!( + call_node, + Node::Expr(hir::Expr { + kind: hir::ExprKind::MethodCall(_, receiver_expr, ..), + .. + }) + if receiver_expr.hir_id == *arg_hir_id + ); + if is_receiver { + err.multipart_suggestion_verbose( + msg, + vec![ + (span.shrink_to_lo(), format!("({derefs}")), + (span.shrink_to_hi(), ")".to_string()), + ], + Applicability::MachineApplicable, + ) + } else { + err.span_suggestion_verbose( + span.shrink_to_lo(), + msg, + derefs, + Applicability::MachineApplicable, + ) + }; + return true; } } else if let ( ObligationCauseCode::BinOp { lhs_hir_id, rhs_hir_id: Some(rhs_hir_id), .. }, diff --git a/tests/ui/no_send-rc.stderr b/tests/ui/no_send-rc.stderr index 3534167870baf..1430a7a29ea26 100644 --- a/tests/ui/no_send-rc.stderr +++ b/tests/ui/no_send-rc.stderr @@ -12,6 +12,10 @@ note: required by a bound in `bar` | LL | fn bar(_: T) {} | ^^^^ required by this bound in `bar` +help: consider dereferencing here + | +LL | bar(*x); + | + error: aborting due to 1 previous error diff --git a/tests/ui/suggestions/issue-84973-blacklist.stderr b/tests/ui/suggestions/issue-84973-blacklist.stderr index a6324a824c1c3..3db400418c711 100644 --- a/tests/ui/suggestions/issue-84973-blacklist.stderr +++ b/tests/ui/suggestions/issue-84973-blacklist.stderr @@ -86,6 +86,10 @@ note: required by a bound in `f_send` | LL | fn f_send(t: T) {} | ^^^^ required by this bound in `f_send` +help: consider dereferencing here + | +LL | f_send(*rc); + | + error: aborting due to 5 previous errors diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.fixed b/tests/ui/traits/suggest-dereferences/deref-argument.fixed new file mode 100644 index 0000000000000..8235ae0b62840 --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.fixed @@ -0,0 +1,37 @@ +//@ run-rustfix +//! diagnostic test for #90997. +//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed. +use std::ops::Deref; + +trait Test { + fn test(self); +} +fn consume_test(x: impl Test) { x.test() } + +impl Test for u32 { + fn test(self) {} +} +struct MyRef(u32); +impl Deref for MyRef { + type Target = u32; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +struct NonCopy; +impl Test for NonCopy { + fn test(self) {} +} + +fn main() { + let my_ref = MyRef(0); + consume_test(*my_ref); + //~^ ERROR the trait bound `MyRef: Test` is not satisfied + //~| SUGGESTION * + + let nested_box = Box::new(Box::new(Box::new(NonCopy))); + consume_test(***nested_box); + //~^ ERROR the trait bound `Box>>: Test` is not satisfied + //~| SUGGESTION *** +} diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.rs b/tests/ui/traits/suggest-dereferences/deref-argument.rs new file mode 100644 index 0000000000000..2f96b75c4e476 --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.rs @@ -0,0 +1,37 @@ +//@ run-rustfix +//! diagnostic test for #90997. +//! test that E0277 suggests dereferences to satisfy bounds when the referent is `Copy` or boxed. +use std::ops::Deref; + +trait Test { + fn test(self); +} +fn consume_test(x: impl Test) { x.test() } + +impl Test for u32 { + fn test(self) {} +} +struct MyRef(u32); +impl Deref for MyRef { + type Target = u32; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +struct NonCopy; +impl Test for NonCopy { + fn test(self) {} +} + +fn main() { + let my_ref = MyRef(0); + consume_test(my_ref); + //~^ ERROR the trait bound `MyRef: Test` is not satisfied + //~| SUGGESTION * + + let nested_box = Box::new(Box::new(Box::new(NonCopy))); + consume_test(nested_box); + //~^ ERROR the trait bound `Box>>: Test` is not satisfied + //~| SUGGESTION *** +} diff --git a/tests/ui/traits/suggest-dereferences/deref-argument.stderr b/tests/ui/traits/suggest-dereferences/deref-argument.stderr new file mode 100644 index 0000000000000..3dc92fd6ab6f8 --- /dev/null +++ b/tests/ui/traits/suggest-dereferences/deref-argument.stderr @@ -0,0 +1,39 @@ +error[E0277]: the trait bound `MyRef: Test` is not satisfied + --> $DIR/deref-argument.rs:29:18 + | +LL | consume_test(my_ref); + | ------------ ^^^^^^ the trait `Test` is not implemented for `MyRef` + | | + | required by a bound introduced by this call + | +note: required by a bound in `consume_test` + --> $DIR/deref-argument.rs:9:25 + | +LL | fn consume_test(x: impl Test) { x.test() } + | ^^^^ required by this bound in `consume_test` +help: consider dereferencing here + | +LL | consume_test(*my_ref); + | + + +error[E0277]: the trait bound `Box>>: Test` is not satisfied + --> $DIR/deref-argument.rs:34:18 + | +LL | consume_test(nested_box); + | ------------ ^^^^^^^^^^ the trait `Test` is not implemented for `Box>>` + | | + | required by a bound introduced by this call + | +note: required by a bound in `consume_test` + --> $DIR/deref-argument.rs:9:25 + | +LL | fn consume_test(x: impl Test) { x.test() } + | ^^^^ required by this bound in `consume_test` +help: consider dereferencing here + | +LL | consume_test(***nested_box); + | +++ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0277`.