Merge pull request #344 from mkhan45/struct_private_leak

Feat: Added private type leak validation to structs

crates/mun_hir/src/code_model.rs

@@ -2,7 +2,7 @@ mod function;
 mod module;
 mod package;
 pub(crate) mod src;
-mod r#struct;
+pub(crate) mod r#struct;
 mod type_alias;
 
 use crate::{expr::BodySourceMap, HirDatabase, Name};

crates/mun_hir/src/code_model/struct.rs

@@ -10,7 +10,7 @@ use crate::{
 };
 use mun_syntax::{
     ast,
-    ast::{NameOwner, TypeAscriptionOwner},
+    ast::{NameOwner, TypeAscriptionOwner, VisibilityOwner},
 };
 use std::{fmt, sync::Arc};
 
@@ -19,6 +19,8 @@ use crate::visibility::RawVisibility;
 pub use ast::StructMemoryKind;
 use std::iter::once;
 
+pub(crate) mod validator;
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct Struct {
     pub(crate) id: StructId,
@@ -125,6 +127,8 @@ impl Struct {
         let data = self.data(db.upcast());
         let lower = self.lower(db);
         lower.add_diagnostics(db, self.file_id(db), data.type_ref_source_map(), sink);
+        let validator = validator::StructValidator::new(self, db, self.file_id(db));
+        validator.validate_privacy(sink);
     }
 }
 
