From f8e20c1c58b79d8b7f16fa1542b6eafac1d9cbf5 Mon Sep 17 00:00:00 2001 From: Mikail Khan Date: Sun, 1 Aug 2021 20:09:24 -0400 Subject: [PATCH] Feat: Added private type leak validation to structs Also exposed struct field visibility in the HIR and created a struct validation module --- crates/mun_hir/src/code_model.rs | 2 +- crates/mun_hir/src/code_model/struct.rs | 9 ++- .../src/code_model/struct/validator.rs | 65 +++++++++++++++++++ ...or__tests__private_leak_struct_fields.snap | 8 +++ .../src/code_model/struct/validator/tests.rs | 47 ++++++++++++++ crates/mun_hir/src/expr/validator/tests.rs | 42 ++---------- crates/mun_hir/src/utils.rs | 47 ++++++++++++++ 7 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 crates/mun_hir/src/code_model/struct/validator.rs create mode 100644 crates/mun_hir/src/code_model/struct/validator/snapshots/mun_hir__code_model__r#struct__validator__tests__private_leak_struct_fields.snap create mode 100644 crates/mun_hir/src/code_model/struct/validator/tests.rs diff --git a/crates/mun_hir/src/code_model.rs b/crates/mun_hir/src/code_model.rs index e81c0d341..dd46e3397 100644 --- a/crates/mun_hir/src/code_model.rs +++ b/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 mod r#struct; mod type_alias; use crate::{expr::BodySourceMap, HirDatabase, Name}; diff --git a/crates/mun_hir/src/code_model/struct.rs b/crates/mun_hir/src/code_model/struct.rs index 75d21d2f4..a2074ed0d 100644 --- a/crates/mun_hir/src/code_model/struct.rs +++ b/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 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) diff --git a/crates/mun_hir/src/code_model/struct/validator.rs b/crates/mun_hir/src/code_model/struct/validator.rs new file mode 100644 index 000000000..ba86aac12 --- /dev/null +++ b/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(), + }) + }); + } +} diff --git a/crates/mun_hir/src/code_model/struct/validator/snapshots/mun_hir__code_model__r#struct__validator__tests__private_leak_struct_fields.snap b/crates/mun_hir/src/code_model/struct/validator/snapshots/mun_hir__code_model__r#struct__validator__tests__private_leak_struct_fields.snap new file mode 100644 index 000000000..7cae42881 --- /dev/null +++ b/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 + diff --git a/crates/mun_hir/src/code_model/struct/validator/tests.rs b/crates/mun_hir/src/code_model/struct/validator/tests.rs new file mode 100644 index 000000000..de892acd9 --- /dev/null +++ b/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); +} diff --git a/crates/mun_hir/src/expr/validator/tests.rs b/crates/mun_hir/src/expr/validator/tests.rs index 29b13cbec..c5bebaf2d 100644 --- a/crates/mun_hir/src/expr/validator/tests.rs +++ b/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); diff --git a/crates/mun_hir/src/utils.rs b/crates/mun_hir/src/utils.rs index 4f9be983c..0ba8b6293 100644 --- a/crates/mun_hir/src/utils.rs +++ b/crates/mun_hir/src/utils.rs @@ -8,3 +8,50 @@ pub(crate) fn make_mut_slice(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 + } +}