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

Lamdas don't capture function scoped variables #1088

Closed
1 task
joss-aztec opened this issue Apr 3, 2023 · 3 comments
Closed
1 task

Lamdas don't capture function scoped variables #1088

joss-aztec opened this issue Apr 3, 2023 · 3 comments
Assignees
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework bug Something isn't working

Comments

@joss-aztec
Copy link
Contributor

joss-aztec commented Apr 3, 2023

Aim

Correctly compile constraints described in the body of a function, that are subsequently referenced in the body of a lamda.

Expected behavior

Circuit A (broken):

use dep::std;

fn do_lamda(x: Field) -> Field {
  let y = x + 1;
  let func = |z| {
    std::hash::pedersen([x, y, z])[0]
  };
  func(x)
}

fn main(x: pub Field) {
  constrain do_lamda(x) != 0;
}

The above code should describe a complete ACIR, with execution equivalent to this next program.

Circuit B (working):

use dep::std;

fn main(x: pub Field) {
  let y = x + 1;
  let func = |z| {
    std::hash::pedersen([x, y, z])[0]
  };
  constrain func(x) != 0;
}

Bug

The first code sample drops the constraints described by let y = x + 1; in the body of do_lamda. Here's the ACIR output:

current witness index : 7
public parameters indices : [1]
return value indices : []
BLACKBOX::PEDERSEN [(_1, num_bits: 254), (_2, num_bits: 254), (_1, num_bits: 254)] [ (_3,...,_4)]
DIR::INVERT (_3, out: _5)
EXPR [ (1, _3, _5) (-1, _6) 0 ]
EXPR [ (1, _3, _6) (-1, _3) 0 ]
EXPR [ (-1, _6) 1 ]

Note that the only references to Witness(2) appears in the pedersen input.

In contrast, here's the ACIR that's correctly generated from circuit B:

current witness index : 7
public parameters indices : [1]
return value indices : []
EXPR [ (1, _1) (-1, _2) 1 ]
BLACKBOX::PEDERSEN [(_1, num_bits: 254), (_2, num_bits: 254), (_1, num_bits: 254)] [ (_3,...,_4)]
DIR::INVERT (_3, out: _5)
EXPR [ (1, _3, _5) (-1, _6) 0 ]
EXPR [ (1, _3, _6) (-1, _3) 0 ]
EXPR [ (-1, _6) 1 ]

Note that the first expression decribes Witness(2).

To reproduce

  1. nargo print-acir for Circuit A

Installation method

Compiled from source

Nargo version

nargo_cli 0.3.2 (git version hash: ad3e9fe, is dirty: true)

@noir-lang/noir_wasm version

No response

@noir-lang/barretenberg version

No response

@noir-lang/aztec_backend version

No response

Additional context

No response

Submission Checklist

  • Once I hit submit, I will assign this issue to the Project Board with the appropriate tags.
@joss-aztec joss-aztec added the bug Something isn't working label Apr 3, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 3, 2023
@kevaundray kevaundray added the aztec.nr Helpful for development of Aztec.nr the smart contract framework label Apr 20, 2023
@kevaundray
Copy link
Contributor

This can be re-reviewed when closure conversion is implemented

alehander92 added a commit to metacraft-labs/noir that referenced this issue Jul 31, 2023
alehander92 added a commit to metacraft-labs/noir that referenced this issue Aug 1, 2023
alehander92 added a commit to metacraft-labs/noir that referenced this issue Aug 1, 2023
alehander92 added a commit to metacraft-labs/noir that referenced this issue Aug 2, 2023
alehander92 added a commit to metacraft-labs/noir that referenced this issue Aug 2, 2023
alehander92 added a commit to metacraft-labs/noir that referenced this issue Aug 2, 2023
github-merge-queue bot pushed a commit that referenced this issue Aug 2, 2023
#1959)

* feat: Initial work on rewriting closures to regular functions with hidden env

This commit implements the following mechanism:

On a line where a lambda expression is encountered, we initialize a tuple for the captured lambda environment and we rewrite the lambda to a regular function taking this environment as an additional parameter. All calls to the closure are then modified to insert this hidden parameter.

In other words, the following code:

```
let x = some_value;
let closure = |a| x + a;

println(closure(10));
println(closure(20));
```

is rewritten to:

```
fn closure(env: (Field,), a: Field) -> Field {
  env.0 + a
}

let x = some_value;
let closure_env = (x,);

println(closure(closure_env, 10));
println(closure(closure_env, 20));
```

In the presence of nested closures, we propagate the captured variables implicitly through all intermediate closures:

```
let x = some_value;

let closure = |a, c|
  # here, `x` is initialized from the hidden env of the outer closure
  let inner_closure = |b|
    a + b + x

  inner_closure(c)
```

To make these transforms possible, the following changes were made to the logic of the HIR resolver and the monomorphization pass:

* In the HIR resolver pass, the code determines the precise list of variables captured by each lambda. Along with the list, we compute the index of each captured var within the parent closure's environment (when the capture is propagated).

* Introduction of a new `Closure` type in order to be able to recognize the call-sites that need the automatic environment variable treatment. It's a bit unfortunate that the Closure type is defined within the `AST` modules that are used to describe the output of the monomorphization pass, because we aim to eliminate all closures during the pass. A better solution would have been possible if the type check pass after HIR resolution was outputting types specific to the HIR pass (then the closures would exist only within this separate non-simplified type system).

* The majority of the work is in the Lambda processing step in the monomorphizer which performs the necessary transformations based on the above information.

Remaining things to do:

* There are a number of pending TODO items for various minor unresolved loose ends in the code.

* There are a lot of possible additional tests to be written.

* Update docs

* refactor: use panic, instead of println+assert

Co-authored-by: jfecher <[email protected]>

* test: add an initial monomorphization rewrite test

a lot of the machinery is copied from similar existing tests
the original authors also note some of those can be refactored
in something reusable

* fix: address some PR comments: comment/refactor/small fixes

* fix: use an unified Function object, fix some problems, comments

* fix: fix code, addressing `cargo clippy` warnings

* fix: replace type_of usage and remove it, as hinted in review

* test: move closure-related tests to test_data

* test: update closure rewrite test output

* chore: apply cargo fmt changes

* test: capture some variables in some tests, fix warnings, add a TODO

add a TODO about returning closures

* test: add simplification of #1088 as a resolve test, enable another test

* fix: fix unify for closures, fix display for fn/closure types

* test: update closure tests after resolving mutable bug

* fix: address some review comments for closure PR: fixes/cleanup

* refactor: cleanup, remove a line

Co-authored-by: jfecher <[email protected]>

* refactor: cleanup

Co-authored-by: jfecher <[email protected]>

* fix: fix bind_function_type env_type handling type variable binding

* test: improve higher_order_fn_selector test

* fix: remove skip_params/additional param logic from typechecking/display

* fix: don't use closure capture logic for lambdas without captures

* fix: apply cargo fmt & clippy

* chore: apply cargo fmt

* test: fix closure rewrite test: actually capture

* chore: remove type annotation for `params`

* chore: run cargo fmt

---------

Co-authored-by: jfecher <[email protected]>
Co-authored-by: Alex Vitkov <[email protected]>
@kevaundray
Copy link
Contributor

@jfecher can this be closed?

@jfecher
Copy link
Contributor

jfecher commented Sep 5, 2023

Looks to be working now

@jfecher jfecher closed this as completed Sep 5, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants