From 1316422a7c2ef26e9da78fa23f170407b1cb39bb Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Mon, 28 Dec 2020 13:41:15 +0000 Subject: Add diagnostic for filter_map followed by next --- crates/hir/src/diagnostics.rs | 11 +++++- crates/hir_ty/src/diagnostics.rs | 48 ++++++++++++++++++++++-- crates/hir_ty/src/diagnostics/expr.rs | 70 ++++++++++++++++++++++++++++++----- crates/ide/src/diagnostics.rs | 3 ++ crates/ide/src/diagnostics/fixes.rs | 33 ++++++++++++++++- 5 files changed, 150 insertions(+), 15 deletions(-) diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 447faa04f..f7fd3e237 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -5,5 +5,14 @@ pub use hir_expand::diagnostics::{ }; pub use hir_ty::diagnostics::{ IncorrectCase, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, - NoSuchField, RemoveThisSemicolon, + NoSuchField, RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, }; + +// PHIL: +// hir/src/diagnostics.rs - just pub uses the type from hir_ty::diagnostics (DONE) +// hir_ty/src/diagnostics.rs - defines the type (DONE) +// hir_ty/src/diagnostics.rs - plus a test (DONE) <--- one example found, need to copy the not-applicable tests from the assist version +// ide/src/diagnostics.rs - define handler for when this diagnostic is raised (DONE) + +// ide/src/diagnostics/fixes.rs - pulls in type from hir, and impls DiagnosticWithFix (TODO) +// hir_ty/src/diagnostics/expr.rs - do the real work (TODO) diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 247da43f2..b4cf81c10 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -247,7 +247,7 @@ impl Diagnostic for RemoveThisSemicolon { // Diagnostic: break-outside-of-loop // -// This diagnostic is triggered if `break` keyword is used outside of a loop. +// This diagnostic is triggered if the `break` keyword is used outside of a loop. #[derive(Debug)] pub struct BreakOutsideOfLoop { pub file: HirFileId, @@ -271,7 +271,7 @@ impl Diagnostic for BreakOutsideOfLoop { // Diagnostic: missing-unsafe // -// This diagnostic is triggered if operation marked as `unsafe` is used outside of `unsafe` function or block. +// This diagnostic is triggered if an operation marked as `unsafe` is used outside of an `unsafe` function or block. #[derive(Debug)] pub struct MissingUnsafe { pub file: HirFileId, @@ -295,7 +295,7 @@ impl Diagnostic for MissingUnsafe { // Diagnostic: mismatched-arg-count // -// This diagnostic is triggered if function is invoked with an incorrect amount of arguments. +// This diagnostic is triggered if a function is invoked with an incorrect amount of arguments. #[derive(Debug)] pub struct MismatchedArgCount { pub file: HirFileId, @@ -347,7 +347,7 @@ impl fmt::Display for CaseType { // Diagnostic: incorrect-ident-case // -// This diagnostic is triggered if item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention]. +// This diagnostic is triggered if an item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention]. #[derive(Debug)] pub struct IncorrectCase { pub file: HirFileId, @@ -386,6 +386,31 @@ impl Diagnostic for IncorrectCase { } } +// Diagnostic: replace-filter-map-next-with-find-map +// +// This diagnostic is triggered when `.filter_map(..).next()` is used, rather than the more concise `.find_map(..)`. +#[derive(Debug)] +pub struct ReplaceFilterMapNextWithFindMap { + pub file: HirFileId, + pub filter_map_expr: AstPtr, + pub next_expr: AstPtr, +} + +impl Diagnostic for ReplaceFilterMapNextWithFindMap { + fn code(&self) -> DiagnosticCode { + DiagnosticCode("replace-filter-map-next-with-find-map") + } + fn message(&self) -> String { + "replace filter_map(..).next() with find_map(..)".to_string() + } + fn display_source(&self) -> InFile { + InFile { file_id: self.file, value: self.filter_map_expr.clone().into() } + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } +} + #[cfg(test)] mod tests { use base_db::{fixture::WithFixture, FileId, SourceDatabase, SourceDatabaseExt}; @@ -644,4 +669,19 @@ fn foo() { break; } "#, ); } + + #[test] + fn replace_missing_filter_next_with_find_map() { + check_diagnostics( + r#" + fn foo() { + let m = [1, 2, 3] + .iter() + .filter_map(|x| if *x == 2 { Some (4) } else { None }) + .next(); + //^^^ Replace .filter_map(..).next() with .find_map(..) + } + "#, + ); + } } diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 107417c27..170d23178 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -24,6 +24,8 @@ pub(crate) use hir_def::{ LocalFieldId, VariantId, }; +use super::ReplaceFilterMapNextWithFindMap; + pub(super) struct ExprValidator<'a, 'b: 'a> { owner: DefWithBodyId, infer: Arc, @@ -39,7 +41,18 @@ impl<'a, 'b> ExprValidator<'a, 'b> { ExprValidator { owner, infer, sink } } + fn bar() { + // LOOK FOR THIS + let m = [1, 2, 3] + .iter() + .filter_map(|x| if *x == 2 { Some(4) } else { None }) + .next(); + } + pub(super) fn validate_body(&mut self, db: &dyn HirDatabase) { + // DO NOT MERGE: just getting something working for now + self.check_for_filter_map_next(db); + let body = db.body(self.owner.into()); for (id, expr) in body.exprs.iter() { @@ -150,20 +163,58 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } } - fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) -> Option<()> { + fn check_for_filter_map_next(&mut self, db: &dyn HirDatabase) { + let body = db.body(self.owner.into()); + let mut prev = None; + + for (id, expr) in body.exprs.iter() { + if let Expr::MethodCall { receiver, method_name, args, .. } = expr { + let method_name_hack_do_not_merge = format!("{}", method_name); + + if method_name_hack_do_not_merge == "filter_map" && args.len() == 1 { + prev = Some((id, args[0])); + continue; + } + + if method_name_hack_do_not_merge == "next" { + if let Some((filter_map_id, filter_map_args)) = prev { + if *receiver == filter_map_id { + let (_, source_map) = db.body_with_source_map(self.owner.into()); + if let (Ok(filter_map_source_ptr), Ok(next_source_ptr)) = ( + source_map.expr_syntax(filter_map_id), + source_map.expr_syntax(id), + ) { + self.sink.push(ReplaceFilterMapNextWithFindMap { + file: filter_map_source_ptr.file_id, + filter_map_expr: filter_map_source_ptr.value, + next_expr: next_source_ptr.value, + }); + } + } + } + } + } + prev = None; + } + } + + fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) { // Check that the number of arguments matches the number of parameters. // 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 None; + return; } let is_method_call = matches!(expr, Expr::MethodCall { .. }); let (sig, args) = match expr { Expr::Call { callee, args } => { let callee = &self.infer.type_of_expr[*callee]; - let sig = callee.callable_sig(db)?; + let sig = match callee.callable_sig(db) { + Some(sig) => sig, + None => return, + }; (sig, args.clone()) } Expr::MethodCall { receiver, args, .. } => { @@ -175,22 +226,25 @@ impl<'a, 'b> ExprValidator<'a, 'b> { // if the receiver is of unknown type, it's very likely we // don't know enough to correctly resolve the method call. // This is kind of a band-aid for #6975. - return None; + return; } // FIXME: note that we erase information about substs here. This // is not right, but, luckily, doesn't matter as we care only // about the number of params - let callee = self.infer.method_resolution(call_id)?; + let callee = match self.infer.method_resolution(call_id) { + Some(callee) => callee, + None => return, + }; let sig = db.callable_item_signature(callee.into()).value; (sig, args) } - _ => return None, + _ => return, }; if sig.is_varargs { - return None; + return; } let params = sig.params(); @@ -213,8 +267,6 @@ impl<'a, 'b> ExprValidator<'a, 'b> { }); } } - - None } fn validate_match( diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index b35bc2bae..8607139ba 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -136,6 +136,9 @@ pub(crate) fn diagnostics( .on::(|d| { res.borrow_mut().push(warning_with_fix(d, &sema)); }) + .on::(|d| { + res.borrow_mut().push(warning_with_fix(d, &sema)); + }) .on::(|d| { // If there's inactive code somewhere in a macro, don't propagate to the call-site. if d.display_source().file_id.expansion_info(db).is_some() { diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 579d5a308..eafbac43a 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -4,7 +4,7 @@ use hir::{ db::AstDatabase, diagnostics::{ Diagnostic, IncorrectCase, MissingFields, MissingOkOrSomeInTailExpr, NoSuchField, - RemoveThisSemicolon, UnresolvedModule, + RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, UnresolvedModule, }, HasSource, HirDisplay, InFile, Semantics, VariantDef, }; @@ -144,6 +144,37 @@ impl DiagnosticWithFix for IncorrectCase { } } +// Bugs: +// * Action is applicable for both iter() and filter_map() rows +// * Action deletes the entire method chain +impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { + fn fix(&self, sema: &Semantics) -> Option { + let root = sema.db.parse_or_expand(self.file)?; + + let next_expr = self.next_expr.to_node(&root); + let next_expr_range = next_expr.syntax().text_range(); + + let filter_map_expr = self.filter_map_expr.to_node(&root); + let filter_map_expr_range = filter_map_expr.syntax().text_range(); + + let edit = TextEdit::delete(next_expr_range); + + // This is the entire method chain, including the array literal + eprintln!("NEXT EXPR: {:#?}", next_expr); + // This is the entire method chain except for the final next() + eprintln!("FILTER MAP EXPR: {:#?}", filter_map_expr); + + let source_change = + SourceFileEdit { file_id: self.file.original_file(sema.db), edit }.into(); + + Some(Fix::new( + "Replace filter_map(..).next() with find_map()", + source_change, + filter_map_expr_range, + )) + } +} + fn missing_record_expr_field_fix( sema: &Semantics, usage_file_id: FileId, -- cgit v1.2.3 From 1ff860b93c972e0f8d3a8ee582c255fa59e9b284 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 30 Dec 2020 15:46:05 +0000 Subject: Implement fix, add tests --- crates/hir/src/diagnostics.rs | 9 ------ crates/hir_ty/src/diagnostics.rs | 56 ++++++++++++++++++++++++++++++----- crates/hir_ty/src/diagnostics/expr.rs | 27 +++++------------ crates/ide/src/diagnostics/fixes.rs | 29 +++++++----------- 4 files changed, 66 insertions(+), 55 deletions(-) diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index f7fd3e237..5343a036c 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -7,12 +7,3 @@ pub use hir_ty::diagnostics::{ IncorrectCase, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, NoSuchField, RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, }; - -// PHIL: -// hir/src/diagnostics.rs - just pub uses the type from hir_ty::diagnostics (DONE) -// hir_ty/src/diagnostics.rs - defines the type (DONE) -// hir_ty/src/diagnostics.rs - plus a test (DONE) <--- one example found, need to copy the not-applicable tests from the assist version -// ide/src/diagnostics.rs - define handler for when this diagnostic is raised (DONE) - -// ide/src/diagnostics/fixes.rs - pulls in type from hir, and impls DiagnosticWithFix (TODO) -// hir_ty/src/diagnostics/expr.rs - do the real work (TODO) diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index b4cf81c10..3d7663f6a 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -392,7 +392,7 @@ impl Diagnostic for IncorrectCase { #[derive(Debug)] pub struct ReplaceFilterMapNextWithFindMap { pub file: HirFileId, - pub filter_map_expr: AstPtr, + /// This expression is the whole method chain up to and including `.filter_map(..).next()`. pub next_expr: AstPtr, } @@ -404,7 +404,7 @@ impl Diagnostic for ReplaceFilterMapNextWithFindMap { "replace filter_map(..).next() with find_map(..)".to_string() } fn display_source(&self) -> InFile { - InFile { file_id: self.file, value: self.filter_map_expr.clone().into() } + InFile { file_id: self.file, value: self.next_expr.clone().into() } } fn as_any(&self) -> &(dyn Any + Send + 'static) { self @@ -671,15 +671,55 @@ fn foo() { break; } } #[test] - fn replace_missing_filter_next_with_find_map() { + fn replace_filter_next_with_find_map() { check_diagnostics( r#" fn foo() { - let m = [1, 2, 3] - .iter() - .filter_map(|x| if *x == 2 { Some (4) } else { None }) - .next(); - //^^^ Replace .filter_map(..).next() with .find_map(..) + let m = [1, 2, 3].iter().filter_map(|x| if *x == 2 { Some (4) } else { None }).next(); + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ replace filter_map(..).next() with find_map(..) + } + "#, + ); + } + + #[test] + fn replace_filter_next_with_find_map_no_diagnostic_without_next() { + check_diagnostics( + r#" + fn foo() { + let m = [1, 2, 3] + .iter() + .filter_map(|x| if *x == 2 { Some (4) } else { None }) + .len(); + } + "#, + ); + } + + #[test] + fn replace_filter_next_with_find_map_no_diagnostic_with_intervening_methods() { + check_diagnostics( + r#" + fn foo() { + let m = [1, 2, 3] + .iter() + .filter_map(|x| if *x == 2 { Some (4) } else { None }) + .map(|x| x + 2) + .len(); + } + "#, + ); + } + + #[test] + fn replace_filter_next_with_find_map_no_diagnostic_if_not_in_chain() { + check_diagnostics( + r#" + fn foo() { + let m = [1, 2, 3] + .iter() + .filter_map(|x| if *x == 2 { Some (4) } else { None }); + let n = m.next(); } "#, ); diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 170d23178..b87557ff5 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -41,16 +41,7 @@ impl<'a, 'b> ExprValidator<'a, 'b> { ExprValidator { owner, infer, sink } } - fn bar() { - // LOOK FOR THIS - let m = [1, 2, 3] - .iter() - .filter_map(|x| if *x == 2 { Some(4) } else { None }) - .next(); - } - pub(super) fn validate_body(&mut self, db: &dyn HirDatabase) { - // DO NOT MERGE: just getting something working for now self.check_for_filter_map_next(db); let body = db.body(self.owner.into()); @@ -169,24 +160,20 @@ impl<'a, 'b> ExprValidator<'a, 'b> { for (id, expr) in body.exprs.iter() { if let Expr::MethodCall { receiver, method_name, args, .. } = expr { - let method_name_hack_do_not_merge = format!("{}", method_name); + let method_name = format!("{}", method_name); - if method_name_hack_do_not_merge == "filter_map" && args.len() == 1 { - prev = Some((id, args[0])); + if method_name == "filter_map" && args.len() == 1 { + prev = Some(id); continue; } - if method_name_hack_do_not_merge == "next" { - if let Some((filter_map_id, filter_map_args)) = prev { + if method_name == "next" { + if let Some(filter_map_id) = prev { if *receiver == filter_map_id { let (_, source_map) = db.body_with_source_map(self.owner.into()); - if let (Ok(filter_map_source_ptr), Ok(next_source_ptr)) = ( - source_map.expr_syntax(filter_map_id), - source_map.expr_syntax(id), - ) { + if let Ok(next_source_ptr) = source_map.expr_syntax(id) { self.sink.push(ReplaceFilterMapNextWithFindMap { - file: filter_map_source_ptr.file_id, - filter_map_expr: filter_map_source_ptr.value, + file: next_source_ptr.file_id, next_expr: next_source_ptr.value, }); } diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index eafbac43a..7bbf1d8c7 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -1,5 +1,6 @@ //! Provides a way to attach fixes to the diagnostics. //! The same module also has all curret custom fixes for the diagnostics implemented. +use ast::MethodCallExpr; use hir::{ db::AstDatabase, diagnostics::{ @@ -13,11 +14,7 @@ use ide_db::{ source_change::{FileSystemEdit, SourceChange}, RootDatabase, }; -use syntax::{ - algo, - ast::{self, edit::IndentLevel, make}, - AstNode, -}; +use syntax::{AstNode, TextRange, algo, ast::{self, ArgList, edit::IndentLevel, make}}; use text_edit::TextEdit; use crate::{diagnostics::Fix, references::rename::rename_with_semantics, FilePosition}; @@ -144,25 +141,21 @@ impl DiagnosticWithFix for IncorrectCase { } } -// Bugs: -// * Action is applicable for both iter() and filter_map() rows -// * Action deletes the entire method chain impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { fn fix(&self, sema: &Semantics) -> Option { let root = sema.db.parse_or_expand(self.file)?; - let next_expr = self.next_expr.to_node(&root); - let next_expr_range = next_expr.syntax().text_range(); + let next_call = MethodCallExpr::cast(next_expr.syntax().clone())?; - let filter_map_expr = self.filter_map_expr.to_node(&root); - let filter_map_expr_range = filter_map_expr.syntax().text_range(); + let filter_map_call = MethodCallExpr::cast(next_call.receiver()?.syntax().clone())?; + let filter_map_name_range = filter_map_call.name_ref()?.ident_token()?.text_range(); + let filter_map_args = filter_map_call.syntax().children().find_map(ArgList::cast)?; - let edit = TextEdit::delete(next_expr_range); + let range_to_replace = TextRange::new(filter_map_name_range.start(), next_expr.syntax().text_range().end()); + let replacement = format!("find_map{}", filter_map_args.syntax().text()); + let trigger_range = next_expr.syntax().text_range(); - // This is the entire method chain, including the array literal - eprintln!("NEXT EXPR: {:#?}", next_expr); - // This is the entire method chain except for the final next() - eprintln!("FILTER MAP EXPR: {:#?}", filter_map_expr); + let edit = TextEdit::replace(range_to_replace, replacement); let source_change = SourceFileEdit { file_id: self.file.original_file(sema.db), edit }.into(); @@ -170,7 +163,7 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { Some(Fix::new( "Replace filter_map(..).next() with find_map()", source_change, - filter_map_expr_range, + trigger_range, )) } } -- cgit v1.2.3 From 449ced4d218f413e152785fb81d161a6389974f7 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 30 Dec 2020 15:52:36 +0000 Subject: cargo fmt --- crates/ide/src/diagnostics/fixes.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 7bbf1d8c7..a73c0778b 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -14,7 +14,11 @@ use ide_db::{ source_change::{FileSystemEdit, SourceChange}, RootDatabase, }; -use syntax::{AstNode, TextRange, algo, ast::{self, ArgList, edit::IndentLevel, make}}; +use syntax::{ + algo, + ast::{self, edit::IndentLevel, make, ArgList}, + AstNode, TextRange, +}; use text_edit::TextEdit; use crate::{diagnostics::Fix, references::rename::rename_with_semantics, FilePosition}; @@ -151,7 +155,8 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { let filter_map_name_range = filter_map_call.name_ref()?.ident_token()?.text_range(); let filter_map_args = filter_map_call.syntax().children().find_map(ArgList::cast)?; - let range_to_replace = TextRange::new(filter_map_name_range.start(), next_expr.syntax().text_range().end()); + let range_to_replace = + TextRange::new(filter_map_name_range.start(), next_expr.syntax().text_range().end()); let replacement = format!("find_map{}", filter_map_args.syntax().text()); let trigger_range = next_expr.syntax().text_range(); -- cgit v1.2.3 From e62e4ed148021a80b3a4672dab10b828fbd712ee Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 30 Dec 2020 16:14:35 +0000 Subject: Address review comments --- crates/ide/src/diagnostics/fixes.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index a73c0778b..5828ea35a 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -1,6 +1,5 @@ //! Provides a way to attach fixes to the diagnostics. //! The same module also has all curret custom fixes for the diagnostics implemented. -use ast::MethodCallExpr; use hir::{ db::AstDatabase, diagnostics::{ @@ -14,11 +13,7 @@ use ide_db::{ source_change::{FileSystemEdit, SourceChange}, RootDatabase, }; -use syntax::{ - algo, - ast::{self, edit::IndentLevel, make, ArgList}, - AstNode, TextRange, -}; +use syntax::{AstNode, TextRange, algo, ast::{self, ArgListOwner, edit::IndentLevel, make}}; use text_edit::TextEdit; use crate::{diagnostics::Fix, references::rename::rename_with_semantics, FilePosition}; @@ -149,11 +144,11 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { fn fix(&self, sema: &Semantics) -> Option { let root = sema.db.parse_or_expand(self.file)?; let next_expr = self.next_expr.to_node(&root); - let next_call = MethodCallExpr::cast(next_expr.syntax().clone())?; + let next_call = ast::MethodCallExpr::cast(next_expr.syntax().clone())?; - let filter_map_call = MethodCallExpr::cast(next_call.receiver()?.syntax().clone())?; + let filter_map_call = ast::MethodCallExpr::cast(next_call.receiver()?.syntax().clone())?; let filter_map_name_range = filter_map_call.name_ref()?.ident_token()?.text_range(); - let filter_map_args = filter_map_call.syntax().children().find_map(ArgList::cast)?; + let filter_map_args = filter_map_call.arg_list()?; let range_to_replace = TextRange::new(filter_map_name_range.start(), next_expr.syntax().text_range().end()); -- cgit v1.2.3 From 920e57bd153fdafeddb1dc34a11c6ef05ace2f70 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Wed, 30 Dec 2020 16:15:00 +0000 Subject: cargo fmt --- crates/ide/src/diagnostics/fixes.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 5828ea35a..1f112c5b8 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -13,7 +13,11 @@ use ide_db::{ source_change::{FileSystemEdit, SourceChange}, RootDatabase, }; -use syntax::{AstNode, TextRange, algo, ast::{self, ArgListOwner, edit::IndentLevel, make}}; +use syntax::{ + algo, + ast::{self, edit::IndentLevel, make, ArgListOwner}, + AstNode, TextRange, +}; use text_edit::TextEdit; use crate::{diagnostics::Fix, references::rename::rename_with_semantics, FilePosition}; -- cgit v1.2.3 From 8c7ccdc29d071649e816030ac744338e91eb5558 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Fri, 1 Jan 2021 21:11:08 +0000 Subject: Identify methods using functions ids rather than string names --- crates/hir_def/src/path.rs | 1 + crates/hir_expand/src/name.rs | 3 +++ crates/hir_ty/src/diagnostics/expr.rs | 34 +++++++++++++++++++++++++++------- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/crates/hir_def/src/path.rs b/crates/hir_def/src/path.rs index e34cd7f2f..84ea09b53 100644 --- a/crates/hir_def/src/path.rs +++ b/crates/hir_def/src/path.rs @@ -304,6 +304,7 @@ pub use hir_expand::name as __name; #[macro_export] macro_rules! __known_path { (core::iter::IntoIterator) => {}; + (core::iter::Iterator) => {}; (core::result::Result) => {}; (core::option::Option) => {}; (core::ops::Range) => {}; diff --git a/crates/hir_expand/src/name.rs b/crates/hir_expand/src/name.rs index d692cec14..c7609e90d 100644 --- a/crates/hir_expand/src/name.rs +++ b/crates/hir_expand/src/name.rs @@ -186,6 +186,9 @@ pub mod known { Neg, Not, Index, + // Components of known path (function name) + filter_map, + next, // Builtin macros file, column, diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index b87557ff5..16bbd48fb 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -2,8 +2,8 @@ use std::sync::Arc; -use hir_def::{expr::Statement, path::path, resolver::HasResolver, AdtId, DefWithBodyId}; -use hir_expand::diagnostics::DiagnosticSink; +use hir_def::{AdtId, AssocItemId, DefWithBodyId, expr::Statement, path::path, resolver::HasResolver}; +use hir_expand::{diagnostics::DiagnosticSink, name}; use rustc_hash::FxHashSet; use syntax::{ast, AstPtr}; @@ -155,19 +155,39 @@ impl<'a, 'b> ExprValidator<'a, 'b> { } fn check_for_filter_map_next(&mut self, db: &dyn HirDatabase) { + // Find the FunctionIds for Iterator::filter_map and Iterator::next + let iterator_path = path![core::iter::Iterator]; + let resolver = self.owner.resolver(db.upcast()); + let iterator_trait_id = match resolver.resolve_known_trait(db.upcast(), &iterator_path) { + Some(id) => id, + None => return, + }; + let iterator_trait_items = &db.trait_data(iterator_trait_id).items; + let filter_map_function_id = match iterator_trait_items.iter().find(|item| item.0 == name![filter_map]) { + Some((_, AssocItemId::FunctionId(id))) => id, + _ => return, + }; + let next_function_id = match iterator_trait_items.iter().find(|item| item.0 == name![next]) { + Some((_, AssocItemId::FunctionId(id))) => id, + _ => return, + }; + + // Search function body for instances of .filter_map(..).next() let body = db.body(self.owner.into()); let mut prev = None; - for (id, expr) in body.exprs.iter() { - if let Expr::MethodCall { receiver, method_name, args, .. } = expr { - let method_name = format!("{}", method_name); + if let Expr::MethodCall { receiver, .. } = expr { + let function_id = match self.infer.method_resolution(id) { + Some(id) => id, + None => continue, + }; - if method_name == "filter_map" && args.len() == 1 { + if function_id == *filter_map_function_id { prev = Some(id); continue; } - if method_name == "next" { + if function_id == *next_function_id { if let Some(filter_map_id) = prev { if *receiver == filter_map_id { let (_, source_map) = db.body_with_source_map(self.owner.into()); -- cgit v1.2.3 From 7c691f51f9974572e1e56c0e368b973ed3e58365 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Fri, 1 Jan 2021 21:17:54 +0000 Subject: Fix test names --- crates/hir_ty/src/diagnostics.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 3d7663f6a..6eaa1beb8 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -671,7 +671,7 @@ fn foo() { break; } } #[test] - fn replace_filter_next_with_find_map() { + fn replace_filter_map_next_with_find_map() { check_diagnostics( r#" fn foo() { @@ -683,7 +683,7 @@ fn foo() { break; } } #[test] - fn replace_filter_next_with_find_map_no_diagnostic_without_next() { + fn replace_filter_map_next_with_find_map_no_diagnostic_without_next() { check_diagnostics( r#" fn foo() { @@ -697,7 +697,7 @@ fn foo() { break; } } #[test] - fn replace_filter_next_with_find_map_no_diagnostic_with_intervening_methods() { + fn replace_filter_map_next_with_find_map_no_diagnostic_with_intervening_methods() { check_diagnostics( r#" fn foo() { @@ -712,7 +712,7 @@ fn foo() { break; } } #[test] - fn replace_filter_next_with_find_map_no_diagnostic_if_not_in_chain() { + fn replace_filter_map_next_with_find_map_no_diagnostic_if_not_in_chain() { check_diagnostics( r#" fn foo() { -- cgit v1.2.3 From 65a5ea581d547c36e98b4a3c5a99671ad5d4c117 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Fri, 1 Jan 2021 21:40:11 +0000 Subject: Update tests to register the required standard library types --- crates/hir_ty/src/diagnostics.rs | 48 +++++++++++++++++++++++++++-------- crates/hir_ty/src/diagnostics/expr.rs | 16 +++++++----- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 6eaa1beb8..323c5f963 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs @@ -670,21 +670,49 @@ fn foo() { break; } ); } + // Register the required standard library types to make the tests work + fn add_filter_map_with_find_next_boilerplate(body: &str) -> String { + let prefix = r#" + //- /main.rs crate:main deps:core + use core::iter::Iterator; + use core::option::Option::{self, Some, None}; + "#; + let suffix = r#" + //- /core/lib.rs crate:core + pub mod option { + pub enum Option { Some(T), None } + } + pub mod iter { + pub trait Iterator { + type Item; + fn filter_map(self, f: F) -> FilterMap where F: FnMut(Self::Item) -> Option { FilterMap } + fn next(&mut self) -> Option; + } + pub struct FilterMap {} + impl Iterator for FilterMap { + type Item = i32; + fn next(&mut self) -> i32 { 7 } + } + } + "#; + format!("{}{}{}", prefix, body, suffix) + } + #[test] - fn replace_filter_map_next_with_find_map() { - check_diagnostics( + fn replace_filter_map_next_with_find_map2() { + check_diagnostics(&add_filter_map_with_find_next_boilerplate( r#" fn foo() { let m = [1, 2, 3].iter().filter_map(|x| if *x == 2 { Some (4) } else { None }).next(); //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ replace filter_map(..).next() with find_map(..) } - "#, - ); + "#, + )); } #[test] fn replace_filter_map_next_with_find_map_no_diagnostic_without_next() { - check_diagnostics( + check_diagnostics(&add_filter_map_with_find_next_boilerplate( r#" fn foo() { let m = [1, 2, 3] @@ -693,12 +721,12 @@ fn foo() { break; } .len(); } "#, - ); + )); } #[test] fn replace_filter_map_next_with_find_map_no_diagnostic_with_intervening_methods() { - check_diagnostics( + check_diagnostics(&add_filter_map_with_find_next_boilerplate( r#" fn foo() { let m = [1, 2, 3] @@ -708,12 +736,12 @@ fn foo() { break; } .len(); } "#, - ); + )); } #[test] fn replace_filter_map_next_with_find_map_no_diagnostic_if_not_in_chain() { - check_diagnostics( + check_diagnostics(&add_filter_map_with_find_next_boilerplate( r#" fn foo() { let m = [1, 2, 3] @@ -722,6 +750,6 @@ fn foo() { break; } let n = m.next(); } "#, - ); + )); } } diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 16bbd48fb..d740b7265 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs @@ -2,7 +2,9 @@ use std::sync::Arc; -use hir_def::{AdtId, AssocItemId, DefWithBodyId, expr::Statement, path::path, resolver::HasResolver}; +use hir_def::{ + expr::Statement, path::path, resolver::HasResolver, AdtId, AssocItemId, DefWithBodyId, +}; use hir_expand::{diagnostics::DiagnosticSink, name}; use rustc_hash::FxHashSet; use syntax::{ast, AstPtr}; @@ -163,11 +165,13 @@ impl<'a, 'b> ExprValidator<'a, 'b> { None => return, }; let iterator_trait_items = &db.trait_data(iterator_trait_id).items; - let filter_map_function_id = match iterator_trait_items.iter().find(|item| item.0 == name![filter_map]) { - Some((_, AssocItemId::FunctionId(id))) => id, - _ => return, - }; - let next_function_id = match iterator_trait_items.iter().find(|item| item.0 == name![next]) { + let filter_map_function_id = + match iterator_trait_items.iter().find(|item| item.0 == name![filter_map]) { + Some((_, AssocItemId::FunctionId(id))) => id, + _ => return, + }; + let next_function_id = match iterator_trait_items.iter().find(|item| item.0 == name![next]) + { Some((_, AssocItemId::FunctionId(id))) => id, _ => return, }; -- cgit v1.2.3 From db6dda94a39534bbf20da844a4f221c3d14509c4 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sat, 23 Jan 2021 07:54:45 +0000 Subject: Remove use of SourceFileEdit --- crates/ide/src/diagnostics/fixes.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 1f112c5b8..cbfc66ab3 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -161,8 +161,7 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { let edit = TextEdit::replace(range_to_replace, replacement); - let source_change = - SourceFileEdit { file_id: self.file.original_file(sema.db), edit }.into(); + let source_change = SourceChange::from_text_edit(self.file.original_file(sema.db), edit); Some(Fix::new( "Replace filter_map(..).next() with find_map()", -- cgit v1.2.3