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

Support hint function declaration to bind with builtin function #199

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

katat
Copy link
Collaborator

@katat katat commented Oct 9, 2024

This is the first step to support hint function, by making use the builtin functions as hint functions.

Although it is not yet fully supporting the native hint function, this PR makes it able to type check the unsafe/hint attribute.

Usage:

Declares a hint function in a .no file to wire up with a builtin function under the same full qualified name.

hint fn calc(val: Field) -> Field;

It requires unsafe attribute when calling a hint function, otherwise the type checker will throw error. So the hint function caller has to use unsafe to acknowledge they are calling a hint function.

fn main(pub xx: Field) {
    let yy = unsafe calc(xx);
}

This can make the type checking ready for hint calculations before the native hint function support.
Once the hint function support is ready, we can just fill the hint function body in the native code and deprecate the hint builtin functions, while keeping existing hint function callers intact.

@katat katat changed the base branch from main to fix/mono-node-override October 9, 2024 09:45
@katat katat force-pushed the feat/hint-builtin-wir branch from 87ad74a to 5353d42 Compare October 9, 2024 09:47
@katat katat changed the base branch from fix/mono-node-override to main October 18, 2024 09:19
Copy link
Contributor

@mimoo mimoo 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'll have another pass but let's not wait to merge it!

src/parser/mod.rs Outdated Show resolved Hide resolved
// unsafe attribute should only be used on hints
if !fn_info.is_hint && *unsafe_attr {
return Err(self.error(ErrorKind::UnexpectedUnsafeAttribute, expr.span));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe more concise:

match (fn_info.is_hint, unsafe_attr) {
  (true, true) | (false, false) => (),
  _ => return Err(self.error(ErrorKind::ExpectedUnsafeAttribute, expr.span));
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually there are two error types to handle:

let res = test_hint_builtin_fn(&qualified, invalid_code);
assert!(matches!(
res.unwrap_err().kind,
ErrorKind::ExpectedUnsafeAttribute
));
}
#[test]
fn test_nonhint_call_with_unsafe() {
let code = r#"
fn calc(val: Field) -> Field {
return val + 1;
}
fn main(pub xx: Field) {
let yy = unsafe calc(xx);
}
"#;
let res = tast_pass(code).0;
assert!(matches!(
res.unwrap_err().kind,
ErrorKind::UnexpectedUnsafeAttribute
));
}

}

/// Parse a hint function signature
pub fn parse_hint(ctx: &mut ParserCtx, tokens: &mut Tokens) -> Result<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it'd make sense to merge this with the parse function above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This new parser function is for hint function without a body.
So I think once we support the native hints, we can just remove this function, while the native hint function can get parsed using parser function above without changes.

@katat katat closed this Oct 24, 2024
@katat katat reopened this Oct 24, 2024
@katat katat merged commit 78ff4ba into main Oct 24, 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.

2 participants