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

Build dependency graph between variables in name resolution #1122

Closed
1 task
jfecher opened this issue Apr 7, 2023 · 6 comments · Fixed by #4266
Closed
1 task

Build dependency graph between variables in name resolution #1122

jfecher opened this issue Apr 7, 2023 · 6 comments · Fixed by #4266
Assignees
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework bug Something isn't working
Milestone

Comments

@jfecher
Copy link
Contributor

jfecher commented Apr 7, 2023

Aim

Trying to write a program

struct ShouldError {
    uses_nested_arrays: [Array; 2],
}

struct Array {
    array: [Field; 3],
}

Expected behavior

Noir should issue an error that nested arrays are currently unimplemented

Bug

The error is only issued sometimes when the program is compiled. This is because on some runs Array is resolved first and when ShouldError is subsequently resolved, it can see all the fields of Array and will error that nested arrays are used. On other runs however, ShouldError is resolved first and sees only an empty struct for Array and thus does not issue an error. In this case, the program continues until monomorphization where it fails with an unreachable assert.

To reproduce

Installation method

None

Nargo version

No response

@noir-lang/noir_wasm version

No response

@noir-lang/barretenberg version

No response

@noir-lang/aztec_backend version

No response

Additional context

No response

Submission Checklist

  • Once I hit submit, I will assign this issue to the Project Board with the appropriate tags.
@jfecher jfecher added the bug Something isn't working label Apr 7, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 7, 2023
@kevaundray kevaundray added the aztec.nr Helpful for development of Aztec.nr the smart contract framework label Apr 20, 2023
@kevaundray
Copy link
Contributor

@jfecher what is the status of this issue?

@kevaundray
Copy link
Contributor

Lowest hanging fruit to attempt to fix would be replacing suspicious usages of hashmap with something like indexmap

@kevaundray
Copy link
Contributor

@jfecher - "The above is not enough, we would need a dependency tree of structures, we could also use this in places where globals depend on other globals"

@kevaundray
Copy link
Contributor

Spoke with Jake; he says, ideally name resolution is split into smaller components and we build this dependency tree at the last step to be used in later passes.

@kevaundray kevaundray changed the title Struct resolution order is nondeterministic Build dependency graph between variables in name resolution Oct 9, 2023
@kevaundray kevaundray added this to the 0.24.0 milestone Jan 15, 2024
@jfecher
Copy link
Contributor Author

jfecher commented Jan 30, 2024

Can we re-tag this E-HIGH? I think it'd require a large rewrite of name resolution to first create the graph then follow its nodes to resolve names in that order.

Edit: Retagged it as "Large" myself. Not sure what the precise difference is between Large and X-Large so I kept it on large.

@Savio-Sou
Copy link
Collaborator

precise difference is between Large and X-Large

Choice is all good as long as it feels right. The Size field is used just as a subjective reference for now.

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

## Problem\*

Resolves #1122

## Summary\*

Adds a dependency graph to catch cases where structs will cyclically
depend on one other.

For example, the program:

```rs
struct Foo {
    bar: Bar,
}

struct Bar {
    foo: Foo,
}
```
Will now give the following error:
```
error: Dependency cycle found
  ┌─ /home/user/Code/Noir/noir/short/src/main.nr:6:1
  │  
6 │ ╭ struct Bar {
7 │ │     foo: Foo,
8 │ │ }
  │ ╰─' 'Bar' recursively depends on itself: Bar -> Foo -> Bar
  │  
```
The error is still issued if these are split across separate modules.

## Additional Context

Note that self-referential structs like `struct Foo { foo: Foo }` are
not currently caught - we may need a separate check for this.

This error is also only currently possible for cyclic struct
dependencies, but once this is merged I'm going to expand this to more
PRs to allow for globals and type aliases as well. This would implement
#1734 as well as a similar
feature to allow type aliases to reference other type aliases which is
currently disallowed.

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

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants