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

Chained constructors give Expression type is ambiguous #4732

Closed
Tracked by #4710
sirasistant opened this issue Apr 8, 2024 · 3 comments · Fixed by #5131
Closed
Tracked by #4710

Chained constructors give Expression type is ambiguous #4732

sirasistant opened this issue Apr 8, 2024 · 3 comments · Fixed by #5131
Labels
bug Something isn't working

Comments

@sirasistant
Copy link
Contributor

sirasistant commented Apr 8, 2024

Aim

Code like this used to compile:

use dep::aztec::{prelude::{NoteViewerOptions, PrivateSet}};
use dep::value_note::value_note::ValueNote;

fn main() -> pub Field {
    let set: PrivateSet<ValueNote> = PrivateSet::new(dep::std::unsafe::zeroed(), 0);
    let options = NoteViewerOptions::new().set_limit(1); // chained construction happening here
    let notes = set.view_notes(options);
    notes[0].unwrap_unchecked().value
}

With this in dependencies:

[dependencies]
aztec = { git = "https://github.com/AztecProtocol/aztec-packages", tag = "master", directory = "noir-projects/aztec-nr/aztec" }
value_note = { git = "https://github.com/AztecProtocol/aztec-packages", tag = "master", directory = "noir-projects/aztec-nr/value-note" }

Here is the code for NoteViewerOptions and private set:
https://github.com/AztecProtocol/aztec-packages/blob/master/noir-projects/aztec-nr/aztec/src/note/note_viewer_options.nr
https://github.com/AztecProtocol/aztec-packages/blob/master/noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr

Expected Behavior

I think it should compile, since the generic of NoteViewerOptions should be resolved to the generic of set that is ValueNote

Bug

Instead it errors with Expression type is ambiguous

To Reproduce

Project Impact

Nice-to-have

Impact Context

No response

Workaround

Some

Workaround Description

Splitting it in two lines works:

use dep::aztec::{prelude::{NoteViewerOptions, PrivateSet}};
use dep::value_note::value_note::ValueNote;

fn main() -> pub Field {
    let set: PrivateSet<ValueNote> = PrivateSet::new(dep::std::unsafe::zeroed(), 0);
    let mut options = NoteViewerOptions::new();
    options = options.set_limit(1);
    let notes = set.view_notes(options);
    notes[0].unwrap_unchecked().value
}

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

@sirasistant sirasistant added the bug Something isn't working label Apr 8, 2024
@jfecher
Copy link
Contributor

jfecher commented Apr 10, 2024

Can you give an example without the aztec dependency? I've tried encoding a version of NoteViewerOptions directly but it works as expected.

Instead it errors with Expression type is ambiguous

Where is the error? On the set_limit call, NoteViewerOptions::new, or somewhere else?

@jfecher
Copy link
Contributor

jfecher commented Apr 10, 2024

Actually, I added a trait constraint like in the original and the error immediately appeared afterward:

fn main() -> pub Field {
    let _options = NoteViewerOptions::new().set_limit(1);
    0
}

struct NoteViewerOptions<Note, N> {
    limit: u32
}

impl<Note, N> NoteViewerOptions<Note, N> {
    pub fn new() -> NoteViewerOptions<Note, N> where Note: Default {
        NoteViewerOptions { limit: 0 }
    }

    pub fn set_limit(&mut self, limit: u32) -> Self {
        self.limit = limit;
        *self
    }
}

The error is on NoteViewerOptions::new which makes sense since the Note type is never constrained so it doesn't know which type to use to try to find Note: Default

I think an improvement for this error message is that it could try to mention the Note and N generics specifically as needing to be specified.

Rust gives a similar error message here but mentions the original Note and N names, and mentions the context of solving for a trait:

error[E0283]: type annotations needed for `NoteViewerOptions<Note, N>`
  --> foo.rs:2:9
   |
2  |     let _options = NoteViewerOptions::new().set_limit(1); // chained construction happening here
   |         ^^^^^^^^   ---------------------- type must be known at this point
   |
   = note: cannot satisfy `_: Default`
note: required by a bound in `NoteViewerOptions::<Note, N>::new`
  --> foo.rs:12:60
   |
12 |     pub fn new() -> NoteViewerOptions<Note, N> where Note: Default {
   |                                                            ^^^^^^^ required by this bound in `NoteViewerOptions::<Note, N>::new`
help: consider giving `_options` an explicit type, where the type for type parameter `Note` is specified
   |
2  |     let _options: NoteViewerOptions<Note, N> = NoteViewerOptions::new().set_limit(1); // chained construction happening here
   |                 ++++++++++++++++++++++++++++

@jfecher
Copy link
Contributor

jfecher commented Apr 10, 2024

In your original example, I think it is occurring because let notes = set.view_notes(options); constrains the Note type to be ValueNote but does not constrain what N should be. I'm not sure why this wouldn't occur when a mutation is done. It may be related to the that type variable now being bound to another type variable.

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

# Description

## Problem\*

Resolves #5065 

Probably resolves #4732 but need to test it or have aztec team test it.

## Summary\*

When working with a program like such where `MyType` implements
`MyTrait`:
```rust
fn foo<T>() -> T where T: MyTrait {
    MyTrait::new()
}
fn concise_regression() -> MyType {
    Wrapper::new(foo()).unwrap()
}
```
We should be able to infer the return type of `foo`. We currently always
push trait constraints onto a a `Vec<(TraitConstraint, ExprId)>`. We
need to do this as we can have multiple trait constraints that need to
be handled during monomorphization due to generics. However, when
working with a method call this can cause us to store an old trait
constraint that does not necessarily apply to the expression.

The nested function call in `concise_regression` initially adds a trait
constraint simply for `foo` due to the call to `Wrapper::new(foo())` and
then another constraint for `Wrapper::new(foo()).unwrap()`. The call to
`Wrapper::new(foo())` cannot be bound to anything unless we introduce an
intermediate variable. This felt like it would be overly complex and we
just need to follow the accurate trait constraint for a function call
expression.

Taking the test in the issue and this PR we have the following trait
constraints on master for the `foo` expression:
```
TraitConstraint {
        typ: '23646 -> '23647,
        trait_id: TraitId(
            ModuleId {
                krate: Root(
                    1,
                ),
                local_id: LocalModuleId(
                    Index(
                        1,
                    ),
                ),
            },
        ),
        trait_generics: [],
    },
    TraitConstraint {
        typ: '23648 -> '23649 -> '23650 -> MyType,
        trait_id: TraitId(
            ModuleId {
                krate: Root(
                    1,
                ),
                local_id: LocalModuleId(
                    Index(
                        1,
                    ),
                ),
            },
        ),
        trait_generics: [],
    }
```
This is occurring due to an unnecessary type check on a method call's
object type. This is cause a repeated trait constraint where one has
incorrect type variables that cannot be resolved.

I have altered how MethodCall's and Call's are resolved as to avoid
repeated type checks on the object type.

## 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.

---------

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.

2 participants