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

Compiler error with generic impls and traits #4502

Closed
nventuro opened this issue Mar 7, 2024 · 8 comments · Fixed by #4648
Closed

Compiler error with generic impls and traits #4502

nventuro opened this issue Mar 7, 2024 · 8 comments · Fixed by #4648
Labels
bug Something isn't working

Comments

@nventuro
Copy link
Contributor

nventuro commented Mar 7, 2024

Aim

This is a minimal reproduction of an issue I encountered while trying to write tests for aztec-nr libraries. The libraries have generic types, and in my tests I needed to use concrete types with them. This broke the build as unrelated callsites started complaining about mismatching types. Not being an expert in the language, this seems like a bug.

Expected Behavior

I expected to be able to have generic functions and to be able to call them using concrete types.

Bug

The following does not compile:

// This is some trait
trait Trait {
    fn new() -> Self;
}

// This is a library function over a type that impls the trait
pub fn library_fn<TypeThatImpls>() -> TypeThatImpls where TypeThatImpls: Trait {
    TypeThatImpls::new()
}

// This is a concrete struct
struct ConcreteTraitImpl;

// That impls the trait expected by library_fn
impl Trait for ConcreteTraitImpl {
    fn new() -> Self {
        ConcreteTraitImpl {}
    }
}

// This is a generic struct
struct LibraryUser<SomeType>;

// This is a generic impl for the generic struct
impl<SomeType> LibraryUser<SomeType> {

    // The following function causes a compiler error. Comment it out to compile.
   // This calls the original library function with a generic type. I imagine library_fn's generic 
   // type TypeThatImpls is made the same as SomeType because of the return type.
    fn call_library_fn() -> SomeType {
        library_fn()
    }
}

fn main() { }

The error is the following:

