feat: add `unsafe` blocks for calling unconstrained code from constrained functions

[TomAFrench]

Description

Problem*

Resolves

Summary*

This is an experimental PR to play around with unsafe blocks as a method to make it clearer when unconstrained values are entering the circuit to avoid under-constrained errors.

Calls to unconstrained functions will now fail to compile unless inside of an unsafe block. Note that any code inside of an unsafe block still lays down constraints as usual so it's possible to fully constrain the return values from any unconstrained function calls before returning a safe value out from the block.

Additional Context

Documentation*

Check one:

• [ ] No documentation needed.
• [ ] Documentation included in this PR.
• [ ] [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

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

[github-actions[bot]]

🚀 Deployed on https://66bcbbf79028aea1fa537200--noir-docs.netlify.app

[TomAFrench]

This is going to need a decent amount of documentation added before it can be merged. I wanna get a temp check on this PR first however.

[TomAFrench]

We're only emitting a warning for now so this should be non-breaking afaik.

[jfecher]

I'm still undecided if we want to force users to write unsafe { ... } every time they call an unconstrained function. I'm aware of the dangers but it just seems very cumbersome to me. I'm not definitely saying "no" to this, I'm just a bit hesitant on it.

[TomAFrench]

Yeah, I'm a little hesitant on this as well tbh. I don't like the status quo where any function call could be spewing junk into your program but it seems like these unsafe blocks have a tendency to expand to cover most of the function.

[asterite]

Chatting with Tom, he said this:

it's going to need some extra testing to make sure you can't e.g. pass an unconstrained function as a callback into another function and have it run outside of an unsafe block, etc.

So for example this shouldn't compile:

fn main() {
  let callback = foo;
  foo(); // error: callback is unconstrained and it's not in an unsafe block
}

unconstrained fn foo() {}

For that, callback's type needs to know that it's a reference to an unconstrained fn. That means that now the fn type can also be unconstrained (right now this information isn't carried). Then we can error on foo().

Then, I guess we'd also want to be able to do this:

fn some_func(x: unconstrained fn(i32) -> i32) {}

that is, specifying a callback as an unconstrained fn. This is also similar to Rust, where you can write unsafe fn(i32) -> i32. Then the rules are:

• you can pass an unconstrained fn to an unconstrained fn parameter
• you can pass an unconstrained fn to a regular fn parameter
• you cannot pass a regular fn to an unconstrained fn` parameter

I guess, technically, that means that we can't unify fn and unconstrained fn.

[TomAFrench]

Then the rules are:

• you can pass an unconstrained fn to an unconstrained fn parameter
• you can pass an unconstrained fn to a regular fn parameter
• you cannot pass a regular fn to an unconstrained fn` parameter

These are slightly incorrect. You should not be able to pass an unconstrained function in place of a constrained function as this will likely result in underconstraining errors. It's fine to pass a regular function in place of an unconstrained function however.

[asterite]

@noir-lang/core this is ready to review.

Caveat: I made call arguments to unify with coercions. That worked but it also made one test fail because it's a test that runs in noirc_frontend but relies on to_slice being defined in the standard library. Because the frontend doesn't have access to the standard library, it panicked. What I did is to only coerce array to slice if that builtin method actually exists (if the standard library is defined). That's probably not ideal... ideally that builtin method is defined regardless of the standard library, or maybe the standard library should be defined on the frontend side, if the frontend depends on it.

In any case, the tests was already "failing" because passing an Array to a Slice argument errored (there was no coercion). Now there's coercion, but the coercion isn't applied, so it errors in the same way (the test just checks that changing some names in a program produces an error, but it doesn't check which error, so the test was already slightly flawed).

If we merge this, I guess we'd need to add docs about this change, right? Now sure if in this PR or in another one.

[vezenovm]

because the frontend doesn't have access to the standard library, it panicked. What I did is to only coerce array to slice if that builtin method actually exists (if the standard library is defined). That's probably not ideal... ideally that builtin method is defined regardless of the standard library, or maybe the standard library should be defined on the frontend side, if the frontend depends on it.

You can also define the method in the test itself, for example:

// Converts an array into a slice.
fn as_slice<T, let N: u32>(xs: [T; N]) -> [T] {
    let mut slice = &[];
    for elem in xs {
        slice = slice.push_back(elem);
    }
    slice
}

In any case, the tests was already "failing" because passing an Array to a Slice argument errored (there was no coercion). Now there's coercion, but the coercion isn't applied, so it errors in the same way (the test just checks that changing some names in a program produces an error, but it doesn't check which error, so the test was already slightly flawed).

Could you point out which test exactly? Makes it easier to reason about.

[asterite]

Ah, it's this entire this: https://github.com/noir-lang/noir/blob/master/compiler/noirc_frontend/src/tests/name_shadowing.rs

I think what it's doing is renaming some names to check they produce an error... and I think when that happens a function call that is sending an array to a function that expects an array suddenly starts sending an array to a function that expects a slice (because of the rename) so we can't call to_slice explicitly... and I think it won't be picked up implicitly because it's a trait impl function.

[vezenovm]

and I think it won't be picked up implicitly because it's a trait impl function.

It is a trait impl function. You will probably have to do something like as we do in our std lib:

impl<T, let N: u32> [T; N] {
     pub fn as_slice(self) -> [T] {
           let mut slice = &[];
           for elem in self {
               slice = slice.push_back(elem);
           }
           slice
     }
}

[jfecher]

It is a trait impl function. You will probably have to do something like as we do in our std lib:

I think this won't work either since we prevent impls on primitive types unless it's in the stdlib

[asterite]

Ah, yes. I tried adding that code and it wasn't triggering the add_method call, until I found this:

https://github.com/noir-lang/noir/blob/6109ddc4a12a4f7593c87fcc42a059737febd470/compiler/noirc_frontend/src/elaborator/mod.rs#L1048-L1050

(I read Jake's comment too late 😅)

[vezenovm]

I think this won't work either since we prevent impls on primitive types unless it's in the stdlib

Ah yes you are correct. Apologies for steering you the wrong way @asterite

[vezenovm]

What I did is to only coerce array to slice if that builtin method actually exists (if the standard library is defined). That's probably not ideal...

I guess this is probably fine for now as any actual Noir program will include the std lib.

[github-actions[bot]]

FYI @noir-lang/developerrelations on Noir doc changes.