Merge #9253

9253: internal: refactor missing or or some diagnostic r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Author: bors[bot]

crates/hir/src/diagnostics.rs

@@ -37,8 +37,11 @@ diagnostics![
     MacroError,
     MismatchedArgCount,
     MissingFields,
+    MissingOkOrSomeInTailExpr,
     MissingUnsafe,
     NoSuchField,
+    RemoveThisSemicolon,
+    ReplaceFilterMapNextWithFindMap,
     UnimplementedBuiltinMacro,
     UnresolvedExternCrate,
     UnresolvedImport,
@@ -119,31 +122,13 @@ pub struct MissingFields {
     pub missed_fields: Vec<Name>,
 }
 
-// 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,
     /// This expression is the whole method chain up to and including `.filter_map(..).next()`.
     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.next_expr.clone().into() }
-    }
-    fn as_any(&self) -> &(dyn Any + Send + 'static) {
-        self
-    }
-}
-
 #[derive(Debug)]
 pub struct MismatchedArgCount {
     pub call_expr: InFile<AstPtr<ast::Expr>>,
@@ -153,63 +138,16 @@ pub struct MismatchedArgCount {
 
 #[derive(Debug)]
 pub struct RemoveThisSemicolon {
-    pub file: HirFileId,
-    pub expr: AstPtr<ast::Expr>,
-}
-
-impl Diagnostic for RemoveThisSemicolon {
-    fn code(&self) -> DiagnosticCode {
-        DiagnosticCode("remove-this-semicolon")
-    }
-
-    fn message(&self) -> String {
-        "Remove this semicolon".to_string()
-    }
-
-    fn display_source(&self) -> InFile<SyntaxNodePtr> {
-        InFile { file_id: self.file, value: self.expr.clone().into() }
-    }
-
-    fn as_any(&self) -> &(dyn Any + Send + 'static) {
-        self
-    }
+    pub expr: InFile<AstPtr<ast::Expr>>,
 }
 
-// Diagnostic: missing-ok-or-some-in-tail-expr
-//
-// This diagnostic is triggered if a block that should return `Result` returns a value not wrapped in `Ok`,
-// or if a block that should return `Option` returns a value not wrapped in `Some`.
-//
-// Example:
-//
-// ```rust
-// fn foo() -> Result<u8, ()> {
-//     10
-// }
-// ```
 #[derive(Debug)]
 pub struct MissingOkOrSomeInTailExpr {
-    pub file: HirFileId,
-    pub expr: AstPtr<ast::Expr>,
+    pub expr: InFile<AstPtr<ast::Expr>>,
     // `Some` or `Ok` depending on whether the return type is Result or Option
     pub required: String,
 }
 
-impl Diagnostic for MissingOkOrSomeInTailExpr {
-    fn code(&self) -> DiagnosticCode {
-        DiagnosticCode("missing-ok-or-some-in-tail-expr")
-    }
-    fn message(&self) -> String {
-        format!("wrap return expression in {}", self.required)
-    }
-    fn display_source(&self) -> InFile<SyntaxNodePtr> {
-        InFile { file_id: self.file, value: self.expr.clone().into() }
-    }
-    fn as_any(&self) -> &(dyn Any + Send + 'static) {
-        self
-    }
-}
-
 // Diagnostic: missing-match-arm
 //
 // This diagnostic is triggered if `match` block is missing one or more match arms.

crates/hir/src/lib.rs

@@ -1168,10 +1168,13 @@ impl Function {
                 }
                 BodyValidationDiagnostic::ReplaceFilterMapNextWithFindMap { method_call_expr } => {
                     if let Ok(next_source_ptr) = source_map.expr_syntax(method_call_expr) {
-                        sink.push(ReplaceFilterMapNextWithFindMap {
-                            file: next_source_ptr.file_id,
-                            next_expr: next_source_ptr.value,
-                        });
+                        acc.push(
+                            ReplaceFilterMapNextWithFindMap {
+                                file: next_source_ptr.file_id,
+                                next_expr: next_source_ptr.value,
+                            }
+                            .into(),
+                        );
                     }
                 }
                 BodyValidationDiagnostic::MismatchedArgCount { call_expr, expected, found } => {
@@ -1184,20 +1187,13 @@ impl Function {
                 }
                 BodyValidationDiagnostic::RemoveThisSemicolon { expr } => {
                     match source_map.expr_syntax(expr) {
-                        Ok(source_ptr) => sink.push(RemoveThisSemicolon {
-                            file: source_ptr.file_id,
-                            expr: source_ptr.value,
-                        }),
+                        Ok(expr) => acc.push(RemoveThisSemicolon { expr }.into()),
                         Err(SyntheticSyntax) => (),
                     }
                 }
                 BodyValidationDiagnostic::MissingOkOrSomeInTailExpr { expr, required } => {
                     match source_map.expr_syntax(expr) {
-                        Ok(source_ptr) => sink.push(MissingOkOrSomeInTailExpr {
-                            file: source_ptr.file_id,
-                            expr: source_ptr.value,
-                            required,
-                        }),
+                        Ok(expr) => acc.push(MissingOkOrSomeInTailExpr { expr, required }.into()),
                         Err(SyntheticSyntax) => (),
                     }
                 }

crates/ide/src/diagnostics.rs

@@ -9,8 +9,11 @@ mod inactive_code;
 mod macro_error;
 mod mismatched_arg_count;
 mod missing_fields;
+mod missing_ok_or_some_in_tail_expr;
 mod missing_unsafe;
 mod no_such_field;
+mod remove_this_semicolon;
+mod replace_filter_map_next_with_find_map;
 mod unimplemented_builtin_macro;
 mod unresolved_extern_crate;
 mod unresolved_import;
@@ -162,18 +165,9 @@ pub(crate) fn diagnostics(
     }
     let res = RefCell::new(res);
     let sink_builder = DiagnosticSinkBuilder::new()
-        .on::<hir::diagnostics::MissingOkOrSomeInTailExpr, _>(|d| {
-            res.borrow_mut().push(diagnostic_with_fix(d, &sema, resolve));
-        })
-        .on::<hir::diagnostics::RemoveThisSemicolon, _>(|d| {
-            res.borrow_mut().push(diagnostic_with_fix(d, &sema, resolve));
-        })
         .on::<hir::diagnostics::IncorrectCase, _>(|d| {
             res.borrow_mut().push(warning_with_fix(d, &sema, resolve));
         })
-        .on::<hir::diagnostics::ReplaceFilterMapNextWithFindMap, _>(|d| {
-            res.borrow_mut().push(warning_with_fix(d, &sema, resolve));
-        })
         .on::<UnlinkedFile, _>(|d| {
             // Limit diagnostic to the first few characters in the file. This matches how VS Code
             // renders it with the full span, but on other editors, and is less invasive.
@@ -223,10 +217,13 @@ pub(crate) fn diagnostics(
         let d = match diag {
             AnyDiagnostic::BreakOutsideOfLoop(d) => break_outside_of_loop::break_outside_of_loop(&ctx, &d),
             AnyDiagnostic::MacroError(d) => macro_error::macro_error(&ctx, &d),
+            AnyDiagnostic::MismatchedArgCount(d) => mismatched_arg_count::mismatched_arg_count(&ctx, &d),
             AnyDiagnostic::MissingFields(d) => missing_fields::missing_fields(&ctx, &d),
+            AnyDiagnostic::MissingOkOrSomeInTailExpr(d) => missing_ok_or_some_in_tail_expr::missing_ok_or_some_in_tail_expr(&ctx, &d),
             AnyDiagnostic::MissingUnsafe(d) => missing_unsafe::missing_unsafe(&ctx, &d),
-            AnyDiagnostic::MismatchedArgCount(d) => mismatched_arg_count::mismatched_arg_count(&ctx, &d),
             AnyDiagnostic::NoSuchField(d) => no_such_field::no_such_field(&ctx, &d),
+            AnyDiagnostic::RemoveThisSemicolon(d) => remove_this_semicolon::remove_this_semicolon(&ctx, &d),
+            AnyDiagnostic::ReplaceFilterMapNextWithFindMap(d) => replace_filter_map_next_with_find_map::replace_filter_map_next_with_find_map(&ctx, &d),
             AnyDiagnostic::UnimplementedBuiltinMacro(d) => unimplemented_builtin_macro::unimplemented_builtin_macro(&ctx, &d),
             AnyDiagnostic::UnresolvedExternCrate(d) => unresolved_extern_crate::unresolved_extern_crate(&ctx, &d),
             AnyDiagnostic::UnresolvedImport(d) => unresolved_import::unresolved_import(&ctx, &d),
@@ -253,16 +250,6 @@ pub(crate) fn diagnostics(
     res
 }
 
-fn diagnostic_with_fix<D: DiagnosticWithFixes>(
-    d: &D,
-    sema: &Semantics<RootDatabase>,
-    resolve: &AssistResolveStrategy,
-) -> Diagnostic {
-    Diagnostic::error(sema.diagnostics_display_range(d.display_source()).range, d.message())
-        .with_fixes(d.fixes(sema, resolve))
-        .with_code(Some(d.code()))
-}
-
 fn warning_with_fix<D: DiagnosticWithFixes>(
     d: &D,
     sema: &Semantics<RootDatabase>,
@@ -448,39 +435,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn range_mapping_out_of_macros() {
-        // FIXME: this is very wrong, but somewhat tricky to fix.
-        check_fix(
-            r#"
-fn some() {}
-fn items() {}
-fn here() {}
-
-macro_rules! id { ($($tt:tt)*) => { $($tt)*}; }
-
-fn main() {
-    let _x = id![Foo { a: $042 }];
-}
-
-pub struct Foo { pub a: i32, pub b: i32 }
-"#,
-            r#"
-fn some(, b: () ) {}
-fn items() {}
-fn here() {}
-
-macro_rules! id { ($($tt:tt)*) => { $($tt)*}; }
-
-fn main() {
-    let _x = id![Foo { a: 42 }];
-}
-
-pub struct Foo { pub a: i32, pub b: i32 }
-"#,
-        );
-    }
-
     #[test]
     fn test_check_unnecessary_braces_in_use_statement() {
         check_diagnostics(
@@ -717,137 +671,6 @@ mod foo;
         );
     }
 
-    // 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<T> { Some(T), None }
-        }
-        pub mod iter {
-            pub trait Iterator {
-                type Item;
-                fn filter_map<B, F>(self, f: F) -> FilterMap where F: FnMut(Self::Item) -> Option<B> { FilterMap }
-                fn next(&mut self) -> Option<Self::Item>;
-            }
-            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_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(&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 })
-                    .len();
-            }
-            "#,
-        ));
-    }
-
-    #[test]
-    fn replace_filter_map_next_with_find_map_no_diagnostic_with_intervening_methods() {
-        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 })
-                    .map(|x| x + 2)
-                    .len();
-            }
-            "#,
-        ));
-    }
-
-    #[test]
-    fn replace_filter_map_next_with_find_map_no_diagnostic_if_not_in_chain() {
-        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 });
-                let n = m.next();
-            }
-            "#,
-        ));
-    }
-
-    #[test]
-    fn missing_record_pat_field_no_diagnostic_if_not_exhaustive() {
-        check_diagnostics(
-            r"
-struct S { foo: i32, bar: () }
-fn baz(s: S) -> i32 {
-    match s {
-        S { foo, .. } => foo,
-    }
-}
-",
-        )
-    }
-
-    #[test]
-    fn missing_record_pat_field_box() {
-        check_diagnostics(
-            r"
-struct S { s: Box<u32> }
-fn x(a: S) {
-    let S { box s } = a;
-}
-",
-        )
-    }
-
-    #[test]
-    fn missing_record_pat_field_ref() {
-        check_diagnostics(
-            r"
-struct S { s: u32 }
-fn x(a: S) {
-    let S { ref s } = a;
-}
-",
-        )
-    }
-
-    #[test]
-    fn missing_semicolon() {
-        check_diagnostics(
-            r#"
-                fn test() -> i32 { 123; }
-                                 //^^^ Remove this semicolon
-            "#,
-        );
-    }
-
     #[test]
     fn import_extern_crate_clash_with_inner_item() {
         // This is more of a resolver test, but doesn't really work with the hir_def testsuite.