25 │     fn call_library_fn() -> SomeType {
   │                             -------- expected SomeType because of return type
26 │         library_fn()
   │         ------------ ConcreteTraitImpl returned here

I'm confused as to why ConcreteTraitImpl suddenly shows up as the return type. Maybe because it is the only type that impls the trait? Even then this seems very odd to me (though again, far from an expert).

I expected the error to go away if I deleted ConcreteTraitImpl, but get a different one instead:

17 │         library_fn()
   │         ---------- No impl for `SomeType: Trait`

This seems to hint at me missing something, though I don't really know how to interpret this. Sure, there's no impl for that trait, but I'm not trying to instantiate the struct, call the function, etc., so why does this matter at this stage?

Project Impact

Blocker

Installation Method

Compiled from source

Nargo Version

nargo version = 0.24.0 noirc version = 0.24.0+6522738a781fcff2353d1edca4402c0978a62653 (git version hash: 6522738, is dirty: false)

@nventuro nventuro added the bug Something isn't working label Mar 7, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Mar 7, 2024
@nventuro nventuro changed the title Type issues Compiler error with generic impls and traits Mar 7, 2024
@nventuro
Copy link
Contributor Author

nventuro commented Mar 7, 2024

For what it's worth, the above code compiles in Rust (with minor modifications: Rust requires that we use the type parameter somewhere for SomeType, and also that we further constrain SomeType to implement Trait, which it implicitly should due to SomeType == TypeThatImpls as mentioned above).

@TomAFrench
Copy link
Member

TomAFrench commented Mar 7, 2024

I've noticed that you don't have a trait bound on the impl of Library user. library_fn can't know that SomeType will satisfy the bound on its return type. Looks like we don't support this currently.

@nventuro
Copy link
Contributor Author

nventuro commented Mar 7, 2024

Something worth mentioning is that in the original aztec-nr scenario, this was triggered when I added a concrete implementation (in the same crate). I am not sure why this example does not work even when there is no concrete implementation - perhaps there's some other detail missing.

@jfecher
Copy link
Contributor

jfecher commented Mar 7, 2024

25 │     fn call_library_fn() -> SomeType {
   │                             -------- expected SomeType because of return type
26 │         library_fn()
   │         ------------ ConcreteTraitImpl returned here

This is odd, I wonder if this is related to #4450.

17 │         library_fn()
   │         ---------- No impl for `SomeType: Trait`

This is the expected error, your call_library_fn is generic over any type SomeType, so we don't know if this type implements Trait or not. You need to restrict this method to be generic over not all types, but only types that implement Trait:

impl<SomeType> LibraryUser<SomeType> {
    // The following function causes a compiler error. Comment it out to compile.
   // This calls the original library function with a generic type. I imagine library_fn's generic 
   // type TypeThatImpls is made the same as SomeType because of the return type.
    fn call_library_fn() -> SomeType where SomeType: Trait {
        library_fn()
    }
}

@nventuro
Copy link
Contributor Author

nventuro commented Mar 7, 2024

This is the expected error, your call_library_fn is generic over any type SomeType, so we don't know if this type implements Trait or not. You need to restrict this method to be generic over not all types, but only types that implement Trait:

Ahhh so it's the same error as in Rust, I didn't realize this from the error message. Theirs is quite clear:

error[E0277]: the trait bound `SomeType: Trait` is not satisfied
  --> src/main.rs:33:9
   |
33 |         library_fn()
   |         ^^^^^^^^^^ the trait `Trait` is not implemented for `SomeType`
   |
note: required by a bound in `library_fn`
  --> src/main.rs:7:74
   |
7  | pub fn library_fn<TypeThatImpls>() -> TypeThatImpls where TypeThatImpls: Trait {
   |                                                                          ^^^^^ required by this bound in `library_fn`
help: consider restricting type parameter `SomeType`
   |
27 | impl<SomeType: Trait> LibraryUser<SomeType>  {
   |              +++++++

As a side note, it'd be convenient if I could apply the where clause to the entire impl and not just that function. In our actual aztec-nr library that where is currently repeated 8 times.

@jfecher
Copy link
Contributor

jfecher commented Mar 7, 2024

@nventuro do you mind creating an issue for the where clause on an impl? It is mentioned in the Epic: #2568 under "Implement trait constraints" .. "free impl blocks" but we don't have an issue yet for it. Since it is just syntactic sugar it's helpful to know how much it is wanted.

An issue for an error message that can be improved would be helpful as well.

@nventuro
Copy link
Contributor Author

nventuro commented Mar 7, 2024

Sure, created #4508 and #4509. I didn't want to spam you with random requests, but I'm happy to do things that way in the future!

@sklppy88
Copy link
Contributor

Had a call w/ @nventuro we think there's another use case where it fails under similar conditions. Would be happy to supply more doc if this minimal reproduction needs extension. This was a weird blocker that lead me down a few rabbit-holes 😅

github-merge-queue bot pushed a commit that referenced this issue Mar 29, 2024
…4648)

# Description

## Problem\*

Resolves #4635 
Resolves #4502

## Summary\*

This was more difficult to fix than it originally seemed. The main issue
was between interactions with unbound type variables, type aliases, type
rules for operators, and operator traits.

Removing the "infer unbound type variables to be numeric" rule on
operators causes a lot of stdlib code to break where it'd be
unreasonable to have type annotations. This caused unbound type
variables to be bound to the first object type whose impl it was checked
against when calling verify trait impl.

I eventually settled on just delaying the verify trait impl check for
operators until the end of a function when more types are known.

## Additional Context



## 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.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Mar 29, 2024
TomAFrench pushed a commit that referenced this issue Apr 3, 2024
…4648)

# Description

## Problem\*

Resolves #4635 
Resolves #4502

## Summary\*

This was more difficult to fix than it originally seemed. The main issue
was between interactions with unbound type variables, type aliases, type
rules for operators, and operator traits.

Removing the "infer unbound type variables to be numeric" rule on
operators causes a lot of stdlib code to break where it'd be
unreasonable to have type annotations. This caused unbound type
variables to be bound to the first object type whose impl it was checked
against when calling verify trait impl.

I eventually settled on just delaying the verify trait impl check for
operators until the end of a function when more types are known.

## Additional Context



## 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.
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue May 23, 2024
Some time ago @sklppy88 created some note getter tests in noir. These
could not be placed in the same crate as `NoteInterface` due to
noir-lang/noir#4502. However, this prevented
us from importing the `TestNote` in the original aztec-nr crate as that
would've created cyclical imports, which are not allowed.

Now that noir-lang/noir#4502 is fixed, we can
have the `TestNote` live alongisde `NoteInterface`. This PR deletes the
`test` crate and creates instead a `test` module inside the `aztec_nr`
crate. I moved the note getter tests into a `test` submodule, meaning we
can access internal functions (i.e. no more `pub` internals) while still
having the tests live in separate files.

I also created a context builder object, which is something I've had to
manually craft multiple times, and is now slightly more complicated due
to the new AVM opcodes.

Hopefully this will be a better foundation over which to continue
writing Noir tests for aztec-nr.
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue May 24, 2024
Some time ago @sklppy88 created some note getter tests in noir. These
could not be placed in the same crate as `NoteInterface` due to
noir-lang/noir#4502. However, this prevented
us from importing the `TestNote` in the original aztec-nr crate as that
would've created cyclical imports, which are not allowed.

Now that noir-lang/noir#4502 is fixed, we can
have the `TestNote` live alongisde `NoteInterface`. This PR deletes the
`test` crate and creates instead a `test` module inside the `aztec_nr`
crate. I moved the note getter tests into a `test` submodule, meaning we
can access internal functions (i.e. no more `pub` internals) while still
having the tests live in separate files.

I also created a context builder object, which is something I've had to
manually craft multiple times, and is now slightly more complicated due
to the new AVM opcodes.

Hopefully this will be a better foundation over which to continue
writing Noir tests for aztec-nr.
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.

4 participants