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

Type variables still bound from previous trait implementations #4436

Closed
Thunkar opened this issue Feb 27, 2024 · 2 comments · Fixed by #4450
Closed

Type variables still bound from previous trait implementations #4436

Thunkar opened this issue Feb 27, 2024 · 2 comments · Fixed by #4450
Labels
bug Something isn't working

Comments

@Thunkar
Copy link
Contributor

Thunkar commented Feb 27, 2024

Aim

Use (different) globals as generic parameters for trait implementations, in both libs and app code. This is a very common situation in aztec contracts, in which it's typical to have a contract that stores both standard aztec-nr notes and custom ones.

The project structure is as follows:

a_trait_lib/
├─ src/
│  ├─ lib.nr
├─ Nargo.toml
lib_struct/
├─ src/
│  ├─ lib.nr
├─ Nargo.toml
main/
├─ src/
│  ├─ custom_struct.nr
│  ├─ main.nr
├─ Nargo.toml

Where a_trait_lib exports:

trait LibTrait<N> {
    fn broadcast();
    fn get_constant() -> Field;
}

lib_struct

use dep::a_trait_lib::LibTrait;

global LIB_STRUCT_LEN: Field = 3; 

struct LibStruct {
}

impl LibTrait<LIB_STRUCT_LEN> for LibStruct {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        1
    }
}

impl LibStruct {
    pub fn new() -> Self {
        LibStruct {}
    }
}

custom_struct.nr

use dep::a_trait_lib::LibTrait;

global EVENTUALLY_CLASHES: Field = 5;

struct CustomStruct {

}

impl LibTrait<EVENTUALLY_CLASHES> for CustomStruct {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        2
    }
}

impl CustomStruct {
    pub fn new() -> CustomStruct {
        CustomStruct {}
    }
}

main does nothing besides importing the custom_struct module. It is enough that dependencies are declared in Nargo.toml to trigger the bug.

mod custom_struct;

fn main() {}
[package]
name = "broken_globals"
type = "bin"
authors = [""]
compiler_version = ">=0.24.0"

[dependencies]
lib_struct = { path = "../lib_struct"}
a_trait_lib = { path = "../a_trait_lib"}

Expected Behavior

It should be possible to use both library and custom app defined structs (implementing LibTrait<N>), in which impl LibTrait<A_GLOBAL_VAR> can be defined for each of them, without the compiler panicking.

Bug

The compiler panics with:

The application panicked (crashed).
Message:  internal error: entered unreachable code: TypeVariable::bind, cannot bind bound var 3 to 5
Location: compiler/noirc_frontend/src/hir_def/types.rs:472

Notice the values of the bindings, which correspond to LIB_STRUCT_LEN and EVENTUALLY_CLASHES. Altering those values change the error message.

It is very possible that there are several red herrings on the minimal reproduction, but I could not reduce it anymore without the problem disappearing. Specially baffling is that an apparently unrelated method get_constant that just returns a Field must be present and be called with Self:: within the implementation (equivalent to get_note_type_id() and broadcast() in Aztec)

To Reproduce

Use the provided minimal example:

broken_globals.tgz

Project Impact

Blocker

Impact Context

It's a very common pattern in aztec contracts

Workaround

Yes

Workaround Description

It is possible to work around this issue by moving the library struct into the main crate, or commenting out the Self::get_constant() lines, using Field literals instead. In some cases this would trigger another error, but I couldn't get it to fail reliably.

Additional Context

No response

Installation Method

Compiled from source

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@Thunkar Thunkar added the bug Something isn't working label Feb 27, 2024
@Thunkar Thunkar changed the title global variables clashing in trait implementations Global variables clashing in trait implementations Feb 27, 2024
@TomAFrench
Copy link
Member

Flattening this example down into a single module also fails

trait LibTrait<N> {
    fn broadcast();
    fn get_constant() -> Field;
}

global STRUCT_A_LEN: Field = 3;
global STRUCT_B_LEN: Field = 5;

struct StructA;
struct StructB;

impl LibTrait<STRUCT_A_LEN> for StructA {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        1
    }
}
impl LibTrait<STRUCT_B_LEN> for StructB {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        1
    }
}

fn main() {}

This gives the error

error: No matching impl found for `StructB: LibTrait<3>`
   ┌─ /home/tom/Programming/aztec/noir/tmep/src/main.nr:23:9
   │
23 │         Self::get_constant();
   │         ------------------ No impl for `StructB: LibTrait<3>`
   │

Swapping out Self for LibTrait results in us resolving the correct get_constant implementation.

@jfecher jfecher changed the title Global variables clashing in trait implementations Variables still bound from previous trait implementations Feb 27, 2024
@jfecher
Copy link
Contributor

jfecher commented Feb 27, 2024

Updated the title since this is about previous variables still being bound in general, not specifically globals. The following still exhibits the bug:

trait LibTrait<N> {
    fn broadcast();
    fn get_constant() -> Field;
}

struct StructA;
struct StructB;

impl LibTrait<u32> for StructA {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        1
    }
}
impl LibTrait<u64> for StructB {
    fn broadcast() {
        Self::get_constant();
    }

    fn get_constant() -> Field {
        1
    }
}

fn main() {}

@jfecher jfecher changed the title Variables still bound from previous trait implementations Type variables still bound from previous trait implementations Feb 27, 2024
jfecher added a commit that referenced this issue Feb 28, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 1, 2024
…en used within a trait impl (#4450)

# Description

## Problem\*

Resolves #4436 

## Summary\*

This issue stemmed from calling another trait method within an impl for
the trait. The trait method required a trait constraint of the same
trait to be valid and was using the trait with its original type
variables. These would later be bound over when an impl was searched for
for this trait.

I've added a somewhat hacky solution of avoiding inserting `t = t`
bindings from the trait constraint when checking identifiers. Avoiding
inserting this binding means `t` in this case will later be instantiated
to a fresh type variable later on in the `instantiate_with_bindings`
call.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.
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
3 participants