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 boolean flag to skip argument type checking of builtin functions. #231

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

bufferhe4d
Copy link
Contributor

Attempts to resolve #228.

Here is my approach:

As suggested in #228 I first introduced a new flag for the FnKind::Builtin called ignore_arg_types:

https://github.com/bufferhe4d/noname/blob/b243dce51ddbb96babf627fd3c1a8b5266d261ab/src/imports.rs#L78

Propagated this to all the occurrences of this. Then implemented the actual skip in the type checker.

@@ -833,6 +835,19 @@ impl<B: Backend> TypeChecker<B> {
None => (),
};

// get the ignore_arg_types flag from the function info if it's a builtin
let ignore_arg_types = match 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.

Seems to work for me but I am not sure if this is the correct way to check if a function is builtin.

@@ -392,7 +392,8 @@ impl<B: Backend> TypeChecker<B> {
qualified,
FnInfo {
is_hint: true,
kind: FnKind::BuiltIn(function.sig.clone(), fn_handle),
// todo: is there a case where we want to ignore argument types for hint functions?
kind: FnKind::BuiltIn(function.sig.clone(), fn_handle, false),
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 couldn't immediately decide what to do here since we first convert a hint function to a builtin one. It felt like we would never disable argument type checking for a hint function so I defaulted to false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The builtin hints will be deprecated soon. So don't worry bout this.
Good insight!

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!
@mimoo may want another pass

@@ -392,7 +392,8 @@ impl<B: Backend> TypeChecker<B> {
qualified,
FnInfo {
is_hint: true,
kind: FnKind::BuiltIn(function.sig.clone(), fn_handle),
// todo: is there a case where we want to ignore argument types for hint functions?
kind: FnKind::BuiltIn(function.sig.clone(), fn_handle, false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The builtin hints will be deprecated soon. So don't worry bout this.
Good insight!

@mimoo
Copy link
Contributor

mimoo commented Nov 14, 2024

maybe we should check if this allows us to implement what we wanted to implement on top of that?

@varunthakore do you want to try to implement a generic log() function on top of this branch to see if it works?

@bufferhe4d if you want to do something similar, a generic assert_eq() function would be great, as long as it checks that the two types passed as arguments match

@bufferhe4d
Copy link
Contributor Author

bufferhe4d commented Nov 22, 2024

@bufferhe4d if you want to do something similar, a generic assert_eq() function would be great, as long as it checks that the two types passed as arguments match

I currently have an assert_eq that is working with all the types (Field, Bool, arbitrary depth structs and arrays) in the language (using the boolean flag we introduced in this PR). It only checks whether the two argument types matched as you described.

Since it requires the flag included here, should I push this functionality to this branch to make it a part of this PR or should I wait until this is merged to open a separate PR?

@mimoo
Copy link
Contributor

mimoo commented Nov 24, 2024

can you open a separate PR that targets this branch? Even if it doesn't work because this branch is on a remote repo, can you still open a separate PR?

also can you fix the CI issues here? It seems like there's some formatting issues

@bufferhe4d
Copy link
Contributor Author

I created a draft PR from my generic_assert_eq here: bufferhe4d#2

The CI should be fixed now.

@katat katat merged commit c21739d into zksecurity:main Dec 10, 2024
1 check passed
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.

make log work with any types
3 participants