From 5cec7c66ffeac5f21e0fcade58b714d2ae85ed59 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 11 Nov 2024 10:53:05 -0300 Subject: [PATCH 1/4] fix: don't report visibility errors when elaborating comptime value --- .../src/elaborator/expressions.rs | 12 ++++++++- compiler/noirc_frontend/src/elaborator/mod.rs | 7 ++++++ .../src/elaborator/statements.rs | 4 +++ .../noirc_frontend/src/tests/visibility.rs | 25 +++++++++++++++++++ 4 files changed, 47 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index ff482dca4fb..dddfb0577de 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -907,7 +907,17 @@ impl<'context> Elaborator<'context> { let location = Location::new(span, self.file); match value.into_expression(self.interner, location) { - Ok(new_expr) => self.elaborate_expression(new_expr), + Ok(new_expr) => { + // At this point the Expression was already elaborated and we got a Value. + // We'll elaborate this value turned into Expression to inline it and get + // an ExprId and Type, but we don't want any visibility errors to happen + // here (they could if we have `Foo { inner: 5 }` and `inner` is not + // accessible from where this expression is being elaborated). + self.gag_field_visibility_errors += 1; + let value = self.elaborate_expression(new_expr); + self.gag_field_visibility_errors -= 1; + value + } Err(error) => make_error(self, error), } } diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index ee79b62671f..02c27459679 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -164,6 +164,12 @@ pub struct Elaborator<'context> { unresolved_globals: BTreeMap, pub(crate) interpreter_call_stack: im::Vector, + + /// If greather than 0, field visibility errors won't be reported. + /// This is used when elaborating a comptime expression that is a struct constructor + /// like `Foo { inner: 5 }`: in that case we already elaborated the code that led to + /// that comptime value and any visibility errors were already reported. + gag_field_visibility_errors: usize, } #[derive(Default)] @@ -213,6 +219,7 @@ impl<'context> Elaborator<'context> { current_trait: None, interpreter_call_stack, in_comptime_context: false, + gag_field_visibility_errors: 0, } } diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 757def16a93..aef6f37a311 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -509,6 +509,10 @@ impl<'context> Elaborator<'context> { visibility: ItemVisibility, span: Span, ) { + if self.gag_field_visibility_errors > 0 { + return; + } + if !struct_member_is_visible(struct_type.id, visibility, self.module_id(), self.def_maps) { self.push_err(ResolverError::PathResolutionError(PathResolutionError::Private( Ident::new(field_name.to_string(), span), diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index 7cfec32062d..f2d683514f3 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/compiler/noirc_frontend/src/tests/visibility.rs @@ -493,3 +493,28 @@ fn does_not_error_if_referring_to_top_level_private_module_via_crate() { "#; assert_no_errors(src); } + +#[test] +fn visibility_bug_inside_comptime() { + let src = r#" + mod foo { + pub struct Foo { + inner: Field, + } + + impl Foo { + pub fn new(inner: Field) -> Self { + Self { inner } + } + } + } + + use foo::Foo; + + fn main() { + let _ = Foo::new(5); + let _ = comptime { Foo::new(5) }; + } + "#; + assert_no_errors(src); +} From c9c2ad1ec661bee2b00b36576372646c7a30e76f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 12 Nov 2024 08:25:49 -0300 Subject: [PATCH 2/4] Use a more common word --- compiler/noirc_frontend/src/elaborator/expressions.rs | 4 ++-- compiler/noirc_frontend/src/elaborator/mod.rs | 6 +++--- compiler/noirc_frontend/src/elaborator/statements.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index dddfb0577de..f801c1817ef 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -913,9 +913,9 @@ impl<'context> Elaborator<'context> { // an ExprId and Type, but we don't want any visibility errors to happen // here (they could if we have `Foo { inner: 5 }` and `inner` is not // accessible from where this expression is being elaborated). - self.gag_field_visibility_errors += 1; + self.silence_field_visibility_errors += 1; let value = self.elaborate_expression(new_expr); - self.gag_field_visibility_errors -= 1; + self.silence_field_visibility_errors -= 1; value } Err(error) => make_error(self, error), diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 02c27459679..a87347c88ee 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -165,11 +165,11 @@ pub struct Elaborator<'context> { pub(crate) interpreter_call_stack: im::Vector, - /// If greather than 0, field visibility errors won't be reported. + /// If greater than 0, field visibility errors won't be reported. /// This is used when elaborating a comptime expression that is a struct constructor /// like `Foo { inner: 5 }`: in that case we already elaborated the code that led to /// that comptime value and any visibility errors were already reported. - gag_field_visibility_errors: usize, + silence_field_visibility_errors: usize, } #[derive(Default)] @@ -219,7 +219,7 @@ impl<'context> Elaborator<'context> { current_trait: None, interpreter_call_stack, in_comptime_context: false, - gag_field_visibility_errors: 0, + silence_field_visibility_errors: 0, } } diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index aef6f37a311..6bdaae32730 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -509,7 +509,7 @@ impl<'context> Elaborator<'context> { visibility: ItemVisibility, span: Span, ) { - if self.gag_field_visibility_errors > 0 { + if self.silence_field_visibility_errors > 0 { return; } From d9b96f39ab284936567ed9a22edc6c49d2e62bb9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 18 Nov 2024 11:13:56 -0300 Subject: [PATCH 3/4] Add a test for accessing private struct member inside comptime context --- .../noirc_frontend/src/tests/visibility.rs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index f2d683514f3..3173a2e0e78 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/compiler/noirc_frontend/src/tests/visibility.rs @@ -518,3 +518,41 @@ fn visibility_bug_inside_comptime() { "#; assert_no_errors(src); } + +#[test] +fn errors_if_accessing_private_struct_member_inside_comptime_context() { + let src = r#" + mod foo { + pub struct Foo { + inner: Field, + } + + impl Foo { + pub fn new(inner: Field) -> Self { + Self { inner } + } + } + } + + use foo::Foo; + + fn main() { + comptime { + let foo = Foo::new(5); + let _ = foo.inner; + }; + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::Private(ident), + )) = &errors[0].0 + else { + panic!("Expected a private error"); + }; + + assert_eq!(ident.to_string(), "inner"); +} From ff8c9871134c163b5b482e9e6371374358646ad9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 21 Nov 2024 12:17:59 -0300 Subject: [PATCH 4/4] Add another test --- .../noirc_frontend/src/tests/visibility.rs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/compiler/noirc_frontend/src/tests/visibility.rs b/compiler/noirc_frontend/src/tests/visibility.rs index 3173a2e0e78..824a1de4c37 100644 --- a/compiler/noirc_frontend/src/tests/visibility.rs +++ b/compiler/noirc_frontend/src/tests/visibility.rs @@ -556,3 +556,45 @@ fn errors_if_accessing_private_struct_member_inside_comptime_context() { assert_eq!(ident.to_string(), "inner"); } + +#[test] +fn errors_if_accessing_private_struct_member_inside_function_generated_at_comptime() { + let src = r#" + mod foo { + pub struct Foo { + foo_inner: Field, + } + } + + use foo::Foo; + + #[generate_inner_accessor] + struct Bar { + bar_inner: Foo, + } + + comptime fn generate_inner_accessor(_s: StructDefinition) -> Quoted { + quote { + fn bar_get_foo_inner(x: Bar) -> Field { + x.bar_inner.foo_inner + } + } + } + + fn main(x: Bar) { + let _ = bar_get_foo_inner(x); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::Private(ident), + )) = &errors[0].0 + else { + panic!("Expected a private error"); + }; + + assert_eq!(ident.to_string(), "foo_inner"); +}