Merge #8345

8345: Add pub mod option for UnlinkedFile r=rainy-me a=rainy-me

close #8228

This is a draft that changes `Diagnostic` to contain multiple fixes. Pre analysis is in https://github.com/rust-analyzer/rust-analyzer/issues/8228#issuecomment-812887085 Because this solution is straightforward so I decided to type it out for discussion.

Currently the `check_fix` is not able to test the situation when multiple fixes available. <del>Also because `Insert 'mod x;'` and  `Insert 'pub mod x;'` are so similar, I don't know how to test them correctly and want some suggestions.</del>. I added 
 `check_fixes` to allow checking mutiple possible fixes.

In additional, instead of append after possible existing `mod y`, I think it's possible to Insert `pub mod x;` after `pub mod y`. Should I implement this too?

Co-authored-by: rainy-me <[email protected]>

10 files changed, 115 insertions, 99 deletions

diff --git a/crates/ide/src/diagnostics/field_shorthand.rs b/crates/ide/src/diagnostics/field_shorthand.rs
index 2b1787f9b..01bd2dba6 100644
--- a/crates/ide/src/diagnostics/field_shorthand.rs
+++ b/crates/ide/src/diagnostics/field_shorthand.rs
@@ -46,14 +46,13 @@ fn check_expr_field_shorthand(
 
         let field_range = record_field.syntax().text_range();
         acc.push(
-            Diagnostic::hint(field_range, "Shorthand struct initialization".to_string()).with_fix(
-                Some(fix(
+            Diagnostic::hint(field_range, "Shorthand struct initialization".to_string())
+                .with_fixes(Some(vec![fix(
                     "use_expr_field_shorthand",
                     "Use struct shorthand initialization",
                     SourceChange::from_text_edit(file_id, edit),
                     field_range,
-                )),
-            ),
+                )])),
         );
     }
 }
