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

Builtin functions. #920

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Builtin functions. #920

merged 4 commits into from
Jan 23, 2024

Conversation

chriseth
Copy link
Member

No description provided.

@chriseth chriseth marked this pull request as ready for review January 19, 2024 12:43
@chriseth chriseth force-pushed the builtins branch 2 times, most recently from a20d591 to 896ea2e Compare January 19, 2024 13:30
/// This is a built-in function taking a string argument and terminating
/// evaluation unsuccessfully with this argument as explanation.
/// This symbol is not an empty array, the actual semantics are overridden.
let panic = [];
Copy link
Member

Choose a reason for hiding this comment

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

This is quite confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

The main other way I could think of is something like:

std::core:
let builtin = |name| (); // The only weird thing
std::check:
let panic = std::core::builtin("panic");

And then builtin would be the only real builtin function.

Copy link
Member Author

Choose a reason for hiding this comment

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

although this would not pass strict type checking

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, we could use something like let panic = std::core::builtin::<"panic">();

Copy link
Member

Choose a reason for hiding this comment

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

if it's already built-in, why not make it fully built-in?

Copy link
Member

Choose a reason for hiding this comment

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

so we keep it like this for now?

test_data/asm/pil_at_module_level.asm Outdated Show resolved Hide resolved
test_data/pil/book/generic_to_algebraic.pil Outdated Show resolved Hide resolved
test_data/pil/arith_improved.pil Outdated Show resolved Hide resolved
/// This is a built-in function taking an array argument and returning
/// the length of the array.
/// This symbol is not an empty array, the actual semantics are overridden.
let len = [];
Copy link
Member

Choose a reason for hiding this comment

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

This is also confusing. Why is it done like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I just use let len;, then it will be a witness column. I have to assign something.
Although thinking about it, maybe it could work?
But in any case, since we don't have any annotations or stuff like this, it has to be weird in some way.

@chriseth chriseth force-pushed the builtins branch 5 times, most recently from 521a4e0 to fba1089 Compare January 22, 2024 14:21
@leonardoalt
Copy link
Member

if you rebase circle vanishes :D

@chriseth
Copy link
Member Author

rebased

@@ -130,12 +137,27 @@ impl<'a, T: FieldElement, C: Custom> Value<'a, T, C> {
format!("[{}]", elements.iter().map(|e| e.type_name()).format(", "))
}
Value::Closure(c) => c.type_name(),
Value::BuiltinFunction(b) => format!("builtin{b:?}"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe "builtin::{b:?}"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this should usually be the type name of a closure. Let me make it proper.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, turns out we don't even know the types in a closure.

What about builtin_? I don't particularly like :: because it is not a namespace.

As soon as we add proper types, this will be replaced anyway.

leonardoalt
leonardoalt previously approved these changes Jan 23, 2024
@leonardoalt leonardoalt added this pull request to the merge queue Jan 23, 2024
@leonardoalt leonardoalt removed this pull request from the merge queue due to a manual request Jan 23, 2024
@chriseth chriseth changed the base branch from main to rename-crates January 23, 2024 19:30
Base automatically changed from rename-crates to main January 23, 2024 19:30
@chriseth chriseth enabled auto-merge January 23, 2024 19:38
@chriseth chriseth added this pull request to the merge queue Jan 23, 2024
Merged via the queue into main with commit 25cfdbd Jan 23, 2024
2 checks passed
@chriseth chriseth deleted the builtins branch January 23, 2024 20:57
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.

3 participants