-
Notifications
You must be signed in to change notification settings - Fork 206
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
fix: initialise arrays returned by brillig #2048
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes:
-
Lets not change the visibility of AcirVarData and vars, we can create methods on the context which expose the public functionality needed instead of just exposing the public type.
-
Either way I think we want to avoid pub here in favour of a more restrictive pub(crate)
-
On mobile, so perhaps I'm wrong -- it sems like we don't need to change the visibility
I was debugging the nested arrays, it should not have been commit, I just reverted it. |
Can you include a test case? |
Would this handle returned nested arrays created by #2047 ? |
I added one |
No, the nested arrays test is failing after updating from master. |
We still do not handle nested arrays, but the test is now passing, we simply don't add the init opcode for nested arrays, since they are not supported anyways. |
We don't support multidimensional arrays but we do support nested arrays, for example, if you have an array of structs that contain arrays. This might break the initialization? |
@sirasistant what were your thoughts on this PR? |
This is needed for dynamic accesses to arrays returned from brillig, but if it's not too complicated we should also init nested returned arrays, if it's complicated we should create an issue and merge this as is since it's already is solving a problem :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh I see that what's not supported is nested dynamic arrays, got it. Let's merge this!
crates/nargo_cli/tests/test_data_ssa_refactor/brillig_unitialised_arrays/src/main.nr
Outdated
Show resolved
Hide resolved
* master: chore: Move tooling related items into their own directory (#2644) chore: add `CompilationResult` helper type (#2639) fix: initialise arrays returned by brillig (#2048) chore: clippy fix (#2631) fix(wasm): Remove stacker from dependencies (#2637) chore(ci): reenable CI for `noir_wasm` (#2636) fix: avoid overflows in integer division (#2180) chore(ci): Nightly Integration testing (#2596) feat(parser): allow multiple attributes (#2537) feat(nargo): Allow installing custom backends from the CLI (#2632) chore(ci): enforce clippy and `cargo fmt` in CI (#2628)
Description
Problem*
We generate an init opcode for arrays in acir, in order to handle dynamic arrays.
However arrays returned from brillig were not initialised.
Resolves
Summary*
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmt
on default settings.