Add pub attribute for struct fields #232

bufferhe4d · 2024-11-12T22:33:22Z

Attempts to resolve #224

Here is my approach:

I introduced a new component Option<Attribute> to fields in order to make use of the already defined Pub attribute 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 TypedFnEnv called current_fn_kind that keeps track of the function we are in. This is further utilized in the type checker.

katat

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.

katat · 2024-11-13T09:48:59Z

src/negative_tests.rs

+    fn main(pub beds: Field) {
+        let mut room = Room {beds: 2, size: 10};
+        room.beds = beds; // allowed
+        room.size = 5; // not allowed


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

agree ^ we can simplify the test

src/inputs.rs

src/name_resolution/context.rs

katat · 2024-11-14T08:41:08Z

@mimoo may want another pass

mimoo

left a few more comments! Sorry for the delay

mimoo · 2024-11-20T11:54:22Z

src/parser/structs.rs

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


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)

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

mimoo · 2024-11-20T11:56:57Z

src/parser/structs.rs

+                tokens.bump(ctx);
+                Some(Attribute {
+                    kind: AttributeKind::Pub,
+                    span: ctx.last_span(),


should be the span from the matching token

mimoo · 2024-11-20T12:12:34Z

src/stdlib/native/int/lib.no

@@ -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,


imo we shouldn't expose it like this, we can have a to_field function but we shouldn't allow direct mutation

Indeed, having this as private was failing the tests here:

noname/src/tests/stdlib/uints/mod.rs

Line 17 in b17ea20

return res.inner;

I changed it to to_field() instead now.

mimoo · 2024-11-20T12:15:19Z

src/type_checker/fn_env.rs

@@ -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>,


why is it an option?

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.

mimoo · 2024-11-20T12:16:26Z

src/type_checker/mod.rs

@@ -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());


can you just do let mut typed_fn_env = TypeFnEnv::new(&function.sig.kind);

mimoo · 2024-11-20T12:20:26Z

src/type_checker/checker.rs

+                    let is_public = attribute
+                        .as_ref()
+                        .map(|attr| matches!(attr.kind, AttributeKind::Pub))
+                        .unwrap_or(false);


can't you just do let is_public = matches!(attribute, &Some(Attribute { kind: AttributeKind::Pub, .. })) ?

(btw to avoid the option we could also have a AttributeKind::None)

mimoo · 2024-11-20T12:24:38Z

src/type_checker/checker.rs

+                        false
+                    };


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);

bufferhe4d · 2024-11-22T17:42:33Z

Alright, I did another iteration after the comments from @mimoo. I have one question about how we initialize the TypedFnEnv now below.

bufferhe4d · 2024-11-22T17:46:00Z

src/type_checker/fn_env.rs

-    pub fn new() -> Self {
-        Self::default()
+    /// Creates a new TypeEnv with the given function kind
+    pub fn new(fn_kind: &FuncOrMethod) -> Self {


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!:

noname/src/parser/types.rs

Line 739 in b17ea20

unreachable!()

How can we do it better?

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

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?

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

bufferhe4d added 2 commits November 12, 2024 23:24

implement pub attribute for struct fields

90903bb

fix tests with private field access.

3ba79be

katat approved these changes Nov 14, 2024

View reviewed changes

bufferhe4d added 4 commits November 18, 2024 17:18

formatting and fixing unused variables.

c39bcde

Merge branch 'zksecurity:main' into struct-pub-fields-224

ac98569

fix ir.s

f811633

fix hint example to use pub

e472009

mimoo reviewed Nov 20, 2024

View reviewed changes

bufferhe4d added 5 commits November 22, 2024 18:35

Simplify negative test

5eebfa9

fix: token span

48e4e5a

change Uint visibility to private

8085c27

Simplify type checker

99fc9cc

fix formatting

416884f

bufferhe4d commented Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pub attribute for struct fields #232

Add pub attribute for struct fields #232

bufferhe4d commented Nov 12, 2024

katat left a comment

katat Nov 13, 2024

mimoo Nov 20, 2024

katat commented Nov 14, 2024

mimoo left a comment •

edited

Loading

mimoo Nov 20, 2024

mimoo Nov 20, 2024

mimoo Nov 20, 2024

mimoo Nov 20, 2024

bufferhe4d Nov 22, 2024

mimoo Nov 20, 2024

bufferhe4d Nov 22, 2024

mimoo Nov 20, 2024

mimoo Nov 20, 2024

mimoo Nov 20, 2024

mimoo Nov 20, 2024

bufferhe4d commented Nov 22, 2024

bufferhe4d Nov 22, 2024

katat Dec 10, 2024

bufferhe4d Dec 12, 2024

katat Dec 13, 2024

Add pub attribute for struct fields #232

Are you sure you want to change the base?

Add pub attribute for struct fields #232

Conversation

bufferhe4d commented Nov 12, 2024

katat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katat commented Nov 14, 2024

mimoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufferhe4d commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo left a comment •

edited

Loading