From 67ac0d60c3e8b450a9e871f3edb29322ac5045d2 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Sat, 19 Oct 2024 20:26:30 +0100 Subject: [PATCH] feat: Warn about private types leaking in public functions and struct fields (#6296) # Description ## Problem\* Resolves #6248 ## Summary\* Issue warnings in more cases when a public item refers to private types: * public fields of a public struct fields using private types * a public function taking or returning private types ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/mod.rs | 107 +++++++-- .../noirc_frontend/src/tests/visibility.rs | 203 +++++++++++++++--- .../arithmetic_generics/src/main.nr | 4 +- .../comptime_function_definition/src/main.nr | 2 +- .../comptime_module/src/main.nr | 2 +- .../regression_6077/src/main.nr | 2 +- 6 files changed, 270 insertions(+), 50 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index dafafe421eb..aef0771c486 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -3,7 +3,9 @@ use std::{ rc::Rc, }; -use crate::{ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, StructField, TypeBindings}; +use crate::{ + ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, StructField, StructType, TypeBindings, +}; use crate::{ ast::{ BlockExpression, FunctionKind, GenericTypeArgs, Ident, NoirFunction, NoirStruct, Param, @@ -53,7 +55,7 @@ mod unquote; use fm::FileId; use iter_extended::vecmap; -use noirc_errors::{Location, Span}; +use noirc_errors::{Location, Span, Spanned}; use types::bind_ordered_generics; use self::traits::check_trait_impl_method_matches_declaration; @@ -398,6 +400,28 @@ impl<'context> Elaborator<'context> { self.run_function_lints(&func_meta, &modifiers); + // Check arg and return-value visibility of standalone functions. + if self.should_check_function_visibility(&func_meta, &modifiers) { + let name = Ident(Spanned::from( + func_meta.name.location.span, + self.interner.definition_name(func_meta.name.id).to_string(), + )); + for (_, typ, _) in func_meta.parameters.iter() { + self.check_type_is_not_more_private_then_item( + &name, + modifiers.visibility, + typ, + name.span(), + ); + } + self.check_type_is_not_more_private_then_item( + &name, + modifiers.visibility, + func_meta.return_type(), + name.span(), + ); + } + self.introduce_generics_into_scope(func_meta.all_generics.clone()); // The DefinitionIds for each parameter were already created in define_function_meta @@ -1280,14 +1304,49 @@ impl<'context> Elaborator<'context> { let typ = self.resolve_type(alias.type_alias_def.typ); if visibility != ItemVisibility::Private { - self.check_aliased_type_is_not_more_private(name, visibility, &typ, span); + self.check_type_is_not_more_private_then_item(name, visibility, &typ, span); } self.interner.set_type_alias(alias_id, typ, generics); self.generics.clear(); } - fn check_aliased_type_is_not_more_private( + /// Find the struct in the parent module so we can know its visibility + fn find_struct_visibility(&self, struct_type: &StructType) -> Option { + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); + let parent_module_data = self.get_module(parent_module_id); + let per_ns = parent_module_data.find_name(&struct_type.name); + per_ns.types.map(|(_, vis, _)| vis) + } + + /// Check whether a functions return value and args should be checked for private type visibility. + fn should_check_function_visibility( + &self, + func_meta: &FuncMeta, + modifiers: &FunctionModifiers, + ) -> bool { + // Private functions don't leak anything. + if modifiers.visibility == ItemVisibility::Private { + return false; + } + // Implementing public traits on private types is okay, they can't be used unless the type itself is accessible. + if func_meta.trait_impl.is_some() { + return false; + } + // Public struct functions should not expose private types. + if let Some(struct_visibility) = func_meta.struct_id.and_then(|id| { + let struct_def = self.get_struct(id); + let struct_def = struct_def.borrow(); + self.find_struct_visibility(&struct_def) + }) { + return struct_visibility != ItemVisibility::Private; + } + // Standalone functions should be checked + true + } + + /// Check that an item such as a struct field or type alias is not more visible than the type it refers to. + fn check_type_is_not_more_private_then_item( &mut self, name: &Ident, visibility: ItemVisibility, @@ -1303,11 +1362,7 @@ impl<'context> Elaborator<'context> { // then it's either accessible (all good) or it's not, in which case a different // error will happen somewhere else, but no need to error again here. if struct_module_id.krate == self.crate_id { - // Find the struct in the parent module so we can know its visibility - let parent_module_id = struct_type.id.parent_module_id(self.def_maps); - let parent_module_data = self.get_module(parent_module_id); - let per_ns = parent_module_data.find_name(&struct_type.name); - if let Some((_, aliased_visibility, _)) = per_ns.types { + if let Some(aliased_visibility) = self.find_struct_visibility(&struct_type) { if aliased_visibility < visibility { self.push_err(ResolverError::TypeIsMorePrivateThenItem { typ: struct_type.name.to_string(), @@ -1319,16 +1374,16 @@ impl<'context> Elaborator<'context> { } for generic in generics { - self.check_aliased_type_is_not_more_private(name, visibility, generic, span); + self.check_type_is_not_more_private_then_item(name, visibility, generic, span); } } Type::Tuple(types) => { for typ in types { - self.check_aliased_type_is_not_more_private(name, visibility, typ, span); + self.check_type_is_not_more_private_then_item(name, visibility, typ, span); } } Type::Alias(alias_type, generics) => { - self.check_aliased_type_is_not_more_private( + self.check_type_is_not_more_private_then_item( name, visibility, &alias_type.borrow().get_type(generics), @@ -1337,17 +1392,17 @@ impl<'context> Elaborator<'context> { } Type::Function(args, return_type, env, _) => { for arg in args { - self.check_aliased_type_is_not_more_private(name, visibility, arg, span); + self.check_type_is_not_more_private_then_item(name, visibility, arg, span); } - self.check_aliased_type_is_not_more_private(name, visibility, return_type, span); - self.check_aliased_type_is_not_more_private(name, visibility, env, span); + self.check_type_is_not_more_private_then_item(name, visibility, return_type, span); + self.check_type_is_not_more_private_then_item(name, visibility, env, span); } Type::MutableReference(typ) | Type::Array(_, typ) | Type::Slice(typ) => { - self.check_aliased_type_is_not_more_private(name, visibility, typ, span); + self.check_type_is_not_more_private_then_item(name, visibility, typ, span); } Type::InfixExpr(left, _op, right) => { - self.check_aliased_type_is_not_more_private(name, visibility, left, span); - self.check_aliased_type_is_not_more_private(name, visibility, right, span); + self.check_type_is_not_more_private_then_item(name, visibility, left, span); + self.check_type_is_not_more_private_then_item(name, visibility, right, span); } Type::FieldElement | Type::Integer(..) @@ -1384,6 +1439,22 @@ impl<'context> Elaborator<'context> { } } + // Check that the a public struct doesn't have a private type as a public field. + if typ.struct_def.visibility != ItemVisibility::Private { + for field in &fields { + let ident = Ident(Spanned::from( + field.name.span(), + format!("{}::{}", typ.struct_def.name, field.name), + )); + self.check_type_is_not_more_private_then_item( + &ident, + field.visibility, + &field.typ, + field.name.span(), + ); + } + } + let fields_len = fields.len(); self.interner.update_struct(*type_id, |struct_def| { struct_def.set_fields(fields); diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index f02771b3760..7cfec32062d 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/compiler/noirc_frontend/src/tests/visibility.rs @@ -28,29 +28,38 @@ fn errors_once_on_unused_import_that_is_not_accessible() { )) )); } + +fn assert_type_is_more_private_than_item_error(src: &str, private_typ: &str, public_item: &str) { + let errors = get_program_errors(src); + + assert!(!errors.is_empty(), "expected visibility error, got nothing"); + for (error, _) in &errors { + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, + item, + .. + }) = error + else { + panic!("Expected a type vs item visibility error, got {}", error); + }; + + assert_eq!(typ, private_typ); + assert_eq!(item, public_item); + } + assert_eq!(errors.len(), 1, "only expected one error"); +} + #[test] fn errors_if_type_alias_aliases_more_private_type() { let src = r#" struct Foo {} pub type Bar = Foo; - pub fn no_unused_warnings(_b: Bar) { - let _ = Foo {}; + pub fn no_unused_warnings() { + let _: Bar = Foo {}; } fn main() {} "#; - - let errors = get_program_errors(src); - assert_eq!(errors.len(), 1); - - let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { - typ, item, .. - }) = &errors[0].0 - else { - panic!("Expected an unused item error"); - }; - - assert_eq!(typ, "Foo"); - assert_eq!(item, "Bar"); + assert_type_is_more_private_than_item_error(src, "Foo", "Bar"); } #[test] @@ -59,25 +68,165 @@ fn errors_if_type_alias_aliases_more_private_type_in_generic() { pub struct Generic { value: T } struct Foo {} pub type Bar = Generic; - pub fn no_unused_warnings(_b: Bar) { + pub fn no_unused_warnings() { let _ = Foo {}; - let _ = Generic { value: 1 }; + let _: Bar = Generic { value: Foo {} }; } fn main() {} "#; + assert_type_is_more_private_than_item_error(src, "Foo", "Bar"); +} - let errors = get_program_errors(src); - assert_eq!(errors.len(), 1); +#[test] +fn errors_if_pub_type_alias_leaks_private_type_in_generic() { + let src = r#" + pub mod moo { + struct Bar {} + pub struct Foo { pub value: T } + pub type FooBar = Foo; - let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { - typ, item, .. - }) = &errors[0].0 - else { - panic!("Expected an unused item error"); - }; + pub fn no_unused_warnings() { + let _: FooBar = Foo { value: Bar {} }; + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "FooBar"); +} - assert_eq!(typ, "Foo"); - assert_eq!(item, "Bar"); +#[test] +fn errors_if_pub_struct_field_leaks_private_type_in_generic() { + let src = r#" + pub mod moo { + struct Bar {} + pub struct Foo { pub value: T } + pub struct FooBar { pub value: Foo } + + pub fn no_unused_warnings() { + let _ = FooBar { value: Foo { value: Bar {} } }; + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "FooBar::value"); +} + +#[test] +fn errors_if_pub_function_leaks_private_type_in_return() { + let src = r#" + pub mod moo { + struct Bar {} + + pub fn bar() -> Bar { + Bar {} + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "bar"); +} + +#[test] +fn errors_if_pub_function_leaks_private_type_in_arg() { + let src = r#" + pub mod moo { + struct Bar {} + pub fn bar(_bar: Bar) {} + + pub fn no_unused_warnings() { + let _ = Bar {}; + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "bar"); +} + +#[test] +fn does_not_error_if_pub_function_is_on_private_struct() { + let src = r#" + pub mod moo { + struct Bar {} + + impl Bar { + pub fn bar() -> Bar { + Bar {} + } + } + + pub fn no_unused_warnings() { + let _ = Bar {}; + } + } + fn main() {} + "#; + assert_no_errors(src); +} + +#[test] +fn errors_if_pub_function_on_pub_struct_returns_private() { + let src = r#" + pub mod moo { + struct Bar {} + pub struct Foo {} + + impl Foo { + pub fn bar() -> Bar { + Bar {} + } + } + + pub fn no_unused_warnings() { + let _ = Foo {}; + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "bar"); +} + +#[test] +fn does_not_error_if_pub_trait_is_defined_on_private_struct() { + let src = r#" + pub mod moo { + struct Bar {} + + pub trait Foo { + fn foo() -> Self; + } + + impl Foo for Bar { + fn foo() -> Self { + Bar {} + } + } + + pub fn no_unused_warnings() { + let _ = Bar {}; + } + } + fn main() {} + "#; + assert_no_errors(src); +} + +#[test] +fn errors_if_pub_trait_returns_private_struct() { + let src = r#" + pub mod moo { + struct Bar {} + + pub trait Foo { + fn foo() -> Bar; + } + + pub fn no_unused_warnings() { + let _ = Bar {}; + } + } + fn main() {} + "#; + assert_type_is_more_private_than_item_error(src, "Bar", "foo"); } #[test] diff --git a/test_programs/compile_success_empty/arithmetic_generics/src/main.nr b/test_programs/compile_success_empty/arithmetic_generics/src/main.nr index 4a057a75e43..fba85bbbae2 100644 --- a/test_programs/compile_success_empty/arithmetic_generics/src/main.nr +++ b/test_programs/compile_success_empty/arithmetic_generics/src/main.nr @@ -52,9 +52,9 @@ fn push_multiple(array: [Field; N]) -> [Field; N + 2] { // The rest of this file is setup for demo_proof // ********************************************* -struct W { } +pub struct W { } -struct Equiv { +pub struct Equiv { // TODO(https://github.com/noir-lang/noir/issues/5644): // Bug with struct_obj.field_thats_a_fn(x) diff --git a/test_programs/compile_success_empty/comptime_function_definition/src/main.nr b/test_programs/compile_success_empty/comptime_function_definition/src/main.nr index 4266d3734f9..f41642dde79 100644 --- a/test_programs/compile_success_empty/comptime_function_definition/src/main.nr +++ b/test_programs/compile_success_empty/comptime_function_definition/src/main.nr @@ -1,6 +1,6 @@ use std::meta::type_of; -struct Foo { x: Field, field: Field } +pub struct Foo { x: Field, field: Field } #[function_attr] pub fn foo(w: i32, y: Field, Foo { x, field: some_field }: Foo, mut a: bool, (b, c): (i32, i32)) -> i32 { diff --git a/test_programs/compile_success_empty/comptime_module/src/main.nr b/test_programs/compile_success_empty/comptime_module/src/main.nr index 09b1e98744d..3062d765814 100644 --- a/test_programs/compile_success_empty/comptime_module/src/main.nr +++ b/test_programs/compile_success_empty/comptime_module/src/main.nr @@ -47,7 +47,7 @@ comptime fn outer_attribute_separate_module(m: Module) { increment_counter(); } -struct Foo {} +pub struct Foo {} #[add_function] mod add_to_me { diff --git a/test_programs/compile_success_empty/regression_6077/src/main.nr b/test_programs/compile_success_empty/regression_6077/src/main.nr index fe067177e77..429468b90df 100644 --- a/test_programs/compile_success_empty/regression_6077/src/main.nr +++ b/test_programs/compile_success_empty/regression_6077/src/main.nr @@ -1,4 +1,4 @@ -struct WeirdStruct { +pub struct WeirdStruct { a: T, b: U, }