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

to_le_bytes/to_be_bytes return fixed size arrays when applied to constants #4048

Closed
Tracked by #3015
ggiraldez opened this issue Jan 15, 2024 · 0 comments · Fixed by #4049
Closed
Tracked by #3015

to_le_bytes/to_be_bytes return fixed size arrays when applied to constants #4048

ggiraldez opened this issue Jan 15, 2024 · 0 comments · Fixed by #4049
Labels
bug Something isn't working

Comments

@ggiraldez
Copy link
Contributor

Aim

The documentation states that these Field methods should return slices, but that is not the case when applied to constants. The compiler will apply an optimization to precompute the result but it will return it in an array instead of a slice.

I also think the documentation is a bit outdated on to_le_bits/to_be_bits (see here) because it says it returns an array, but the stdlib definition returns a slice.

Expected Behavior

to_be_bytes/to_le_bytes should return a slice consistently, regardless if it's applied to a constant or not.

Bug

Given this program:

use dep::std;

fn main() {
    let x = 2040124;
    let mut a = x.to_le_bytes(7);
    let mut a1 = a.push_back(123);

    std::println(a.len());
    std::println(a1.len());
    std::println(a1[7]);
}

Fails compilation with this error:

error: Index out of bounds, array has size 7, but index was 7
   ┌─ /home/ggiraldez/Scratchpad/to_radix/src/main.nr:10:18
   │
10 │     std::println(a1[7]);
   │                  -----
   │
   = Call stack:
     1. /home/ggiraldez/Scratchpad/to_radix/src/main.nr:10:18

Aborting due to 1 previous error

But this program (with the appropriate inputs defined):

use dep::std;

fn main(x: Field) {
    let mut a = x.to_le_bytes(7);
    let mut a1 = a.push_back(123);

    std::println(a.len());
    std::println(a1.len());
    std::println(a1[7]);
}

Produce the expected output:

0x07
0x08
123
[to_radix] Circuit witness successfully solved

To Reproduce

Attempting to compile or execute the above program will fail with the error mentioned above.

Installation Method

None

Nargo Version

No response

Additional Context

As stated above, this happens because the compiler will apply an optimization when resolving a ToRadix intrinsic used on a constant value. The relevant code is here.

Would you like to submit a PR for this Issue?

Yes

Support Needs

No response

@ggiraldez ggiraldez added the bug Something isn't working label Jan 15, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jan 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 16, 2024
# Description

The compiler optimizes uses of the `ToRadix` intrinsic applied to
constants by precomputing the results.

## Problem\*

Resolves #4048

The optimization produces a fixed sized array instead of a slice.

## Summary\*

The PR changes the `constant_to_radix` function to produce a slice and
generates a tuple with the length and the slice itself, which fits the
internal representation of slices in SSA.

## Additional Context

This was found and fixed during the implementation of the debugger (see
#3015).

## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant