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

Encountering "ERROR: ICE expected slice sizes" for slice members #3476

Closed
Tracked by #3363
grasshopper47 opened this issue Nov 10, 2023 · 10 comments · Fixed by #3634
Closed
Tracked by #3363

Encountering "ERROR: ICE expected slice sizes" for slice members #3476

grasshopper47 opened this issue Nov 10, 2023 · 10 comments · Fixed by #3634
Assignees
Labels
bug Something isn't working

Comments

@grasshopper47
Copy link
Contributor

Aim

Set a slice-of-slices member property value

Consider the following structs:

struct Property
{
    key : [u8],
    value : [u8],
}

struct JSON
{
    doc : [Property]
}

Let's look at the snippet below.

It does a linear search of key property bytes (eq is an extension method for [u8] provided privately), and looks to set/replace the value property bytes with a local prop instance bytes.

let json = JSON { doc: [] };
let prop = Property { key: [], value:[] };

// .. parse json .. 

// .. set the property's key bytes .. 
// .. set the property's value bytes .. 

// add property to json or replace existing
let len : Field = json.doc.len();
let mut found = false;
for i in 0..len 
{ 
    if (!found) 
    { 
        if (prop.key.eq(json.doc[i].key)) 
        { 
            json.doc[i].value = prop.value; 
            found = true; 
        } 
    } 
}

This code worked fine until nightly-2023-11-04. From this version onward the error "ICE expected slice sizes" is thrown by the compiler. This is confusing because it is missing contextual information.

The PR that lead to this breaking change is 48cf1a7

A workaround is to refactor the code: create a copy of the doc property, skip the existing property, push the new property instance in the copy, then re-assign the copy to the initial json instance's doc property:

let mut copy : [Property] = [];
for current in json.doc { if (!prop.key.eq(current.key)) { copy = copy.push_back(current); } }
json.doc = copy.push_back(prop);

Expected Behavior

The code provided should work as it did before nightly-2023-11-04

Bug

  1. Should work as before
  2. The error should include more contextual information until 1 is fixed

To Reproduce

See details in Aim section

Installation Method

Binary

Nargo Version

nargo 0.19.2

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@grasshopper47 grasshopper47 added the bug Something isn't working label Nov 10, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Nov 10, 2023
@vezenovm
Copy link
Contributor

@grasshopper47 Do you have a more reproducible example? It would be helpful to see what your extension method eq is doing, as you shouldn't be able to declare impls on non-struct types unless you have it in your own version of the stdlib?

Also in the provided snippet this should not be possible to compile to ACIR as the loop bound that comes from let len : Field = json.doc.len(); is dynamic. I assume you are doing this in an unconstrained function?

I have this program here:

struct Property
{
    key : [u8],
    value : [u8],
}

struct JSON
{
    doc : [Property]
}

unconstrained fn main() {
    let mut json = JSON { doc: [] };
    let mut prop = Property { key: [], value:[] };

    let other_prop = Property { key: [0, 1, 2], value:[10] };
    json.doc = json.doc.push_back(other_prop);

    for i in 0..3 {
        prop.key = prop.key.push_back(i as u8);
    }
    prop.value = prop.value.push_back(5);

    // add property to json or replace existing
    let len : Field = json.doc.len();
    let mut found = false;
    for i in 0..len 
    { 
        if (!found) 
        { 
            if (prop.key.eq(json.doc[i].key)) 
            { 
                json.doc[i].value = prop.value; 
                found = true; 
            } 
        } 
    }
    assert(found == true);
}

