fix: Defunctionalize foreign functions in pre-SSA pass over mAST

[aakoshh]

Description

Problem*

Resolves #10156
Resolves #10154

Summary*

Adds a new create_foreign_proxies method to the monomorphized Program, similar to handle_ownership, which is executed in monomorphize_debug, as a pass on the AST before SSA generation.

The pass finds all instances where we use a "foreign" function as a value - where "foreign" can be intrinsic, oracle or built-in function identifier -, creates proxy functions to call them, and replaces the definition in the identifiers with the ID of the new global function. After this pass we should only ever see function values refer to global functions, and only direct calls refer to intrinsic/builtin/oracle functions.

Further changes:

• Added a defuncitonalize_pre_check to ascertain that we no longer have non-global functions as values.
• Moved the visitor module from the AST fuzzer to the monomorphization module.
• Added extra logic to the to_hir_type function in the AST fuzzer, to handle function types and convert them back to non-tuple HIR type. Made the Monomorphizer public so that I can add a property based test that Monomorphizer::convert_type and Type::to_hir_type are on the same page. (We already agreed that the Monomorphizer can be public in chore: monomorphizer public fields #9979).
• Changed the SSA interpreter to not panic if an Intrinsic or Oracle has to be printed, but use the hash instead, although now this shouldn't come up, since there will always be a wrapper with a global ID.

Additional Context

Started by handling oracle and intrinsic functions in the SSA interpreter printing so that Initial SSA can be processed, thinking I'd be replacing these values during the defuncitonalize pass with wrapper functions, but then realised the wrapping will have to happen in the monomorphizer, which will solve it even for the Initial SSA.

I looked at the monomorphizer queue, and thought that while it would be possible to extend the queue mechanism to non-global functions, it would go against the grain of it more than adding a post-processing pass to create new function. The current queue processing logic expects to be able to find the definition of a function in the NodeInterner, whereas these new ones would not exists, and would have to be projected based on the definition of something else.

I thought this can be achieved without making the monomorphizer more complicated by traversing the AST.

I found it a little surprising that an identifier for a function looks something like this:

Ident { 
  name: "as_witness", 
  definition: Definition::Intrinsic(Intrinsic:AsWitness), 
  type: Type::Tuple(vec![
    Type::Function {..., unconstrained: false}, 
    Type::Function {..., unconstrained: true}
  ])
}

This is true in both of these cases:

foo(as_witness);
as_witness(0);

The first one will appear in the AST as Call with an arguments of vec![Tuple(vec![Ident, Ident])], where both idents are the same, carrying both constrained and unconstrained in their type:

foo((as_witness$as_witness, as_witness$as_witness));

while the second has just 1 Ident in Call::func, but its type is again a tuple of functions:

as_witness$as_witness(0);

I initially thought there would be two idents with the same definition (e.g. Intrinsic), and the Ident::typ would be Type::Function.

The fact at there is a Expression::Tuple makes it easy to detect when we are dealing with a function value, and then seek confirmation from the Ident::typ which should also be a tuple of Type::Function with false and true for unconstrained.

Example

unconstrained fn main() {
    foo(bar);
    unsafe {
        bar(0);
    }
}

unconstrained fn foo(f: unconstrained fn(Field) -> ()) {
    f(0);
}

#[oracle(my_oracle)]
unconstrained fn bar(f: Field) {}

❯ cargo run -q -p nargo_cli -- compile --force --silence-warnings --show-ssa-pass Initial --show-monomorphized

unconstrained fn main$f0() -> () {
    foo$f1((bar$f2, bar$f3));;
    {
        bar$my_oracle(0);
    }
}
unconstrained fn foo$f1(f$l0: (fn(Field) -> (), unconstrained fn(Field) -> ())) -> () {
    f$l0.1(0);
}
#[inline_always]
fn bar_proxy$f2(p0$l0: Field) -> () {
    bar$my_oracle(p0$l0)
}
#[inline_always]
unconstrained fn bar_proxy$f3(p0$l0: Field) -> () {
    bar$my_oracle(p0$l0)
}

After Initial SSA:
brillig(inline) fn main f0 {
  b0():
    call f1(f2, f3)
    call my_oracle(Field 0)
    return
}
brillig(inline) fn foo f1 {
  b0(v0: function, v1: function):
    call v1(Field 0)
    return
}
acir(inline_always) fn bar_proxy f2 {
  b0(v0: Field):
    call my_oracle(v0)
    return
}
brillig(inline_always) fn bar_proxy f3 {
  b0(v0: Field):
    call my_oracle(v0)
    return
}

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] 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]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: eb39da3	Previous: 9b08130	Ratio
sha512-100-bytes	0.072 s	0.053 s	1.36

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: eb39da3	Previous: 9b08130	Ratio
test_report_zkpassport_noir-ecdsa_	3 s	2 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[jfecher]

