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

Numeric generic struct parameter panics when accessed uninitialized #5436

Closed
michaeljklein opened this issue Jul 8, 2024 · 4 comments · Fixed by #5619
Closed

Numeric generic struct parameter panics when accessed uninitialized #5436

michaeljklein opened this issue Jul 8, 2024 · 4 comments · Fixed by #5619
Assignees
Labels
bug Something isn't working

Comments

@michaeljklein
Copy link
Contributor

Aim

Attempted to compile the following program:

struct Foo<let N: u32> { }

impl<let N: u32> Foo<N> {
    fn size(self) -> u32 {
        N
    }
}

fn main() {
    let foo = Foo { };
    assert(foo.size() == 0);
}

Expected Behavior

Expected the program to either compile successfully (having defaulted N = 0) or fail with a user error because N is uninitialized when declaring foo.

Bug

The application panicked (crashed).
Message:  Non-numeric type variable used in expression expecting a value
Location: compiler/noirc_frontend/src/monomorphization/mod.rs:909

To Reproduce

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@michaeljklein michaeljklein added the bug Something isn't working label Jul 8, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 8, 2024
@vezenovm
Copy link
Contributor

vezenovm commented Jul 8, 2024

fail with a user error because N is uninitialized when declaring foo.

Yeah I think we should fail with that a type annotation is needed here.

@asterite
Copy link
Collaborator

@noir-lang/core Rust gives an error whenever a type's generic can't be deduced. For example:

struct Foo<const N: u32> {}

impl<const N: u32> Foo<N> {
    fn size(self) -> u32 {
        N
    }
}

fn main() {
    let foo = Foo {}; // type annotations needed for `Foo<N>`
    let x = foo.size();
}

That said, the error happens regardless of size() being called or not. This produces the same error:

fn main() {
    let foo = Foo {}; // type annotations needed for `Foo<N>`
}

However in Noir we don't error in that case. Here's a Noir program that compiles fine:

struct Foo<N> { }

fn main() {
    let foo = Foo {}; // No error here, `foo` is deduced to be `Foo<_>`
}

But maybe the above program should give an error similar to Rust? What do you think?

@michaeljklein
Copy link
Contributor Author

michaeljklein commented Jul 25, 2024

I would prefer to error whenever a type's generic can't be deduced.

Consider the following code:

fn copy_from_slice<let N: u32>(xs: [u8]) -> [u8; N] {
  let mut inp = xs;
  let mut out = [0; N];
  for i in 0..N {
    let (x, rest) = inp.pop_front();
    inp = rest;
    out[i] = x;
  }
  out
}

let my_slice: [u8] = [0; 8].as_slice();
let output_array = copy_from_slice(my_slice);

If we default N, output_array will have length == 0 and nothing will be copied/output.

@jfecher
Copy link
Contributor

jfecher commented Jul 25, 2024

We should error in this case - I'm surprised we don't. Perhaps we treat these numeric generics as TypeVariableKind::Integers and thus default them instead?

@asterite asterite self-assigned this Jul 26, 2024
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 31, 2024
guipublic pushed a commit that referenced this issue Jul 31, 2024
# Description

## Problem

Resolves #5436

## Summary

The monomorphizer checks that no types exist that aren't unbound and
can't have a default type. This is also the case for structs, but this
check is done on field types after they are paired with generics. For
example:

```rust
struct Foo<T> {
  x: T
}

fn main() {
  let _ = Foo { x: 1 };
}
```

Here we pair `T` with the generic type of 1, then it's defaulted to
`Field`, no error.

However, it can happen that a generic parameter isn't mentioned at all
in a struct field. For example:

```rust
struct Foo<T> {
}

fn main() {
  let _ = Foo {};
}
```

Here `T` isn't paired with anything, so it's left unbound. However,
because we only check that the resulting field types aren't unbound, no
error happens.

The solution in this PR is to check that generic parameters aren't
unbound. This required a method like `convert_type`, called
`check_type`, that only checks for this errors. The reason is that I
wanted to avoid converting types twice. But let me know if that would be
fine (or if there's another solution for this).

## Additional Context

In Rust if a generic parameter isn't used in a field, an error about
that is made. However, no error happens if the generic parameter is a
`const` (similar to a `let N: u32` parameter in Noir) so here I chose to
error un unbound generics, not check if a generic is unused.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** 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.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
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: Done
Development

Successfully merging a pull request may close this issue.

4 participants