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

feat: Remove comptime and warn upon usage #2178

Merged
merged 15 commits into from
Aug 8, 2023
Merged

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Aug 4, 2023

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. The comptime 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 the comptime tracking did not catch.

Example error message for a loop whose length is not known at compile-time:

error: 
   ┌─ /.../noir/short/src/main.nr:28:22
   │
28 │         for _i in 0..self.count {
   │                      ---------- Could not determine loop bound 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.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.
      • All references to 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.
      • We should also remove the 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 between comptime and rust's const, 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 follow comptime 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*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@TomAFrench
Copy link
Member

comptime has been removed because it is no longer needed for accessing arrays.

It's still cheaper to perform comptime array accesses, no?

@jfecher
Copy link
Contributor Author

jfecher commented Aug 4, 2023

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 phated changed the title feat: Remove comptime feat!: Remove comptime Aug 4, 2023
@jfecher jfecher changed the title feat!: Remove comptime feat: Remove comptime Aug 4, 2023
@jfecher
Copy link
Contributor Author

jfecher commented Aug 4, 2023

@phated this should not be a breaking change, comptime is still in the language but is ignored and issues a warning when used now.

@phated
Copy link
Contributor

phated commented Aug 4, 2023

The PR title should indicate that then. I'll update.

@phated phated changed the title feat: Remove comptime feat: Warn on usage of comptime Aug 4, 2023
@jfecher
Copy link
Contributor Author

jfecher commented Aug 4, 2023

@phated it's more than just warning though since comptime is removed from the compiler internally. Perhaps Deprecate comptime? It is removed from the language itself, it is just that we keep parser support for it to issue deprecation warnings

@phated
Copy link
Contributor

phated commented Aug 4, 2023

That's a tough one, I think I'd call it breaking then since it changes programs. "feat!: Remove comptime and warn upon usage"

@jfecher
Copy link
Contributor Author

jfecher commented Aug 4, 2023

This shouldn't break any existing programs and the circuits generated should be unchanged as well since comptime in the type system never affected codegen

@jfecher jfecher changed the title feat: Warn on usage of comptime feat: Remove comptime and warn upon usage Aug 4, 2023
@Savio-Sou
Copy link
Collaborator

cc @critesjosh @signorecello in case if you have any thoughts from external devs' perspectives on the PR.

kevaundray
kevaundray previously approved these changes Aug 8, 2023
Copy link
Contributor

@kevaundray kevaundray left a 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

@kevaundray kevaundray mentioned this pull request Aug 8, 2023
@kevaundray kevaundray added this pull request to the merge queue Aug 8, 2023
Merged via the queue into master with commit 98d0de3 Aug 8, 2023
7 checks passed
@kevaundray kevaundray deleted the jf/remove-comptime branch August 8, 2023 20:58
TomAFrench added a commit that referenced this pull request Aug 10, 2023
* 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)
  ...
TomAFrench added a commit that referenced this pull request Aug 11, 2023
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants