Skip to content

fix(print): Print enums#10472

Merged
aakoshh merged 13 commits intomasterfrom
af/9294-print-enum
Nov 12, 2025
Merged

fix(print): Print enums#10472
aakoshh merged 13 commits intomasterfrom
af/9294-print-enum

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 11, 2025

Description

Problem

Resolves #9294

Summary

  • Fixes decode_printable_value to consume the correct number of fields when an enum is encountered.
  • Fixes to_string to format enums, rather than return None.
  • Adds a new PrintableValue::Enum to communicate the tag from the decoder to the formatter.
  • Refactored to_string to so it can force us to implement handling for any new printable type.

Additional Context

decode_printable_value looked at the tag and picked the corresponding list of fields from the variants, then consumed those fields. This assumes that we will see a varying number of fields, depending on which variant was serialised.

I can't remember now what kind of repercussions this would have on slices and arrays, which generally assume that each element has a fixed size when we calculate the flattened size as length * element_size. It would have worked in the decoder, but it was suspicious.

It turns out that Noir in fact passes a fixed number of fields regardless of the variant. I thought maybe this would be a number of Fields to accommodate the largest variant, but instead it is the concatenation of all variants, with all but one being defaults.

For example the following program:

enum Foo {
    One(Field),
    Two(u32, u64),
    Three,
}

fn main() {
    let c: [Foo; 3] = [Foo::One(10), Foo::Two(20, 30), Foo::Three];
    println(c);
}

turns into the following monomorphized AST:

fn main$f0() -> () {
    let c$l0 = [One$f1(10), Two$f2(20, 30), Three$g0];
    println$f3(c$l0);
}
fn One$f1($0$l1: Field) -> (Field, (Field,), (u32, u64), ()) {
    (0, ($0$l1), (0, 0), ())
}
fn Two$f2($0$l2: u32, $1$l3: u64) -> (Field, (Field,), (u32, u64), ()) {
    (1, (0), ($0$l2, $1$l3), ())
}
fn println$f3(input$l4: [(Field, (Field,), (u32, u64), ()); 3]) -> () {
    {
        print_unconstrained$f4(true, input$l4);
    }
}
unconstrained fn print_unconstrained$f4(with_newline$l5: bool, input$l6: [(Field, (Field,), (u32, u64), ()); 3]) -> () {
    print_oracle$print(with_newline$l5, input$l6, r#"{"kind":"array","length":3,"type":{"kind":"enum","name":"Foo","variants":[["One",[{"kind":"field"}]],["Two",[{"kind":"unsignedinteger","width":32},{"kind":"unsignedinteger","width":64}]],["Three",[]]]}}"#, false);
}

It clearly shows that One and Two are constructors return identical data structures with (<tag>, (<One fields>,), (<Two fields>,), (<Three has no fields>)).

In SSA these are further flattened:

g0 = Field 2
g1 = Field 0
g2 = u32 0
g3 = u64 0

acir(inline) impure fn main f0 {
  b0():
    v8 = make_array [Field 0, Field 10, u32 0, u64 0, Field 1, Field 0, u32 20, u64 30, Field 2, Field 0, u32 0, u64 0] : [(Field, Field, u32, u64); 3]
    call f1(u1 1, v8)
    return
}
brillig(inline) impure fn print_unconstrained f1 {
  b0(v4: u1, v5: [(Field, Field, u32, u64); 3]):
    v40 = make_array b"{\"kind\":\"array\",\"length\":3,\"type\":{\"kind\":\"enum\",\"name\":\"Foo\",\"variants\":[[\"One\",[{\"kind\":\"field\"}]],[\"Two\",[{\"kind\":\"unsignedinteger\",\"width\":32},{\"kind\":\"unsignedinteger\",\"width\":64}]],[\"Three\",[]]]}}"
    call print(v4, v5, v40, u1 0)
    return
}

The array [(Field, Field, u32, u64); 3] has the tag, followed by the single Field in One, then two fields in Two:

make_array [
  Field 0,   Field 10,   u32 0, u64 0,   // One
  Field 1,   Field 0,    u32 20, u64 30, // Two
  Field 2,   Field 0,    u32 0, u64 0    // Three
] : [(Field, Field, u32, u64); 3]

User Documentation

Check one:

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

@aakoshh aakoshh requested a review from a team November 11, 2025 16:08
Copy link
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: c4d7ef5 Previous: 62a4432 Ratio
test_report_zkpassport_noir_rsa_ 1 s 0 s +∞

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

CC: @TomAFrench

Copy link
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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 6380504 Previous: 1b1985e Ratio
rollup-block-root-single-tx 0.003 s 0.002 s 1.50

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

CC: @TomAFrench

Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Nice!

@aakoshh aakoshh enabled auto-merge November 11, 2025 18:27
Copy link
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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 52d5716 Previous: fe0bfa0 Ratio
rollup-checkpoint-root 482 s 391 s 1.23

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

CC: @TomAFrench

@aakoshh aakoshh added this pull request to the merge queue Nov 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2025
@aakoshh aakoshh enabled auto-merge November 11, 2025 22:09
@aakoshh aakoshh added this pull request to the merge queue Nov 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2025
@aakoshh aakoshh enabled auto-merge November 12, 2025 12:25
Copy link
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 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: c4d7ef5 Previous: 62a4432 Ratio
perfectly_parallel_batch_inversion_opcodes 2782829 ns/iter (± 1692) 2264195 ns/iter (± 1976) 1.23

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

CC: @TomAFrench

@aakoshh aakoshh added this pull request to the merge queue Nov 12, 2025
@aakoshh
Copy link
Contributor Author

aakoshh commented Nov 12, 2025

It took me a while to realise that the noir_wasm failures are not temporary: https://github.com/noir-lang/noir/actions/runs/19297373404/job/55184101587?pr=10472

They fail because this test requires the enums unstable feature, which is given in Nargo.toml as compiler_unstable_features = ["enums"], however this field is ignored in Wasm here.

I thought about adding it, but the Package type never makes it into the Wasm compile_program.

Even if we passed that package somehow, we aren't passing packages of the dependencies. The dependency setup in Wasm works by building up an in-memory source map and dependency map. Packages would at that point be redundant.

Perhaps it's okay if Wasm doesn't have access to unstable features for now.

Merged via the queue into master with commit c65d5d9 Nov 12, 2025
132 checks passed
@aakoshh aakoshh deleted the af/9294-print-enum branch November 12, 2025 14:21
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 13, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: remove `local_annotations` from flattening
(noir-lang/noir#10483)
chore: better error recovery for multiple mut in pattern
(noir-lang/noir#10490)
chore(frontend): Tuple pattern tests and remove confusing arity error
(noir-lang/noir#10480)
chore: monomorphizer public fields
(noir-lang/noir#9979)
chore: remove a bunch of dummy definitions
(noir-lang/noir#10482)
feat(ssa): Limit the number of steps executed by the SSA interpreter
during constant folding (noir-lang/noir#10481)
fix: remove saturation from loop bound increments
(noir-lang/noir#10479)
fix(print): Print enums (noir-lang/noir#10472)
fix(frontend): No negative overflow when quoting signed integer
(noir-lang/noir#10331)
chore: green light Brillig for audit
(noir-lang/noir#10376)
END_COMMIT_OVERRIDE
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.

Compiler crash: Trying to print enum

2 participants