Skip to content

fix(ssa): Empty array of higher order functions#8677

Closed
vezenovm wants to merge 13 commits intomasterfrom
mv/empty-array-of-lambdas
Closed

fix(ssa): Empty array of higher order functions#8677
vezenovm wants to merge 13 commits intomasterfrom
mv/empty-array-of-lambdas

Conversation

@vezenovm
Copy link
Copy Markdown
Contributor

@vezenovm vezenovm commented May 23, 2025

Description

Problem*

Builds upon #8674

Resolves the final panic in the linked issue that is slightly separate from lambdas in arrays:

fn main() {
    let lambdas: [fn(()) -> (); 0] = [];
    lambdas[0](());
}

Summary*

This is the SSA for the above snippet pre-DIE:

acir(inline) fn main f0 {
  b0():
    v0 = make_array [] : [Field; 0]
    constrain u1 0 == u1 1, "Index out of bounds"
    v4 = array_get v0, index u32 0 -> Field
    call v4()
    return
}

If we leave call v4() this leads to issues in the underconstrained check and then during ACIR as function pointers are not supported. Also, the fact that we still have a function as a value means that there are no function variants and we should be able to safely remove this function.

We now have this final SSA:

acir(inline) predicate_pure fn main f0 {
  b0():
    constrain u1 0 == u1 1, "Index out of bounds"
    return
}

This matches the final SSA and behavior of an empty array:

fn main() {
    let x = [];
    x[0];
}

Additional Context

I'm not really sure in what other case we may have a function value which is an instruction. This seems like the only case. However, I'm not a huge fan of this solution as it makes assumptions across passes. I think a better solution would be to simply error out on accessing an empty array at compile-time #8676.

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.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ 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: 86b7da9 Previous: eb3eef9 Ratio
test_report_zkpassport_noir_rsa_ 3 s 2 s 1.50

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

CC: @TomAFrench

// It is safe to remove these functions.
// We also check whether the type of the instruction is a field as all function pointers
// are transformed to field IDs during defunctionalization.
Value::Instruction { typ, .. } if typ.is_field() => true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not 100% on this. Maybe we could add an assertion that the instruction originates from an array get where the array is of length zero? Although if it is a slice maybe we won't know the length?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could check the length of the array in an array get but it breaks with slices. With slices we would have to track backwards to see whether it originated from an empty make array (or track slice capacities as we do in RemoveIfElse), which I think would be overkill.

I'm also not a big fan of this solution. Even with #8676, if we eventually allow escaping unconditional panics this issue will arise again. I'll think of something else. Going to switch this back to a draft.

Base automatically changed from mv/lambda-from-array-2 to mv/apply-func-dynmaic-input May 23, 2025 22:00
@vezenovm vezenovm marked this pull request as draft May 23, 2025 22:11
Base automatically changed from mv/apply-func-dynmaic-input to master May 23, 2025 22:35
@vezenovm
Copy link
Copy Markdown
Contributor Author

Here is an alternative #8697 which I prefer.

@vezenovm
Copy link
Copy Markdown
Contributor Author

vezenovm commented Jul 1, 2025

Superceded by #8697

@vezenovm vezenovm closed this Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants