Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: visibility for struct fields #6221

Merged
merged 21 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/ast/structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub struct NoirStruct {

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct StructField {
pub visibility: ItemVisibility,
pub name: Ident,
pub typ: UnresolvedType,
}
Expand Down
35 changes: 23 additions & 12 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
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},
Expand Down Expand Up @@ -548,7 +548,7 @@
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 {
Expand Down Expand Up @@ -576,7 +576,7 @@
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)> {
Expand All @@ -588,10 +588,11 @@
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);
Expand All @@ -618,11 +619,21 @@
});
}

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_visibiilty(

Check warning on line 626 in compiler/noirc_frontend/src/elaborator/expressions.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (visibiilty)
asterite marked this conversation as resolved.
Show resolved Hide resolved
&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),
);
}

Expand Down
7 changes: 4 additions & 3 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1321,7 +1321,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));

Expand All @@ -1333,7 +1333,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);
Expand Down
64 changes: 59 additions & 5 deletions compiler/noirc_frontend/src/elaborator/statements.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
use std::collections::BTreeMap;

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,
},
graph::CrateId,
hir::{
resolution::errors::ResolverError,
def_map::{CrateDefMap, ModuleId},
resolution::{
errors::ResolverError,
import::{module_descendent_of_target, PathResolutionError},
},
type_check::{Source, TypeCheckError},
},
hir_def::{
Expand All @@ -18,7 +25,7 @@
},
},
node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId},
Type,
StructType, Type,
};

use super::{lints, Elaborator};
Expand Down Expand Up @@ -439,10 +446,12 @@
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_visibiilty(&s, field_name, visibility, span);

Check warning on line 453 in compiler/noirc_frontend/src/elaborator/statements.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (visibiilty)

return Some((field, index));
}
}
Expand Down Expand Up @@ -497,6 +506,20 @@
None
}

pub(super) fn check_struct_field_visibiilty(

Check warning on line 509 in compiler/noirc_frontend/src/elaborator/statements.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (visibiilty)
&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) =
Expand All @@ -511,3 +534,34 @@
(HirStatement::Expression(expr), typ)
}
}

fn struct_field_is_visible(
struct_type: &StructType,
visibility: ItemVisibility,
current_module_id: ModuleId,
def_maps: &BTreeMap<CrateId, CrateDefMap>,
) -> bool {
match visibility {
ItemVisibility::Public => true,
ItemVisibility::PublicCrate => {
struct_type.id.parent_module_id(def_maps).krate == current_module_id.krate
}
ItemVisibility::Private => {
let struct_parent_module_id = struct_type.id.parent_module_id(def_maps);
if struct_parent_module_id.krate != current_module_id.krate {
return false;
}

if struct_parent_module_id.local_id == current_module_id.local_id {
return true;
}

let def_map = &def_maps[&current_module_id.krate];
module_descendent_of_target(
def_map,
struct_parent_module_id.local_id,
current_module_id.local_id,
)
}
}
}
17 changes: 11 additions & 6 deletions compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,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::{
Expand All @@ -34,6 +34,7 @@ use crate::{
def_map::ModuleDefId,
},
hir_def::{
self,
expr::{HirExpression, HirLiteral},
function::FunctionBody,
},
Expand Down Expand Up @@ -487,9 +488,9 @@ fn struct_def_fields(

let mut fields = im::Vector::new();

asterite marked this conversation as resolved.
Show resolved Hide resolved
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]));
}

Expand Down Expand Up @@ -556,7 +557,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::Private,
asterite marked this conversation as resolved.
Show resolved Hide resolved
name: Ident::new(name.clone(), field_location.span),
typ,
})
}
_ => {
let value = name_value.display(interner).to_string();
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/hir/resolution/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,12 +437,12 @@ pub fn can_reference_module_id(
// 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 => {
let target_crate_def_map = &def_maps[&target_module.krate];
asterite marked this conversation as resolved.
Show resolved Hide resolved
same_crate
&& (module_descendent_of_target(
target_crate_def_map,
Expand All @@ -459,7 +459,7 @@ pub fn can_reference_module_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(
pub(crate) fn module_descendent_of_target(
def_map: &CrateDefMap,
target: LocalModuleId,
current: LocalModuleId,
Expand Down
Loading
Loading