From 63ce2c7b5fd96e6688796f2ddd1cd7316df8d11d Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 8 Jul 2020 19:58:45 +0200 Subject: [PATCH 1/7] Add argument count mismatch diagnostic --- crates/ra_hir/src/diagnostics.rs | 4 +- crates/ra_hir_ty/src/diagnostics.rs | 29 ++++++++++++ crates/ra_hir_ty/src/expr.rs | 68 ++++++++++++++++++++++++++--- crates/ra_ide/src/diagnostics.rs | 8 ++++ 4 files changed, 103 insertions(+), 6 deletions(-) diff --git a/crates/ra_hir/src/diagnostics.rs b/crates/ra_hir/src/diagnostics.rs index c82883d0c1..11a0ecb8b2 100644 --- a/crates/ra_hir/src/diagnostics.rs +++ b/crates/ra_hir/src/diagnostics.rs @@ -1,4 +1,6 @@ //! FIXME: write short doc here pub use hir_def::diagnostics::UnresolvedModule; pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink}; -pub use hir_ty::diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, NoSuchField}; +pub use hir_ty::diagnostics::{ + MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkInTailExpr, NoSuchField, +}; diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index 0289911de0..daac669e65 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -197,3 +197,32 @@ impl AstDiagnostic for MissingUnsafe { ast::Expr::cast(node).unwrap() } } + +#[derive(Debug)] +pub struct MismatchedArgCount { + pub file: HirFileId, + pub call_expr: AstPtr, + pub expected: usize, + pub found: usize, +} + +impl Diagnostic for MismatchedArgCount { + fn message(&self) -> String { + format!("Expected {} arguments, found {}", self.expected, self.found) + } + fn source(&self) -> InFile { + InFile { file_id: self.file, value: self.call_expr.clone().into() } + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + +impl AstDiagnostic for MismatchedArgCount { + type AST = ast::CallExpr; + fn ast(&self, db: &dyn AstDatabase) -> Self::AST { + let root = db.parse_or_expand(self.source().file_id).unwrap(); + let node = self.source().value.to_node(&root); + ast::CallExpr::cast(node).unwrap() + } +} diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 7db928dded..7c3cd7952c 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -9,9 +9,11 @@ use rustc_hash::FxHashSet; use crate::{ db::HirDatabase, - diagnostics::{MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields}, + diagnostics::{ + MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkInTailExpr, MissingPatFields, + }, utils::variant_data, - ApplicationTy, InferenceResult, Ty, TypeCtor, + ApplicationTy, CallableDef, InferenceResult, Ty, TypeCtor, _match::{is_useful, MatchCheckCtx, Matrix, PatStack, Usefulness}, }; @@ -24,7 +26,8 @@ pub use hir_def::{ ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, UnaryOp, }, - LocalFieldId, VariantId, + src::HasSource, + LocalFieldId, Lookup, VariantId, }; pub struct ExprValidator<'a, 'b: 'a> { @@ -56,8 +59,15 @@ impl<'a, 'b> ExprValidator<'a, 'b> { missed_fields, ); } - if let Expr::Match { expr, arms } = expr { - self.validate_match(id, *expr, arms, db, self.infer.clone()); + + match expr { + Expr::Match { expr, arms } => { + self.validate_match(id, *expr, arms, db, self.infer.clone()); + } + Expr::Call { .. } | Expr::MethodCall { .. } => { + self.validate_call(db, id, expr); + } + _ => {} } } for (id, pat) in body.pats.iter() { @@ -138,6 +148,54 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } } + fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) -> Option<()> { + // Check that the number of arguments matches the number of parameters. + let (callee, args) = match expr { + Expr::Call { callee, args } => { + let callee = &self.infer.type_of_expr[*callee]; + let (callable, _) = callee.as_callable()?; + let callee = match callable { + CallableDef::FunctionId(func) => func, + _ => return None, + }; + + (callee, args.clone()) + } + Expr::MethodCall { receiver, args, .. } => { + let callee = self.infer.method_resolution(call_id)?; + let mut args = args.clone(); + args.insert(0, *receiver); + (callee, args) + } + _ => return None, + }; + + let loc = callee.lookup(db.upcast()); + let ast = loc.source(db.upcast()); + let params = ast.value.param_list()?; + + let mut param_count = params.params().count(); + if params.self_param().is_some() { + param_count += 1; + } + let arg_count = args.len(); + + if arg_count != param_count { + let (_, source_map): (Arc, Arc) = + db.body_with_source_map(self.func.into()); + if let Ok(source_ptr) = source_map.expr_syntax(call_id) { + self.sink.push(MismatchedArgCount { + file: source_ptr.file_id, + call_expr: source_ptr.value, + expected: param_count, + found: arg_count, + }); + } + } + + None + } + fn validate_match( &mut self, id: ExprId, diff --git a/crates/ra_ide/src/diagnostics.rs b/crates/ra_ide/src/diagnostics.rs index 46f8c31c7a..d984f58ba2 100644 --- a/crates/ra_ide/src/diagnostics.rs +++ b/crates/ra_ide/src/diagnostics.rs @@ -127,6 +127,14 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec severity: Severity::Error, fix: missing_struct_field_fix(&sema, file_id, d), }) + }) + .on::(|d| { + res.borrow_mut().push(Diagnostic { + range: sema.diagnostics_range(d).range, + message: d.message(), + severity: Severity::Error, + fix: None, + }) }); if let Some(m) = sema.to_module_def(file_id) { From 47d0cf201c3b88675ab940369b889a43b9197d6b Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 12:41:35 +0200 Subject: [PATCH 2/7] Don't emit diagnostic if there are type errors --- crates/ra_hir_ty/src/expr.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 7c3cd7952c..f363046691 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -150,6 +150,13 @@ impl<'a, 'b> ExprValidator<'a, 'b> { fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) -> Option<()> { // Check that the number of arguments matches the number of parameters. + + // Due to shortcomings in the current type system implementation, only emit this diagnostic + // if there are no type mismatches in the containing function. + if self.infer.type_mismatches.iter().next().is_some() { + return Some(()); + } + let (callee, args) = match expr { Expr::Call { callee, args } => { let callee = &self.infer.type_of_expr[*callee]; From 73327c647dfdc36fb550967271d12db16978c5d6 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 15:50:35 +0200 Subject: [PATCH 3/7] Remove unnecessary DiagnosticSink handlers --- crates/ra_ide/src/diagnostics.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/ra_ide/src/diagnostics.rs b/crates/ra_ide/src/diagnostics.rs index d984f58ba2..f4bc0d6198 100644 --- a/crates/ra_ide/src/diagnostics.rs +++ b/crates/ra_ide/src/diagnostics.rs @@ -99,14 +99,6 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec fix, }) }) - .on::(|d| { - res.borrow_mut().push(Diagnostic { - range: sema.diagnostics_range(d).range, - message: d.message(), - severity: Severity::Error, - fix: None, - }) - }) .on::(|d| { let node = d.ast(db); let replacement = format!("Ok({})", node.syntax()); @@ -127,14 +119,6 @@ pub(crate) fn diagnostics(db: &RootDatabase, file_id: FileId) -> Vec severity: Severity::Error, fix: missing_struct_field_fix(&sema, file_id, d), }) - }) - .on::(|d| { - res.borrow_mut().push(Diagnostic { - range: sema.diagnostics_range(d).range, - message: d.message(), - severity: Severity::Error, - fix: None, - }) }); if let Some(m) = sema.to_module_def(file_id) { From d04f3604d5b054674720276900ebc35564e2df96 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 15:50:53 +0200 Subject: [PATCH 4/7] Correctly pluralize message --- crates/ra_hir_ty/src/diagnostics.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs index daac669e65..5b0dda634f 100644 --- a/crates/ra_hir_ty/src/diagnostics.rs +++ b/crates/ra_hir_ty/src/diagnostics.rs @@ -208,7 +208,8 @@ pub struct MismatchedArgCount { impl Diagnostic for MismatchedArgCount { fn message(&self) -> String { - format!("Expected {} arguments, found {}", self.expected, self.found) + let s = if self.expected == 1 { "" } else { "s" }; + format!("Expected {} argument{}, found {}", self.expected, s, self.found) } fn source(&self) -> InFile { InFile { file_id: self.file, value: self.call_expr.clone().into() } From 3ce4407dcb6ca358e647b961a5bd1ad7182f59cd Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 15:51:32 +0200 Subject: [PATCH 5/7] Fix diagnostic for method calls --- crates/ra_hir_ty/src/expr.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index f363046691..53828d29d9 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -157,18 +157,23 @@ impl<'a, 'b> ExprValidator<'a, 'b> { return Some(()); } + let is_method_call; let (callee, args) = match expr { Expr::Call { callee, args } => { + is_method_call = false; let callee = &self.infer.type_of_expr[*callee]; let (callable, _) = callee.as_callable()?; let callee = match callable { CallableDef::FunctionId(func) => func, + + // FIXME: Handle tuple struct/variant constructor calls. _ => return None, }; (callee, args.clone()) } Expr::MethodCall { receiver, args, .. } => { + is_method_call = true; let callee = self.infer.method_resolution(call_id)?; let mut args = args.clone(); args.insert(0, *receiver); @@ -182,15 +187,19 @@ impl<'a, 'b> ExprValidator<'a, 'b> { let params = ast.value.param_list()?; let mut param_count = params.params().count(); + let mut arg_count = args.len(); + if params.self_param().is_some() { param_count += 1; } - let arg_count = args.len(); if arg_count != param_count { - let (_, source_map): (Arc, Arc) = - db.body_with_source_map(self.func.into()); + let (_, source_map) = db.body_with_source_map(self.func.into()); if let Ok(source_ptr) = source_map.expr_syntax(call_id) { + if is_method_call { + param_count -= 1; + arg_count -= 1; + } self.sink.push(MismatchedArgCount { file: source_ptr.file_id, call_expr: source_ptr.value, From 984b6889ebeb0cb2e29fb893f5168a9b6de2842f Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 15:52:10 +0200 Subject: [PATCH 6/7] Add tests --- crates/ra_hir_ty/src/expr.rs | 129 +++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index 53828d29d9..e9f7853835 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -386,3 +386,132 @@ pub fn record_pattern_missing_fields( } Some((variant_def, missed_fields, exhaustive)) } + +#[cfg(test)] +mod tests { + use insta::assert_snapshot; + use ra_db::fixture::WithFixture; + + use crate::{diagnostics::MismatchedArgCount, test_db::TestDB}; + + fn check_diagnostic_message(ra_fixture: &str) -> String { + TestDB::with_single_file(ra_fixture).0.diagnostic::().0 + } + + fn check_no_diagnostic(ra_fixture: &str) { + let (s, diagnostic_count) = + TestDB::with_single_file(ra_fixture).0.diagnostic::(); + + assert_eq!(0, diagnostic_count, "expected no diagnostic, found one: {}", s); + } + + #[test] + fn simple_free_fn_zero() { + assert_snapshot!(check_diagnostic_message( + r" + fn zero() {} + + fn f() { + zero(1); + } + " + ), + @"\"zero(1)\": Expected 0 arguments, found 1\n"); + + check_no_diagnostic( + r" + fn zero() {} + + fn f() { + zero(); + } + ", + ); + } + + #[test] + fn simple_free_fn_one() { + assert_snapshot!(check_diagnostic_message( + r" + fn one(arg: u8) {} + + fn f() { + one(); + } + " + ), + @"\"one()\": Expected 1 argument, found 0\n"); + + check_no_diagnostic( + r" + fn one(arg: u8) {} + + fn f() { + one(1); + } + ", + ); + } + + #[test] + fn method_as_fn() { + assert_snapshot!(check_diagnostic_message( + r" + struct S; + impl S { + fn method(&self) {} + } + + fn f() { + S::method(); + } + " + ), + @"\"S::method()\": Expected 1 argument, found 0\n"); + + check_no_diagnostic( + r" + struct S; + impl S { + fn method(&self) {} + } + + fn f() { + S::method(&S); + S.method(); + } + ", + ); + } + + #[test] + fn method_with_arg() { + assert_snapshot!(check_diagnostic_message( + r" + struct S; + impl S { + fn method(&self, arg: u8) {} + } + + fn f() { + S.method(); + } + " + ), + @"\"S.method()\": Expected 1 argument, found 0\n"); + + check_no_diagnostic( + r" + struct S; + impl S { + fn method(&self, arg: u8) {} + } + + fn f() { + S::method(&S, 0); + S.method(1); + } + ", + ); + } +} From f4a9d9a00f0462bce92ddbac24cb91825c8ab192 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 9 Jul 2020 17:33:49 +0200 Subject: [PATCH 7/7] Address review comments --- Cargo.lock | 1 + crates/ra_hir_ty/Cargo.toml | 1 + crates/ra_hir_ty/src/expr.rs | 67 +++++++++++++++--------------------- 3 files changed, 29 insertions(+), 40 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 79a793084b..510c80b904 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1104,6 +1104,7 @@ dependencies = [ "chalk-ir", "chalk-solve", "ena", + "expect", "insta", "itertools", "log", diff --git a/crates/ra_hir_ty/Cargo.toml b/crates/ra_hir_ty/Cargo.toml index d6df48db20..ce257dc0bb 100644 --- a/crates/ra_hir_ty/Cargo.toml +++ b/crates/ra_hir_ty/Cargo.toml @@ -32,3 +32,4 @@ chalk-ir = { version = "0.15.0" } [dev-dependencies] insta = "0.16.0" +expect = { path = "../expect" } diff --git a/crates/ra_hir_ty/src/expr.rs b/crates/ra_hir_ty/src/expr.rs index e9f7853835..6f34aaf170 100644 --- a/crates/ra_hir_ty/src/expr.rs +++ b/crates/ra_hir_ty/src/expr.rs @@ -151,16 +151,15 @@ impl<'a, 'b> ExprValidator<'a, 'b> { fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) -> Option<()> { // Check that the number of arguments matches the number of parameters. - // Due to shortcomings in the current type system implementation, only emit this diagnostic - // if there are no type mismatches in the containing function. + // FIXME: Due to shortcomings in the current type system implementation, only emit this + // diagnostic if there are no type mismatches in the containing function. if self.infer.type_mismatches.iter().next().is_some() { return Some(()); } - let is_method_call; + let is_method_call = matches!(expr, Expr::MethodCall { .. }); let (callee, args) = match expr { Expr::Call { callee, args } => { - is_method_call = false; let callee = &self.infer.type_of_expr[*callee]; let (callable, _) = callee.as_callable()?; let callee = match callable { @@ -173,7 +172,6 @@ impl<'a, 'b> ExprValidator<'a, 'b> { (callee, args.clone()) } Expr::MethodCall { receiver, args, .. } => { - is_method_call = true; let callee = self.infer.method_resolution(call_id)?; let mut args = args.clone(); args.insert(0, *receiver); @@ -389,13 +387,14 @@ pub fn record_pattern_missing_fields( #[cfg(test)] mod tests { - use insta::assert_snapshot; + use expect::{expect, Expect}; use ra_db::fixture::WithFixture; use crate::{diagnostics::MismatchedArgCount, test_db::TestDB}; - fn check_diagnostic_message(ra_fixture: &str) -> String { - TestDB::with_single_file(ra_fixture).0.diagnostic::().0 + fn check_diagnostic(ra_fixture: &str, expect: Expect) { + let msg = TestDB::with_single_file(ra_fixture).0.diagnostic::().0; + expect.assert_eq(&msg); } fn check_no_diagnostic(ra_fixture: &str) { @@ -407,55 +406,43 @@ mod tests { #[test] fn simple_free_fn_zero() { - assert_snapshot!(check_diagnostic_message( + check_diagnostic( r" fn zero() {} - - fn f() { - zero(1); - } - " - ), - @"\"zero(1)\": Expected 0 arguments, found 1\n"); + fn f() { zero(1); } + ", + expect![["\"zero(1)\": Expected 0 arguments, found 1\n"]], + ); check_no_diagnostic( r" fn zero() {} - - fn f() { - zero(); - } + fn f() { zero(); } ", ); } #[test] fn simple_free_fn_one() { - assert_snapshot!(check_diagnostic_message( + check_diagnostic( r" fn one(arg: u8) {} - - fn f() { - one(); - } - " - ), - @"\"one()\": Expected 1 argument, found 0\n"); + fn f() { one(); } + ", + expect![["\"one()\": Expected 1 argument, found 0\n"]], + ); check_no_diagnostic( r" fn one(arg: u8) {} - - fn f() { - one(1); - } + fn f() { one(1); } ", ); } #[test] fn method_as_fn() { - assert_snapshot!(check_diagnostic_message( + check_diagnostic( r" struct S; impl S { @@ -465,9 +452,9 @@ mod tests { fn f() { S::method(); } - " - ), - @"\"S::method()\": Expected 1 argument, found 0\n"); + ", + expect![["\"S::method()\": Expected 1 argument, found 0\n"]], + ); check_no_diagnostic( r" @@ -486,7 +473,7 @@ mod tests { #[test] fn method_with_arg() { - assert_snapshot!(check_diagnostic_message( + check_diagnostic( r" struct S; impl S { @@ -496,9 +483,9 @@ mod tests { fn f() { S.method(); } - " - ), - @"\"S.method()\": Expected 1 argument, found 0\n"); + ", + expect![["\"S.method()\": Expected 1 argument, found 0\n"]], + ); check_no_diagnostic( r"