Skip to content

fix: error on unused generic in trait impl#8395

Merged
TomAFrench merged 11 commits intomasterfrom
ab/check-generics-appear-in-type-or-trait-in-trait-impl
May 22, 2025
Merged

fix: error on unused generic in trait impl#8395
TomAFrench merged 11 commits intomasterfrom
ab/check-generics-appear-in-type-or-trait-in-trait-impl

Conversation

@asterite
Copy link
Copy Markdown
Collaborator

@asterite asterite commented May 7, 2025

Description

Problem

Resolves #8394

Summary

In addition to applying the same logic that existed for impls to trait impls, some code stopped compiling when it was generated via macros. These end up using resolved types which weren't handled before, and now they are.

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.

@asterite asterite marked this pull request as draft May 7, 2025 16:18
@asterite asterite requested a review from a team May 7, 2025 16:18
@asterite
Copy link
Copy Markdown
Collaborator Author

asterite commented May 7, 2025

Asking for a review because I'm not sure how to fix HashMap and UHashMap in the standard library so this error doesn't trigger... but maybe one of you know!

@jfecher
Copy link
Copy Markdown
Contributor

jfecher commented May 7, 2025

Asking for a review because I'm not sure how to fix HashMap and UHashMap in the standard library so this error doesn't trigger... but maybe one of you know!

Rust's BuildHasher uses an associated type to avoid this: https://doc.rust-lang.org/std/hash/trait.BuildHasher.html. So we should do the same since we don't want to allow multiple impls of BuildHasher for the same input type either.

@asterite
Copy link
Copy Markdown
Collaborator Author

asterite commented May 7, 2025

Cool, thank you! I'll try to do that.

@asterite
Copy link
Copy Markdown
Collaborator Author

asterite commented May 7, 2025

Hmm, I think it still won't work. In Rust it's something like this:

pub trait BuildHasher {
    type H: Hasher;
}

so we know the type H is a Hasher. Right now in Noir that doesn't work but we can do this:

pub trait BuildHasher {
    type H;

    fn build_hasher(self) -> H
    where
        H: Hasher;
}

but it's not exactly the same. Whenever we have something of type B we have to constrain that to B: BuildHasher<H>, H: Hasher over and over again, but sometimes we can't introduce that type H, for example in a trait implementation where we can't add new where clauses.

Is this something that should be implemented for Noir 1.0? Otherwise once we do this it's going to be a large breaking change.

@asterite asterite mentioned this pull request May 20, 2025
5 tasks
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: ef04db6 Previous: 79fb482 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 2 s 1 s 2

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

CC: @TomAFrench

asterite added 3 commits May 22, 2025 09:01
…' of github.com:noir-lang/noir into ab/check-generics-appear-in-type-or-trait-in-trait-impl
@asterite asterite marked this pull request as ready for review May 22, 2025 12:12
@TomAFrench
Copy link
Copy Markdown
Member

I updated the bigcurve dep on bignum

@asterite
Copy link
Copy Markdown
Collaborator Author

Ah! Mystery solved 😄 Thanks!

@asterite
Copy link
Copy Markdown
Collaborator Author

This is ready for review now.

