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

feat: Adding support for global arrays #826

Closed

Conversation

michaelneuder
Copy link

@michaelneuder michaelneuder commented Feb 12, 2023

Related issue(s)

Resolves #439. Done in collaboration and with the guidance of @vezenovm :-)

Description

Summary of changes

There are 3 components of the change:

  1. parser --- in global_type_annotation we need to expand the set of allowed types to include the array type and we also expand the global_declaration function to take any expression.
  2. resolver --- we change resolve_global_let to (1) match against literals, (2a) if that literal is ArrayLiteral::Standard, check that each element of the array is also literal, (2b) if that literal is ArrayLiteral::Repeated, check that the repeated type is a literal. This means that resolve_global_let now returns a Vec<ResolverError> in the same way that resolve_struct_fields and resolve_function do. These errors are then propagated up to the def collected
  3. def_collector --- in resolve_globals we now check the return errors from resolve_global_let, if there are errors, we extend the set of current errors, thus resolve_globals now takes a new param errors: &mut Vec<FileDiagnostic> that is passed in during collect.

Dependency additions / changes

N/A

Test additions / changes

We test this in crates/nargo/tests/test_data/global_consts/src/main.nr, where we define a number of bad globals. Importantly, we needed to test that a global array can not have global elements as brought up by @jfecher in #439 (comment).

Valid global arrays

// Global arrays that match the input arrays c and d.
global GlobalC: [Field; 3] = [3, 3, 3];
global GlobalD: [Field; 5] = [5, 5, 5, 5, 5];

successfully compile.

The following invalid globals, if uncommented, will result in compilation errors.

// global badGlobal = if M == 32 {
//      constrain 1 == 1;
// };

// global badGlobal2 = for i in 0..1 {
//      constrain i == 0;
// };

// global BAD = 10;
// global A = [BAD, ELEMS];
// global ELEMS = 11;

When these are uncommented, the following compiler error is shown to the user, as expected.

error: expected globals can only be array, bool, integer or field literals got if (plain::M == 32) {
    constrain (1 == 1)
}
   ┌─ /Users/michaelneuder/github/noir2/crates/nargo/tests/test_data/global_consts/src/main.nr:14:20
   │
14 │   global badGlobal = if M == 32 {
   │ ╭────────────────────'
15 │ │      constrain 1 == 1;
16 │ │ };
   │ ╰─'

error: expected globals can only be array, bool, integer or field literals got for i in 0 .. 1 {
    constrain (plain::i == 0)
}
   ┌─ /Users/michaelneuder/github/noir2/crates/nargo/tests/test_data/global_consts/src/main.nr:18:21
   │
18 │   global badGlobal2 = for i in 0..1 {
   │ ╭─────────────────────'
19 │ │      constrain i == 0;
20 │ │ };
   │ ╰─'

error: expected global array elements can only be array, bool, integer or field literals got plain::BAD
   ┌─ /Users/michaelneuder/github/noir2/crates/nargo/tests/test_data/global_consts/src/main.nr:23:12
   │
23 │ global A = [BAD, ELEMS];
   │            ------------

error: expected global array elements can only be array, bool, integer or field literals got plain::ELEMS
   ┌─ /Users/michaelneuder/github/noir2/crates/nargo/tests/test_data/global_consts/src/main.nr:23:12
   │
23 │ global A = [BAD, ELEMS];
   │            ------------

error: aborting due to 4 previous errors

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.
  • This PR requires documentation updates when merged.

@kevaundray kevaundray changed the title Adding support for global arrays feat: Adding support for global arrays Feb 12, 2023
michaelneuder and others added 4 commits February 17, 2023 16:18
* master: (223 commits)
  chore(noir): Release 0.5.0 (noir-lang#1202)
  chore(ci): Utilize new workflow to build binaries (noir-lang#1250)
  chore(ssa refactor): Fix loading from mutable parameters (noir-lang#1248)
  fix(wasm): add std after dependencies (noir-lang#1245)
  chore(ssa refactor): Fix no returns & duplicate main (noir-lang#1243)
  chore(ssa refactor): Implement intrinsics (noir-lang#1241)
  chore(ssa refactor): Implement first-class functions (noir-lang#1238)
  chore: address clippy warnings (noir-lang#1239)
  chore(ssa refactor): Implement function calls (noir-lang#1235)
  chore(ssa refactor): Implement mutable and immutable variables (noir-lang#1234)
  chore(ssa refactor): Fix recursive printing of blocks (noir-lang#1230)
  feat(noir): added assert keyword (noir-lang#1227)
  chore(ssa refactor): Implement ssa-gen for indexing, cast, constrain, if, unary (noir-lang#1225)
  feat(noir): added `distinct` keyword (noir-lang#1219)
  chore(nargo): update panic message to suggest searching for similar issues (noir-lang#1224)
  chore(ssa refactor): Update how instruction result types are retrieved (noir-lang#1222)
  chore(ssa refactor): Implement ssa-gen for binary, block, tuple, extract-tuple-field, and semi expressions (noir-lang#1217)
  chore: add RUST_BACKTRACE environment variable to nix config (noir-lang#1216)
  chore(ssa): Add intial control flow graph  (noir-lang#1200)
  chore(ssa refactor): Handle codegen for literals (noir-lang#1209)
  ...
@TomAFrench
Copy link
Member

TomAFrench commented Apr 29, 2023

I've merged master back into this branch to address the merge conflicts which have popped up over time. Thanks for your work on this @michaelneuder!

resolve_globals in master now uses resolver.take_errors() to access the errors encountered in resolve_global_let so I've removed the errors from the return values.

@jfecher
Copy link
Contributor

jfecher commented May 1, 2023

May need to test this PR with some of the aztec 3 work. In particular we started allowing constructor expressions as globals as well but did not check them thoroughly since we wanted to allow function calls within for convenience. We'll need to add constructor expressions to this check and see if there's any code that breaks once they get restricted a bit by this check.

@kevaundray
Copy link
Contributor

superceded by #1054

@kevaundray kevaundray closed this Jul 25, 2023
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.

Global arrays
4 participants