Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arithmetic generics cannot simplify [Field; (N-1) + 1] to [Field; N] #6495

Closed
TomAFrench opened this issue Nov 11, 2024 · 2 comments · Fixed by #6502
Closed

Arithmetic generics cannot simplify [Field; (N-1) + 1] to [Field; N] #6495

TomAFrench opened this issue Nov 11, 2024 · 2 comments · Fixed by #6502

Comments

@TomAFrench
Copy link
Member

On the latest sync we've started getting errors of the form

  ./noir-projects+build-mock-protocol-circuits *failed* | error: expected type [Field; N], found type [Field; ((N - 1) + 1)]
  ./noir-projects+build-mock-protocol-circuits *failed* |    ┌─ /usr/src/noir-projects/noir-protocol-circuits/crates/types/src/abis/side_effect.nr:78:27
  ./noir-projects+build-mock-protocol-circuits *failed* |    │
  ./noir-projects+build-mock-protocol-circuits *failed* | 78 │     fn serialize(self) -> [Field; N] {
  ./noir-projects+build-mock-protocol-circuits *failed* |    │                           ---------- expected [Field; N] because of return type
  ./noir-projects+build-mock-protocol-circuits *failed* | 79 │         array_concat(self.inner.serialize(), [self.counter as Field])
  ./noir-projects+build-mock-protocol-circuits *failed* |    │         ------------------------------------------------------------- [Field; ((N - 1) + 1)] returned here
  ./noir-projects+build-mock-protocol-circuits *failed* |    │

In #6494 I've added a unit test showing how we fail to canonicalize the length of these arrays.

We should update the canonicalization logic such that the solves_n_plus_one_minus_one_array_length test passes.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Nov 11, 2024
@TomAFrench
Copy link
Member Author

TomAFrench commented Nov 11, 2024

@michaeljklein potentially related to your recent changes? Looks like we just weren't canonicalizing array lengths at all.

@jfecher
Copy link
Contributor

jfecher commented Nov 11, 2024

Hmm, it's possible the new CheckedCast variant is messing with recursive matches when we try to check for Infix(Infix(N, -, 1), +, 1) since it now could be Infix(CheckedCast(Infix(N, -, 1)), +, 1) or another variant with CheckedCast wrapping the constants as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
2 participants