This program successfully executes following this PR (#3617).

@vezenovm
Copy link
Contributor

This program successfully executes following this PR (#3617).

I have pulled this out into the PR here: #3634.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Nov 29, 2023
@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

@kevaundray @vezenovm

This is still an issue!

I've put out an even simpler example below:

struct Property
{
    key : [u8]
,   value : [u8]
}

type Object = [Property];

struct JSON
{
    doc : Object
}

impl JSON
{
    unconstrained
    fn store(mut self, prop : Property) -> Object
    {
        for i in 0..self.doc.len() { self.doc[i].value = prop.value; }

        self.doc
    }
}

#[test]
unconstrained
fn test()
{
    let mut json = JSON { doc: [ Property { key: "a".as_bytes(), value: "1".as_bytes() } ] };
    let mut prop = Property { key: "a".as_bytes(), value:"123".as_bytes() };

    json.doc = json.store(prop);
}

You can ignore the type alias

My gut says it's a problem with slices of structs with slices as members

@vezenovm
Copy link
Contributor

@grasshopper47 Yes this is the issue, but this was only just merged into master. On the next nightly and release this should be fixed. The most recent nightly was published 16 hours ago. What version of nargo are you using? I just tested your simple example when building from source and it passes as expected.

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

@grasshopper47 Yes this is the issue, but this was only just merged into master. On the next nightly and release this should be fixed. The most recent nightly was published 16 hours ago. What version of nargo are you using? I just tested your simple example when building from source and it passes as expected.

I'm getting a bit confused; i thought the change went in via #3634, which is already in master. I've built the latest master, with this change in, and the error still happens....

@vezenovm
Copy link
Contributor

vezenovm commented Nov 29, 2023

I'm getting a bit confused; i thought the change went in via #3634, which is already in master. I've built the latest master, with this change in, and the error still happens....

When I build from master the error is gone. Are you using noirup to build from source? I just wanted to make sure you weren't using a binary that had not been published with this change yet.

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

I'm getting a bit confused; i thought the change went in via #3634, which is already in master. I've built the latest master, with this change in, and the error still happens....

When I build from master the error is gone. Are you using noirup to build from source?

Building from source via updating my fork...and wouldn't you know it there was an issue with that and I just thought I was building the latest, when in fact it was something else....sorry for the confusion, appreciate the reply!

OK to close!

Still not OK, see below comment

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

@vezenovm @kevaundray I'm re-reopening this

Still an issue, please run the code below and check the output
I've included printing to see the actual values in memory

global BEGIN_OBJECT    : u8 = 0x7B; // { left curly bracket
global END_OBJECT      : u8 = 0x7D; // } right curly bracket
global BACKSLASH       : u8 = 0x5C; // \ reverse solidus

global chars =
[
    "\0","\0","\0","\0","\0","\0","\0","\0","\0","\t","\n","\0","\0","\r","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0","\0",
    " ","!","\"","#","$","%","&","'","(",")","*","+",",","-",".","/",
    "0","1","2","3","4","5","6","7","8","9",
    ":",";","<","=",">","?","@",
    "A","B","C","D","E","F","G","H","I","J","K","L","M","N","O","P","Q","R","S","T","U","V","W","X","Y","Z",
    "[","\\","]","^","_","`",
    "a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z",
    "{","|","}","~"
];

struct Property
{
    key : [u8]
,   value : [u8]
}

type Object = [Property];

struct JSON
{
    doc : Object
}

impl Property
{
    unconstrained
    fn print(self)
    {
        dep::std::println("{\nkey");
        for byte in self.key { let c = chars[byte]; dep::std::println(f"    {c}"); }
        dep::std::println("}");

        dep::std::println("value\n{");

        let mut object = false;
        for byte in self.value
        {
            if (object & (byte != END_OBJECT) & (byte != BACKSLASH))
            {
                dep::std::println(f"        {byte}");
            }
            else
            {
                let c = chars[byte];
                dep::std::println(f"    {c}");

                object = (byte == BEGIN_OBJECT);
            }
        }

        dep::std::println("}");
    }
}

impl JSON
{
    unconstrained
    fn store(mut self, prop : Property) -> Object
    {
        for i in 0..self.doc.len() { self.doc[i].value = prop.value; }

        self.doc
    }
}

#[test]
unconstrained
fn property_key_number_and_key_array_object()
{
    let mut json = JSON { doc: [ Property { key: "a".as_bytes(), value: "1".as_bytes() } ] };
    let mut prop = Property { key: "a".as_bytes(), value:"123".as_bytes() };

    json.doc = json.store(prop);

    for prop in json.doc { prop.print(); }
}

the output is:

key
{
    1
    
    
    a
    1
    2
    3
    
    
    
    
    
    
    
    
}
value
{
    
}

while the expected output is:

key
{
    a
}
value
{
    1
    2
    3
}

@grasshopper47
Copy link
Contributor Author

If this works better as a separate issue, I can open a new one, just let me know what is preferred.

@vezenovm
Copy link
Contributor

If this works better as a separate issue, I can open a new one, just let me know what is preferred.

This is a separate bug so a new issue would be preferred.

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.

2 participants