Add diagnostic for filter_map followed by next

Author: theotherphil

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)

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<ast::Expr>,
+    pub next_expr: AstPtr<ast::Expr>,
+}
+
+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<SyntaxNodePtr> {
+        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(..)
+            }
+            "#,
+        );
+    }
 }

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<InferenceResult>,
@@ -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(

crates/ide/src/diagnostics.rs

@@ -136,6 +136,9 @@ pub(crate) fn diagnostics(
         .on::<hir::diagnostics::IncorrectCase, _>(|d| {
             res.borrow_mut().push(warning_with_fix(d, &sema));
         })
+        .on::<hir::diagnostics::ReplaceFilterMapNextWithFindMap, _>(|d| {
+            res.borrow_mut().push(warning_with_fix(d, &sema));
+        })
         .on::<hir::diagnostics::InactiveCode, _>(|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() {

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<RootDatabase>) -> Option<Fix> {
+        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<RootDatabase>,
     usage_file_id: FileId,