@@ -144,6 +148,7 @@ impl Struct {
 pub struct FieldData {
     pub name: Name,
     pub type_ref: LocalTypeRefId,
+    pub visibility: RawVisibility,
 }
 
 /// A struct's fields' data (record, tuple, or unit struct)
@@ -198,6 +203,7 @@ impl StructData {
                     .map(|fd| FieldData {
                         name: fd.name().map(|n| n.as_name()).unwrap_or_else(Name::missing),
                         type_ref: type_ref_builder.alloc_from_node_opt(fd.ascribed_type().as_ref()),
+                        visibility: RawVisibility::from_ast(fd.visibility()),
                     })
                     .collect();
                 (fields, StructKind::Record)
@@ -209,6 +215,7 @@ impl StructData {
                     .map(|(index, fd)| FieldData {
                         name: Name::new_tuple_field(index),
                         type_ref: type_ref_builder.alloc_from_node_opt(fd.type_ref().as_ref()),
+                        visibility: RawVisibility::from_ast(fd.visibility()),
                     })
                     .collect();
                 (fields, StructKind::Tuple)

crates/mun_hir/src/code_model/struct/validator.rs

@@ -0,0 +1,65 @@
+use super::Struct;
+use crate::diagnostics::ExportedPrivate;
+use crate::resolve::HasResolver;
+use crate::DiagnosticSink;
+use crate::{HasVisibility, Ty, Visibility};
+
+use crate::FileId;
+use crate::HirDatabase;
+
+use crate::visibility::RawVisibility;
+
+#[cfg(test)]
+mod tests;
+
+pub struct StructValidator<'a> {
+    strukt: Struct,
+    db: &'a dyn HirDatabase,
+    file_id: FileId,
+}
+
+impl<'a> StructValidator<'a> {
+    pub fn new(strukt: Struct, db: &'a dyn HirDatabase, file_id: FileId) -> Self {
+        StructValidator {
+            strukt,
+            db,
+            file_id,
+        }
+    }
+
+    pub fn validate_privacy(&self, sink: &mut DiagnosticSink) {
+        let resolver = self.strukt.id.resolver(self.db.upcast());
+        let struct_data = self.strukt.data(self.db.upcast());
+
+        let public_fields = struct_data
+            .fields
+            .iter()
+            .filter(|(_, field_data)| field_data.visibility == RawVisibility::Public);
+
+        let field_types = public_fields.map(|(_, field_data)| {
+            let type_ref = field_data.type_ref;
+            let ty = Ty::from_hir(self.db, &resolver, &struct_data.type_ref_map(), type_ref).ty;
+            (ty, type_ref)
+        });
+
+        let struct_visibility = self.strukt.visibility(self.db);
+        let type_is_allowed = |ty: &Ty| match struct_visibility {
+            Visibility::Module(module_id) => {
+                ty.visibility(self.db).is_visible_from(self.db, module_id)
+            }
+            Visibility::Public => ty.visibility(self.db).is_externally_visible(),
+        };
+
+        field_types
+            .filter(|(ty, _)| !type_is_allowed(&ty))
+            .for_each(|(_, type_ref)| {
+                sink.push(ExportedPrivate {
+                    file: self.file_id,
+                    type_ref: struct_data
+                        .type_ref_source_map()
+                        .type_ref_syntax(type_ref)
+                        .unwrap(),
+                })
+            });
+    }
+}

crates/mun_hir/src/code_model/struct/validator/snapshots/mun_hir__code_model__r#struct__validator__tests__private_leak_struct_fields.snap

@@ -0,0 +1,8 @@
+---
+source: crates/mun_hir/src/code_model/struct/validator/tests.rs
+expression: "struct Foo(usize);\npub struct Bar(usize);\n\n// valid, bar is public\npub struct Baz {\n    foo: Foo,\n    pub bar: Bar,\n}\n\n// invalid, Foo is private\npub struct FooBar {\n    pub foo: Foo,\n    pub bar: Bar,\n}\n\n// valid, FooBaz is private\nstruct FooBaz {\n    pub foo: Foo,\n    pub bar: Bar,\n}\n\npub(crate) struct BarBaz;\n\n// invalid, exporting pub(crate) to pub\npub struct FooBarBaz {\n    pub foo: Foo,\n    pub bar: Bar,\n}"
+
+---
+179..182: can't leak private type
+391..394: can't leak private type
+

crates/mun_hir/src/code_model/struct/validator/tests.rs

@@ -0,0 +1,47 @@
+#[cfg(test)]
+use crate::utils::tests::*;
+
+#[test]
+fn test_private_leak_struct_fields() {
+    diagnostics_snapshot(
+        r#"
+    
+    struct Foo(usize);
+    pub struct Bar(usize);
+
+    // valid, bar is public
+    pub struct Baz {
+        foo: Foo,
+        pub bar: Bar,
+    }
+
+    // invalid, Foo is private
+    pub struct FooBar {
+        pub foo: Foo,
+        pub bar: Bar,
+    }
+
+    // valid, FooBaz is private
+    struct FooBaz {
+        pub foo: Foo,
+        pub bar: Bar,
+    }
+
+    pub(crate) struct BarBaz;
+
+    // invalid, exporting pub(crate) to pub
+    pub struct FooBarBaz {
+        pub foo: Foo,
+        pub bar: Bar,
+    }
+    "#,
+    )
+}
+
+// this function needs to be declared in each file separately
+// since insta's AutoName creates files in the directory from which
+// the macro is called.
+fn diagnostics_snapshot(text: &str) {
+    let text = text.trim().replace("\n    ", "\n");
+    insta::assert_snapshot!(insta::_macro_support::AutoName, diagnostics(&text), &text);
+}

crates/mun_hir/src/expr/validator/tests.rs

@@ -1,11 +1,5 @@
-use crate::{
-    diagnostics::DiagnosticSink,
-    expr::validator::{ExprValidator, TypeAliasValidator},
-    mock::MockDatabase,
-    with_fixture::WithFixture,
-    ModuleDef, Package,
-};
-use std::fmt::Write;
+#[cfg(test)]
+use crate::utils::tests::*;
 
 #[test]
 fn test_uninitialized_access() {
@@ -161,35 +155,9 @@ fn test_private_leak_alias() {
     )
 }
 
-fn diagnostics(content: &str) -> String {
-    let (db, _file_id) = MockDatabase::with_single_file(content);
-
-    let mut diags = String::new();
-
-    let mut diag_sink = DiagnosticSink::new(|diag| {
-        write!(diags, "{:?}: {}\n", diag.highlight_range(), diag.message()).unwrap();
-    });
-
-    for item in Package::all(&db)
-        .iter()
-        .flat_map(|pkg| pkg.modules(&db))
-        .flat_map(|module| module.declarations(&db))
-    {
-        match item {
-            ModuleDef::Function(item) => {
-                ExprValidator::new(item, &db).validate_body(&mut diag_sink);
-            }
-            ModuleDef::TypeAlias(item) => {
-                TypeAliasValidator::new(item, &db).validate_target_type_existence(&mut diag_sink);
-            }
-            _ => {}
-        }
-    }
-
-    drop(diag_sink);
-    diags
-}
-
+// this function needs to be declared in each file separately
+// since insta's AutoName creates files in the directory from which
+// the macro is called.
 fn diagnostics_snapshot(text: &str) {
     let text = text.trim().replace("\n    ", "\n");
     insta::assert_snapshot!(insta::_macro_support::AutoName, diagnostics(&text), &text);

crates/mun_hir/src/utils.rs

@@ -8,3 +8,50 @@ pub(crate) fn make_mut_slice<T: Clone>(a: &mut Arc<[T]>) -> &mut [T] {
     }
     Arc::get_mut(a).unwrap()
 }
+
+#[cfg(test)]
+pub mod tests {
+    use std::fmt::Write;
+
+    use crate::{
+        code_model::r#struct::validator::StructValidator,
+        diagnostics::DiagnosticSink,
+        expr::validator::{ExprValidator, TypeAliasValidator},
+        mock::MockDatabase,
+        with_fixture::WithFixture,
+        FileId, ModuleDef, Package,
+    };
+
+    pub fn diagnostics(content: &str) -> String {
+        let (db, _file_id) = MockDatabase::with_single_file(content);
+
+        let mut diags = String::new();
+
+        let mut diag_sink = DiagnosticSink::new(|diag| {
+            writeln!(diags, "{:?}: {}", diag.highlight_range(), diag.message()).unwrap();
+        });
+
+        for item in Package::all(&db)
+            .iter()
+            .flat_map(|pkg| pkg.modules(&db))
+            .flat_map(|module| module.declarations(&db))
+        {
+            match item {
+                ModuleDef::Function(item) => {
+                    ExprValidator::new(item, &db).validate_body(&mut diag_sink);
+                }
+                ModuleDef::TypeAlias(item) => {
+                    TypeAliasValidator::new(item, &db)
+                        .validate_target_type_existence(&mut diag_sink);
+                }
+                ModuleDef::Struct(item) => {
+                    StructValidator::new(item, &db, FileId(0)).validate_privacy(&mut diag_sink);
+                }
+                _ => {}
+            }
+        }
+
+        drop(diag_sink);
+        diags
+    }
+}