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

fix: detect cycles in globals #6859

Merged
merged 4 commits into from
Dec 18, 2024
Merged

fix: detect cycles in globals #6859

merged 4 commits into from
Dec 18, 2024

Conversation

asterite
Copy link
Collaborator

Description

Problem

Resolves #6780

Summary

This program:

comptime global A: u32 = B;
comptime global B: u32 = A;

fn main() { }

now gives these errors:

error: Variable not in scope
  ┌─ src/main.nr:2:26
  │
2 │ comptime global B: u32 = A;
  │                          - Could not find variable
  │

error: This global recursively depends on itself
  ┌─ src/main.nr:1:26
  │
1 │ comptime global A: u32 = B;
  │                          -
  │

error: Dependency cycle found
  ┌─ src/main.nr:1:17
  │
1 │ comptime global A: u32 = B;
  │                 - 'A' recursively depends on itself: A -> B -> A
  │

Aborting due to 3 previous errors

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Peak Memory Sample

Program Peak Memory
keccak256 78.15M
workspace 122.62M
regression_4709 423.26M
ram_blowup_regression 1.58G
private-kernel-tail 206.71M
private-kernel-reset 720.29M
private-kernel-inner 291.91M
parity-root 171.71M

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.374s -1%
regression_4709 0m0.838s 7%
ram_blowup_regression 0m14.549s 0%
rollup-base-public 3m33.736s 2%
rollup-base-private 3m3.473s 1%
private-kernel-tail 0m1.493s 26%
private-kernel-reset 0m7.754s -4%
private-kernel-inner 0m2.326s -1%
parity-root 0m0.886s -9%
noir-contracts 2m57.923s -5%

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought, but LGTM

compiler/noirc_frontend/src/tests.rs Show resolved Hide resolved
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't see a way to avoid the double error, and double errors are already common when running comptime code. To avoid them we'd have to return something like Result<_, Option<Error>> which would get messy.

@asterite
Copy link
Collaborator Author

I was thinking of another way to do this, which is to do it similarly to FunctionBody:

pub enum FunctionBody {
    Unresolved(FunctionKind, BlockExpression, Span),
    Resolving,
    Resolved,
}

So a global's value would be a similar enum, and we wouldn't have to add a field to the interpreter. But I don't know which way is preferred...

@jfecher
Copy link
Contributor

jfecher commented Dec 18, 2024

Either is fine, and either way you'd have to issue an extra error still

@asterite
Copy link
Collaborator Author

I just pushed a change for that. I think the code ends up a bit more clear, as it kind of forces us to handle the different states of a global's value.

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Execution Sample

Program Execution Time %
sha256_regression 0m0.601s 0%
regression_4709 0m0.394s -9%
ram_blowup_regression 0m4.390s -1%
rollup-base-public 0m28.294s -1%
rollup-base-private 0m26.053s -1%
private-kernel-tail 0m0.714s -2%
private-kernel-reset 0m1.456s -3%
private-kernel-inner 0m0.954s -3%
parity-root 0m0.522s -15%

@asterite asterite enabled auto-merge December 18, 2024 17:42
@asterite asterite added this pull request to the merge queue Dec 18, 2024
Merged via the queue into master with commit 0d7642c Dec 18, 2024
85 checks passed
@asterite asterite deleted the ab/cyclic-globals branch December 18, 2024 17:52
TomAFrench added a commit that referenced this pull request Dec 18, 2024
* master:
  fix: detect cycles in globals (#6859)
  chore(ci): Execution time report (#6827)
  chore(ci): Add non determinism check and fixes (#6847)
  chore(docs): updating the solidity contract how-to guide (#6804)
  fix: double alias in path (#6855)
  feat: configurable external check failures (#6810)
  chore: move constant creation out of loop (#6836)
  fix: implement `as_field` and `from_field` in the interpreter (#6829)
  chore: Use Vec for callstacks (#6821)
  feat: replace `eval_global_as_array_length` with type/interpreter evaluation (#6469)
  chore: refactor `DataFlowGraph.insert_instruction_and_results` (#6823)
  chore(docs): updating noirjs tutorial for 1.0.0 (#6792)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comptime cyclic globals stack overflow
3 participants