-
Notifications
You must be signed in to change notification settings - Fork 204
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
feat: Remove comptime
and warn upon usage
#2178
Conversation
It's still cheaper to perform comptime array accesses, no? |
It is cheaper, but this was determined by how many optimizations the ssa could do rather than whether it was classified as such by the type checker. So the runtime is unchanged but it may be more difficult for users to track whether something is comptime in complex cases. |
@phated this should not be a breaking change, |
The PR title should indicate that then. I'll update. |
@phated it's more than just warning though since |
That's a tough one, I think I'd call it breaking then since it changes programs. "feat!: Remove |
This shouldn't break any existing programs and the circuits generated should be unchanged as well since |
comptime
comptime
and warn upon usage
cc @critesjosh @signorecello in case if you have any thoughts from external devs' perspectives on the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - some small nits
* master: (80 commits) fix: properly capture lvalues in closure environments (#2120) (#2257) fix: Optimize contracts built by `nargo info` (#2259) chore: impl Display for DebugType (#2258) chore: update `noir_wasm` build process to match `acvm_js` (#2067) feat: Implement traits - parser support #2094 (#2230) chore: Refactor DefCollector duplicate errors (#2231) chore: Address clippy warnings (#2246) feat: Support `contract` package type in `nargo info` command (#2249) feat: Add slice append (#2241) chore: Bump `async-lsp` to v0.0.5 (#2186) chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225) chore: Add test for eddsa (#2237) chore: Split `Nargo.toml` operations into separate package (#2224) fix(stdlib): correct `tecurve::contains` formula (#1821) feat: Remove `comptime` and warn upon usage (#2178) fix: Remove last vestige of array of structs to struct of arrays conversion (#2217) fix: Add foreign impl error (#2216) feat(nargo)!: Replace `--contracts` flag with `contract` package type (#2204) feat: Optimize `x < 0` for unsigned `x` to false (#2206) fix: Initialize numeric generics' type to a polymorphic integer when used in an expression (#2179) ...
* master: feat: Add slice append (#2241) chore: Bump `async-lsp` to v0.0.5 (#2186) chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225) chore: Add test for eddsa (#2237) chore: Split `Nargo.toml` operations into separate package (#2224) fix(stdlib): correct `tecurve::contains` formula (#1821) feat: Remove `comptime` and warn upon usage (#2178) fix: Remove last vestige of array of structs to struct of arrays conversion (#2217) fix: Add foreign impl error (#2216) feat(nargo)!: Replace `--contracts` flag with `contract` package type (#2204) feat: Optimize `x < 0` for unsigned `x` to false (#2206) fix: Initialize numeric generics' type to a polymorphic integer when used in an expression (#2179) chore(nargo)!: remove `flat_witness` feature flag (#2208) chore: Revert "feat: Only create new witnesses for distinctiveness when duplicates exist" (#2209)
Description
Problem
Resolves #2040
Resolves #280
Resolves #1188
Resolves #793
Resolves #1035 - the failure to unroll the for loop is now an error instead of a panic
Summary
This PR removes
comptime
from the language. Thecomptime
keyword and syntax are currently still kept and parsed for backwards compatibility, but are now deprecated and will issue a warning when used.comptime
has been removed because it is no longer needed for accessing arrays. However, since for loops still must be known at compile-time I've taken the liberty of adding an error message for this which also solves some past panics that thecomptime
tracking did not catch.Example error message for a loop whose length is not known at compile-time:
The error message will always correctly point to only the part of the range that is unknown (start or end). If both are unknown, only the start range error is issued since we halt on the first error.
Documentation
This PR requires documentation updates when merged.
comptime
can be removed. We should still mention that the bounds of a for loop need to be known at compile-time. Since the compiler inlines every function, function parameters are still okay for use as for loop bounds.RepeatedValue
example. It has been broken in Noir for a while.Additional Context
I'd like some discussion on this PR. Removing
comptime
simplifies the system and removes the confusing subtle differences betweencomptime
and rust'sconst
, but also means it may be more difficult for users to track why a for loop's bounds may or may not be known. Instead of being able to followcomptime
annotations in the program, they will only get an error on the for loop itself and would have to work backward to figure out why the bounds are not compile-time known.PR Checklist*
cargo fmt
on default settings.