Skip to content

Commit

Permalink
Suggest using deref in patterns
Browse files Browse the repository at this point in the history
Fixes #132784
  • Loading branch information
uellenberg committed Nov 12, 2024
1 parent 42b2496 commit ca71995
Show file tree
Hide file tree
Showing 11 changed files with 404 additions and 16 deletions.
3 changes: 3 additions & 0 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ pub(super) fn collect_return_position_impl_trait_in_trait_tys<'tcx>(
}))),
terr,
false,
None,
);
return Err(diag.emit());
}
Expand Down Expand Up @@ -1055,6 +1056,7 @@ fn report_trait_method_mismatch<'tcx>(
}))),
terr,
false,
None,
);

diag.emit()
Expand Down Expand Up @@ -1852,6 +1854,7 @@ fn compare_const_predicate_entailment<'tcx>(
}))),
terr,
false,
None,
);
return Err(diag.emit());
};
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_analysis/src/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ pub fn check_function_signature<'tcx>(
}))),
err,
false,
None,
);
return Err(diag.emit());
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(self.param_env.and(trace.values)),
e,
true,
None,
);
}
}
Expand Down
16 changes: 15 additions & 1 deletion compiler/rustc_hir_typeck/src/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use rustc_errors::{
};
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::pat_util::EnumerateAndAdjustIterator;
use rustc_hir::{self as hir, BindingMode, ByRef, HirId, LangItem, Mutability, Pat, PatKind};
use rustc_hir::{
self as hir, BindingMode, ByRef, HirId, LangItem, Mutability, Pat, PatKind, is_range_literal,
};
use rustc_infer::infer;
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
use rustc_middle::{bug, span_bug};
Expand Down Expand Up @@ -94,10 +96,22 @@ struct PatInfo<'a, 'tcx> {

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn pattern_cause(&self, ti: &TopInfo<'tcx>, cause_span: Span) -> ObligationCause<'tcx> {
let needs_parens = ti
.origin_expr
.map(|expr| match expr.kind {
// parenthesize if needed (Issue #46756)
hir::ExprKind::Cast(_, _) | hir::ExprKind::Binary(_, _, _) => true,
// parenthesize borrows of range literals (Issue #54505)
_ if is_range_literal(expr) => true,
_ => false,
})
.unwrap_or(false);

let code = ObligationCauseCode::Pattern {
span: ti.span,
root_ty: ti.expected,
origin_expr: ti.origin_expr.is_some(),
prefix_suggestion_parentheses: needs_parens,
};
self.cause(cause_span, code)
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,9 @@ pub enum ObligationCauseCode<'tcx> {
root_ty: Ty<'tcx>,
/// Whether the `Span` came from an expression or a type expression.
origin_expr: bool,
/// If the `Span` came from an expression, does that expression need to be wrapped in
/// parentheses for a prefix suggestion (i.e., dereference) to be valid.
prefix_suggestion_parentheses: bool,
},

/// Computing common supertype in an if expression
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2321,6 +2321,7 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
}))),
terr,
false,
None,
);
diag.emit();
self.abort.set(true);
Expand Down
134 changes: 123 additions & 11 deletions compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,15 @@ use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::Visitor;
use rustc_hir::lang_items::LangItem;
use rustc_hir::{self as hir};
use rustc_infer::infer::DefineOpaqueTypes;
use rustc_infer::traits::Obligation;
use rustc_macros::extension;
use rustc_middle::bug;
use rustc_middle::dep_graph::DepContext;
use rustc_middle::ty::error::{ExpectedFound, TypeError, TypeErrorToStringExt};
use rustc_middle::ty::print::{PrintError, PrintTraitRefExt as _, with_forced_trimmed_paths};
use rustc_middle::ty::{
self, List, Region, Ty, TyCtxt, TypeFoldable, TypeSuperVisitable, TypeVisitable,
self, List, ParamEnv, Region, Ty, TyCtxt, TypeFoldable, TypeSuperVisitable, TypeVisitable,
TypeVisitableExt,
};
use rustc_span::{BytePos, DesugaringKind, Pos, Span, sym};
Expand All @@ -76,8 +78,10 @@ use crate::infer;
use crate::infer::relate::{self, RelateResult, TypeRelation};
use crate::infer::{InferCtxt, TypeTrace, ValuePairs};
use crate::solve::deeply_normalize_for_diagnostics;
use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::{
IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
ObligationCtxt,
};

