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

Cannot resolve static struct methods in global declarations #1440

Closed
sirasistant opened this issue May 30, 2023 · 9 comments · Fixed by #4490
Closed

Cannot resolve static struct methods in global declarations #1440

sirasistant opened this issue May 30, 2023 · 9 comments · Fixed by #4490
Assignees
Labels
bug Something isn't working

Comments

@sirasistant
Copy link
Contributor

sirasistant commented May 30, 2023

Aim

I was trying to create a global struct instance with a property computed by a static method of another struct. A boiled down version of this would be:

struct Foo {
  a: Field,
}

struct Bar {}

impl Bar {
  fn get_a() -> Field {
    1
  }
}

global foo = Foo {
  a: Bar::get_a(),
};


fn main() -> pub Field {
  foo.a
}

Expected Behavior

It should compile without issues, given that the following code does work and is functionally equivalent:

struct Foo {
  a: Field,
}

fn get_a() -> Field {
  1
}

global foo = Foo {
  a: get_a(),
};


fn main() -> pub Field {
  foo.a
}

Bug

Seems like a resolution issue (maybe static struct methods are not globals?):

error: Could not resolve 'get_a' in path
   ┌─ /mnt/user-data/alvaro/constructor/src/main.nr:99:11
   │
99 │   a: Bar::get_a(),
   │           -----

error: aborting due to 1 previous errors
Error: Failed to compile circuit

Location:
    crates/nargo_cli/src/cli/mod.rs:70:5

To Reproduce

  1. Compile the above code

Installation Method

Compiled from source

Nargo Version

nargo 0.6.0 (git version hash: 8778a1d, is dirty: false)

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@sirasistant sirasistant added the bug Something isn't working label May 30, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 30, 2023
@kevaundray
Copy link
Contributor

@jfecher is this still an issue?

@jfecher
Copy link
Contributor

jfecher commented Jul 24, 2023

@kevaundray yes, name resolution still does not check if a module name is actually a typename

@kevaundray
Copy link
Contributor

@f01dab1e assigning this issue to you!

@kevaundray kevaundray added P-LOW and removed P-LOW labels Jul 25, 2023
@ghost
Copy link

ghost commented Jul 27, 2023

any hints?

@jfecher
Copy link
Contributor

jfecher commented Jul 27, 2023

@f01dab1e I think this may have to do with resolution order of globals. Since globals are currently resolved before structs, I think Bar::get_a may not be registered when the global is being checked. I should make a tracker issue since there are a few other issues caused by this.

@ghost
Copy link

ghost commented Aug 2, 2023

if I change the resolution order, it solves the problem above, but creates a new one:

error: Expected type [Field; 0], found type [Field; 5]
   ┌─ noir/crates/nargo_cli/tests/test_data/global_consts/src/main.nr:16:24
   │
16 │      let test_struct = Dummy { x: d, y: c };
   │                        --------------------

error: Expected type [Field; 0], found type [Field; 3]
   ┌─ noir/crates/nargo_cli/tests/test_data/global_consts/src/main.nr:16:24
   │
16 │      let test_struct = Dummy { x: d, y: c };
   │                        --------------------

Error: 
   0: Aborting due to 2 previous errors

@jfecher
Copy link
Contributor

jfecher commented Aug 2, 2023

@f01dab1e It looks like your current error is related to globals not being known when those types are resolved, so the array types using globals default to a size of 0. I'm not sure what the desired solution to this ordering problem is - if their is a proper order or if we need a larger solution like creating a dependency tree for the program and using that to solve ordering issues. There are a few other open issues that would need a dependency tree as well.

@TomAFrench
Copy link
Member

@jfecher I think that we added new dependency resolution logic to the compiler which closes this issue. Is this correct?

@jfecher
Copy link
Contributor

jfecher commented Mar 6, 2024

@TomAFrench no, this is still an issue. The dependency graph verifies we don't have cyclic dependencies but does not actually change resolution order.

github-merge-queue bot pushed a commit that referenced this issue Mar 12, 2024
# Description

## Problem\*

Resolves #1440

## Summary\*

This was a fairly simple change - non-integer globals used to be
resolved before struct methods were even collected. I've moved this to
after methods are collected. Now global resolution is the first step of
resolving in general.

Integer globals are still resolved early since structs may refer to them
in numeric generics.

## 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.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Mar 12, 2024
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.

5 participants