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

Adds tuple type #261

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Adds tuple type #261

merged 5 commits into from
Jan 17, 2025

Conversation

0xnullifier
Copy link
Contributor

closes #255

the example file can be found here tuple.no this works and runs the output is:
image

I have not implemented destructuring as that is potentially another pr.

The code is pretty much identical to array type as tuple is just a multi-type array

and also as @mimoo suggested that we should keep the tuple access similar to array access. I have just changes the code for ExprKind::ArrayAccess to ExprKind::ArrayOrTupleAccess as they are identical at lexer level and dealt with this at type checker and writer.

it also refactors the array logging and combines it to array or tuple logging

@0xnullifier
Copy link
Contributor Author

0xnullifier commented Jan 11, 2025

We can also do a better example but I cannot just think of it

maybe we can change some stdlib code with a tuple type for cleaner code?

@0xnullifier
Copy link
Contributor Author

hey @katat can you have a look thanks!

Copy link
Collaborator

@katat katat left a comment

Choose a reason for hiding this comment

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

Looks good. I left you some comment.

The key issue is the way of calculating the starting index to narrow the cell variables for an tuple element in the circuit writer and IR writer.

Also I would suggest having a separate expression type TupleAccess, instead of merging both the array access and tuple access into ArrayOrTupleAccess. Having them separated is to avoid the potential assumption confusions such as the calculation of the start index, although some of the codes are duplicated, which is fine.

src/type_checker/checker.rs Outdated Show resolved Hide resolved
src/type_checker/checker.rs Outdated Show resolved Hide resolved
src/parser/types.rs Outdated Show resolved Hide resolved
src/parser/types.rs Outdated Show resolved Hide resolved
src/type_checker/checker.rs Outdated Show resolved Hide resolved
examples/tuple.no Show resolved Hide resolved
src/circuit_writer/writer.rs Outdated Show resolved Hide resolved
src/circuit_writer/writer.rs Outdated Show resolved Hide resolved
src/circuit_writer/writer.rs Outdated Show resolved Hide resolved
src/circuit_writer/ir.rs Outdated Show resolved Hide resolved
@0xnullifier
Copy link
Contributor Author

Also I would suggest having a separate expression type TupleAccess, instead of merging both the array access and tuple access into ArrayOrTupleAccess.

@katat how can you go about doing this cause at the parser they are exactly same and only at the type checker can we distinguish that whether it was ArrayAccess or TupleAccess.

Is there any way for me to find the type of Variable we access in the parser like here

  // array or tuple access
  Some(Token {
      kind: TokenKind::LeftBracket,
      ..
  }) => {
     // do some type checking here ? to find whether self is tuple variable or not

      Expr::new(
          ctx,
          ExprKind::ArrayAccess {
              container: Box::new(self),
              idx: Box::new(idx),
          },
          span,
      )
  }

One way I can do this is define a ExprKind::Access here and then at the typechecker mutate the variable but this would result in a mutable expr in the compute_type function.

@katat
Copy link
Collaborator

katat commented Jan 16, 2025

Yeah, indeed the parser doesn't have enough information to know whether it is array or tuple. Changing the Expr type at later phase might complicate the flow also.

So looks like we have to keep ArrayOrTupleAccess .

@0xnullifier
Copy link
Contributor Author

0xnullifier commented Jan 16, 2025

The key issue is the way of calculating the starting index to narrow the cell variables for an tuple element in the circuit writer and IR writer.

@katat Fixed this and Also generics did not work forTyKind::Tuple now they do. I have also added an example with a generic array inside tuple

Copy link
Collaborator

@katat katat left a comment

Choose a reason for hiding this comment

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

Lgtm. I left some minor comments.

nohup.out Outdated Show resolved Hide resolved
src/circuit_writer/ir.rs Outdated Show resolved Hide resolved
src/circuit_writer/writer.rs Outdated Show resolved Hide resolved
src/circuit_writer/writer.rs Outdated Show resolved Hide resolved
src/mast/mod.rs Outdated Show resolved Hide resolved
@katat katat merged commit 1bcd765 into zksecurity:main Jan 17, 2025
1 check passed
@0xnullifier 0xnullifier deleted the feat-tuple branch January 17, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support tuple return type
2 participants