mod note_and_explain;
Expand Down Expand Up @@ -339,9 +343,15 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
cause: &ObligationCause<'tcx>,
exp_found: Option<ty::error::ExpectedFound<Ty<'tcx>>>,
terr: TypeError<'tcx>,
param_env: Option<ParamEnv<'tcx>>,
) {
match *cause.code() {
ObligationCauseCode::Pattern { origin_expr: true, span: Some(span), root_ty } => {
ObligationCauseCode::Pattern {
origin_expr: true,
span: Some(span),
root_ty,
prefix_suggestion_parentheses,
} => {
let ty = self.resolve_vars_if_possible(root_ty);
if !matches!(ty.kind(), ty::Infer(ty::InferTy::TyVar(_) | ty::InferTy::FreshTy(_)))
{
Expand All @@ -359,15 +369,35 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
}
}
if let Some(ty::error::ExpectedFound { found, .. }) = exp_found
&& ty.boxed_ty() == Some(found)
&& let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span)
&& let Ok(mut snippet) = self.tcx.sess.source_map().span_to_snippet(span)
{
err.span_suggestion(
span,
"consider dereferencing the boxed value",
format!("*{snippet}"),
Applicability::MachineApplicable,
);
// Parentheses are needed for cases like as casts.
snippet = if prefix_suggestion_parentheses {
format!("({snippet})")
} else {
snippet
};

// Try giving a box suggestion first, as it is a special case of the
// deref suggestion.
if ty.boxed_ty() == Some(found) {
err.span_suggestion(
span,
"consider dereferencing the boxed value",
format!("*{snippet}"),
Applicability::MachineApplicable,
);
} else if let Some(param_env) = param_env
&& let Some(prefix) =
self.should_deref_suggestion_on_mismatch(cause, param_env, found, ty)
{
err.span_suggestion(
span,
"consider dereferencing to access the inner value",
format!("{prefix}{snippet}"),
Applicability::MaybeIncorrect,
);
}
}
}
ObligationCauseCode::Pattern { origin_expr: false, span: Some(span), .. } => {
Expand Down Expand Up @@ -524,6 +554,86 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
}
}

/// Determines whether deref_to == <deref_from as Deref>::Target, and if so,
/// returns a prefix that should be added to deref_from as a suggestion.
fn should_deref_suggestion_on_mismatch(
&self,
cause: &ObligationCause<'tcx>,
param_env: ParamEnv<'tcx>,
deref_to: Ty<'tcx>,
deref_from: Ty<'tcx>,
) -> Option<String> {
let tcx = self.infcx.tcx;

let deref_trait = tcx.lang_items().deref_trait()?;
let deref_mut_trait = tcx.lang_items().deref_mut_trait()?;
let deref_target = tcx.lang_items().deref_target()?;

// Count the number of references in deref_from, since we need
// that plus 1 to invoke Deref.
// This is also needed to ensure we aren't checking for Deref on a reference.
let mut deref_from_cur = deref_from;
// Extra required dereference to invoke Deref.
let mut num_refs = 1;
while let ty::Ref(_, inner_ty, _) = deref_from_cur.kind() {
deref_from_cur = *inner_ty;
num_refs += 1;
}

// <deref_from_cur as Deref>
let trait_ref = ty::TraitRef::new(tcx, deref_trait, [deref_from_cur]);
let obligation =
Obligation::new(tcx, cause.clone(), param_env, ty::Binder::dummy(trait_ref));
if !self.infcx.predicate_may_hold(&obligation) {
return None;
}

let ocx = ObligationCtxt::new(self.infcx);

// <deref_from_cur as Deref>::Target
let target = Ty::new_projection(tcx, deref_target, [deref_from_cur]);

let target_normalized = ocx.structurally_normalize(cause, param_env, target).ok()?;
let expected_normalized = ocx.structurally_normalize(cause, param_env, deref_to).ok()?;

// Determine whether target_normalized == expected_normalized with inference.
let types_eq = self.infcx.probe(|_| {
// FIXME: I don't know what DefineOpaqueTypes should be used here.
self.infcx
.at(cause, param_env)
.eq(DefineOpaqueTypes::No, expected_normalized, target_normalized)
.is_ok()
});

if !types_eq {
return None;
}

let deref_part = "*".repeat(num_refs);

// FIXME: Edge case: how should this handle double references?
// Currently, they're dereferenced correctly, but only the
// outer reference is added back after the dereferences.

// Check if we have DerefMut, as it's required to create a mut reference.
let trait_ref_deref_mut = ty::TraitRef::new(tcx, deref_mut_trait, [deref_from_cur]);
let obligation_deref_mut =
Obligation::new(tcx, cause.clone(), param_env, ty::Binder::dummy(trait_ref_deref_mut));
let has_deref_mut = self.infcx.predicate_may_hold(&obligation_deref_mut);

// Try to give a suggestion with the same type of reference as the original code.
// For example, if deref_from was a reference, then make the suggestion a reference as well.
let valid_mutability = deref_from
.ref_mutability()
.map(|v| if has_deref_mut { v } else { hir::Mutability::Not });

match valid_mutability {
Some(hir::Mutability::Mut) => Some(format!("&mut {deref_part}")),
Some(hir::Mutability::Not) => Some(format!("&{deref_part}")),
None => Some(deref_part),
}
}