@TomAFrench TomAFrench added this pull request to the merge queue May 22, 2025
Merged via the queue into master with commit d992ad5 May 22, 2025
98 checks passed
@TomAFrench TomAFrench deleted the ab/check-generics-appear-in-type-or-trait-in-trait-impl branch May 22, 2025 13:31
asterite added a commit that referenced this pull request May 22, 2025
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix(fmt): correctly format mixed secondary attributes and doc comments
(noir-lang/noir#8735)
fix!: require types for trait impl associated constants
(noir-lang/noir#8734)
fix!: Prevent returning references from if expressions
(noir-lang/noir#8731)
fix: cast signed to u1 follow-up
(noir-lang/noir#8730)
fix: cast signed to u1 (noir-lang/noir#8720)
fix: do not mutate arrays later copied inside other arrays
(noir-lang/noir#8701)
chore: box `AsTraitPath` and `TypePath` in `ExpressionKind`
(noir-lang/noir#8716)
fix: general solution for accessing associated constants
(noir-lang/noir#8417)
fix(ssa): Validate checked signed add/sub is followed by a truncate
(noir-lang/noir#8706)
chore: add pre- and post-check on `array_set` optimization pass
(noir-lang/noir#8714)
fix: merge expr bindings with instantiations bindings during
monomorphization (noir-lang/noir#8713)
fix: better way to do LSP file overrides
(noir-lang/noir#8702)
fix(fmt): correct indentation when formatting long struct patterns
(noir-lang/noir#8711)
fix(fuzz): Prevent breaking/continuing out from let blocks in the AST
fuzzer (noir-lang/noir#8708)
chore: remove override for zero length inputs
(noir-lang/noir#8709)
feat: Replace converging jmpif with empty block destinations with a
single jmp (noir-lang/noir#8613)
feat(cli): Add `nargo interpret` command
(noir-lang/noir#8700)
chore: more 1-tuple printing fixes
(noir-lang/noir#8699)
chore(fuzz): Fuzz the SSA parser
(noir-lang/noir#8647)
fix: (SSA parser) translate blocks in logical order rather than syntax
order (noir-lang/noir#8668)
fix(SSA): show and parse range_check's assert_message
(noir-lang/noir#8652)
chore: Show the step number in the SSA message
(noir-lang/noir#8698)
chore(docs): Add pointers to tests
(noir-lang/noir#8695)
chore(fuzz): Capture comptime print output
(noir-lang/noir#8635)
fix: nargo expand reexports correctly implemented
(noir-lang/noir#8693)
feat(cli): Show multiple SSA passes
(noir-lang/noir#8692)
chore(test): Add regression test for defunctionalize
(noir-lang/noir#8691)
chore: add an assertion when parsing SSA that all functions are
well-formed (noir-lang/noir#8671)
feat!: prevent compiling blake3 hashes which barretenberg cannot prove
(noir-lang/noir#8690)
fix(ssa): Remove the array cache from the function inserter
(noir-lang/noir#8607)
chore: bump external pinned commits
(noir-lang/noir#8684)
chore: reenable fuzzing tests in CI
(noir-lang/noir#8688)
fix: Handle `&mut function` in defunctionalize
(noir-lang/noir#8665)
fix(defunctionalize): Higher order functions (HOF) dynamic dispatch and
HOFs in arrays (noir-lang/noir#8672)
chore(ci): avoid running fuzzer tests in the merge queue
(noir-lang/noir#8664)
fix: Make casts in `comptime` consistent with runtime casts
(noir-lang/noir#8669)
fix: relax connectedness requirement on purity analysis pass
(noir-lang/noir#8667)
fix: always use `u32` for indexing arrays in SSA
(noir-lang/noir#8633)
chore: explicitly pull from `next` branch in aztec-packages
(noir-lang/noir#8660)
chore(fuzz): Build the dictionary from the SSA
(noir-lang/noir#8591)
chore(docs): Remove old versioned docs
(noir-lang/noir#8061)
chore: bump external pinned commits
(noir-lang/noir#8634)
fix(SSA): don't use string literal if byte is "form feed" ('\f')
(noir-lang/noir#8653)
chore(defunctionalize): Add regression test for missing lambda variants
panic (noir-lang/noir#8642)
chore(ci): Do not run ast_fuzzer orig vs. morph in ci
(noir-lang/noir#8646)
fix: disable underflow fix for fields
(noir-lang/noir#8631)
fix: revert "fix: error on unused generic in trait impl
(noir-lang/noir#8395)"
(noir-lang/noir#8636)
fix(ssa interpreter): Default to zero when we have an overflowing shl
(noir-lang/noir#8638)
fix: ensure that purity analysis pass explores all functions
(noir-lang/noir#8452)
feat: acir_formal_proofs (noir-lang/noir#8140)
fix: error on unused generic in trait impl
(noir-lang/noir#8395)
fix(expand): use re-exports for non-visibile items
(noir-lang/noir#8374)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <[email protected]>
Co-authored-by: Tom French <[email protected]>
Co-authored-by: Ary Borenszweig <[email protected]>
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.

Unused generics do not error on trait impls

3 participants