-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
90903bb
3ba79be
c39bcde
ac98569
f811633
e472009
5eebfa9
48e4e5a
8085c27
99fc9cc
416884f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
|
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) { | ||
|
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) { | ||
|
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; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
.. | ||
}) | ||
) { | ||
tokens.bump(ctx); | ||
Some(Attribute { | ||
kind: AttributeKind::Pub, | ||
span: ctx.last_span(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be the span from the matching token |
||
}) | ||
} 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 } | ||
// ^ ^ | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -8,7 +8,7 @@ hint fn divmod(dividend: Field, divisor: Field) -> [Field; 2]; | |||
// u8 | ||||
// must use new() to create a Uint8, so the value is range checked | ||||
struct Uint8 { | ||||
inner: Field, | ||||
pub inner: Field, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo we shouldn't expose it like this, we can have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, having this as private was failing the tests here: noname/src/tests/stdlib/uints/mod.rs Line 17 in b17ea20
I changed it to to_field() instead now. |
||||
} | ||||
|
||||
fn Uint8.new(val: Field) -> Uint8 { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,8 @@ use crate::{ | |
imports::FnKind, | ||
parser::{ | ||
types::{ | ||
is_numeric, FnSig, ForLoopArgument, FunctionDef, Stmt, StmtKind, Symbolic, Ty, TyKind, | ||
is_numeric, Attribute, AttributeKind, FnSig, ForLoopArgument, FuncOrMethod, | ||
FunctionDef, Stmt, StmtKind, Symbolic, Ty, TyKind, | ||
}, | ||
CustomType, Expr, ExprKind, Op2, | ||
}, | ||
|
@@ -55,7 +56,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>, | ||
} | ||
|
||
|
@@ -116,14 +117,39 @@ 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 = attribute | ||
.as_ref() | ||
.map(|attr| matches!(attr.kind, AttributeKind::Pub)) | ||
.unwrap_or(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't you just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (btw to avoid the option we could also have a |
||
|
||
// check if we're inside a method of the same struct | ||
let in_method = if let Some(fn_kind) = typed_fn_env.current_fn_kind() { | ||
match fn_kind { | ||
FuncOrMethod::Method(method_struct) => { | ||
method_struct.name == struct_name | ||
} | ||
FuncOrMethod::Function(_) => false, | ||
} | ||
} else { | ||
false | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's gotta be a way to decrease the LOC here as well. Maybe let in_method = matches!(typed_fn_env.fn_kind, FuncOrMethod::Method(struc) if struc.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()), | ||
|
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. | ||
|
@@ -55,6 +55,9 @@ 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: Option<FuncOrMethod>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is it an option? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, I thought we have global scope in the language and we might not be in a function while type checking. Should be fixed now. |
||
} | ||
|
||
impl TypedFnEnv { | ||
|
@@ -182,4 +185,12 @@ impl TypedFnEnv { | |
Ok(None) | ||
} | ||
} | ||
|
||
pub fn current_fn_kind(&self) -> Option<&FuncOrMethod> { | ||
self.current_fn_kind.as_ref() | ||
} | ||
|
||
pub fn set_current_fn_kind(&mut self, kind: FuncOrMethod) { | ||
self.current_fn_kind = Some(kind); | ||
} | ||
} |
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(); | ||
|
||
|
@@ -331,6 +331,8 @@ impl<B: Backend> TypeChecker<B> { | |
// create a new typed fn environment to type check the function | ||
let mut typed_fn_env = TypedFnEnv::default(); | ||
|
||
typed_fn_env.set_current_fn_kind(function.sig.kind.clone()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you just do |
||
|
||
// if we're expecting a library, this should not be the main function | ||
let is_main = function.is_main(); | ||
if is_main && is_lib { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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