Merge pull request #19869 from MatrixFrog/publicize_field

Add a quickfix for accessing a private field of a struct

6 files changed, 138 insertions, 182 deletions

diff --git a/src/tools/rust-analyzer/crates/hir-ty/src/infer.rs b/src/tools/rust-analyzer/crates/hir-ty/src/infer.rs
index 840a916c24a..14eb7160753 100644
--- a/src/tools/rust-analyzer/crates/hir-ty/src/infer.rs
+++ b/src/tools/rust-analyzer/crates/hir-ty/src/infer.rs
@@ -35,7 +35,8 @@ use chalk_ir::{
 use either::Either;
 use hir_def::{
     AdtId, AssocItemId, ConstId, DefWithBodyId, FieldId, FunctionId, GenericDefId, GenericParamId,
-    ImplId, ItemContainerId, Lookup, TraitId, TupleFieldId, TupleId, TypeAliasId, VariantId,
+    ImplId, ItemContainerId, LocalFieldId, Lookup, TraitId, TupleFieldId, TupleId, TypeAliasId,
+    VariantId,
     builtin_type::{BuiltinInt, BuiltinType, BuiltinUint},
     expr_store::{Body, ExpressionStore, HygieneId, path::Path},
     hir::{BindingAnnotation, BindingId, ExprId, ExprOrPatId, LabelId, PatId},
@@ -207,7 +208,7 @@ pub(crate) type InferResult<T> = Result<InferOk<T>, TypeError>;
 pub enum InferenceDiagnostic {
     NoSuchField {
         field: ExprOrPatId,
-        private: bool,
+        private: Option<LocalFieldId>,
         variant: VariantId,
     },
     PrivateField {
diff --git a/src/tools/rust-analyzer/crates/hir-ty/src/infer/expr.rs b/src/tools/rust-analyzer/crates/hir-ty/src/infer/expr.rs
index 87b7f3406ff..64031279296 100644
--- a/src/tools/rust-analyzer/crates/hir-ty/src/infer/expr.rs
+++ b/src/tools/rust-analyzer/crates/hir-ty/src/infer/expr.rs
@@ -554,7 +554,7 @@ impl InferenceContext<'_> {
                                             self.push_diagnostic(
                                                 InferenceDiagnostic::NoSuchField {
                                                     field: field.expr.into(),
-                                                    private: true,
+                                                    private: Some(local_id),
                                                     variant: def,
                                                 },
                                             );
@@ -564,7 +564,7 @@ impl InferenceContext<'_> {
                                     None => {
                                         self.push_diagnostic(InferenceDiagnostic::NoSuchField {
                                             field: field.expr.into(),
-                                            private: false,
+                                            private: None,
                                             variant: def,
                                         });
                                         None
diff --git a/src/tools/rust-analyzer/crates/hir-ty/src/infer/pat.rs b/src/tools/rust-analyzer/crates/hir-ty/src/infer/pat.rs
index a9a3265858e..4bc3e167ebf 100644
--- a/src/tools/rust-analyzer/crates/hir-ty/src/infer/pat.rs
+++ b/src/tools/rust-analyzer/crates/hir-ty/src/infer/pat.rs
@@ -143,7 +143,7 @@ impl InferenceContext<'_> {
                                 {
                                     self.push_diagnostic(InferenceDiagnostic::NoSuchField {
                                         field: inner.into(),
-                                        private: true,
+                                        private: Some(local_id),
                                         variant: def,
                                     });
                                 }
@@ -157,7 +157,7 @@ impl InferenceContext<'_> {
                             None => {
                                 self.push_diagnostic(InferenceDiagnostic::NoSuchField {
                                     field: inner.into(),
-                                    private: false,
+                                    private: None,
                                     variant: def,
                                 });
                                 self.err_ty()
diff --git a/src/tools/rust-analyzer/crates/hir/src/diagnostics.rs b/src/tools/rust-analyzer/crates/hir/src/diagnostics.rs
index b6e3002ed5d..f7b140e03d4 100644
--- a/src/tools/rust-analyzer/crates/hir/src/diagnostics.rs
+++ b/src/tools/rust-analyzer/crates/hir/src/diagnostics.rs
@@ -224,7 +224,7 @@ pub struct MalformedDerive {
 #[derive(Debug)]
 pub struct NoSuchField {
     pub field: InFile<AstPtr<Either<ast::RecordExprField, ast::RecordPatField>>>,
-    pub private: bool,
+    pub private: Option<Field>,
     pub variant: VariantId,
 }
 
@@ -648,6 +648,7 @@ impl AnyDiagnostic {
                     }
                     ExprOrPatId::PatId(pat) => source_map.pat_field_syntax(pat),
                 };
+                let private = private.map(|id| Field { id, parent: variant.into() });
                 NoSuchField { field: expr_or_pat, private, variant }.into()
             }
             &InferenceDiagnostic::MismatchedArgCount { call_expr, expected, found } => {
diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/fix_visibility.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/fix_visibility.rs
index 19e0a73f333..3badc17d01a 100644
--- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/fix_visibility.rs
+++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/fix_visibility.rs
@@ -7,10 +7,10 @@ use syntax::{
 
 use crate::{AssistContext, AssistId, Assists};
 
-// FIXME: this really should be a fix for diagnostic, rather than an assist.
-
 // Assist: fix_visibility
 //
+// Note that there is some duplication between this and the no_such_field diagnostic.
+//
 // Makes inaccessible item public.
 //
 // ```
@@ -32,7 +32,6 @@ use crate::{AssistContext, AssistId, Assists};
 // ```
 pub(crate) fn fix_visibility(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {
     add_vis_to_referenced_module_def(acc, ctx)
-        .or_else(|| add_vis_to_referenced_record_field(acc, ctx))
 }
 
 fn add_vis_to_referenced_module_def(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {
@@ -88,59 +87,6 @@ fn add_vis_to_referenced_module_def(acc: &mut Assists, ctx: &AssistContext<'_>)
     })
 }
 
-fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {
-    let record_field: ast::RecordExprField = ctx.find_node_at_offset()?;
-    let (record_field_def, _, _) = ctx.sema.resolve_record_field(&record_field)?;
-
-    let current_module = ctx.sema.scope(record_field.syntax())?.module();
-    let current_edition = current_module.krate().edition(ctx.db());
-    let visibility = record_field_def.visibility(ctx.db());
-    if visibility.is_visible_from(ctx.db(), current_module.into()) {
-        return None;
-    }
-
-    let parent = record_field_def.parent_def(ctx.db());
-    let parent_name = parent.name(ctx.db());
-    let target_module = parent.module(ctx.db());
-
-    let in_file_source = record_field_def.source(ctx.db())?;
-    let (vis_owner, target) = match in_file_source.value {
-        hir::FieldSource::Named(it) => {
-            let range = it.syntax().text_range();
-            (ast::AnyHasVisibility::new(it), range)
-        }
-        hir::FieldSource::Pos(it) => {
-            let range = it.syntax().text_range();
-            (ast::AnyHasVisibility::new(it), range)
-        }
-    };
-
-    let missing_visibility = if current_module.krate() == target_module.krate() {
-        make::visibility_pub_crate()
-    } else {
-        make::visibility_pub()
-    };
-    let target_file = in_file_source.file_id.original_file(ctx.db());
-
-    let target_name = record_field_def.name(ctx.db());
-    let assist_label = format!(
-        "Change visibility of {}.{} to {missing_visibility}",
-        parent_name.display(ctx.db(), current_edition),
-        target_name.display(ctx.db(), current_edition)
-    );
-
-    acc.add(AssistId::quick_fix("fix_visibility"), assist_label, target, |edit| {
-        edit.edit_file(target_file.file_id(ctx.db()));
-
-        let vis_owner = edit.make_mut(vis_owner);
-        vis_owner.set_visibility(Some(missing_visibility.clone_for_update()));
-
-        if let Some((cap, vis)) = ctx.config.snippet_cap.zip(vis_owner.visibility()) {
-            edit.add_tabstop_before(cap, vis);
-        }
-    })
-}
-
 fn target_data_for_def(
     db: &dyn HirDatabase,
     def: hir::ModuleDef,
@@ -294,44 +240,6 @@ struct Foo;
     }
 
     #[test]
-    fn fix_visibility_of_struct_field() {
-        check_assist(
-            fix_visibility,
-            r"mod foo { pub struct Foo { bar: (), } }
-              fn main() { foo::Foo { $0bar: () }; } ",
-            r"mod foo { pub struct Foo { $0pub(crate) bar: (), } }
-              fn main() { foo::Foo { bar: () }; } ",
-        );
-        check_assist(
-            fix_visibility,
-            r"
-//- /lib.rs
-mod foo;
-fn main() { foo::Foo { $0bar: () }; }
-//- /foo.rs
-pub struct Foo { bar: () }
-",
-            r"pub struct Foo { $0pub(crate) bar: () }
-",
-        );
-        check_assist_not_applicable(
-            fix_visibility,
-            r"mod foo { pub struct Foo { pub bar: (), } }
-              fn main() { foo::Foo { $0bar: () }; } ",
-        );
-        check_assist_not_applicable(
-            fix_visibility,
-            r"
-//- /lib.rs
-mod foo;
-fn main() { foo::Foo { $0bar: () }; }
-//- /foo.rs
-pub struct Foo { pub bar: () }
-",
-        );
-    }
-
-    #[test]
     fn fix_visibility_of_enum_variant_field() {
         // Enum variants, as well as their fields, always get the enum's visibility. In fact, rustc
         // rejects any visibility specifiers on them, so this assist should never fire on them.
@@ -368,44 +276,6 @@ pub struct Foo { pub bar: () }
     }
 
     #[test]
-    fn fix_visibility_of_union_field() {
-        check_assist(
-            fix_visibility,
-            r"mod foo { pub union Foo { bar: (), } }
-              fn main() { foo::Foo { $0bar: () }; } ",
-            r"mod foo { pub union Foo { $0pub(crate) bar: (), } }
-              fn main() { foo::Foo { bar: () }; } ",
-        );
-        check_assist(
-            fix_visibility,
-            r"
-//- /lib.rs
-mod foo;
-fn main() { foo::Foo { $0bar: () }; }
-//- /foo.rs
-pub union Foo { bar: () }
-",
-            r"pub union Foo { $0pub(crate) bar: () }
-",
-        );
-        check_assist_not_applicable(
-            fix_visibility,
-            r"mod foo { pub union Foo { pub bar: (), } }
-              fn main() { foo::Foo { $0bar: () }; } ",
-        );
-        check_assist_not_applicable(
-            fix_visibility,
-            r"
-//- /lib.rs
-mod foo;
-fn main() { foo::Foo { $0bar: () }; }
-//- /foo.rs
-pub union Foo { pub bar: () }
-",
-        );
-    }
-
-    #[test]
     fn fix_visibility_of_const() {
         check_assist(
             fix_visibility,
@@ -572,19 +442,6 @@ pub(crate) struct Bar;
             r"$0pub struct Bar;
 ",
         );
-        check_assist(
-            fix_visibility,
-            r"
-//- /main.rs crate:a deps:foo
-fn main() {
-    foo::Foo { $0bar: () };
-}
-//- /lib.rs crate:foo
-pub struct Foo { pub(crate) bar: () }
-",
-            r"pub struct Foo { $0pub bar: () }
-",
-        );
     }
 
     #[test]
diff --git a/src/tools/rust-analyzer/crates/ide-diagnostics/src/handlers/no_such_field.rs b/src/tools/rust-analyzer/crates/ide-diagnostics/src/handlers/no_such_field.rs
index 84fb467a5ce..ef42f2dc744 100644
--- a/src/tools/rust-analyzer/crates/ide-diagnostics/src/handlers/no_such_field.rs
+++ b/src/tools/rust-analyzer/crates/ide-diagnostics/src/handlers/no_such_field.rs
@@ -1,4 +1,5 @@
 use either::Either;
+use hir::{Field, HasCrate};
 use hir::{HasSource, HirDisplay, Semantics, VariantId, db::ExpandDatabase};
 use ide_db::text_edit::TextEdit;
 use ide_db::{EditionedFileId, RootDatabase, source_change::SourceChange};
@@ -13,44 +14,69 @@ use crate::{Assist, Diagnostic, DiagnosticCode, DiagnosticsContext, fix};
 //
 // This diagnostic is triggered if created structure does not have field provided in record.
 pub(crate) fn no_such_field(ctx: &DiagnosticsContext<'_>, d: &hir::NoSuchField) -> Diagnostic {
-    let node = d.field.map(Into::into);
-    if d.private {
-        // FIXME: quickfix to add required visibility
-        Diagnostic::new_with_syntax_node_ptr(
-            ctx,
-            DiagnosticCode::RustcHardError("E0451"),
-            "field is private",
-            node,
-        )
-        .stable()
+    let (code, message) = if d.private.is_some() {
+        ("E0451", "field is private")
+    } else if let VariantId::EnumVariantId(_) = d.variant {
+        ("E0559", "no such field")
     } else {
-        Diagnostic::new_with_syntax_node_ptr(
-            ctx,
-            match d.variant {
-                VariantId::EnumVariantId(_) => DiagnosticCode::RustcHardError("E0559"),
-                _ => DiagnosticCode::RustcHardError("E0560"),
-            },
-            "no such field",
-            node,
-        )
+        ("E0560", "no such field")
+    };
+
+    let node = d.field.map(Into::into);
+    Diagnostic::new_with_syntax_node_ptr(ctx, DiagnosticCode::RustcHardError(code), message, node)
         .stable()
         .with_fixes(fixes(ctx, d))
-    }
 }
 
 fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::NoSuchField) -> Option<Vec<Assist>> {
     // FIXME: quickfix for pattern
     let root = ctx.sema.db.parse_or_expand(d.field.file_id);
     match &d.field.value.to_node(&root) {
-        Either::Left(node) => missing_record_expr_field_fixes(
-            &ctx.sema,
-            d.field.file_id.original_file(ctx.sema.db),
-            node,
-        ),
+        Either::Left(node) => {
+            if let Some(private_field) = d.private {
+                field_is_private_fixes(
+                    &ctx.sema,
+                    d.field.file_id.original_file(ctx.sema.db),
+                    node,
+                    private_field,
+                )
+            } else {
+                missing_record_expr_field_fixes(
+                    &ctx.sema,
+                    d.field.file_id.original_file(ctx.sema.db),
+                    node,
+                )
+            }
+        }
         _ => None,
     }
 }
 
+fn field_is_private_fixes(
+    sema: &Semantics<'_, RootDatabase>,
+    usage_file_id: EditionedFileId,
+    record_expr_field: &ast::RecordExprField,
+    private_field: Field,
+) -> Option<Vec<Assist>> {
+    let def_crate = private_field.krate(sema.db);
+    let usage_crate = sema.file_to_module_def(usage_file_id.file_id(sema.db))?.krate();
+    let visibility = if usage_crate == def_crate { "pub(crate) " } else { "pub " };
+
+    let source = private_field.source(sema.db)?;
+    let (range, _) = source.syntax().original_file_range_opt(sema.db)?;
+    let source_change = SourceChange::from_text_edit(
+        range.file_id.file_id(sema.db),
+        TextEdit::insert(range.range.start(), visibility.into()),
+    );
+
+    Some(vec![fix(
+        "increase_field_visibility",
+        "Increase field visibility",
+        source_change,
+        sema.original_range(record_expr_field.syntax()).range,
+    )])
+}
+
 fn missing_record_expr_field_fixes(
     sema: &Semantics<'_, RootDatabase>,
     usage_file_id: EditionedFileId,
@@ -118,7 +144,7 @@ fn missing_record_expr_field_fixes(
         "create_field",
         "Create field",
         source_change,
-        record_expr_field.syntax().text_range(),
+        sema.original_range(record_expr_field.syntax()).range,
     )]);
 
     fn record_field_list(field_def_list: ast::FieldList) -> Option<ast::RecordFieldList> {
@@ -387,15 +413,15 @@ fn f(s@m::Struct {
     // assignee expression
     m::Struct {
         field: 0,
-      //^^^^^^^^ error: field is private
+      //^^^^^^^^ 💡 error: field is private
         field2
-      //^^^^^^ error: field is private
+      //^^^^^^ 💡 error: field is private
     } = s;
     m::Struct {
         field: 0,
-      //^^^^^^^^ error: field is private
+      //^^^^^^^^ 💡 error: field is private
         field2
-      //^^^^^^ error: field is private
+      //^^^^^^ 💡 error: field is private
     };
 }
 "#,
@@ -403,6 +429,77 @@ fn f(s@m::Struct {
     }
 
     #[test]
+    fn test_struct_field_private_same_crate_fix() {
+        check_diagnostics(
+            r#"
+mod m {
+    pub struct Struct {
+        field: u32,
+    }
+}
+fn f() {
+    let _ = m::Struct {
+        field: 0,
+      //^^^^^^^^ 💡 error: field is private
+    };
+}
+"#,
+        );
+
+        check_fix(
+            r#"
+mod m {
+    pub struct Struct {
+        field: u32,
+    }
+}
+fn f() {
+    let _ = m::Struct {
+        field$0: 0,
+    };
+}
+"#,
+            r#"
+mod m {
+    pub struct Struct {
+        pub(crate) field: u32,
+    }
+}
+fn f() {
+    let _ = m::Struct {
+        field: 0,
+    };
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn test_struct_field_private_other_crate_fix() {
+        check_fix(
+            r#"
+//- /lib.rs crate:another_crate
+pub struct Struct {
+    field: u32,
+}
+//- /lib.rs crate:this_crate deps:another_crate
+use another_crate;
+
+fn f() {
+    let _ = another_crate::Struct {
+        field$0: 0,
+    };
+}
+"#,
+            r#"
+pub struct Struct {
+    pub field: u32,
+}
+"#,
+        );
+    }
+
+    #[test]
     fn editions_between_macros() {
         check_diagnostics(
             r#"