@@ -86,13 +85,13 @@ fn check_pat_field_shorthand(
         let edit = edit_builder.finish();
 
         let field_range = record_pat_field.syntax().text_range();
-        acc.push(Diagnostic::hint(field_range, "Shorthand struct pattern".to_string()).with_fix(
-            Some(fix(
+        acc.push(Diagnostic::hint(field_range, "Shorthand struct pattern".to_string()).with_fixes(
+            Some(vec![fix(
                 "use_pat_field_shorthand",
                 "Use struct field shorthand",
                 SourceChange::from_text_edit(file_id, edit),
                 field_range,
-            )),
+            )]),
         ));
     }
 }
diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs
index 92b3f5a2d..258ac6974 100644
--- a/crates/ide/src/diagnostics/fixes.rs
+++ b/crates/ide/src/diagnostics/fixes.rs
@@ -14,18 +14,18 @@ use ide_db::RootDatabase;
 
 use crate::Assist;
 
-/// A [Diagnostic] that potentially has a fix available.
+/// A [Diagnostic] that potentially has some fixes available.
 ///
 /// [Diagnostic]: hir::diagnostics::Diagnostic
-pub(crate) trait DiagnosticWithFix: Diagnostic {
+pub(crate) trait DiagnosticWithFixes: Diagnostic {
     /// `resolve` determines if the diagnostic should fill in the `edit` field
     /// of the assist.
     ///
     /// If `resolve` is false, the edit will be computed later, on demand, and
     /// can be omitted.
-    fn fix(
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist>;
+    ) -> Option<Vec<Assist>>;
 }
diff --git a/crates/ide/src/diagnostics/fixes/change_case.rs b/crates/ide/src/diagnostics/fixes/change_case.rs
index 80aca58a1..42be3375f 100644
--- a/crates/ide/src/diagnostics/fixes/change_case.rs
+++ b/crates/ide/src/diagnostics/fixes/change_case.rs
@@ -4,16 +4,16 @@ use ide_db::{base_db::FilePosition, RootDatabase};
 use syntax::AstNode;
 
 use crate::{
-    diagnostics::{unresolved_fix, DiagnosticWithFix},
+    diagnostics::{unresolved_fix, DiagnosticWithFixes},
     references::rename::rename_with_semantics,
 };
 
-impl DiagnosticWithFix for IncorrectCase {
-    fn fix(
+impl DiagnosticWithFixes for IncorrectCase {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
         let name_node = self.ident.to_node(&root);
 
@@ -28,7 +28,7 @@ impl DiagnosticWithFix for IncorrectCase {
             res.source_change = Some(source_change.ok().unwrap_or_default());
         }
 
-        Some(res)
+        Some(vec![res])
     }
 }
 
diff --git a/crates/ide/src/diagnostics/fixes/create_field.rs b/crates/ide/src/diagnostics/fixes/create_field.rs
index 24e0fda52..a5f457dce 100644
--- a/crates/ide/src/diagnostics/fixes/create_field.rs
+++ b/crates/ide/src/diagnostics/fixes/create_field.rs
@@ -7,18 +7,17 @@ use syntax::{
 use text_edit::TextEdit;
 
 use crate::{
-    diagnostics::{fix, DiagnosticWithFix},
+    diagnostics::{fix, DiagnosticWithFixes},
     Assist, AssistResolveStrategy,
 };
-
-impl DiagnosticWithFix for NoSuchField {
-    fn fix(
+impl DiagnosticWithFixes for NoSuchField {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
-        missing_record_expr_field_fix(
+        missing_record_expr_field_fixes(
             &sema,
             self.file.original_file(sema.db),
             &self.field.to_node(&root),
@@ -26,11 +25,11 @@ impl DiagnosticWithFix for NoSuchField {
     }
 }
 
-fn missing_record_expr_field_fix(
+fn missing_record_expr_field_fixes(
     sema: &Semantics<RootDatabase>,
     usage_file_id: FileId,
     record_expr_field: &ast::RecordExprField,
-) -> Option<Assist> {
+) -> Option<Vec<Assist>> {
     let record_lit = ast::RecordExpr::cast(record_expr_field.syntax().parent()?.parent()?)?;
     let def_id = sema.resolve_variant(record_lit)?;
     let module;
@@ -89,12 +88,12 @@ fn missing_record_expr_field_fix(
         TextEdit::insert(last_field_syntax.text_range().end(), new_field),
     );
 
-    return Some(fix(
+    return Some(vec![fix(
         "create_field",
         "Create field",
         source_change,
         record_expr_field.syntax().text_range(),
-    ));
+    )]);
 
     fn record_field_list(field_def_list: ast::FieldList) -> Option<ast::RecordFieldList> {
         match field_def_list {
diff --git a/crates/ide/src/diagnostics/fixes/fill_missing_fields.rs b/crates/ide/src/diagnostics/fixes/fill_missing_fields.rs
index 37a0e37a9..b5dd64c08 100644
--- a/crates/ide/src/diagnostics/fixes/fill_missing_fields.rs
+++ b/crates/ide/src/diagnostics/fixes/fill_missing_fields.rs
@@ -5,16 +5,16 @@ use syntax::{algo, ast::make, AstNode};
 use text_edit::TextEdit;
 
 use crate::{
-    diagnostics::{fix, fixes::DiagnosticWithFix},
+    diagnostics::{fix, fixes::DiagnosticWithFixes},
     Assist,
 };
 
-impl DiagnosticWithFix for MissingFields {
-    fn fix(
+impl DiagnosticWithFixes for MissingFields {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         // Note that although we could add a diagnostics to
         // fill the missing tuple field, e.g :
         // `struct A(usize);`
@@ -41,12 +41,12 @@ impl DiagnosticWithFix for MissingFields {
                 .into_text_edit(&mut builder);
             builder.finish()
         };
-        Some(fix(
+        Some(vec![fix(
             "fill_missing_fields",
             "Fill struct fields",
             SourceChange::from_text_edit(self.file.original_file(sema.db), edit),
             sema.original_range(&field_list_parent.syntax()).range,
-        ))
+        )])
     }
 }
 
diff --git a/crates/ide/src/diagnostics/fixes/remove_semicolon.rs b/crates/ide/src/diagnostics/fixes/remove_semicolon.rs
index 45471da41..f1724d479 100644
--- a/crates/ide/src/diagnostics/fixes/remove_semicolon.rs
+++ b/crates/ide/src/diagnostics/fixes/remove_semicolon.rs
@@ -4,14 +4,14 @@ use ide_db::{source_change::SourceChange, RootDatabase};
 use syntax::{ast, AstNode};
 use text_edit::TextEdit;
 
-use crate::diagnostics::{fix, DiagnosticWithFix};
+use crate::diagnostics::{fix, DiagnosticWithFixes};
 
-impl DiagnosticWithFix for RemoveThisSemicolon {
-    fn fix(
+impl DiagnosticWithFixes for RemoveThisSemicolon {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
 
         let semicolon = self
@@ -26,7 +26,7 @@ impl DiagnosticWithFix for RemoveThisSemicolon {
         let edit = TextEdit::delete(semicolon);
         let source_change = SourceChange::from_text_edit(self.file.original_file(sema.db), edit);
 
-        Some(fix("remove_semicolon", "Remove this semicolon", source_change, semicolon))
+        Some(vec![fix("remove_semicolon", "Remove this semicolon", source_change, semicolon)])
     }
 }
 
diff --git a/crates/ide/src/diagnostics/fixes/replace_with_find_map.rs b/crates/ide/src/diagnostics/fixes/replace_with_find_map.rs
index b0ef7b44a..444bf563b 100644
--- a/crates/ide/src/diagnostics/fixes/replace_with_find_map.rs
+++ b/crates/ide/src/diagnostics/fixes/replace_with_find_map.rs
@@ -7,14 +7,14 @@ use syntax::{
 };
 use text_edit::TextEdit;
 
-use crate::diagnostics::{fix, DiagnosticWithFix};
+use crate::diagnostics::{fix, DiagnosticWithFixes};
 
-impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap {
-    fn fix(
+impl DiagnosticWithFixes for ReplaceFilterMapNextWithFindMap {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
         let next_expr = self.next_expr.to_node(&root);
         let next_call = ast::MethodCallExpr::cast(next_expr.syntax().clone())?;
@@ -32,12 +32,12 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap {
 
         let source_change = SourceChange::from_text_edit(self.file.original_file(sema.db), edit);
 
-        Some(fix(
+        Some(vec![fix(
             "replace_with_find_map",
             "Replace filter_map(..).next() with find_map()",
             source_change,
             trigger_range,
-        ))
+        )])
     }
 }
 
diff --git a/crates/ide/src/diagnostics/fixes/unresolved_module.rs b/crates/ide/src/diagnostics/fixes/unresolved_module.rs
index 81244b293..b3d0283bb 100644
--- a/crates/ide/src/diagnostics/fixes/unresolved_module.rs
+++ b/crates/ide/src/diagnostics/fixes/unresolved_module.rs
@@ -3,17 +3,17 @@ use ide_assists::{Assist, AssistResolveStrategy};
 use ide_db::{base_db::AnchoredPathBuf, source_change::FileSystemEdit, RootDatabase};
 use syntax::AstNode;
 
-use crate::diagnostics::{fix, DiagnosticWithFix};
+use crate::diagnostics::{fix, DiagnosticWithFixes};
 
-impl DiagnosticWithFix for UnresolvedModule {
-    fn fix(
+impl DiagnosticWithFixes for UnresolvedModule {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
         let unresolved_module = self.decl.to_node(&root);
-        Some(fix(
+        Some(vec![fix(
             "create_module",
             "Create module",
             FileSystemEdit::CreateFile {
@@ -25,7 +25,7 @@ impl DiagnosticWithFix for UnresolvedModule {
             }
             .into(),
             unresolved_module.syntax().text_range(),
-        ))
+        )])
     }
 }
 
@@ -45,33 +45,35 @@ mod tests {
                         message: "unresolved module",
                         range: 0..8,
                         severity: Error,
-                        fix: Some(
-                            Assist {
-                                id: AssistId(
-                                    "create_module",
-                                    QuickFix,
-                                ),
-                                label: "Create module",
-                                group: None,
-                                target: 0..8,
-                                source_change: Some(
-                                    SourceChange {
-                                        source_file_edits: {},
-                                        file_system_edits: [
-                                            CreateFile {
-                                                dst: AnchoredPathBuf {
-                                                    anchor: FileId(
-                                                        0,
-                                                    ),
-                                                    path: "foo.rs",
+                        fixes: Some(
+                            [
+                                Assist {
+                                    id: AssistId(
+                                        "create_module",
+                                        QuickFix,
+                                    ),
+                                    label: "Create module",
+                                    group: None,
+                                    target: 0..8,
+                                    source_change: Some(
+                                        SourceChange {
+                                            source_file_edits: {},
+                                            file_system_edits: [
+                                                CreateFile {
+                                                    dst: AnchoredPathBuf {
+                                                        anchor: FileId(
+                                                            0,
+                                                        ),
+                                                        path: "foo.rs",
+                                                    },
+                                                    initial_contents: "",
                                                 },
-                                                initial_contents: "",
-                                            },
-                                        ],
-                                        is_snippet: false,
-                                    },
-                                ),
-                            },
+                                            ],
+                                            is_snippet: false,
+                                        },
+                                    ),
+                                },
+                            ],
                         ),
                         unused: false,
                         code: Some(
diff --git a/crates/ide/src/diagnostics/fixes/wrap_tail_expr.rs b/crates/ide/src/diagnostics/fixes/wrap_tail_expr.rs
index 66676064a..715a403b9 100644
--- a/crates/ide/src/diagnostics/fixes/wrap_tail_expr.rs
+++ b/crates/ide/src/diagnostics/fixes/wrap_tail_expr.rs
@@ -4,14 +4,14 @@ use ide_db::{source_change::SourceChange, RootDatabase};
 use syntax::AstNode;
 use text_edit::TextEdit;
 
-use crate::diagnostics::{fix, DiagnosticWithFix};
+use crate::diagnostics::{fix, DiagnosticWithFixes};
 
-impl DiagnosticWithFix for MissingOkOrSomeInTailExpr {
-    fn fix(
+impl DiagnosticWithFixes for MissingOkOrSomeInTailExpr {
+    fn fixes(
         &self,
         sema: &Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
+    ) -> Option<Vec<Assist>> {
         let root = sema.db.parse_or_expand(self.file)?;
         let tail_expr = self.expr.to_node(&root);
         let tail_expr_range = tail_expr.syntax().text_range();
@@ -19,7 +19,7 @@ impl DiagnosticWithFix for MissingOkOrSomeInTailExpr {
         let edit = TextEdit::replace(tail_expr_range, replacement);
         let source_change = SourceChange::from_text_edit(self.file.original_file(sema.db), edit);
         let name = if self.required == "Ok" { "Wrap with Ok" } else { "Wrap with Some" };
-        Some(fix("wrap_tail_expr", name, source_change, tail_expr_range))
+        Some(vec![fix("wrap_tail_expr", name, source_change, tail_expr_range)])
     }
 }
 
diff --git a/crates/ide/src/diagnostics/unlinked_file.rs b/crates/ide/src/diagnostics/unlinked_file.rs
index 93fd25dea..51fe0f360 100644
--- a/crates/ide/src/diagnostics/unlinked_file.rs
+++ b/crates/ide/src/diagnostics/unlinked_file.rs
@@ -18,7 +18,7 @@ use syntax::{
 use text_edit::TextEdit;
 
 use crate::{
-    diagnostics::{fix, fixes::DiagnosticWithFix},
+    diagnostics::{fix, fixes::DiagnosticWithFixes},
     Assist,
 };
 
@@ -50,13 +50,13 @@ impl Diagnostic for UnlinkedFile {
     }
 }
 
-impl DiagnosticWithFix for UnlinkedFile {
-    fn fix(
+impl DiagnosticWithFixes for UnlinkedFile {
+    fn fixes(
         &self,
         sema: &hir::Semantics<RootDatabase>,
         _resolve: &AssistResolveStrategy,
-    ) -> Option<Assist> {
-        // If there's an existing module that could add a `mod` item to include the unlinked file,
+    ) -> Option<Vec<Assist>> {
+        // If there's an existing module that could add `mod` or `pub mod` items to include the unlinked file,
         // suggest that as a fix.
 
         let source_root = sema.db.source_root(sema.db.file_source_root(self.file_id));
@@ -90,7 +90,7 @@ impl DiagnosticWithFix for UnlinkedFile {
                         }
 
                         if module.origin.file_id() == Some(*parent_id) {
-                            return make_fix(sema.db, *parent_id, module_name, self.file_id);
+                            return make_fixes(sema.db, *parent_id, module_name, self.file_id);
                         }
                     }
                 }
@@ -101,20 +101,23 @@ impl DiagnosticWithFix for UnlinkedFile {
     }
 }
 
-fn make_fix(
+fn make_fixes(
     db: &RootDatabase,
     parent_file_id: FileId,
     new_mod_name: &str,
     added_file_id: FileId,
-) -> Option<Assist> {
+) -> Option<Vec<Assist>> {
     fn is_outline_mod(item: &ast::Item) -> bool {
         matches!(item, ast::Item::Module(m) if m.item_list().is_none())
     }
 
     let mod_decl = format!("mod {};", new_mod_name);
+    let pub_mod_decl = format!("pub mod {};", new_mod_name);
+
     let ast: ast::SourceFile = db.parse(parent_file_id).tree();
 
-    let mut builder = TextEdit::builder();
+    let mut mod_decl_builder = TextEdit::builder();
+    let mut pub_mod_decl_builder = TextEdit::builder();
 
     // If there's an existing `mod m;` statement matching the new one, don't emit a fix (it's
     // probably `#[cfg]`d out).
@@ -138,30 +141,43 @@ fn make_fix(
     {
         Some(last) => {
             cov_mark::hit!(unlinked_file_append_to_existing_mods);
-            builder.insert(last.syntax().text_range().end(), format!("\n{}", mod_decl));
+            let offset = last.syntax().text_range().end();
+            mod_decl_builder.insert(offset, format!("\n{}", mod_decl));
+            pub_mod_decl_builder.insert(offset, format!("\n{}", pub_mod_decl));
         }
         None => {
             // Prepend before the first item in the file.
             match ast.items().next() {
                 Some(item) => {
                     cov_mark::hit!(unlinked_file_prepend_before_first_item);
-                    builder.insert(item.syntax().text_range().start(), format!("{}\n\n", mod_decl));
+                    let offset = item.syntax().text_range().start();
+                    mod_decl_builder.insert(offset, format!("{}\n\n", mod_decl));
+                    pub_mod_decl_builder.insert(offset, format!("{}\n\n", pub_mod_decl));
                 }
                 None => {
                     // No items in the file, so just append at the end.
                     cov_mark::hit!(unlinked_file_empty_file);
-                    builder.insert(ast.syntax().text_range().end(), format!("{}\n", mod_decl));
+                    let offset = ast.syntax().text_range().end();
+                    mod_decl_builder.insert(offset, format!("{}\n", mod_decl));
+                    pub_mod_decl_builder.insert(offset, format!("{}\n", pub_mod_decl));
                 }
             }
         }
     }
 
-    let edit = builder.finish();
     let trigger_range = db.parse(added_file_id).tree().syntax().text_range();
-    Some(fix(
-        "add_mod_declaration",
-        &format!("Insert `{}`", mod_decl),
-        SourceChange::from_text_edit(parent_file_id, edit),
-        trigger_range,
-    ))
+    Some(vec![
+        fix(
+            "add_mod_declaration",
+            &format!("Insert `{}`", mod_decl),
+            SourceChange::from_text_edit(parent_file_id, mod_decl_builder.finish()),
+            trigger_range,
+        ),
+        fix(
+            "add_pub_mod_declaration",
+            &format!("Insert `{}`", pub_mod_decl),
+            SourceChange::from_text_edit(parent_file_id, pub_mod_decl_builder.finish()),
+            trigger_range,
+        ),
+    ])
 }