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

Allow globals to reference other globals #1734

Closed
sirasistant opened this issue Jun 16, 2023 · 3 comments · Fixed by #4293
Closed

Allow globals to reference other globals #1734

sirasistant opened this issue Jun 16, 2023 · 3 comments · Fixed by #4293
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@sirasistant
Copy link
Contributor

Problem

When writing modular code It sometimes happens that we have globals that depend on other globals, such as

global MAX_ARGS = 8;

global PUBLIC_INPUTS_LEN = MAX_ARGS + 3;

However, this is not currently supported by the language, so we need to have explanatory comments and do searches to find usage which is quite error prone:

global MAX_ARGS = 8;

// MAX_ARGS + OTHER_STUFF
global PUBLIC_INPUTS_LEN = 11;

Happy Case

Globals could be the result of compile time arithmetic expressions. Even better if they can be the result of function calls that return comptime values, but just being able to define them with arithmetic expressions would be very helpful.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@sirasistant sirasistant added the enhancement New feature or request label Jun 16, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jun 16, 2023
@sirasistant
Copy link
Contributor Author

Relates to #461

@jfecher
Copy link
Contributor

jfecher commented Jun 16, 2023

Relates to #461

#461 is unrelated actually. It was a potential evaluation scheme for comptime expressions but does not deal with what kinds of expressions we allow for globals.

To provide more of a background on the reasoning why globals aren't allowed to reference other globals currently, if we allowed

global MAX_ARGS = 8;

global PUBLIC_INPUTS_LEN = MAX_ARGS + 3;

Then it follows that the following would also be possible:

global MAX_ARGS = PUBLIC_INPUTS_LEN + 8;

global PUBLIC_INPUTS_LEN = MAX_ARGS + 3;

Since globals, like functions are able to be referenced at a global scope before they're defined. We'd have to find a way to prevent circular dependencies like this in general with a good error message. Solving this is not as simple as "allow globals to reference other globals defined beforehand in the same file" since we must also worry about imports from other files and, if we allow function calls as well, functions that capture globals before they're defined such as:

global BAD = init_bad();

fn init_bad() -> Field {
    BAD + 1
}

You can see how this function would clearly be erroneous but this can occur in more subtle ways accidentally as well. E.g. via indirectly called functions.

@kevaundray
Copy link
Contributor

kevaundray commented Jun 16, 2023

This was my mistake, I mis-remembered and thought we were using that as a potential to evaluate global constants with compile time expressions too

@jfecher jfecher changed the title Allow constant expressions to define globals Allow globals to reference other globals Jun 23, 2023
@kevaundray kevaundray added this to the 1.0 milestone Jan 15, 2024
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 <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Feb 8, 2024
# Description

## Problem\*

Resolves #1734

## Summary\*

Allows globals to be defined as any expression using the dependency
graph to ensure there are no cycles.

Note that globals defined via a function call are allowed: `global FOO =
bar();`, although the function will be
re-called each time the global is used since globals are just
substituted for their definitions where they are used.

Example program:

```rs
global A = B;
global B = foo();

fn foo() -> Field {
    A
}
```

Example error:

```
error: Dependency cycle found
  ┌─ /Users/user/Code/Noir/noir/short/src/main.nr:2:8
  │
2 │ global B = foo();
  │        - 'B' recursively depends on itself: B -> foo -> A -> B
  │

```

## Additional Context

This is a somewhat longer PR because it also includes a refactoring to
use a new `GlobalId` over `StmtId`. I found
this necessary to allow referring to globals in cycles where another
global may not be resolved yet.

WIP because I still need to:
- ~~Track dependencies within functions~~
- ~~Update documentation~~

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] 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: José Pedro Sousa <[email protected]>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants