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

Trait lookup works on an array but not on a struct #5006

Open
LogvinovLeon opened this issue May 9, 2024 · 3 comments
Open

Trait lookup works on an array but not on a struct #5006

LogvinovLeon opened this issue May 9, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@LogvinovLeon
Copy link
Contributor

LogvinovLeon commented May 9, 2024

Aim

I want to have a trait and use it's methods consistently, but I can't call them when I implement by trait on a struct.

Expected Behavior

Here is the code:

use dep::std::unsafe::zeroed;

struct Struct {}

global BYTES32_LENGTH = 32;
type Bytes32 = [u8; BYTES32_LENGTH];

trait Serde<LEN> {
    fn deserialize(_data: [Field; LEN]) -> Self;
}

impl Serde<BYTES32_LENGTH> for Bytes32 {
    fn deserialize(_data: [Field; BYTES32_LENGTH]) -> Self {
        zeroed()
    }
}

global STRUCT_SERIALIZED_LEN = 0;

impl Serde<STRUCT_SERIALIZED_LEN> for Struct {
    fn deserialize(_data: [Field; STRUCT_SERIALIZED_LEN]) -> Self {
        zeroed()
    }
}

fn main() {
    let _: Struct = [].deserialize();
    let _: Bytes32 = [1; 32].deserialize();
}

For some reason - the second line in main is OK while the first one is a compilation error

Bug

error: No method named 'deserialize' found for type '[Field; 0]'
   ┌─ /Users/leonidlogvinov/Dev/ZK/trait_repro/src/main.nr:29:22
   │
27 │     let _: Struct = [].deserialize();
   │                      -----------------
   │

To Reproduce

  1. Try to compile this code

Project Impact

Nice-to-have

Impact Context

Makes traits harder to use - causes worse code quality and hurts maintainability

Workaround

Yes

Workaround Description

Use syntax like: Struct::deserialize

Additional Context

No response

Installation Method

Binary (noirup default)

Nargo Version

0.28.0

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@LogvinovLeon LogvinovLeon added the bug Something isn't working label May 9, 2024
@jfecher
Copy link
Contributor

jfecher commented May 9, 2024

impl Serde<STRUCT_SERIALIZED_LEN> for Struct {
    fn deserialize(_data: [Field; STRUCT_SERIALIZED_LEN]) -> Self {
        zeroed()
    }
}

fn main() {
    let _: Struct = [].deserialize();
    let _: Bytes32 = [1; 32].deserialize();
}

The error here is because you're implementing the trait for the self type of Struct but the self type on the method is an array instead. So the compiler doesn't find an impl when it sees [].deserialize() and looks for impl Serde<?> for [?; 0].

I'm not sure why the compiler mentions the type as [Field; 1] in the error message instead though when the length is zero.

@LogvinovLeon
Copy link
Contributor Author

That was my bad as I've changed array length from 1 to 0 during the writing of this issue. Edited.

But on the nature of the issue:

  • Why does it work for Bytes32 then. From your logic - it should not work for both?
  • Can the compiler take the info from type annotation of the return value and see that I want to get a Struct, so look for impl Serde<?> for Struct?

@jfecher
Copy link
Contributor

jfecher commented May 10, 2024

  • I think what is going on internally is that impl Serde<BYTES32_LENGTH> for Bytes32 is being selected using the object type of the literal: [<integer type variable>; 32] since integer literals are polymorphic in Noir. I've confirmed that this type variable is being correctly bound to a Field type. It's possible we're using the object type as the Self type only as a hint of sorts which could be why it's throwing off the search in the first case but still binding to the Field type in this case. That's just a guess though - there could be a few reasons for this (I could also be wrong).
  • Yes, and we already do so when you call e.g. Default::default(). I think the object type not being the same as the Self type that is just what is throwing things off here. For now the workaround is to use the Trait::method(args) syntax when self is not the first parameter.

Also - looking at your trait definition:

trait Serde<LEN> {
    fn deserialize(_data: [Field; LEN]) -> Self;
}

We're in the process of implementing arithmetic on numeric generics: #4958 which should hopefully greatly help here when implementing Serde on generic types. This was actually the inspiration for this feature - we've had other users trying to implement (de)serialization as well and they've quickly hit this wall. Another thing that'd be nice to have that Noir doesn't have yet is associated constants:

trait Serde {
    let LEN: u64;
    fn deserialize(_data: [Field; Self::LEN]) -> Self;
}

Since you shouldn't be able to implement Serde twice for the same object type but different lengths (this helps type inference as a well).

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
Status: 📋 Backlog
Development

No branches or pull requests

2 participants