fix!: better error message when cannot infer generic numeric type #7843
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: bf0834e | Previous: 4ee2d12 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
223 s |
179 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
2 tasks
asterite
changed the title
|
It seems we'd need to merge #6388 first. |
…generic-numeric-type
…generic-numeric-type
…ic-type' of github.com:noir-lang/noir into ab/better-error-message-when-cannot-infer-generic-numeric-type
…generic-numeric-type
…generic-numeric-type
This was referenced Apr 3, 2025
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
Apr 3, 2025
…pl (#13283) This was discussed a bit over slack, but here's some context. In Rust this gives an error: ```rust pub struct Foo<T> { x: T, } impl<T> Foo<T> { fn one(x: T) { Bar::two(x); // Cannot infer the value of const parameter U } } struct Bar<T, const U: usize> { x: T, } impl<T, const U: usize> Bar<T, U> { fn two(x: T) -> Foo<T> { Foo { x } } } fn main() { Foo::<i32>::one(1); } ``` In the call `Bar::two` Rust needs to know what is the value of `U`. It can be solved by doing `Bar::<T, 1234>::two(x)` or using any numeric value... because the numeric value isn't used in that impl function... which probably means that it doesn't need to "belong" to `Bar`. In this [Noir PR](noir-lang/noir#7843) gets merged that's what will happen. We have a similar case in this codebase where `SharedMutableValues` has two methods: - `unpack_value_change`: doesn't refer to `INITIAL_DELAY` - `unpack_delay_change`: doesn't refer to `T` So, this PR moves those two methods outside of the impl, only specifying the generics that are needed.
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
Apr 3, 2025
…pl (#13283) This was discussed a bit over slack, but here's some context. In Rust this gives an error: ```rust pub struct Foo<T> { x: T, } impl<T> Foo<T> { fn one(x: T) { Bar::two(x); // Cannot infer the value of const parameter U } } struct Bar<T, const U: usize> { x: T, } impl<T, const U: usize> Bar<T, U> { fn two(x: T) -> Foo<T> { Foo { x } } } fn main() { Foo::<i32>::one(1); } ``` In the call `Bar::two` Rust needs to know what is the value of `U`. It can be solved by doing `Bar::<T, 1234>::two(x)` or using any numeric value... because the numeric value isn't used in that impl function... which probably means that it doesn't need to "belong" to `Bar`. In this [Noir PR](noir-lang/noir#7843) gets merged that's what will happen. We have a similar case in this codebase where `SharedMutableValues` has two methods: - `unpack_value_change`: doesn't refer to `INITIAL_DELAY` - `unpack_delay_change`: doesn't refer to `T` So, this PR moves those two methods outside of the impl, only specifying the generics that are needed.
AztecBot
pushed a commit
to AztecProtocol/aztec-nr
that referenced
this pull request
Apr 4, 2025
…pl (#13283)
This was discussed a bit over slack, but here's some context.
In Rust this gives an error:
```rust
pub struct Foo<T> {
x: T,
}
impl<T> Foo<T> {
fn one(x: T) {
Bar::two(x); // Cannot infer the value of const parameter U
}
}
struct Bar<T, const U: usize> {
x: T,
}
impl<T, const U: usize> Bar<T, U> {
fn two(x: T) -> Foo<T> {
Foo { x }
}
}
fn main() {
Foo::<i32>::one(1);
}
```
In the call `Bar::two` Rust needs to know what is the value of `U`. It
can be solved by doing `Bar::<T, 1234>::two(x)` or using any numeric
value... because the numeric value isn't used in that impl function...
which probably means that it doesn't need to "belong" to `Bar`.
In this [Noir PR](noir-lang/noir#7843) gets
merged that's what will happen. We have a similar case in this codebase
where `SharedMutableValues` has two methods:
- `unpack_value_change`: doesn't refer to `INITIAL_DELAY`
- `unpack_delay_change`: doesn't refer to `T`
So, this PR moves those two methods outside of the impl, only specifying
the generics that are needed.
…generic-numeric-type
…generic-numeric-type
This was referenced May 13, 2025
fix: remove unused generic and specify some that can't be deduced
AztecProtocol/aztec-packages#14277
Merged
|
Update:
I think the path forward would be:
Then this PR can be merged. |
jfecher
approved these changes
May 13, 2025
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
May 13, 2025
…4277) Towards noir-lang/noir#7843 The added zeroes happen in test so it should be harmless. The zero is for the `INITIAL_DELAY` which is likely unused in tests (otherwise it would have failed to compile when needing that value).
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
May 13, 2025
…4277) Towards noir-lang/noir#7843 The added zeroes happen in test so it should be harmless. The zero is for the `INITIAL_DELAY` which is likely unused in tests (otherwise it would have failed to compile when needing that value).
…generic-numeric-type
…ic-type' of github.com:noir-lang/noir into ab/better-error-message-when-cannot-infer-generic-numeric-type
…generic-numeric-type
…generic-numeric-type
…generic-numeric-type
…generic-numeric-type
…generic-numeric-type
…ic-type' of github.com:noir-lang/noir into ab/better-error-message-when-cannot-infer-generic-numeric-type
|
@TomAFrench I'll also merge this PR if it's okay with you. The libraries that fail to compile with this change are noir-jwt and the two noir-rsa. |
This reverts commit 5776ff1.
|
LGTM |
TomAFrench
deleted the
ab/better-error-message-when-cannot-infer-generic-numeric-type
branch
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Blocked on noir-bignum getting a new release, then updating dependencies to use this new release.Blocked on noir-bignum dependencies (and Aztec-Packages, I think) updating to bignum 0.7.0.
Description
Problem
Resolves #7617
Summary
Now this code:
Gives this error:
I think it's fine that the error shows up there because, for example, one can specify the type of the let variable to fix this (the issue suggested showing the error on
BoundedVec::new()instead).This PR improves a bit how unbound numeric generics are shown in inlay hints.
Before:
After:
Additional Context
I know there's #7889 but this also solves the issue of
default_typebeing wrong forNumeric, and also the fix for #7889 will be a bit different.Also marked as a breaking change because previously unused numeric generics didn't trigger the error if they were completely unused.
Documentation
Check one:
PR Checklist
cargo fmton default settings.