Add pub attribute for struct fields #232
Conversation
There was a problem hiding this comment.
Nice!
It would be great to support modifying the struct fields inside the methods, as this pub is also about a whitelist to controlling the values of a struct.
Maybe we can create another PR to implement the mut [arg] as described here.
| fn main(pub beds: Field) { | ||
| let mut room = Room {beds: 2, size: 10}; | ||
| room.beds = beds; // allowed | ||
| room.size = 5; // not allowed |
There was a problem hiding this comment.
We can just test the negative case here. Then the positive tests should cover the rest.
There was a problem hiding this comment.
agree ^ we can simplify the test
|
@mimoo may want another pass |
| @@ -55,6 +55,26 @@ impl StructDef { | |||
| tokens.bump(ctx); | |||
| break; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
at the same time it seems OK to allow it...
src/parser/structs.rs
Outdated
| tokens.bump(ctx); | ||
| Some(Attribute { | ||
| kind: AttributeKind::Pub, | ||
| span: ctx.last_span(), |
There was a problem hiding this comment.
should be the span from the matching token
src/stdlib/native/int/lib.no
Outdated
| @@ -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.
imo we shouldn't expose it like this, we can have a to_field function but we shouldn't allow direct mutation
There was a problem hiding this comment.
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.
src/type_checker/fn_env.rs
Outdated
| @@ -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.
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.
src/type_checker/mod.rs
Outdated
| @@ -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.
can you just do let mut typed_fn_env = TypeFnEnv::new(&function.sig.kind);
src/type_checker/checker.rs
Outdated
| let is_public = attribute | ||
| .as_ref() | ||
| .map(|attr| matches!(attr.kind, AttributeKind::Pub)) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
can't you just do let is_public = matches!(attribute, &Some(Attribute { kind: AttributeKind::Pub, .. })) ?
There was a problem hiding this comment.
(btw to avoid the option we could also have a AttributeKind::None)
src/type_checker/checker.rs
Outdated
| false | ||
| }; |
There was a problem hiding this comment.
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);
|
Alright, I did another iteration after the comments from @mimoo. I have one question about how we initialize the TypedFnEnv now below. |
| pub fn new() -> Self { | ||
| Self::default() | ||
| /// Creates a new TypeEnv with the given function kind | ||
| pub fn new(fn_kind: &FuncOrMethod) -> Self { |
There was a problem hiding this comment.
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!:
Line 739 in b17ea20
How can we do it better?
Attempts to resolve #224
Here is my approach:
I introduced a new component
Option<Attribute>tofieldsin order to make use of the already definedPubattribute that is used for public inputs.The tricky part was to check whether we are inside a structs method or not. For that I introduced a new variable inside
TypedFnEnvcalledcurrent_fn_kindthat keeps track of the function we are in. This is further utilized in the type checker.