The current queue processing logic expects to be able to find the definition of a function in the NodeInterner, whereas these new ones would not exists, and would have to be projected based on the definition of something else.

Can you elaborate on this? I'm not sure why we'd need a new pass for this versus adding a if is oracle { ... } check when monomorphizing a function, and if so monomorphize a function wrapper for it with just a call to the real oracle inside.

[aakoshh]

When the monomorphizer encounters a call, it looks up the definition of the called function, and if it hasn't existed yet, it enqueues it, knowing that its ID will be the same. Then, we pick an ID from the queue and elaborate it by looking up the definition of it in the interner.

To change this, what I thought I'd have to do is check that the call I'm making to is an oracle, and then come up with some ID I can call instead of the oracle, and enqueue that ID for monomorphization, but when we get that ID from the queue, there is no regular definition for it in the interner like there is for local functions, and instead it needs to rely on other information to synthesise a function.

I'm sure this is all doable, but I thought it would be much easier to write it as a separate pass, which I was already familiar with, then for me add this aspect to the monomorphizer itself.

[aakoshh]

adding a if is oracle { ... } check when monomorphizing a function

I'm not sure where this check would be added to be honest. As I understood we only monomorphized user-defined functions, while oracle and intrinsics did not need to be enqueued for monomorphization, they were functions identified by "name". That's why I didn't want to go into this: the queue does not expect a foreign function in it, and then I don't know what definition to look up for them in the thing that processes the queue, so I would have had to add more context data captured during the enqueueing of the foreign function.

I thought encapsulating all this into a separate pass that doesn't need to touch anything in the elaborator is cleaner. I wasn't sure if it somehow makes the elaborator "incomplete" that it produces code that isn't meant to be final, but since ownership analysis also modifies the AST, and there is no way to skip this, we can consider it part of the elaboration, even if the means to achieve it lay outside the purview of the queue processing logic.

[aakoshh]

I see that the looking up the definition itself is the same FuncMeta, so I suppose that's where the if oracle would go in Monomorphizer::function. The fields that go in the queue would potentially go unused, since they are not used in Monomorphizer::lookup_function for foreign function. So when an item appears in the queue that refers to a foreign function, it already has its AST FuncId assigned, and we could produce a wrapper for it, instead of looking up its body expression, which it doesn't have.

What I wasn't sure about is that the Monomorphizer::function_reference is obviously used when we use a function as a value, but we call lookup_function also when we call a function, in which case we don't need to create a wrapper for foreign functions.

Altogether, yes, this seems doable, but I found it more straight forward to recognise when a function is a value in the mAST, and knew how to generate a body for it. Since the machinery in the Monomorphizer mostly based on IDs that refer to the interner, it didn't seem like conjuring up an ast::Expression when there isn't a body with a HirExpression would benefit from being done there.

For example it didn't look like I can call Monomorphizer::function_call, unless somehow I add extra information in the function definition of these proxy functions with the ID of the original call expression, and this time make it without redirecting to a proxy. Once again, I chose the "obvious" approach based on the mAST, over figuring out the exact ways of the Monomorphizer.

[jfecher]

I'm going to try some alternate methods of this PR to see if they pan out or if they're worse than expected. This has been up for a little while already so if they don't pan out I'll approve and we can get this merged to solve the issue now.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: eac13e1	Previous: 9b08130	Ratio
private-kernel-inner	2.068 s	1.702 s	1.22
private-kernel-tail	1.706 s	1.35 s	1.26

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[vezenovm]

@jfecher I will leave final approval to you. As mentioned in #10160 (comment) I would still like to follow-up with always wrapping intrinsics/oracles in the elaborator.

[jfecher]

Agreed that we can merge this now and focus on a follow-up later if needed