Skip to content

fix: nargo expand bugs behind ignored integration tests - #13438

Open
asterite wants to merge 18 commits into
masterfrom
ab/fix-nargo-expand-bugs
Open

fix: nargo expand bugs behind ignored integration tests#13438
asterite wants to merge 18 commits into
masterfrom
ab/fix-nargo-expand-bugs

Conversation

@asterite

@asterite asterite commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

I figured that nargo expand is a good tool to understand what the actual code is, so I thought that maybe it's a good idea if we have this working well in (almost) all cases. This PR fixes the remaining bugs we had.

Description

Problem

Resolves the nargo expand bugs tracked by the ignore lists in tooling/nargo_cli/build.rs: IGNORED_NARGO_EXPAND_COMPILE_SUCCESS_EMPTY_TESTS, IGNORED_NARGO_EXPAND_COMPILE_SUCCESS_NO_BUG_TESTS and IGNORED_NARGO_EXPAND_EXECUTION_TESTS — tests that were ignored because the expanded code failed to recompile (or recomputed the wrong value).

After this PR, every remaining ignored entry is a deliberate test-infrastructure limitation (no src/main.nr, workspace layouts, relative-path or transitive-only dependencies), not a bug.

Summary

One commit per fix:

  1. Print Self::item for assumed trait items. Inside a trait's default method body, Self::static_method_2() printed as ATrait::static_method_2(), which doesn't resolve on recompile (unknown receiver type). The printer now tracks the trait's Self type variable while printing its body. Un-ignores trait_function_calls and trait_static_methods.

  2. Don't print impl Trait parameter constraints in where clauses. An impl Trait parameter desugars to a hidden generic named impl {Trait} plus a trait constraint, which printed as where impl SomeTrait: SomeTrait. The constraint is implied by the parameter type, so it's now filtered out. Un-ignores trait_method_mut_self.

  3. Print impl Trait parameters with their trait generic arguments. The hidden generic's synthetic name omits the trait's generics, so _input: impl Foo<N> printed as _input: impl Foo. Such parameters are now printed from their resolved desugared constraint. Un-ignores regression_7648.

  4. Don't embed "(resolved type)" in associated type names. The synthetic name of an associated type's named generic (<{object} as {trait}>::{name}) was built by stringifying the object type as written; a macro-spliced UnresolvedTypeData::Resolved displays as the (resolved type) placeholder, producing unparseable output like <(resolved type) as Ser>::N. These names now look through the resolution to the actual type. Un-ignores regression_10747_associated_constant and (incidentally) the regression_9116 execution test.

  5. Print a global's initializer when its value can't be printed as code. Globals printed their comptime-evaluated value even when that value doesn't reconstruct as compilable source: struct literals whose type or fields are private outside their defining module (BoundedVec { len, storage }, Option { _is_some, _value }), comptime-only values that print as a panic(...) placeholder, or (commit 12) strings whose bytes aren't valid UTF-8 and would print lossily with a different length. For those, the original initializer expression is printed instead; representable values keep printing as values, since a comptime-mutable global's final value can differ from its initializer. Un-ignores trait_call_in_global, function_registry, regression_10887 and regression_12269.

  6. Qualify associated constant references. An expression reference to a trait impl's associated constant (Self::N) printed as the bare name N, which doesn't resolve on recompile. It now prints as Self::N inside the defining impl and <Type as Trait>::N elsewhere; when the trait isn't even visible from the printing module (a comptime Expr::resolve in another module's scope can splice such a reference), the constant's compile-time value is printed instead. Un-ignores trait_associated_constant, regression_10466 and comptime_resolve_associated_constant_scope.

  7. Fix numeric type alias printing. Numeric aliases printed without their numeric type annotation (type Double<let N: u32> = N * 2; instead of ...: u32 = N * 2;), and an alias parameter used as a value (AliasN::<1>) printed as the bare name N — which silently resolves to an unrelated item with the same name, changing the program's meaning (this one is a red/green commit pair: the buggy snapshot is recorded first). Un-ignores numeric_type_alias.

  8. Print associated constant declarations that involve the self type. let N: i32 = -12345i32; in impl Foo for i32 printed as let N: Self = -12345;Self doesn't resolve in that annotation, and the unsuffixed literal is checked as u32. Both the annotation and the value's type suffix are now printed. Un-ignores negative_associated_constants.

  9. Cleanup: 15 noirc_frontend_tests_* entries in the compile_success_no_bug ignore list referred to test programs deleted in chore: remove duplicated frontend tests #9706 and ignored nothing; removed. The ignore lists' doc comments now reflect that only deliberate limitations remain.

