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

Add pub attribute for struct fields #232

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion examples/assignment.no
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
struct Thing {
xx: Field,
pub xx: Field,
}

fn try_to_mutate(thing: Thing) {
4 changes: 2 additions & 2 deletions examples/hint.no
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
struct Thing {
xx: Field,
yy: Field,
pub xx: Field,
pub yy: Field,
}

hint fn mul(lhs: Field, rhs: Field) -> Field {
4 changes: 2 additions & 2 deletions examples/types.no
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
struct Thing {
xx: Field,
yy: Field,
pub xx: Field,
pub yy: Field,
}

fn main(pub xx: Field, pub yy: Field) {
4 changes: 2 additions & 2 deletions examples/types_array.no
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
struct Thing {
xx: Field,
yy: Field,
pub xx: Field,
pub yy: Field,
}

fn main(pub xx: Field, pub yy: Field) {
4 changes: 2 additions & 2 deletions examples/types_array_output.no
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
struct Thing {
xx: Field,
yy: Field,
pub xx: Field,
pub yy: Field,
}

fn main(pub xx: Field, pub yy: Field) -> [Thing; 2] {
2 changes: 1 addition & 1 deletion src/circuit_writer/ir.rs
Original file line number Diff line number Diff line change
@@ -706,7 +706,7 @@ impl<B: Backend> IRWriter<B> {
// find range of field
let mut start = 0;
let mut len = 0;
for (field, field_typ) in &struct_info.fields {
for (field, field_typ, _attribute) in &struct_info.fields {
if field == &rhs.value {
len = self.size_of(field_typ);
break;
4 changes: 2 additions & 2 deletions src/circuit_writer/writer.rs
Original file line number Diff line number Diff line change
@@ -320,7 +320,7 @@ impl<B: Backend> CircuitWriter<B> {
.clone();

let mut offset = 0;
for (_field_name, field_typ) in &struct_info.fields {
for (_field_name, field_typ, _attribute) in &struct_info.fields {
let len = self.size_of(field_typ);
let range = offset..(offset + len);
self.constrain_inputs_to_main(&input[range], field_typ, span)?;
@@ -492,7 +492,7 @@ impl<B: Backend> CircuitWriter<B> {
// find range of field
let mut start = 0;
let mut len = 0;
for (field, field_typ) in &struct_info.fields {
for (field, field_typ, _attribute) in &struct_info.fields {
if field == &rhs.value {
len = self.size_of(field_typ);
break;
4 changes: 4 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -369,6 +369,10 @@ pub enum ErrorKind {
#[error("division by zero")]
DivisionByZero,

#[error("cannot access private field `{1}` of struct `{0}` from outside its methods.")]
PrivateFieldAccess(String, String),

#[error("Not enough variables provided to fill placeholders in the formatted string")]
InsufficientVariables,

}
2 changes: 1 addition & 1 deletion src/inputs.rs
Original file line number Diff line number Diff line change
@@ -141,7 +141,7 @@ impl<B: Backend> CompiledCircuit<B> {

// parse each field
let mut res = vec![];
for (field_name, field_ty) in fields {
for (field_name, field_ty, _attribute) in fields {
let value = map.remove(field_name).ok_or_else(|| {
ParsingError::MissingStructFieldIdent(field_name.to_string())
})?;
6 changes: 3 additions & 3 deletions src/mast/mod.rs
Original file line number Diff line number Diff line change
@@ -446,7 +446,7 @@ impl<B: Backend> Mast<B> {

let mut sum = 0;

for (_, t) in &struct_info.fields {
for (_, t, _) in &struct_info.fields {
sum += self.size_of(t);
}

@@ -549,8 +549,8 @@ fn monomorphize_expr<B: Backend>(
let typ = struct_info
.fields
.iter()
.find(|(name, _)| name == &rhs.value)
.map(|(_, typ)| typ.clone());
.find(|(name, _, _)| name == &rhs.value)
.map(|(_, typ, _)| typ.clone());

let mexpr = expr.to_mast(
ctx,
2 changes: 1 addition & 1 deletion src/name_resolution/context.rs
Original file line number Diff line number Diff line change
@@ -151,7 +151,7 @@ impl NameResCtx {
self.resolve(module, true)?;

// we resolve the fully-qualified types of the fields
for (_field_name, field_typ) in fields {
for (_field_name, field_typ, _attribute) in fields {
self.resolve_typ_kind(&mut field_typ.kind)?;
}

25 changes: 23 additions & 2 deletions src/negative_tests.rs
Original file line number Diff line number Diff line change
@@ -700,7 +700,7 @@ fn test_nonhint_call_with_unsafe() {
fn test_no_cst_struct_field_prop() {
let code = r#"
struct Thing {
val: Field,
pub val: Field,
}

fn gen(const LEN: Field) -> [Field; LEN] {
@@ -725,7 +725,7 @@ fn test_no_cst_struct_field_prop() {
fn test_mut_cst_struct_field_prop() {
let code = r#"
struct Thing {
val: Field,
pub val: Field,
}

fn gen(const LEN: Field) -> [Field; LEN] {
@@ -747,3 +747,24 @@ fn test_mut_cst_struct_field_prop() {
ErrorKind::ArgumentTypeMismatch(..)
));
}

#[test]
fn test_private_field_access() {
let code = r#"
struct Room {
pub beds: Field, // public
size: Field // private
}

fn main(pub beds: Field) {
let room = Room {beds: beds, size: 10};
room.size = 5; // not allowed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just test the negative case here. Then the positive tests should cover the rest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree ^ we can simplify the test

}
"#;

let res = tast_pass(code).0;
assert!(matches!(
res.unwrap_err().kind,
ErrorKind::PrivateFieldAccess(..)
));
}
28 changes: 24 additions & 4 deletions src/parser/structs.rs
Original file line number Diff line number Diff line change
@@ -3,12 +3,12 @@ use serde::{Deserialize, Serialize};
use crate::{
constants::Span,
error::{ErrorKind, Result},
lexer::{Token, TokenKind, Tokens},
lexer::{Keyword, Token, TokenKind, Tokens},
syntax::is_type,
};

use super::{
types::{Ident, ModulePath, Ty, TyKind},
types::{Attribute, AttributeKind, Ident, ModulePath, Ty, TyKind},
Error, ParserCtx,
};

@@ -17,7 +17,7 @@ pub struct StructDef {
//pub attribute: Attribute,
pub module: ModulePath, // name resolution
pub name: CustomType,
pub fields: Vec<(Ident, Ty)>,
pub fields: Vec<(Ident, Ty, Option<Attribute>)>,
pub span: Span,
}

@@ -55,6 +55,26 @@ impl StructDef {
tokens.bump(ctx);
break;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realized looking at this code that we can write an empty struct (e.g. struct Thing {}) and it will work. Should we disallow that? (we could do that by checking that the length of fields is not 0 at the end)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the same time it seems OK to allow it...

// check for pub keyword
// struct Foo { pub a: Field, b: Field }
// ^
let attribute = if matches!(
tokens.peek(),
Some(Token {
kind: TokenKind::Keyword(Keyword::Pub),
..
})
) {
let token = tokens.bump(ctx).unwrap();
Some(Attribute {
kind: AttributeKind::Pub,
span: token.span,
})
} else {
None
};

// struct Foo { a: Field, b: Field }
// ^
let field_name = Ident::parse(ctx, tokens)?;
@@ -67,7 +87,7 @@ impl StructDef {
// ^^^^^
let field_ty = Ty::parse(ctx, tokens)?;
span = span.merge_with(field_ty.span);
fields.push((field_name, field_ty));
fields.push((field_name, field_ty, attribute));

// struct Foo { a: Field, b: Field }
// ^ ^
20 changes: 19 additions & 1 deletion src/stdlib/native/int/lib.no
Original file line number Diff line number Diff line change
@@ -291,4 +291,22 @@ fn Uint32.mod(self, rhs: Uint32) -> Uint32 {
fn Uint64.mod(self, rhs: Uint64) -> Uint64 {
let res = self.divmod(rhs);
return res[1];
}
}

// implement to field
fn Uint8.to_field(self) -> Field {
return self.inner;
}

fn Uint16.to_field(self) -> Field {
return self.inner;
}

fn Uint32.to_field(self) -> Field {
return self.inner;
}

fn Uint64.to_field(self) -> Field {
return self.inner;
}

2 changes: 1 addition & 1 deletion src/tests/modules.rs
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ use mimoo::liblib;

// test a library's type that links to its own type
struct Inner {
inner: Field,
pub inner: Field,
}

struct Lib {
2 changes: 1 addition & 1 deletion src/tests/stdlib/uints/mod.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ fn main(pub lhs: Field, rhs: Field) -> Field {

let res = lhs_u.{opr}(rhs_u);

return res.inner;
return res.to_field();
}
"#;

38 changes: 30 additions & 8 deletions src/type_checker/checker.rs
Original file line number Diff line number Diff line change
@@ -11,8 +11,8 @@ use crate::{
imports::FnKind,
parser::{
types::{
is_numeric, FnSig, ForLoopArgument, FunctionDef, ModulePath, Stmt, StmtKind, Symbolic,
Ty, TyKind,
is_numeric, Attribute, AttributeKind, FnSig, ForLoopArgument, FuncOrMethod,
FunctionDef, Stmt, StmtKind, Symbolic, Ty, TyKind, ModulePath,
},
CustomType, Expr, ExprKind, Op2,
},
@@ -58,7 +58,7 @@ impl<B: Backend> FnInfo<B> {
#[derive(Deserialize, Serialize, Default, Debug, Clone)]
pub struct StructInfo {
pub name: String,
pub fields: Vec<(String, TyKind)>,
pub fields: Vec<(String, TyKind, Option<Attribute>)>,
pub methods: HashMap<String, FunctionDef>,
}

@@ -119,14 +119,36 @@ impl<B: Backend> TypeChecker<B> {
.expect("this struct is not defined, or you're trying to access a field of a struct defined in a third-party library (TODO: better error)");

// find field type
let res = struct_info
if let Some((_, field_typ, attribute)) = struct_info
.fields
.iter()
.find(|(name, _)| name == &rhs.value)
.map(|(_, typ)| typ.clone());
.find(|(field_name, _, _)| field_name == &rhs.value)
{
// check for the pub attribute
let is_public = matches!(
attribute,
&Some(Attribute {
kind: AttributeKind::Pub,
..
})
);

// check if we're inside a method of the same struct
let in_method = matches!(
typed_fn_env.current_fn_kind(),
FuncOrMethod::Method(method_struct) if method_struct.name == struct_name
);

if let Some(res) = res {
Some(ExprTyInfo::new(lhs_node.var_name, res))
if is_public || in_method {
// allow access
Some(ExprTyInfo::new(lhs_node.var_name, field_typ.clone()))
} else {
// block access
Err(self.error(
ErrorKind::PrivateFieldAccess(struct_name.clone(), rhs.value.clone()),
expr.span,
))?
}
} else {
return Err(self.error(
ErrorKind::UndefinedField(struct_info.name.clone(), rhs.value.clone()),
23 changes: 18 additions & 5 deletions src/type_checker/fn_env.rs
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ use std::collections::HashMap;
use crate::{
constants::Span,
error::{Error, ErrorKind, Result},
parser::types::TyKind,
parser::types::{FuncOrMethod, TyKind},
};

/// Some type information on local variables that we want to track in the [TypedFnEnv] environment.
@@ -39,7 +39,7 @@ impl TypeInfo {
}

/// The environment we use to type check functions.
#[derive(Default, Debug, Clone)]
#[derive(Debug, Clone)]
pub struct TypedFnEnv {
/// The current nesting level.
/// Starting at 0 (top level), and increasing as we go into a block.
@@ -55,12 +55,21 @@ pub struct TypedFnEnv {

/// Determines if forloop variables are allowed to be accessed.
forbid_forloop_scope: bool,

/// The kind of function we're currently type checking
current_fn_kind: FuncOrMethod,
}

impl TypedFnEnv {
/// Creates a new TypeEnv
pub fn new() -> Self {
Self::default()
/// Creates a new TypeEnv with the given function kind
pub fn new(fn_kind: &FuncOrMethod) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure there is a more elegant way of doing this. The reason I went with this is that since current_fn_kind is not an option, calling Self::default() calls the default constructor of FuncOrMethod as well, which calls unreachable!:

unreachable!()

How can we do it better?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.
With FuncOrMethod, I think it should be always calling ::new to specify the fn_kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I didn't fully understand the last part. Do you mean the default() for FuncOrMethod should change (return a new object instead of unreachable) or is the current approach okay as is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant default() should be removed, if it is not used anymore.

Self {
current_scope: 0,
vars: HashMap::new(),
forloop_scopes: Vec::new(),
forbid_forloop_scope: false,
current_fn_kind: fn_kind.clone(),
}
}

/// Enters a scoped block.
@@ -182,4 +191,8 @@ impl TypedFnEnv {
Ok(None)
}
}

pub fn current_fn_kind(&self) -> &FuncOrMethod {
&self.current_fn_kind
}
}
6 changes: 3 additions & 3 deletions src/type_checker/mod.rs
Original file line number Diff line number Diff line change
@@ -298,8 +298,8 @@ impl<B: Backend> TypeChecker<B> {
let fields: Vec<_> = fields
.iter()
.map(|field| {
let (name, typ) = field;
(name.value.clone(), typ.kind.clone())
let (name, typ, attribute) = field;
(name.value.clone(), typ.kind.clone(), attribute.clone())
})
.collect();

@@ -329,7 +329,7 @@ impl<B: Backend> TypeChecker<B> {
// `fn main() { ... }`
RootKind::FunctionDef(function) => {
// create a new typed fn environment to type check the function
let mut typed_fn_env = TypedFnEnv::default();
let mut typed_fn_env = TypedFnEnv::new(&function.sig.kind);

// if we're expecting a library, this should not be the main function
let is_main = function.is_main();