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.

Sign up for GitHub

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

Jump to bottom

feat: Syntax for environment types now works with generics #2383

Merged
merged 9 commits into from
Aug 28, 2023
Merged

feat: Syntax for environment types now works with generics #2383

merged 9 commits into from
Aug 28, 2023

Conversation

alexvitkov
Copy link
Contributor

Description

Problem*

The syntax for closure environment types that I added in #2357 is sub-optimal as it does not play well with generics.

Resolves: #2334

Summary*

  • As suggested in feat: add syntax for specifying function type environments #2357 (comment) this PR changes it changed from fn[envarg1, envarg2](arg1, arg2, ...) -> ret to fn[env_type](arg1, arg2, ...) -> ret so that a named generic can be used an environment.

  • Additional error checking is also added, since the new syntax allows passing any type as an environment, but the only valid ones are Unit/NamedGeneric/Tuple.

  • Added tests for generic closure environments

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.

Additional Context

PR Checklist*

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

@alexvitkov alexvitkov changed the title fix: Syntax for environment types now works with generics feat: Syntax for environment types now works with generics Aug 21, 2023
@jfecher jfecher mentioned this pull request Aug 21, 2023
Closed
9 tasks
jfecher
jfecher requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good but if we move the error check to the parser we can get it a bit cleaner

crates/noirc_frontend/src/hir/resolution/resolver.rs Outdated Show resolved Hide resolved
@alexvitkov alexvitkov mentioned this pull request Aug 22, 2023
Merged
5 tasks
@jfecher
Copy link
Contributor

jfecher commented Aug 22, 2023

After testing out this PR, I think it is too confusing to report the error in a different source location. Can you add source locations to UnresolvedTypes so that the error location is correct?

@alexvitkov alexvitkov force-pushed the closure-syntax-change branch from 33e2701 to 4f5cf9e Compare August 23, 2023 06:10
@alexvitkov
Copy link
Contributor Author

I've added spans to UnresolvedType, the error now looks like this:

error: asdkj is not a valid closure environment type
  ┌─ /home/vitkov/code/repos/metacraft-labs/noir/proj/src/main.nr:4:16
  │
4 │ fn bad(foo: fn[asdkj]() -> Field) {
  │                ----- Closure environment must be a tuple or unit type

I thought about submitting a new PR, but it turned out not to be that massive of a change, so I dropped it in here so we can get it done faster

@alexvitkov alexvitkov force-pushed the closure-syntax-change branch from acf9814 to f2a9016 Compare August 28, 2023 05:08
jfecher
jfecher approved these changes Aug 28, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher enabled auto-merge August 28, 2023 16:27
@jfecher jfecher added this pull request to the merge queue Aug 28, 2023
Merged via the queue into noir-lang:master with commit 4609c1a Aug 28, 2023
@kevaundray kevaundray mentioned this pull request Aug 28, 2023
Merged
TomAFrench added a commit that referenced this pull request Aug 28, 2023
@TomAFrench
Merge branch 'master' into assert-eq-builtin
78a57fc 
* master:
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into kw/bb-binaries
47f2ab6 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
TomAFrench added a commit that referenced this pull request Aug 30, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
b04ed6b 
* master: (42 commits)
  fix(ssa): Handle right shift with constants (#2481)
  chore(noir): Release 0.10.4 (#2354)
  fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475)
  fix(ssa): Remove padding from ToRadix call with constant inputs (#2479)
  fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463)
  chore: resolve `Instruction` inputs fully before checking against cache (#2472)
  chore: Move independent `run_test` function into nargo core (#2468)
  feat: Standard library functions can now be called with closure args  (#2471)
  feat(frontend): aztec syntactic sugar (feature flagged) (#2403)
  chore(ci): enforce compliance with `cargo fmt` (#2467)
  chore(ci): Allow releases to have additional feature flags (#2405)
  feat: Add `assert_eq` keyword (#2137)
  fix(ssa): Do not optimize for allocates in constant folding (#2466)
  feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460)
  fix: Closure lvalue capture bugfix (#2457)
  feat: Syntax for environment types now works with generics (#2383)
  fix(parser): fixes for the parsing of 'where' clauses (#2430)
  fix: Run `wasm` nodejs tests with no fails (#2387)
  chore: Run `cargo fmt` (#2455)
  chore: Perform formatting changes to integration tests (#2448)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow higher order functions to be called with closures, and to return closures
2 participants
@alexvitkov @jfecher