Each fix comes with a round-trip regression test in compiler/noirc_frontend/src/tests/expand.rs where the harness allows it (a features-aware assert_no_errors_and_to_string_using_features variant was added for the impl Trait tests), plus the un-ignored integration tests.

Additional Context

Verified with the full nargo_cli execute suite (including all 789 nargo_expand tests and the compile-failure error-message snapshots, relevant since commits 4 and 6 touch elaborator-minted names), the noirc_frontend suite, and clippy.

🤖 Generated with Claude Code

Review follow-ups

Two additional commits address the review findings:

  • fix: suffix instantiated numeric generics with their numeric type — the printed constant for an instantiated numeric generic was unsuffixed, so an inference site could silently select a different trait impl (wrong program that still compiles). Adds the numeric_type_alias_inference execution test that fails at runtime without the suffix.
  • fix: harden nargo expand's unrepresentable-value detection — strings must survive {:?} formatting (control chars/combining marks escape as \u{..}, which Noir does not lex), format-string fragments are checked for raw-printed specials, capturing closures are unrepresentable, plain function values get the same visibility check as structs/enums, a warning comment marks a mutable global whose printed initializer cannot reproduce its final mutated value, the impl {Trait} synthetic-name prefix is now a shared constant, and the Display limitations of the minted associated-type names are documented. Adds the comptime_mutated_global and global_string_control_char execution tests.

asterite and others added 15 commits July 31, 2026 16:49
Inside a trait's default method body, a reference to another item of the
same trait (e.g. `Self::static_method_2()`) was printed by falling back
to the plain function path `ATrait::static_method_2()`. That form does
not resolve when recompiled because the receiver type is unknown.

Track the trait's `Self` type variable while printing its body and print
any assumed trait item over that (still unbound) variable as
`Self::item`.

This un-ignores the `trait_function_calls` and `trait_static_methods`
`nargo expand` integration tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An `impl Trait` parameter desugars to a hidden generic named
`impl {Trait}` plus a trait constraint on it. `nargo expand` printed
that constraint in the function's where clause as
`where impl Trait: Trait`, which doesn't resolve when recompiled.

The constraint is implied by the parameter type itself, so filter it
out of the printed where clause.

This un-ignores the `trait_method_mut_self` `nargo expand` integration
test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The hidden generic minted for an `impl Trait` parameter is named
`impl {trait_path}` without the trait's generic arguments, so
`nargo expand` printed `_input: impl Foo` for `_input: impl Foo<N>`
and the expansion failed to recompile with "Foo expects 1 generic but
0 were given".

Print such parameters from their resolved desugared trait constraint,
which carries the generic arguments.

This un-ignores the `regression_7648` `nargo expand` integration test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The synthetic name of an associated type's named generic
(`<{object} as {trait}>::{name}`) was built by stringifying the object
type as written. When a macro splices an already-resolved type (e.g.
`impl<...> Ser for $typ` or a `$name: Ser` where clause), that
`UnresolvedTypeData::Resolved` displays as the "(resolved type)"
placeholder, which then surfaced in `nargo expand` output as
`<(resolved type) as Ser>::N` and failed to reparse.

Look through the resolution to the actual type when building these
names.

This un-ignores the `regression_10747_associated_constant`
`nargo expand` integration test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…code

`nargo expand` printed a global's comptime-evaluated value, but some
values don't reconstruct as compilable source: struct literals whose
type or fields are private outside their defining module (e.g.
`BoundedVec { len, storage }`, `Option { _is_some, _value }`), and
comptime-only values that print as a `panic(...)` placeholder.

For such values, print the global's original initializer expression
instead, which is at least as visible as it was in the original
program. Representable values keep printing as evaluated values, since
a comptime-mutable global's final value can differ from what its
initializer evaluates to.

This un-ignores the `trait_call_in_global`, `function_registry` and
`regression_10887` `nargo expand` integration tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 15 `noirc_frontend_tests_*` entries referred to test programs that
were generated from frontend unit tests and removed in #9706, so they
no longer ignore anything. The remaining entries in the
`compile_success_empty` list are all deliberate infrastructure
limitations, not bugs, so its doc comment is updated to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Its derive-macro-generated `Serialize` impls previously expanded with
`<(resolved type) as Serialize>::N` associated-constant names; that was
fixed when associated type names stopped embedding the "(resolved
type)" placeholder, so the test passes now.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An expression reference to a trait impl's associated constant (e.g.
`Self::N`) printed as the bare name `N`, which doesn't resolve when
the expansion is recompiled. Print it as `Self::N` inside the defining
impl and as `<Type as Trait>::N` elsewhere.

