Skip to content

chore(frontend): Avoid silent fallback when returning turbofish generics for primitive types#10416

Merged
vezenovm merged 1 commit intomasterfrom
mv/no-silent-fallback-primitive-type-turbo
Nov 7, 2025
Merged

chore(frontend): Avoid silent fallback when returning turbofish generics for primitive types#10416
vezenovm merged 1 commit intomasterfrom
mv/no-silent-fallback-primitive-type-turbo

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Nov 6, 2025

Description

Problem*

Working towards the audit of the elaborator group 7a

I noticed that in resolve_item_turbofish_and_self_type we deconstruct the resolved primitive type to fetch any possible generics as they are used later in the elaboration flow. The logic is currently correct but could be incorrect if we ever add another primitive type with generics and forget to return its generics after instantiating the type.

Summary*

When instantiating a primitive type with turbofish we now also return a bool as to whether that type should have generics. That bool then determines whether we return an empty list of generics, not the instantiated type. Now if we were to add another primitive type with generics and not return any generics, we would hit the new panic added in this PR.

I thought about just adding a has_generics helper on PrimitiveType but this felt like we then still have two sources of truth when removing that is the goal of this refactor. We would have to remember to update this helper if we added a new primitive type.

Additional Context

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.

@vezenovm vezenovm requested a review from a team November 6, 2025 20:55
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: 89ee869 Previous: 4a6d6f9 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

@asterite
Copy link
Collaborator

asterite commented Nov 7, 2025

Nice! Maybe we could return a Vec or Option<Vec> with the generics (instead of the bool)? That way we don't have to repeat checking the actual type later on. It might be a bit more expensive, but likely not something noticeable.

@vezenovm
Copy link
Contributor Author

vezenovm commented Nov 7, 2025

Maybe we could return a Vec or Option<Vec> with the generics (instead of the bool)? That way we don't have to repeat checking the actual type later on. It might be a bit more expensive, but likely not something noticeable.

I had thought about doing that as well. We would have to clone the types but we could remove the need to deconstruct the result type at all. Do you prefer that to this?

@vezenovm vezenovm added this to the Group 7 Audited milestone Nov 7, 2025
@asterite
Copy link
Collaborator

asterite commented Nov 7, 2025

Do you prefer that to this?

Yes, or at least it would make it harder to forget if we eventually add a new generic type.

@asterite
Copy link
Collaborator

asterite commented Nov 7, 2025

Ah, actually, there's an unreachable if we forget to handle that case, so no need to change anything 👍

@vezenovm
Copy link
Contributor Author

vezenovm commented Nov 7, 2025

Ah, actually, there's an unreachable if we forget to handle that case, so no need to change anything 👍

Yeah without the unreachable we would still have a silent fallback. I will keep the type deconstruction as that is what we already have and we can avoid cloning when we don't need to (this method is also used in resolve_type_trait_method which just wants the entire type).

@vezenovm vezenovm enabled auto-merge November 7, 2025 16:41
@vezenovm vezenovm disabled auto-merge November 7, 2025 17:22
@vezenovm
Copy link
Contributor Author

vezenovm commented Nov 7, 2025

@asterite could I get an approval or do you think we should still switch to the cloned type method?

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.

Sorry! I thought I had approved.

@vezenovm vezenovm added this pull request to the merge queue Nov 7, 2025
Merged via the queue into master with commit 8d4b1c7 Nov 7, 2025
132 checks passed
@vezenovm vezenovm deleted the mv/no-silent-fallback-primitive-type-turbo branch November 7, 2025 18:09
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 10, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore(elaborator): Infer type of lambdas in tuple args; more docs (noir-lang/noir#10405)
fix: revert "feat(ACIR): reuse element_type_sizes blocks with the same str… (noir-lang/noir#10428)
chore(frontend): Avoid silent fallback when returning turbofish generics for primitive types (noir-lang/noir#10416)
fix: mark ECDSA verification as `PureWithPredicate` (noir-lang/noir#10423)
fix(parser): don't crash on incomplete type alias (noir-lang/noir#10421)
chore(github): Refine pull request template wording (noir-lang/noir#10418)
chore: refactor codegen_memory (noir-lang/noir#10323)
chore(frontend): Primitive types generic count unit tests (noir-lang/noir#10412)
chore: use SignedField::from_signed in enums.rs (noir-lang/noir#10408)
chore: use `map_data_bus_in_place` in mem2reg (noir-lang/noir#10407)
chore: apply typo fixes from Cantina (noir-lang/noir#10406)
chore(frontend): Various comptime blocks unit tests (noir-lang/noir#10398)
fix: clarify predicate comment in BrilligCall and Call opcodes (noir-lang/noir#10356)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Nov 10, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore(elaborator): Infer type of lambdas in tuple args; more docs
(noir-lang/noir#10405)
fix: revert "feat(ACIR): reuse element_type_sizes blocks with the same
str… (noir-lang/noir#10428)
chore(frontend): Avoid silent fallback when returning turbofish generics
for primitive types (noir-lang/noir#10416)
fix: mark ECDSA verification as `PureWithPredicate`
(noir-lang/noir#10423)
fix(parser): don't crash on incomplete type alias
(noir-lang/noir#10421)
chore(github): Refine pull request template wording
(noir-lang/noir#10418)
chore: refactor codegen_memory
(noir-lang/noir#10323)
chore(frontend): Primitive types generic count unit tests
(noir-lang/noir#10412)
chore: use SignedField::from_signed in enums.rs
(noir-lang/noir#10408)
chore: use `map_data_bus_in_place` in mem2reg
(noir-lang/noir#10407)
chore: apply typo fixes from Cantina
(noir-lang/noir#10406)
chore(frontend): Various comptime blocks unit tests
(noir-lang/noir#10398)
fix: clarify predicate comment in BrilligCall and Call opcodes
(noir-lang/noir#10356)
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.

2 participants