Skip to content

Commit

Permalink
Fix for rust-lang#17497 - Invalid RA diagnostic error: expected 2 arg…
Browse files Browse the repository at this point in the history
…uments, found 1

Also fixes rust-lang#17233

The issue occurs because in some configurations of traits where one of them has Deref as a supertrait, RA's type inference algorithm fails to fully resolve a trait's self type, and instead uses a TyKind::BoundVar. This then erroneously matches with types that do not implement the trait. In other words, unconnected types appear to inherit the trait's methods.

The fix is to insert a heuristic when invoking lookup_method() that allows the caller to determine if the returned method is a good match. If the returned method would result in diagnostic errors, the caller instructs the algorithm to continue iterating the possible methods to see if a better one exists (i.e. one that won't result in diagnostic errors).

Includes a unit test that only passes after applying the fixes in this commit.
  • Loading branch information
mckenfra committed Jun 28, 2024
1 parent 4e836c6 commit 429e028
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 7 deletions.
54 changes: 49 additions & 5 deletions crates/hir-ty/src/infer/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ impl InferenceContext<'_> {
&param_tys,
&indices_to_skip,
is_varargs,
false,
);
self.normalize_associated_types_in(ret_ty)
}
Expand Down Expand Up @@ -1615,14 +1616,22 @@ impl InferenceContext<'_> {
) -> Ty {
let receiver_ty = self.infer_expr_inner(receiver, &Expectation::none());
let canonicalized_receiver = self.canonicalize(receiver_ty.clone());
let traits_in_scope = self.get_traits_in_scope().left_or_else(|it| it.clone());

let resolved = method_resolution::lookup_method(
let resolved = method_resolution::lookup_method_using_heuristic(
self.db,
&canonicalized_receiver,
self.table.trait_env.clone(),
self.get_traits_in_scope().as_ref().left_or_else(|&it| it),
&traits_in_scope,
VisibleFromModule::Filter(self.resolver.module()),
method_name,
|adjust, func| {
let (receiver_ty, _) = adjust.apply(&mut self.table, receiver_ty.clone());
let generics = generics(self.db.upcast(), func.into());
let substs = self.substs_for_method_call(generics, generic_args);
let method_ty = self.db.value_ty(func.into()).unwrap();
self.try_method_call(tgt_expr.clone(), args, method_ty, substs, receiver_ty, expected)
}
);
let (receiver_ty, method_ty, substs) = match resolved {
Some((adjust, func, visible)) => {
Expand Down Expand Up @@ -1684,6 +1693,21 @@ impl InferenceContext<'_> {
self.check_method_call(tgt_expr, args, method_ty, substs, receiver_ty, expected)
}

#[inline]
fn try_method_call(
&mut self,
tgt_expr: ExprId,
args: &[ExprId],
method_ty: Binders<Ty>,
substs: Substitution,
receiver_ty: Ty,
expected: &Expectation,
) -> bool {
self.check_method_call_inner(tgt_expr, args, method_ty, substs, receiver_ty, expected, true)
.is_ok()
}

#[inline]
fn check_method_call(
&mut self,
tgt_expr: ExprId,
Expand All @@ -1693,6 +1717,20 @@ impl InferenceContext<'_> {
receiver_ty: Ty,
expected: &Expectation,
) -> Ty {
self.check_method_call_inner(tgt_expr, args, method_ty, substs, receiver_ty, expected, false)
.unwrap()
}

fn check_method_call_inner(
&mut self,
tgt_expr: ExprId,
args: &[ExprId],
method_ty: Binders<Ty>,
substs: Substitution,
receiver_ty: Ty,
expected: &Expectation,
is_dryrun: bool,
) -> Result<Ty,()> {
let method_ty = method_ty.substitute(Interner, &substs);
self.register_obligations_for_call(&method_ty);
let ((formal_receiver_ty, param_tys), ret_ty, is_varargs) =
Expand All @@ -1713,8 +1751,10 @@ impl InferenceContext<'_> {
let expected_inputs =
self.expected_inputs_for_expected_output(expected, ret_ty.clone(), param_tys.clone());

self.check_call_arguments(tgt_expr, args, &expected_inputs, &param_tys, &[], is_varargs);
self.normalize_associated_types_in(ret_ty)
if !self.check_call_arguments(tgt_expr, args, &expected_inputs, &param_tys, &[], is_varargs, is_dryrun) {
if is_dryrun { return Err(()); }
}
Ok(self.normalize_associated_types_in(ret_ty))
}

fn expected_inputs_for_expected_output(
Expand Down Expand Up @@ -1752,8 +1792,10 @@ impl InferenceContext<'_> {
param_tys: &[Ty],
skip_indices: &[u32],
is_varargs: bool,
) {
is_dryrun: bool,
) -> bool {
if args.len() != param_tys.len() + skip_indices.len() && !is_varargs {
if is_dryrun { return false; }
self.push_diagnostic(InferenceDiagnostic::MismatchedArgCount {
call_expr: expr,
expected: param_tys.len() + skip_indices.len(),
Expand Down Expand Up @@ -1814,13 +1856,15 @@ impl InferenceContext<'_> {
// type vars here to avoid type mismatch false positive.
let coercion_target = self.insert_type_vars(coercion_target);
if self.coerce(Some(arg), &ty, &coercion_target).is_err() {
if is_dryrun { return false; }
self.result.type_mismatches.insert(
arg.into(),
TypeMismatch { expected: coercion_target, actual: ty.clone() },
);
}
}
}
true
}

fn substs_for_method_call(
Expand Down
36 changes: 34 additions & 2 deletions crates/hir-ty/src/method_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,15 +437,38 @@ pub fn def_crates(
}

/// Look up the method with the given name.
#[inline]
pub(crate) fn lookup_method(
db: &dyn HirDatabase,
ty: &Canonical<Ty>,
env: Arc<TraitEnvironment>,
traits_in_scope: &FxHashSet<TraitId>,
visible_from_module: VisibleFromModule,
name: &Name,
) -> Option<(ReceiverAdjustments, FunctionId, bool)> {
lookup_method_using_heuristic(
db,
ty,
env,
traits_in_scope,
visible_from_module,
name,
|_,_| true,
)
}

/// Look up the method with the given name, using a callback to evaluate matches.
pub(crate) fn lookup_method_using_heuristic(
db: &dyn HirDatabase,
ty: &Canonical<Ty>,
env: Arc<TraitEnvironment>,
traits_in_scope: &FxHashSet<TraitId>,
visible_from_module: VisibleFromModule,
name: &Name,
mut is_best_match: impl FnMut(&ReceiverAdjustments, FunctionId) -> bool,
) -> Option<(ReceiverAdjustments, FunctionId, bool)> {
let mut not_visible = None;
let mut not_best_match = None;
let res = iterate_method_candidates(
ty,
db,
Expand All @@ -455,15 +478,24 @@ pub(crate) fn lookup_method(
Some(name),
LookupMode::MethodCall,
|adjustments, f, visible| match f {
AssocItemId::FunctionId(f) if visible => Some((adjustments, f, true)),
AssocItemId::FunctionId(f) if visible => {
if is_best_match(&adjustments, f) {
Some((adjustments, f, true))
} else {
if not_best_match.is_none() {
not_best_match = Some((adjustments, f, true));
}
None
}
},
AssocItemId::FunctionId(f) if not_visible.is_none() => {
not_visible = Some((adjustments, f, false));
None
}
_ => None,
},
);
res.or(not_visible)
res.or(not_best_match).or(not_visible)
}

/// Whether we're looking up a dotted method call (like `v.len()`) or a path
Expand Down
39 changes: 39 additions & 0 deletions crates/hir-ty/src/tests/method_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2050,3 +2050,42 @@ fn test() {
"#,
);
}

#[test]
fn mismatched_args_due_to_subtraits_with_deref() {
check_no_mismatches(
r#"
//- minicore: deref
use core::ops::Deref;
trait Trait1 {
type Assoc: Deref<Target = String>;
}
trait Trait2: Trait1 {
}
trait Trait3 {
type T1: Trait1;
type T2: Trait2;
fn bar(&self, x: bool, y: bool);
}
struct Foo;
impl Foo {
fn bar(&mut self, _: &'static str) {}
}
impl Deref for Foo {
type Target = u32;
fn deref(&self) -> &Self::Target { &0 }
}
fn problem_method<T: Trait3>() {
let mut foo = Foo;
foo.bar("hello"); // Rustc ok, RA errors (mismatched args)
}
"#,
);
}

0 comments on commit 429e028

Please sign in to comment.