From 3c7a0aa00ee4d574af36a1ab982a63488acb211a Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 3 Mar 2023 18:04:24 +0100 Subject: [PATCH 1/3] Diagnose call expression on non-callable things --- crates/hir-ty/src/infer.rs | 9 +++++ crates/hir-ty/src/infer/expr.rs | 8 +++- crates/hir/src/diagnostics.rs | 7 ++++ crates/hir/src/lib.rs | 33 +++++++++------- .../src/handlers/expected_function.rs | 38 +++++++++++++++++++ crates/ide-diagnostics/src/lib.rs | 2 + 6 files changed, 83 insertions(+), 14 deletions(-) create mode 100644 crates/ide-diagnostics/src/handlers/expected_function.rs diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs index 336de1428214..0c7529cffee2 100644 --- a/crates/hir-ty/src/infer.rs +++ b/crates/hir-ty/src/infer.rs @@ -170,6 +170,7 @@ pub enum InferenceDiagnostic { // FIXME: Make this proper BreakOutsideOfLoop { expr: ExprId, is_break: bool, bad_value_break: bool }, MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize }, + ExpectedFunction { call_expr: ExprId, found: Ty }, } /// A mismatch between an expected and an inferred type. @@ -505,6 +506,14 @@ impl<'a> InferenceContext<'a> { mismatch.expected = table.resolve_completely(mismatch.expected.clone()); mismatch.actual = table.resolve_completely(mismatch.actual.clone()); } + for diagnostic in &mut result.diagnostics { + match diagnostic { + InferenceDiagnostic::ExpectedFunction { found, .. } => { + *found = table.resolve_completely(found.clone()) + } + _ => (), + } + } for (_, subst) in result.method_resolutions.values_mut() { *subst = table.resolve_completely(subst.clone()); } diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs index e64b020c7fbd..9d75b67bc789 100644 --- a/crates/hir-ty/src/infer/expr.rs +++ b/crates/hir-ty/src/infer/expr.rs @@ -364,7 +364,13 @@ impl<'a> InferenceContext<'a> { } (params, ret_ty) } - None => (Vec::new(), self.err_ty()), // FIXME diagnostic + None => { + self.result.diagnostics.push(InferenceDiagnostic::ExpectedFunction { + call_expr: tgt_expr, + found: callee_ty.clone(), + }); + (Vec::new(), self.err_ty()) + } }; let indices_to_skip = self.check_legacy_const_generics(derefed_callee, args); self.register_obligations_for_call(&callee_ty); diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index bb7468d46604..3a6f26a4ea08 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -31,6 +31,7 @@ macro_rules! diagnostics { diagnostics![ BreakOutsideOfLoop, + ExpectedFunction, InactiveCode, IncorrectCase, InvalidDeriveTarget, @@ -130,6 +131,12 @@ pub struct PrivateAssocItem { pub item: AssocItem, } +#[derive(Debug)] +pub struct ExpectedFunction { + pub call: InFile>, + pub found: Type, +} + #[derive(Debug)] pub struct PrivateField { pub expr: InFile>, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index bfc0d58cc783..08ccf38b654d 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -84,9 +84,9 @@ use crate::db::{DefDatabase, HirDatabase}; pub use crate::{ attrs::{HasAttrs, Namespace}, diagnostics::{ - AnyDiagnostic, BreakOutsideOfLoop, InactiveCode, IncorrectCase, InvalidDeriveTarget, - MacroError, MalformedDerive, MismatchedArgCount, MissingFields, MissingMatchArms, - MissingUnsafe, NoSuchField, PrivateAssocItem, PrivateField, + AnyDiagnostic, BreakOutsideOfLoop, ExpectedFunction, InactiveCode, IncorrectCase, + InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount, MissingFields, + MissingMatchArms, MissingUnsafe, NoSuchField, PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, UnimplementedBuiltinMacro, UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, UnresolvedProcMacro, @@ -1377,8 +1377,8 @@ impl DefWithBody { let source_map = Lazy::new(|| db.body_with_source_map(self.into()).1); for d in &infer.diagnostics { match d { - hir_ty::InferenceDiagnostic::NoSuchField { expr } => { - let field = source_map.field_syntax(*expr); + &hir_ty::InferenceDiagnostic::NoSuchField { expr } => { + let field = source_map.field_syntax(expr); acc.push(NoSuchField { field }.into()) } &hir_ty::InferenceDiagnostic::BreakOutsideOfLoop { @@ -1391,15 +1391,10 @@ impl DefWithBody { .expect("break outside of loop in synthetic syntax"); acc.push(BreakOutsideOfLoop { expr, is_break, bad_value_break }.into()) } - hir_ty::InferenceDiagnostic::MismatchedArgCount { call_expr, expected, found } => { - match source_map.expr_syntax(*call_expr) { + &hir_ty::InferenceDiagnostic::MismatchedArgCount { call_expr, expected, found } => { + match source_map.expr_syntax(call_expr) { Ok(source_ptr) => acc.push( - MismatchedArgCount { - call_expr: source_ptr, - expected: *expected, - found: *found, - } - .into(), + MismatchedArgCount { call_expr: source_ptr, expected, found }.into(), ), Err(SyntheticSyntax) => (), } @@ -1423,6 +1418,18 @@ impl DefWithBody { let item = item.into(); acc.push(PrivateAssocItem { expr_or_pat, item }.into()) } + hir_ty::InferenceDiagnostic::ExpectedFunction { call_expr, found } => { + let call_expr = + source_map.expr_syntax(*call_expr).expect("unexpected synthetic"); + + acc.push( + ExpectedFunction { + call: call_expr, + found: Type::new(db, DefWithBodyId::from(self), found.clone()), + } + .into(), + ) + } } } for (pat_or_expr, mismatch) in infer.type_mismatches() { diff --git a/crates/ide-diagnostics/src/handlers/expected_function.rs b/crates/ide-diagnostics/src/handlers/expected_function.rs new file mode 100644 index 000000000000..23bc778da290 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/expected_function.rs @@ -0,0 +1,38 @@ +use hir::HirDisplay; + +use crate::{Diagnostic, DiagnosticsContext}; + +// Diagnostic: expected-function +// +// This diagnostic is triggered if a call is made on something that is not callable. +pub(crate) fn expected_function( + ctx: &DiagnosticsContext<'_>, + d: &hir::ExpectedFunction, +) -> Diagnostic { + Diagnostic::new( + "expected-function", + format!("expected function, found {}", d.found.display(ctx.sema.db)), + ctx.sema.diagnostics_display_range(d.call.clone().map(|it| it.into())).range, + ) +} + +#[cfg(test)] +mod tests { + use crate::tests::check_diagnostics; + + #[test] + fn smoke_test() { + check_diagnostics( + r#" +fn foo() { + let x = 3; + x(); + // ^^^ error: expected function, found i32 + ""(); + // ^^^^ error: expected function, found &str + foo(); +} +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 64ba08ac883b..b878119fee1f 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -27,6 +27,7 @@ mod handlers { pub(crate) mod break_outside_of_loop; + pub(crate) mod expected_function; pub(crate) mod inactive_code; pub(crate) mod incorrect_case; pub(crate) mod invalid_derive_target; @@ -248,6 +249,7 @@ pub fn diagnostics( #[rustfmt::skip] let d = match diag { AnyDiagnostic::BreakOutsideOfLoop(d) => handlers::break_outside_of_loop::break_outside_of_loop(&ctx, &d), + AnyDiagnostic::ExpectedFunction(d) => handlers::expected_function::expected_function(&ctx, &d), AnyDiagnostic::IncorrectCase(d) => handlers::incorrect_case::incorrect_case(&ctx, &d), AnyDiagnostic::MacroError(d) => handlers::macro_error::macro_error(&ctx, &d), AnyDiagnostic::MalformedDerive(d) => handlers::malformed_derive::malformed_derive(&ctx, &d), From 78b2dd813a927012328bf870d680fe952c8157b9 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 3 Mar 2023 19:32:18 +0100 Subject: [PATCH 2/3] Diagnose unresolved field accesses --- crates/hir-ty/src/infer.rs | 18 ++- crates/hir-ty/src/infer/expr.rs | 152 ++++++++++-------- crates/hir/src/diagnostics.rs | 9 ++ crates/hir/src/lib.rs | 47 +++--- .../src/completions/flyimport.rs | 5 +- .../src/handlers/unresolved_field.rs | 134 +++++++++++++++ crates/ide-diagnostics/src/lib.rs | 2 + 7 files changed, 273 insertions(+), 94 deletions(-) create mode 100644 crates/ide-diagnostics/src/handlers/unresolved_field.rs diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs index 0c7529cffee2..a663a568b919 100644 --- a/crates/hir-ty/src/infer.rs +++ b/crates/hir-ty/src/infer.rs @@ -31,7 +31,7 @@ use hir_def::{ AdtId, AssocItemId, DefWithBodyId, EnumVariantId, FieldId, FunctionId, HasModule, ItemContainerId, Lookup, TraitId, TypeAliasId, VariantId, }; -use hir_expand::name::name; +use hir_expand::name::{name, Name}; use la_arena::ArenaMap; use rustc_hash::FxHashMap; use stdx::always; @@ -167,6 +167,7 @@ pub enum InferenceDiagnostic { NoSuchField { expr: ExprId }, PrivateField { expr: ExprId, field: FieldId }, PrivateAssocItem { id: ExprOrPatId, item: AssocItemId }, + UnresolvedField { expr: ExprId, receiver: Ty, name: Name, method_with_same_name_exists: bool }, // FIXME: Make this proper BreakOutsideOfLoop { expr: ExprId, is_break: bool, bad_value_break: bool }, MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize }, @@ -506,14 +507,17 @@ impl<'a> InferenceContext<'a> { mismatch.expected = table.resolve_completely(mismatch.expected.clone()); mismatch.actual = table.resolve_completely(mismatch.actual.clone()); } - for diagnostic in &mut result.diagnostics { - match diagnostic { - InferenceDiagnostic::ExpectedFunction { found, .. } => { - *found = table.resolve_completely(found.clone()) + result.diagnostics.retain_mut(|diagnostic| { + if let InferenceDiagnostic::ExpectedFunction { found: ty, .. } + | InferenceDiagnostic::UnresolvedField { receiver: ty, .. } = diagnostic + { + *ty = table.resolve_completely(ty.clone()); + if ty.is_unknown() { + return false; } - _ => (), } - } + true + }); for (_, subst) in result.method_resolutions.values_mut() { *subst = table.resolve_completely(subst.clone()); } diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs index 9d75b67bc789..531a359a96a4 100644 --- a/crates/hir-ty/src/infer/expr.rs +++ b/crates/hir-ty/src/infer/expr.rs @@ -552,71 +552,7 @@ impl<'a> InferenceContext<'a> { } ty } - Expr::Field { expr, name } => { - let receiver_ty = self.infer_expr_inner(*expr, &Expectation::none()); - - let mut autoderef = Autoderef::new(&mut self.table, receiver_ty); - let mut private_field = None; - let ty = autoderef.by_ref().find_map(|(derefed_ty, _)| { - let (field_id, parameters) = match derefed_ty.kind(Interner) { - TyKind::Tuple(_, substs) => { - return name.as_tuple_index().and_then(|idx| { - substs - .as_slice(Interner) - .get(idx) - .map(|a| a.assert_ty_ref(Interner)) - .cloned() - }); - } - TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), parameters) => { - let local_id = self.db.struct_data(*s).variant_data.field(name)?; - let field = FieldId { parent: (*s).into(), local_id }; - (field, parameters.clone()) - } - TyKind::Adt(AdtId(hir_def::AdtId::UnionId(u)), parameters) => { - let local_id = self.db.union_data(*u).variant_data.field(name)?; - let field = FieldId { parent: (*u).into(), local_id }; - (field, parameters.clone()) - } - _ => return None, - }; - let is_visible = self.db.field_visibilities(field_id.parent)[field_id.local_id] - .is_visible_from(self.db.upcast(), self.resolver.module()); - if !is_visible { - if private_field.is_none() { - private_field = Some(field_id); - } - return None; - } - // can't have `write_field_resolution` here because `self.table` is borrowed :( - self.result.field_resolutions.insert(tgt_expr, field_id); - let ty = self.db.field_types(field_id.parent)[field_id.local_id] - .clone() - .substitute(Interner, ¶meters); - Some(ty) - }); - let ty = match ty { - Some(ty) => { - let adjustments = auto_deref_adjust_steps(&autoderef); - self.write_expr_adj(*expr, adjustments); - let ty = self.insert_type_vars(ty); - let ty = self.normalize_associated_types_in(ty); - ty - } - _ => { - // Write down the first private field resolution if we found no field - // This aids IDE features for private fields like goto def - if let Some(field) = private_field { - self.result.field_resolutions.insert(tgt_expr, field); - self.result - .diagnostics - .push(InferenceDiagnostic::PrivateField { expr: tgt_expr, field }); - } - self.err_ty() - } - }; - ty - } + Expr::Field { expr, name } => self.infer_field_access(tgt_expr, *expr, name), Expr::Await { expr } => { let inner_ty = self.infer_expr_inner(*expr, &Expectation::none()); self.resolve_associated_type(inner_ty, self.resolve_future_future_output()) @@ -1276,6 +1212,92 @@ impl<'a> InferenceContext<'a> { } } + fn infer_field_access(&mut self, tgt_expr: ExprId, expr: ExprId, name: &Name) -> Ty { + let receiver_ty = self.infer_expr_inner(expr, &Expectation::none()); + + let mut autoderef = Autoderef::new(&mut self.table, receiver_ty.clone()); + let mut private_field = None; + let ty = autoderef.by_ref().find_map(|(derefed_ty, _)| { + let (field_id, parameters) = match derefed_ty.kind(Interner) { + TyKind::Tuple(_, substs) => { + return name.as_tuple_index().and_then(|idx| { + substs + .as_slice(Interner) + .get(idx) + .map(|a| a.assert_ty_ref(Interner)) + .cloned() + }); + } + TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), parameters) => { + let local_id = self.db.struct_data(*s).variant_data.field(name)?; + let field = FieldId { parent: (*s).into(), local_id }; + (field, parameters.clone()) + } + TyKind::Adt(AdtId(hir_def::AdtId::UnionId(u)), parameters) => { + let local_id = self.db.union_data(*u).variant_data.field(name)?; + let field = FieldId { parent: (*u).into(), local_id }; + (field, parameters.clone()) + } + _ => return None, + }; + let is_visible = self.db.field_visibilities(field_id.parent)[field_id.local_id] + .is_visible_from(self.db.upcast(), self.resolver.module()); + if !is_visible { + if private_field.is_none() { + private_field = Some(field_id); + } + return None; + } + // can't have `write_field_resolution` here because `self.table` is borrowed :( + self.result.field_resolutions.insert(tgt_expr, field_id); + let ty = self.db.field_types(field_id.parent)[field_id.local_id] + .clone() + .substitute(Interner, ¶meters); + Some(ty) + }); + let ty = match ty { + Some(ty) => { + let adjustments = auto_deref_adjust_steps(&autoderef); + self.write_expr_adj(expr, adjustments); + let ty = self.insert_type_vars(ty); + let ty = self.normalize_associated_types_in(ty); + ty + } + _ => { + // Write down the first private field resolution if we found no field + // This aids IDE features for private fields like goto def + if let Some(field) = private_field { + self.result.field_resolutions.insert(tgt_expr, field); + // FIXME: Merge this diagnostic into UnresolvedField + self.result + .diagnostics + .push(InferenceDiagnostic::PrivateField { expr: tgt_expr, field }); + } else { + // no field found, try looking for a method of the same name + let canonicalized_receiver = self.canonicalize(receiver_ty.clone()); + let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast()); + + let resolved = method_resolution::lookup_method( + self.db, + &canonicalized_receiver.value, + self.trait_env.clone(), + &traits_in_scope, + VisibleFromModule::Filter(self.resolver.module()), + name, + ); + self.result.diagnostics.push(InferenceDiagnostic::UnresolvedField { + expr: tgt_expr, + receiver: receiver_ty, + name: name.clone(), + method_with_same_name_exists: resolved.is_some(), + }); + } + self.err_ty() + } + }; + ty + } + fn infer_method_call( &mut self, tgt_expr: ExprId, diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 3a6f26a4ea08..58d02479e57c 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -48,6 +48,7 @@ diagnostics![ TypeMismatch, UnimplementedBuiltinMacro, UnresolvedExternCrate, + UnresolvedField, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, @@ -137,6 +138,14 @@ pub struct ExpectedFunction { pub found: Type, } +#[derive(Debug)] +pub struct UnresolvedField { + pub expr: InFile>, + pub receiver: Type, + pub name: Name, + pub method_with_same_name_exists: bool, +} + #[derive(Debug)] pub struct PrivateField { pub expr: InFile>, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 08ccf38b654d..bac6d4cd8540 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -88,8 +88,8 @@ pub use crate::{ InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount, MissingFields, MissingMatchArms, MissingUnsafe, NoSuchField, PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, UnimplementedBuiltinMacro, - UnresolvedExternCrate, UnresolvedImport, UnresolvedMacroCall, UnresolvedModule, - UnresolvedProcMacro, + UnresolvedExternCrate, UnresolvedField, UnresolvedImport, UnresolvedMacroCall, + UnresolvedModule, UnresolvedProcMacro, }, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits}, @@ -1375,6 +1375,7 @@ impl DefWithBody { let infer = db.infer(self.into()); let source_map = Lazy::new(|| db.body_with_source_map(self.into()).1); + let expr_syntax = |expr| source_map.expr_syntax(expr).expect("unexpected synthetic"); for d in &infer.diagnostics { match d { &hir_ty::InferenceDiagnostic::NoSuchField { expr } => { @@ -1386,30 +1387,23 @@ impl DefWithBody { is_break, bad_value_break, } => { - let expr = source_map - .expr_syntax(expr) - .expect("break outside of loop in synthetic syntax"); + let expr = expr_syntax(expr); acc.push(BreakOutsideOfLoop { expr, is_break, bad_value_break }.into()) } &hir_ty::InferenceDiagnostic::MismatchedArgCount { call_expr, expected, found } => { - match source_map.expr_syntax(call_expr) { - Ok(source_ptr) => acc.push( - MismatchedArgCount { call_expr: source_ptr, expected, found }.into(), - ), - Err(SyntheticSyntax) => (), - } + acc.push( + MismatchedArgCount { call_expr: expr_syntax(call_expr), expected, found } + .into(), + ) } &hir_ty::InferenceDiagnostic::PrivateField { expr, field } => { - let expr = source_map.expr_syntax(expr).expect("unexpected synthetic"); + let expr = expr_syntax(expr); let field = field.into(); acc.push(PrivateField { expr, field }.into()) } &hir_ty::InferenceDiagnostic::PrivateAssocItem { id, item } => { let expr_or_pat = match id { - ExprOrPatId::ExprId(expr) => source_map - .expr_syntax(expr) - .expect("unexpected synthetic") - .map(Either::Left), + ExprOrPatId::ExprId(expr) => expr_syntax(expr).map(Either::Left), ExprOrPatId::PatId(pat) => source_map .pat_syntax(pat) .expect("unexpected synthetic") @@ -1419,8 +1413,7 @@ impl DefWithBody { acc.push(PrivateAssocItem { expr_or_pat, item }.into()) } hir_ty::InferenceDiagnostic::ExpectedFunction { call_expr, found } => { - let call_expr = - source_map.expr_syntax(*call_expr).expect("unexpected synthetic"); + let call_expr = expr_syntax(*call_expr); acc.push( ExpectedFunction { @@ -1430,6 +1423,24 @@ impl DefWithBody { .into(), ) } + hir_ty::InferenceDiagnostic::UnresolvedField { + expr, + receiver, + name, + method_with_same_name_exists, + } => { + let expr = expr_syntax(*expr); + + acc.push( + UnresolvedField { + expr, + name: name.clone(), + receiver: Type::new(db, DefWithBodyId::from(self), receiver.clone()), + method_with_same_name_exists: *method_with_same_name_exists, + } + .into(), + ) + } } } for (pat_or_expr, mismatch) in infer.type_mismatches() { diff --git a/crates/ide-completion/src/completions/flyimport.rs b/crates/ide-completion/src/completions/flyimport.rs index 364969af9c9a..0979f6a6dfc7 100644 --- a/crates/ide-completion/src/completions/flyimport.rs +++ b/crates/ide-completion/src/completions/flyimport.rs @@ -5,10 +5,7 @@ use ide_db::imports::{ insert_use::ImportScope, }; use itertools::Itertools; -use syntax::{ - ast::{self}, - AstNode, SyntaxNode, T, -}; +use syntax::{ast, AstNode, SyntaxNode, T}; use crate::{ context::{ diff --git a/crates/ide-diagnostics/src/handlers/unresolved_field.rs b/crates/ide-diagnostics/src/handlers/unresolved_field.rs new file mode 100644 index 000000000000..33c39de085d8 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/unresolved_field.rs @@ -0,0 +1,134 @@ +use hir::{db::AstDatabase, HirDisplay, InFile}; +use ide_db::{ + assists::{Assist, AssistId, AssistKind}, + base_db::FileRange, + label::Label, + source_change::SourceChange, +}; +use syntax::{ast, AstNode, AstPtr}; +use text_edit::TextEdit; + +use crate::{Diagnostic, DiagnosticsContext}; + +// Diagnostic: unresolved-field +// +// This diagnostic is triggered if a field does not exist on a given type. +pub(crate) fn unresolved_field( + ctx: &DiagnosticsContext<'_>, + d: &hir::UnresolvedField, +) -> Diagnostic { + let method_suffix = if d.method_with_same_name_exists { + ", but a method with a similar name exists" + } else { + "" + }; + Diagnostic::new( + "unresolved-field", + format!( + "no field `{}` on type `{}`{method_suffix}", + d.name, + d.receiver.display(ctx.sema.db) + ), + ctx.sema.diagnostics_display_range(d.expr.clone().map(|it| it.into())).range, + ) + .with_fixes(fixes(ctx, d)) +} + +fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::UnresolvedField) -> Option> { + if d.method_with_same_name_exists { + method_fix(ctx, &d.expr) + } else { + // FIXME: add quickfix + + None + } +} + +// FIXME: We should fill out the call here, mvoe the cursor and trigger signature help +fn method_fix( + ctx: &DiagnosticsContext<'_>, + expr_ptr: &InFile>, +) -> Option> { + let root = ctx.sema.db.parse_or_expand(expr_ptr.file_id)?; + let expr = expr_ptr.value.to_node(&root); + let FileRange { range, file_id } = ctx.sema.original_range_opt(expr.syntax())?; + Some(vec![Assist { + id: AssistId("expected-field-found-method-call-fix", AssistKind::QuickFix), + label: Label::new("Use parentheses to call the method".to_string()), + group: None, + target: range, + source_change: Some(SourceChange::from_text_edit( + file_id, + TextEdit::insert(range.end(), "()".to_owned()), + )), + trigger_signature_help: false, + }]) +} +#[cfg(test)] +mod tests { + use crate::tests::check_diagnostics; + + #[test] + fn smoke_test() { + check_diagnostics( + r#" +fn main() { + ().foo; + // ^^^^^^ error: no field `foo` on type `()` +} +"#, + ); + } + + #[test] + fn method_clash() { + check_diagnostics( + r#" +struct Foo; +impl Foo { + fn bar(&self) {} +} +fn foo() { + Foo.bar; + // ^^^^^^^ 💡 error: no field `bar` on type `Foo`, but a method with a similar name exists +} +"#, + ); + } + + #[test] + fn method_trait_() { + check_diagnostics( + r#" +struct Foo; +trait Bar { + fn bar(&self) {} +} +impl Bar for Foo {} +fn foo() { + Foo.bar; + // ^^^^^^^ 💡 error: no field `bar` on type `Foo`, but a method with a similar name exists +} +"#, + ); + } + + #[test] + fn method_trait_2() { + check_diagnostics( + r#" +struct Foo; +trait Bar { + fn bar(&self); +} +impl Bar for Foo { + fn bar(&self) {} +} +fn foo() { + Foo.bar; + // ^^^^^^^ 💡 error: no field `bar` on type `Foo`, but a method with a similar name exists +} +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index b878119fee1f..a0e8bca5cb90 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -44,6 +44,7 @@ mod handlers { pub(crate) mod type_mismatch; pub(crate) mod unimplemented_builtin_macro; pub(crate) mod unresolved_extern_crate; + pub(crate) mod unresolved_field; pub(crate) mod unresolved_import; pub(crate) mod unresolved_macro_call; pub(crate) mod unresolved_module; @@ -269,6 +270,7 @@ pub fn diagnostics( AnyDiagnostic::UnresolvedModule(d) => handlers::unresolved_module::unresolved_module(&ctx, &d), AnyDiagnostic::UnresolvedProcMacro(d) => handlers::unresolved_proc_macro::unresolved_proc_macro(&ctx, &d, config.proc_macros_enabled, config.proc_attr_macros_enabled), AnyDiagnostic::InvalidDeriveTarget(d) => handlers::invalid_derive_target::invalid_derive_target(&ctx, &d), + AnyDiagnostic::UnresolvedField(d) => handlers::unresolved_field::unresolved_field(&ctx, &d), AnyDiagnostic::InactiveCode(d) => match handlers::inactive_code::inactive_code(&ctx, &d) { Some(it) => it, From e7485a04169f79c63333f2b0d39e159d10b92cee Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 3 Mar 2023 20:41:17 +0100 Subject: [PATCH 3/3] Diagnose unresolved method calls --- crates/hir-ty/src/infer.rs | 63 +++++++-- crates/hir-ty/src/infer/expr.rs | 121 +++++++++++----- crates/hir/src/diagnostics.rs | 9 ++ crates/hir/src/lib.rs | 22 ++- crates/ide-db/src/source_change.rs | 8 ++ .../replace_filter_map_next_with_find_map.rs | 13 +- .../src/handlers/unresolved_method.rs | 130 ++++++++++++++++++ crates/ide-diagnostics/src/lib.rs | 2 + 8 files changed, 320 insertions(+), 48 deletions(-) create mode 100644 crates/ide-diagnostics/src/handlers/unresolved_method.rs diff --git a/crates/hir-ty/src/infer.rs b/crates/hir-ty/src/infer.rs index a663a568b919..22dcea8fcd45 100644 --- a/crates/hir-ty/src/infer.rs +++ b/crates/hir-ty/src/infer.rs @@ -164,14 +164,45 @@ pub(crate) type InferResult = Result, TypeError>; #[derive(Debug, PartialEq, Eq, Clone)] pub enum InferenceDiagnostic { - NoSuchField { expr: ExprId }, - PrivateField { expr: ExprId, field: FieldId }, - PrivateAssocItem { id: ExprOrPatId, item: AssocItemId }, - UnresolvedField { expr: ExprId, receiver: Ty, name: Name, method_with_same_name_exists: bool }, + NoSuchField { + expr: ExprId, + }, + PrivateField { + expr: ExprId, + field: FieldId, + }, + PrivateAssocItem { + id: ExprOrPatId, + item: AssocItemId, + }, + UnresolvedField { + expr: ExprId, + receiver: Ty, + name: Name, + method_with_same_name_exists: bool, + }, + UnresolvedMethodCall { + expr: ExprId, + receiver: Ty, + name: Name, + /// Contains the type the field resolves to + field_with_same_name: Option, + }, // FIXME: Make this proper - BreakOutsideOfLoop { expr: ExprId, is_break: bool, bad_value_break: bool }, - MismatchedArgCount { call_expr: ExprId, expected: usize, found: usize }, - ExpectedFunction { call_expr: ExprId, found: Ty }, + BreakOutsideOfLoop { + expr: ExprId, + is_break: bool, + bad_value_break: bool, + }, + MismatchedArgCount { + call_expr: ExprId, + expected: usize, + found: usize, + }, + ExpectedFunction { + call_expr: ExprId, + found: Ty, + }, } /// A mismatch between an expected and an inferred type. @@ -509,12 +540,28 @@ impl<'a> InferenceContext<'a> { } result.diagnostics.retain_mut(|diagnostic| { if let InferenceDiagnostic::ExpectedFunction { found: ty, .. } - | InferenceDiagnostic::UnresolvedField { receiver: ty, .. } = diagnostic + | InferenceDiagnostic::UnresolvedField { receiver: ty, .. } + | InferenceDiagnostic::UnresolvedMethodCall { receiver: ty, .. } = diagnostic { *ty = table.resolve_completely(ty.clone()); + // FIXME: Remove this when we are on par with rustc in terms of inference if ty.is_unknown() { return false; } + + if let InferenceDiagnostic::UnresolvedMethodCall { field_with_same_name, .. } = + diagnostic + { + let clear = if let Some(ty) = field_with_same_name { + *ty = table.resolve_completely(ty.clone()); + ty.is_unknown() + } else { + false + }; + if clear { + *field_with_same_name = None; + } + } } true }); diff --git a/crates/hir-ty/src/infer/expr.rs b/crates/hir-ty/src/infer/expr.rs index 531a359a96a4..02024e1ea780 100644 --- a/crates/hir-ty/src/infer/expr.rs +++ b/crates/hir-ty/src/infer/expr.rs @@ -1212,12 +1212,14 @@ impl<'a> InferenceContext<'a> { } } - fn infer_field_access(&mut self, tgt_expr: ExprId, expr: ExprId, name: &Name) -> Ty { - let receiver_ty = self.infer_expr_inner(expr, &Expectation::none()); - + fn lookup_field( + &mut self, + receiver_ty: &Ty, + name: &Name, + ) -> Option<(Ty, Option, Vec, bool)> { let mut autoderef = Autoderef::new(&mut self.table, receiver_ty.clone()); let mut private_field = None; - let ty = autoderef.by_ref().find_map(|(derefed_ty, _)| { + let res = autoderef.by_ref().find_map(|(derefed_ty, _)| { let (field_id, parameters) = match derefed_ty.kind(Interner) { TyKind::Tuple(_, substs) => { return name.as_tuple_index().and_then(|idx| { @@ -1226,6 +1228,7 @@ impl<'a> InferenceContext<'a> { .get(idx) .map(|a| a.assert_ty_ref(Interner)) .cloned() + .map(|ty| (None, ty)) }); } TyKind::Adt(AdtId(hir_def::AdtId::StructId(s)), parameters) => { @@ -1244,58 +1247,81 @@ impl<'a> InferenceContext<'a> { .is_visible_from(self.db.upcast(), self.resolver.module()); if !is_visible { if private_field.is_none() { - private_field = Some(field_id); + private_field = Some((field_id, parameters)); } return None; } - // can't have `write_field_resolution` here because `self.table` is borrowed :( - self.result.field_resolutions.insert(tgt_expr, field_id); let ty = self.db.field_types(field_id.parent)[field_id.local_id] .clone() .substitute(Interner, ¶meters); - Some(ty) + Some((Some(field_id), ty)) }); - let ty = match ty { - Some(ty) => { + + Some(match res { + Some((field_id, ty)) => { let adjustments = auto_deref_adjust_steps(&autoderef); - self.write_expr_adj(expr, adjustments); let ty = self.insert_type_vars(ty); let ty = self.normalize_associated_types_in(ty); + + (ty, field_id, adjustments, true) + } + None => { + let (field_id, subst) = private_field?; + let adjustments = auto_deref_adjust_steps(&autoderef); + let ty = self.db.field_types(field_id.parent)[field_id.local_id] + .clone() + .substitute(Interner, &subst); + let ty = self.insert_type_vars(ty); + let ty = self.normalize_associated_types_in(ty); + + (ty, Some(field_id), adjustments, false) + } + }) + } + + fn infer_field_access(&mut self, tgt_expr: ExprId, receiver: ExprId, name: &Name) -> Ty { + let receiver_ty = self.infer_expr_inner(receiver, &Expectation::none()); + match self.lookup_field(&receiver_ty, name) { + Some((ty, field_id, adjustments, is_public)) => { + self.write_expr_adj(receiver, adjustments); + if let Some(field_id) = field_id { + self.result.field_resolutions.insert(tgt_expr, field_id); + } + if !is_public { + if let Some(field) = field_id { + // FIXME: Merge this diagnostic into UnresolvedField? + self.result + .diagnostics + .push(InferenceDiagnostic::PrivateField { expr: tgt_expr, field }); + } + } ty } - _ => { - // Write down the first private field resolution if we found no field - // This aids IDE features for private fields like goto def - if let Some(field) = private_field { - self.result.field_resolutions.insert(tgt_expr, field); - // FIXME: Merge this diagnostic into UnresolvedField - self.result - .diagnostics - .push(InferenceDiagnostic::PrivateField { expr: tgt_expr, field }); - } else { - // no field found, try looking for a method of the same name + None => { + // no field found, + let method_with_same_name_exists = { let canonicalized_receiver = self.canonicalize(receiver_ty.clone()); let traits_in_scope = self.resolver.traits_in_scope(self.db.upcast()); - let resolved = method_resolution::lookup_method( + method_resolution::lookup_method( self.db, &canonicalized_receiver.value, self.trait_env.clone(), &traits_in_scope, VisibleFromModule::Filter(self.resolver.module()), name, - ); - self.result.diagnostics.push(InferenceDiagnostic::UnresolvedField { - expr: tgt_expr, - receiver: receiver_ty, - name: name.clone(), - method_with_same_name_exists: resolved.is_some(), - }); - } + ) + .is_some() + }; + self.result.diagnostics.push(InferenceDiagnostic::UnresolvedField { + expr: tgt_expr, + receiver: receiver_ty, + name: name.clone(), + method_with_same_name_exists, + }); self.err_ty() } - }; - ty + } } fn infer_method_call( @@ -1335,11 +1361,30 @@ impl<'a> InferenceContext<'a> { } (ty, self.db.value_ty(func.into()), substs) } - None => ( - receiver_ty, - Binders::empty(Interner, self.err_ty()), - Substitution::empty(Interner), - ), + None => { + let field_with_same_name_exists = match self.lookup_field(&receiver_ty, method_name) + { + Some((ty, field_id, adjustments, _public)) => { + self.write_expr_adj(receiver, adjustments); + if let Some(field_id) = field_id { + self.result.field_resolutions.insert(tgt_expr, field_id); + } + Some(ty) + } + None => None, + }; + self.result.diagnostics.push(InferenceDiagnostic::UnresolvedMethodCall { + expr: tgt_expr, + receiver: receiver_ty.clone(), + name: method_name.clone(), + field_with_same_name: field_with_same_name_exists, + }); + ( + receiver_ty, + Binders::empty(Interner, self.err_ty()), + Substitution::empty(Interner), + ) + } }; let method_ty = method_ty.substitute(Interner, &substs); self.register_obligations_for_call(&method_ty); diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 58d02479e57c..b30c664e24f1 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -51,6 +51,7 @@ diagnostics![ UnresolvedField, UnresolvedImport, UnresolvedMacroCall, + UnresolvedMethodCall, UnresolvedModule, UnresolvedProcMacro, ]; @@ -146,6 +147,14 @@ pub struct UnresolvedField { pub method_with_same_name_exists: bool, } +#[derive(Debug)] +pub struct UnresolvedMethodCall { + pub expr: InFile>, + pub receiver: Type, + pub name: Name, + pub field_with_same_name: Option, +} + #[derive(Debug)] pub struct PrivateField { pub expr: InFile>, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index bac6d4cd8540..269c45943e51 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -89,7 +89,7 @@ pub use crate::{ MissingMatchArms, MissingUnsafe, NoSuchField, PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, UnimplementedBuiltinMacro, UnresolvedExternCrate, UnresolvedField, UnresolvedImport, UnresolvedMacroCall, - UnresolvedModule, UnresolvedProcMacro, + UnresolvedMethodCall, UnresolvedModule, UnresolvedProcMacro, }, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits}, @@ -1441,6 +1441,26 @@ impl DefWithBody { .into(), ) } + hir_ty::InferenceDiagnostic::UnresolvedMethodCall { + expr, + receiver, + name, + field_with_same_name, + } => { + let expr = expr_syntax(*expr); + + acc.push( + UnresolvedMethodCall { + expr, + name: name.clone(), + receiver: Type::new(db, DefWithBodyId::from(self), receiver.clone()), + field_with_same_name: field_with_same_name + .clone() + .map(|ty| Type::new(db, DefWithBodyId::from(self), ty)), + } + .into(), + ) + } } } for (pat_or_expr, mismatch) in infer.type_mismatches() { diff --git a/crates/ide-db/src/source_change.rs b/crates/ide-db/src/source_change.rs index 8e338061df43..936354f29613 100644 --- a/crates/ide-db/src/source_change.rs +++ b/crates/ide-db/src/source_change.rs @@ -83,6 +83,14 @@ impl From> for SourceChange { } } +impl FromIterator<(FileId, TextEdit)> for SourceChange { + fn from_iter>(iter: T) -> Self { + let mut this = SourceChange::default(); + this.extend(iter); + this + } +} + pub struct SourceChangeBuilder { pub edit: TextEditBuilder, pub file_id: FileId, diff --git a/crates/ide-diagnostics/src/handlers/replace_filter_map_next_with_find_map.rs b/crates/ide-diagnostics/src/handlers/replace_filter_map_next_with_find_map.rs index 9826e1c707ea..a0c276cc3328 100644 --- a/crates/ide-diagnostics/src/handlers/replace_filter_map_next_with_find_map.rs +++ b/crates/ide-diagnostics/src/handlers/replace_filter_map_next_with_find_map.rs @@ -55,7 +55,18 @@ fn fixes( #[cfg(test)] mod tests { - use crate::tests::{check_diagnostics, check_fix}; + use crate::{ + tests::{check_diagnostics_with_config, check_fix}, + DiagnosticsConfig, + }; + + #[track_caller] + pub(crate) fn check_diagnostics(ra_fixture: &str) { + let mut config = DiagnosticsConfig::test_sample(); + config.disabled.insert("inactive-code".to_string()); + config.disabled.insert("unresolved-method".to_string()); + check_diagnostics_with_config(config, ra_fixture) + } #[test] fn replace_filter_map_next_with_find_map2() { diff --git a/crates/ide-diagnostics/src/handlers/unresolved_method.rs b/crates/ide-diagnostics/src/handlers/unresolved_method.rs new file mode 100644 index 000000000000..0d1f91f02c3b --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/unresolved_method.rs @@ -0,0 +1,130 @@ +use hir::{db::AstDatabase, HirDisplay}; +use ide_db::{ + assists::{Assist, AssistId, AssistKind}, + base_db::FileRange, + label::Label, + source_change::SourceChange, +}; +use syntax::{ast, AstNode, TextRange}; +use text_edit::TextEdit; + +use crate::{Diagnostic, DiagnosticsContext}; + +// Diagnostic: unresolved-method +// +// This diagnostic is triggered if a method does not exist on a given type. +pub(crate) fn unresolved_method( + ctx: &DiagnosticsContext<'_>, + d: &hir::UnresolvedMethodCall, +) -> Diagnostic { + let field_suffix = if d.field_with_same_name.is_some() { + ", but a field with a similar name exists" + } else { + "" + }; + Diagnostic::new( + "unresolved-method", + format!( + "no method `{}` on type `{}`{field_suffix}", + d.name, + d.receiver.display(ctx.sema.db) + ), + ctx.sema.diagnostics_display_range(d.expr.clone().map(|it| it.into())).range, + ) + .with_fixes(fixes(ctx, d)) +} + +fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::UnresolvedMethodCall) -> Option> { + if let Some(ty) = &d.field_with_same_name { + field_fix(ctx, d, ty) + } else { + // FIXME: add quickfix + None + } +} + +fn field_fix( + ctx: &DiagnosticsContext<'_>, + d: &hir::UnresolvedMethodCall, + ty: &hir::Type, +) -> Option> { + if !ty.impls_fnonce(ctx.sema.db) { + return None; + } + let expr_ptr = &d.expr; + let root = ctx.sema.db.parse_or_expand(expr_ptr.file_id)?; + let expr = expr_ptr.value.to_node(&root); + let (file_id, range) = match expr { + ast::Expr::MethodCallExpr(mcall) => { + let FileRange { range, file_id } = + ctx.sema.original_range_opt(mcall.receiver()?.syntax())?; + let FileRange { range: range2, file_id: file_id2 } = + ctx.sema.original_range_opt(mcall.name_ref()?.syntax())?; + if file_id != file_id2 { + return None; + } + (file_id, TextRange::new(range.start(), range2.end())) + } + _ => return None, + }; + Some(vec![Assist { + id: AssistId("expected-method-found-field-fix", AssistKind::QuickFix), + label: Label::new("Use parentheses to call the value of the field".to_string()), + group: None, + target: range, + source_change: Some(SourceChange::from_iter([ + (file_id, TextEdit::insert(range.start(), "(".to_owned())), + (file_id, TextEdit::insert(range.end(), ")".to_owned())), + ])), + trigger_signature_help: false, + }]) +} + +#[cfg(test)] +mod tests { + use crate::tests::{check_diagnostics, check_fix}; + + #[test] + fn smoke_test() { + check_diagnostics( + r#" +fn main() { + ().foo(); + // ^^^^^^^^ error: no method `foo` on type `()` +} +"#, + ); + } + + #[test] + fn field() { + check_diagnostics( + r#" +struct Foo { bar: i32 } +fn foo() { + Foo { bar: i32 }.bar(); + // ^^^^^^^^^^^^^^^^^^^^^^ error: no method `bar` on type `Foo`, but a field with a similar name exists +} +"#, + ); + } + + #[test] + fn callable_field() { + check_fix( + r#" +//- minicore: fn +struct Foo { bar: fn() } +fn foo() { + Foo { bar: foo }.b$0ar(); +} +"#, + r#" +struct Foo { bar: fn() } +fn foo() { + (Foo { bar: foo }.bar)(); +} +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index a0e8bca5cb90..c8635ff80110 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -45,6 +45,7 @@ mod handlers { pub(crate) mod unimplemented_builtin_macro; pub(crate) mod unresolved_extern_crate; pub(crate) mod unresolved_field; + pub(crate) mod unresolved_method; pub(crate) mod unresolved_import; pub(crate) mod unresolved_macro_call; pub(crate) mod unresolved_module; @@ -271,6 +272,7 @@ pub fn diagnostics( AnyDiagnostic::UnresolvedProcMacro(d) => handlers::unresolved_proc_macro::unresolved_proc_macro(&ctx, &d, config.proc_macros_enabled, config.proc_attr_macros_enabled), AnyDiagnostic::InvalidDeriveTarget(d) => handlers::invalid_derive_target::invalid_derive_target(&ctx, &d), AnyDiagnostic::UnresolvedField(d) => handlers::unresolved_field::unresolved_field(&ctx, &d), + AnyDiagnostic::UnresolvedMethodCall(d) => handlers::unresolved_method::unresolved_method(&ctx, &d), AnyDiagnostic::InactiveCode(d) => match handlers::inactive_code::inactive_code(&ctx, &d) { Some(it) => it,