feat: visibility for struct fields (#6221)

# Description

## Problem

Resolves #4837

Part of #4515

## Summary

This checks struct visibility in member access, constructors and
patterns.

## Additional Context

Private fields are still suggested in LSP. I plan to hide those fields
in a follow-up PR.


## Documentation

Check one:
- [ ] No documentation needed.
- [x] 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.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

compiler/noirc_frontend/src/ast/structure.rs

@@ -21,6 +21,7 @@ pub struct NoirStruct {
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct StructField {
+    pub visibility: ItemVisibility,
     pub name: Ident,
     pub typ: UnresolvedType,
 }

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -6,9 +6,9 @@ use rustc_hash::FxHashSet as HashSet;
 use crate::{
     ast::{
         ArrayLiteral, BlockExpression, CallExpression, CastExpression, ConstructorExpression,
-        Expression, ExpressionKind, Ident, IfExpression, IndexExpression, InfixExpression, Lambda,
-        Literal, MemberAccessExpression, MethodCallExpression, PrefixExpression, StatementKind,
-        UnaryOp, UnresolvedTypeData, UnresolvedTypeExpression,
+        Expression, ExpressionKind, Ident, IfExpression, IndexExpression, InfixExpression,
+        ItemVisibility, Lambda, Literal, MemberAccessExpression, MethodCallExpression,
+        PrefixExpression, StatementKind, UnaryOp, UnresolvedTypeData, UnresolvedTypeExpression,
     },
     hir::{
         comptime::{self, InterpreterError},
@@ -548,7 +548,7 @@ impl<'context> Elaborator<'context> {
         let generics = struct_generics.clone();
 
         let fields = constructor.fields;
-        let field_types = r#type.borrow().get_fields(&struct_generics);
+        let field_types = r#type.borrow().get_fields_with_visibility(&struct_generics);
         let fields =
             self.resolve_constructor_expr_fields(struct_type.clone(), field_types, fields, span);
         let expr = HirExpression::Constructor(HirConstructorExpression {
@@ -576,7 +576,7 @@ impl<'context> Elaborator<'context> {
     fn resolve_constructor_expr_fields(
         &mut self,
         struct_type: Shared<StructType>,
-        field_types: Vec<(String, Type)>,
+        field_types: Vec<(String, ItemVisibility, Type)>,
         fields: Vec<(Ident, Expression)>,
         span: Span,
     ) -> Vec<(Ident, ExprId)> {
@@ -588,10 +588,11 @@ impl<'context> Elaborator<'context> {
             let expected_field_with_index = field_types
                 .iter()
                 .enumerate()
-                .find(|(_, (name, _))| name == &field_name.0.contents);
-            let expected_index = expected_field_with_index.map(|(index, _)| index);
+                .find(|(_, (name, _, _))| name == &field_name.0.contents);
+            let expected_index_and_visibility =
+                expected_field_with_index.map(|(index, (_, visibility, _))| (index, visibility));
             let expected_type =
-                expected_field_with_index.map(|(_, (_, typ))| typ).unwrap_or(&Type::Error);
+                expected_field_with_index.map(|(_, (_, _, typ))| typ).unwrap_or(&Type::Error);
 
             let field_span = field.span;
             let (resolved, field_type) = self.elaborate_expression(field);
@@ -618,11 +619,21 @@ impl<'context> Elaborator<'context> {
                 });
             }
 
-            if let Some(expected_index) = expected_index {
+            if let Some((index, visibility)) = expected_index_and_visibility {
+                let struct_type = struct_type.borrow();
+                let field_span = field_name.span();
+                let field_name = &field_name.0.contents;
+                self.check_struct_field_visibility(
+                    &struct_type,
+                    field_name,
+                    *visibility,
+                    field_span,
+                );
+
                 self.interner.add_struct_member_reference(
-                    struct_type.borrow().id,
-                    expected_index,
-                    Location::new(field_name.span(), self.file),
+                    struct_type.id,
+                    index,
+                    Location::new(field_span, self.file),
                 );
             }
 

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -3,7 +3,7 @@ use std::{
     rc::Rc,
 };
 
-use crate::ast::ItemVisibility;
+use crate::{ast::ItemVisibility, StructField};
 use crate::{
     ast::{
         BlockExpression, FunctionKind, GenericTypeArgs, Ident, NoirFunction, NoirStruct, Param,
@@ -1315,7 +1315,7 @@ impl<'context> Elaborator<'context> {
         &mut self,
         unresolved: &NoirStruct,
         struct_id: StructId,
-    ) -> Vec<(Ident, Type)> {
+    ) -> Vec<StructField> {
         self.recover_generics(|this| {
             this.current_item = Some(DependencyId::Struct(struct_id));
 
@@ -1327,7 +1327,8 @@ impl<'context> Elaborator<'context> {
             let fields = vecmap(&unresolved.fields, |field| {
                 let ident = &field.item.name;
                 let typ = &field.item.typ;
-                (ident.clone(), this.resolve_type(typ.clone()))
+                let visibility = field.item.visibility;
+                StructField { visibility, name: ident.clone(), typ: this.resolve_type(typ.clone()) }
             });
 
             this.resolving_ids.remove(&struct_id);

compiler/noirc_frontend/src/elaborator/patterns.rs

@@ -4,7 +4,8 @@ use rustc_hash::FxHashSet as HashSet;
 
 use crate::{
     ast::{
-        Expression, ExpressionKind, Ident, Path, Pattern, TypePath, UnresolvedType, ERROR_IDENT,
+        Expression, ExpressionKind, Ident, ItemVisibility, Path, Pattern, TypePath, UnresolvedType,
+        ERROR_IDENT,
     },
     hir::{
         def_collector::dc_crate::CompilationError,
@@ -272,7 +273,9 @@ impl<'context> Elaborator<'context> {
         let mut unseen_fields = struct_type.borrow().field_names();
 
         for (field, pattern) in fields {
-            let field_type = expected_type.get_field_type(&field.0.contents).unwrap_or(Type::Error);
+            let (field_type, visibility) = expected_type
+                .get_field_type_and_visibility(&field.0.contents)
+                .unwrap_or((Type::Error, ItemVisibility::Public));
             let resolved = self.elaborate_pattern_mut(
                 pattern,
                 field_type,
@@ -286,6 +289,13 @@ impl<'context> Elaborator<'context> {
             if unseen_fields.contains(&field) {
                 unseen_fields.remove(&field);
                 seen_fields.insert(field.clone());
+
+                self.check_struct_field_visibility(
+                    &struct_type.borrow(),
+                    &field.0.contents,
+                    visibility,
+                    field.span(),
+                );
             } else if seen_fields.contains(&field) {
                 // duplicate field
                 self.push_err(ResolverError::DuplicateField { field: field.clone() });

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -3,11 +3,13 @@ use noirc_errors::{Location, Span, Spanned};
 use crate::{
     ast::{
         AssignStatement, BinaryOpKind, ConstrainKind, ConstrainStatement, Expression,
-        ExpressionKind, ForLoopStatement, ForRange, InfixExpression, LValue, LetStatement, Path,
-        Statement, StatementKind,
+        ExpressionKind, ForLoopStatement, ForRange, Ident, InfixExpression, ItemVisibility, LValue,
+        LetStatement, Path, Statement, StatementKind,
     },
     hir::{
-        resolution::errors::ResolverError,
+        resolution::{
+            errors::ResolverError, import::PathResolutionError, visibility::struct_field_is_visible,
+        },
         type_check::{Source, TypeCheckError},
     },
     hir_def::{
@@ -18,7 +20,7 @@ use crate::{
         },
     },
     node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId},
-    Type,
+    StructType, Type,
 };
 
 use super::{lints, Elaborator};
@@ -439,10 +441,12 @@ impl<'context> Elaborator<'context> {
         match &lhs_type {
             Type::Struct(s, args) => {
                 let s = s.borrow();
-                if let Some((field, index)) = s.get_field(field_name, args) {
+                if let Some((field, visibility, index)) = s.get_field(field_name, args) {
                     let reference_location = Location::new(span, self.file);
                     self.interner.add_struct_member_reference(s.id, index, reference_location);
 
+                    self.check_struct_field_visibility(&s, field_name, visibility, span);
+
                     return Some((field, index));
                 }
             }
@@ -497,6 +501,20 @@ impl<'context> Elaborator<'context> {
         None
     }
 
+    pub(super) fn check_struct_field_visibility(
+        &mut self,
+        struct_type: &StructType,
+        field_name: &str,
+        visibility: ItemVisibility,
+        span: Span,
+    ) {
+        if !struct_field_is_visible(struct_type, visibility, self.module_id(), self.def_maps) {
+            self.push_err(ResolverError::PathResolutionError(PathResolutionError::Private(
+                Ident::new(field_name.to_string(), span),
+            )));
+        }
+    }
+
     fn elaborate_comptime_statement(&mut self, statement: Statement) -> (HirStatement, Type) {
         let span = statement.span;
         let (hir_statement, _typ) =

compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs

@@ -19,8 +19,8 @@ use rustc_hash::FxHashMap as HashMap;
 use crate::{
     ast::{
         ArrayLiteral, BlockExpression, ConstrainKind, Expression, ExpressionKind, ForRange,
-        FunctionKind, FunctionReturnType, Ident, IntegerBitSize, LValue, Literal, Pattern,
-        Signedness, Statement, StatementKind, UnaryOp, UnresolvedType, UnresolvedTypeData,
+        FunctionKind, FunctionReturnType, Ident, IntegerBitSize, ItemVisibility, LValue, Literal,
+        Pattern, Signedness, Statement, StatementKind, UnaryOp, UnresolvedType, UnresolvedTypeData,
         Visibility,
     },
     hir::{
@@ -32,10 +32,9 @@ use crate::{
         def_collector::dc_crate::CollectedItems,
         def_map::ModuleDefId,
     },
-    hir_def::{
-        expr::{HirExpression, HirLiteral},
-        function::FunctionBody,
-    },
+    hir_def::expr::{HirExpression, HirLiteral},
+    hir_def::function::FunctionBody,
+    hir_def::{self},
     node_interner::{DefinitionKind, NodeInterner, TraitImplKind},
     parser::{Parser, StatementOrExpressionOrLValue},
     token::{Attribute, SecondaryAttribute, Token},
@@ -498,9 +497,9 @@ fn struct_def_fields(
 
     let mut fields = im::Vector::new();
 
-    for (name, typ) in struct_def.get_fields_as_written() {
-        let name = Value::Quoted(Rc::new(vec![Token::Ident(name)]));
-        let typ = Value::Type(typ);
+    for field in struct_def.get_fields_as_written() {
+        let name = Value::Quoted(Rc::new(vec![Token::Ident(field.name.to_string())]));
+        let typ = Value::Type(field.typ);
         fields.push_back(Value::Tuple(vec![name, typ]));
     }
 
@@ -567,7 +566,11 @@ fn struct_def_set_fields(
 
                 match name_tokens.first() {
                     Some(Token::Ident(name)) if name_tokens.len() == 1 => {
-                        Ok((Ident::new(name.clone(), field_location.span), typ))
+                        Ok(hir_def::types::StructField {
+                            visibility: ItemVisibility::Public,
+                            name: Ident::new(name.clone(), field_location.span),
+                            typ,
+                        })
                     }
                     _ => {
                         let value = name_value.display(interner).to_string();

compiler/noirc_frontend/src/hir/resolution/import.rs

@@ -12,6 +12,7 @@ use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment};
 use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs};
 
 use super::errors::ResolverError;
+use super::visibility::can_reference_module_id;
 
 #[derive(Debug, Clone)]
 pub struct ImportDirective {
@@ -169,7 +170,7 @@ fn resolve_path_to_ns(
                     import_path,
                     import_directive.module_id,
                     def_maps,
-                    true,
+                    true, // plain or crate
                     usage_tracker,
                     path_references,
                 );
@@ -199,7 +200,7 @@ fn resolve_path_to_ns(
                 import_path,
                 import_directive.module_id,
                 def_maps,
-                true,
+                true, // plain or crate
                 usage_tracker,
                 path_references,
             )
@@ -224,7 +225,7 @@ fn resolve_path_to_ns(
                     import_path,
                     parent_module_id,
                     def_maps,
-                    false,
+                    false, // plain or crate
                     usage_tracker,
                     path_references,
                 )
@@ -253,7 +254,7 @@ fn resolve_path_from_crate_root(
         import_path,
         starting_mod,
         def_maps,
-        false,
+        true, // plain or crate
         usage_tracker,
         path_references,
     )
@@ -266,7 +267,7 @@ fn resolve_name_in_module(
     import_path: &[PathSegment],
     starting_mod: LocalModuleId,
     def_maps: &BTreeMap<CrateId, CrateDefMap>,
-    plain: bool,
+    plain_or_crate: bool,
     usage_tracker: &mut UsageTracker,
     path_references: &mut Option<&mut Vec<ReferenceId>>,
 ) -> NamespaceResolutionResult {
@@ -331,9 +332,9 @@ fn resolve_name_in_module(
         };
 
         warning = warning.or_else(|| {
-            // If the path is plain, the first segment will always refer to
+            // If the path is plain or crate, the first segment will always refer to
             // something that's visible from the current module.
-            if (plain && index == 0)
+            if (plain_or_crate && index == 0)
                 || can_reference_module_id(
                     def_maps,
                     importing_crate,
@@ -424,61 +425,3 @@ fn resolve_external_dep(
         path_references,
     )
 }
-
-// Returns false if the given private function is being called from a non-child module, or
-// if the given pub(crate) function is being called from another crate. Otherwise returns true.
-pub fn can_reference_module_id(
-    def_maps: &BTreeMap<CrateId, CrateDefMap>,
-    importing_crate: CrateId,
-    current_module: LocalModuleId,
-    target_module: ModuleId,
-    visibility: ItemVisibility,
-) -> bool {
-    // Note that if the target module is in a different crate from the current module then we will either
-    // return true as the target module is public or return false as it is private without looking at the `CrateDefMap` in either case.
-    let same_crate = target_module.krate == importing_crate;
-    let target_crate_def_map = &def_maps[&target_module.krate];
-
-    match visibility {
-        ItemVisibility::Public => true,
-        ItemVisibility::PublicCrate => same_crate,
-        ItemVisibility::Private => {
-            same_crate
-                && (module_descendent_of_target(
-                    target_crate_def_map,
-                    target_module.local_id,
-                    current_module,
-                ) || module_is_parent_of_struct_module(
-                    target_crate_def_map,
-                    current_module,
-                    target_module.local_id,
-                ))
-        }
-    }
-}
-
-// Returns true if `current` is a (potentially nested) child module of `target`.
-// This is also true if `current == target`.
-fn module_descendent_of_target(
-    def_map: &CrateDefMap,
-    target: LocalModuleId,
-    current: LocalModuleId,
-) -> bool {
-    if current == target {
-        return true;
-    }
-
-    def_map.modules[current.0]
-        .parent
-        .map_or(false, |parent| module_descendent_of_target(def_map, target, parent))
-}
-
-/// Returns true if `target` is a struct and its parent is `current`.
-fn module_is_parent_of_struct_module(
-    def_map: &CrateDefMap,
-    current: LocalModuleId,
-    target: LocalModuleId,
-) -> bool {
-    let module_data = &def_map.modules[target.0];
-    module_data.is_struct && module_data.parent == Some(current)
-}

compiler/noirc_frontend/src/hir/resolution/mod.rs

@@ -8,3 +8,4 @@
 pub mod errors;
 pub mod import;
 pub mod path_resolver;
+pub mod visibility;