This un-ignores the `trait_associated_constant` and `regression_10466`
`nargo expand` integration tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`nargo expand` printed `type Double<let N: u32> = N * 2;` without the
`: u32` annotation, so the right-hand side was rejected as a type
expression on recompile.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regression test for `nargo expand` printing `AliasN::<1>` as the bare
name `N`, which silently rebinds to the global `N` at the use site.
The accepted snapshot records the buggy output; the fix follows.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A numeric type alias's parameter used as a value (`AliasN::<1>`)
printed as its bare name `N`, which doesn't resolve at the use site —
or worse, silently resolves to an unrelated item with the same name
(e.g. `global N`), changing the program's meaning. The definition's
type variable is bound to the resolved value in this case, so print
that value.

Together with the numeric-type-annotation fix, this un-ignores the
`numeric_type_alias` `nargo expand` integration test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two problems with `let N: <type> = <value>;` inside a trait impl:

- When the numeric type is the impl's self type (`impl Foo for i32`),
  it printed as `let N: Self = ...`, and `Self` doesn't resolve in an
  associated constant's type annotation.
- The value printed as an unsuffixed literal, which is checked as
  `u32` in this position, rejecting constants of other numeric types
  (e.g. `-12345` for an `i32` constant). Constants now print with
  their type suffix (`-12345_i32`).

This un-ignores the `negative_associated_constants` `nargo expand`
integration test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `str` global whose bytes aren't valid UTF-8 (e.g. built with
`as_str_unchecked`) printed via `from_utf8_lossy`, replacing invalid
bytes with U+FFFD and changing the string's content and byte length —
the expansion then failed to recompile with a length mismatch. Treat
such values as unrepresentable so the global prints its initializer
expression instead.

This un-ignores the `regression_12269` `nargo expand` integration
test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A comptime `Expr::resolve` running in another module's scope can
splice in a reference to an associated constant of a trait that is
private to that module. The qualified `<Type as Trait>::N` form then
names a trait that isn't visible from the printing module and fails
to recompile. Since the constant's value is compile-time known, print
the value in that case.

This un-ignores the `comptime_resolve_associated_constant_scope`
`nargo expand` integration test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every remaining entry is a deliberate test-infrastructure limitation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@asterite
asterite marked this pull request as draft July 31, 2026 22:18
@asterite
asterite marked this pull request as ready for review August 3, 2026 12:16
@asterite
asterite requested a review from TomAFrench August 3, 2026 12:16
@TomAFrench

Copy link
Copy Markdown
Member

Claudebox found a few issues on this PR: https://gist.github.com/AztecBot/018be6562f9cb4ecbf81a76bd14ac8f2

asterite and others added 3 commits August 3, 2026 18:23
An instantiated numeric generic printed as a bare integer literal, so
wherever the surrounding context didn't pin the type the literal was
inferred as a different one and could silently select a different trait
impl — a wrong program that still compiles. Print the constant with the
numeric type that the definition already carries (`200_u8`).

Adds the `numeric_type_alias_inference` execution test, whose expansion
picks the wrong impl at runtime without the suffix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses review findings on the value-printing commits:

- A string value is now representable only if every character survives
  the round trip through Rust's `{:?}` formatting: valid UTF-8 isn't
  enough, since control characters and combining marks escape as
  `\u{..}`, which Noir's lexer doesn't accept. Format string fragments
  are stricter still (printed raw with only `"` re-escaped), so
  backslashes, braces and control characters make them unrepresentable.
- A closure value prints as just its lambda, so a closure with captured
  variables is now unrepresentable (the captures would be dangling).
- When a mutable global falls back to its initializer, the printed
  initializer doesn't reproduce the final mutated value (the mutating
  attributes are already expanded away), so a warning comment is
  appended to mark the divergence.
- `Value::Function` now gets the same visibility check as structs and
  enums for plain (non-method) functions.
- The `impl {Trait}` synthetic-name prefix shared by the elaborator and
  the printer is now a single constant with a `NamedGeneric` helper, so
  the sites can't silently drift apart.
- Documented that `unresolved_type_name` renders through `Type`'s
  non-source-faithful `Display`, and that the visible-or-reexported
  checks deliberately over-approximate to match how references print.

Adds the `comptime_mutated_global` execution test (pins that a
runtime-observable comptime-mutated global expands to its final value)
and the `global_string_control_char` execution test (a valid-UTF-8
string that can't print as a literal falls back to its initializer).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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