/// Given that `other_ty` is the same as a type argument for `name` in `sub`, populate `value`
/// highlighting `name` and every type argument that isn't at `pos` (which is `other_ty`), and
/// populate `other_value` with `other_ty`.
Expand Down Expand Up @@ -1209,6 +1319,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
mut values: Option<ty::ParamEnvAnd<'tcx, ValuePairs<'tcx>>>,
terr: TypeError<'tcx>,
prefer_label: bool,
param_env: Option<ty::ParamEnv<'tcx>>,
) {
let span = cause.span;

Expand Down Expand Up @@ -1693,7 +1804,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {

// It reads better to have the error origin as the final
// thing.
self.note_error_origin(diag, cause, exp_found, terr);
self.note_error_origin(diag, cause, exp_found, terr, param_env);

debug!(?diag);
}
Expand Down Expand Up @@ -1868,6 +1979,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
Some(param_env.and(trace.values)),
terr,
false,
Some(param_env),
);
diag
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
None,
TypeError::Sorts(ty::error::ExpectedFound::new(true, expected_ty, ct_ty)),
false,
None,
);
diag
}
Expand Down Expand Up @@ -1456,6 +1457,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
}),
err,
false,
None,
);
self.note_obligation_cause(&mut diag, obligation);
diag.emit()
Expand Down
12 changes: 8 additions & 4 deletions tests/ui/let-else/let-else-deref-coercion.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@ error[E0308]: mismatched types
--> $DIR/let-else-deref-coercion.rs:37:13
|
LL | let Bar::Present(z) = self else {
| ^^^^^^^^^^^^^^^ ---- this expression has type `&mut Foo`
| |
| ^^^^^^^^^^^^^^^ ----
| | |
| | this expression has type `&mut Foo`
| | help: consider dereferencing to access the inner value: `&mut **self`
| expected `Foo`, found `Bar`

error[E0308]: mismatched types
--> $DIR/let-else-deref-coercion.rs:68:13
|
LL | let Bar(z) = x;
| ^^^^^^ - this expression has type `&mut irrefutable::Foo`
| |
| ^^^^^^ -
| | |
| | this expression has type `&mut irrefutable::Foo`
| | help: consider dereferencing to access the inner value: `&mut **x`
| expected `Foo`, found `Bar`

error: aborting due to 2 previous errors
Expand Down
64 changes: 64 additions & 0 deletions tests/ui/suggestions/suggest-deref-in-match-issue-132784.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
use std::sync::Arc;
fn main() {
let mut x = Arc::new(Some(1));
match x {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}

let mut y = Box::new(Some(1));
match y {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}

let mut z = Arc::new(Some(1));
match z as Arc<Option<i32>> {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}

let z_const: &Arc<Option<i32>> = &z;
match z_const {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}

// Normal reference because Arc doesn't implement DerefMut.
let z_mut: &mut Arc<Option<i32>> = &mut z;
match z_mut {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}

// Mutable reference because Box does implement DerefMut.
let y_mut: &mut Box<Option<i32>> = &mut y;
match y_mut {
//~^ HELP consider dereferencing to access the inner value
//~| HELP consider dereferencing to access the inner value
Some(_) => {}
//~^ ERROR mismatched types
None => {}
//~^ ERROR mismatched types
}
}
Loading

0 comments on commit ca71995

Please sign in to comment.