chore!: Allow lowercase field type by kevaundray · Pull Request #3631 · noir-lang/noir

kevaundray · 2023-11-29T14:31:18Z

Description

Related to #3535 . This is breaking for users who were using field as a variable or strruct name previously.

This is not breaking for all users because we still allow the uppercase variant. The formatter should rename this to lower case variant and overtime, I expect we can whean out the uppercase variant.

Problem*

Resolves

Summary*

Additional Context

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[Exceptional Case] 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.

kevaundray · 2023-11-29T15:18:55Z

This is also breaking for files named field.nr since to import them you need to do something like crate::field

* master: chore(docs): address visibility issues in docs (#3643) chore: type formatting (#3618) fix: Restrict fill_internal_slices pass to acir functions (#3634) chore(docs): docs for v0.19.4 (#3601) feat: aztec-packages (#3626) chore: Move tests to the correct root (#3633) feat: Implement integer printing (#3577) fix: corrected the formatting of error message parameters in index out of bounds error (#3630) chore: Update ACIR artifacts (#3619) chore(debugger): Run debugger REPL in thread (#3611) chore: remove deprecated method (#3625)

kevaundray · 2023-11-30T17:30:30Z

@TomAFrench given that we can see the changes needed and its potential ramifications, what are your thoughts on this?

TomAFrench · 2023-11-30T17:36:08Z

I think it's an ok change from the user's perspective. It's very unlikely that people are directly importing dep::std::field as that's only necessary for the field modulus functions, not the to_be_bytes, etc. That's the main breaking change imo.

I'm good with merging this and just having Field get autoformatted into field over time. Although there's a bunch of remaining instances of Field which we would want to change in the compiler before merging this.

I think we can let this PR sit for a little bit as there's no rush.

kevaundray · 2023-11-30T17:37:56Z

I think it's an ok change from the user's perspective. It's very unlikely that people are directly importing dep::std::field as that's only necessary for the field modulus functions, not the to_be_bytes, etc. That's the main breaking change imo.

I'm good with merging this and just having Field get autoformatted into field over time. Although there's a bunch of remaining instances of Field which we would want to change in the compiler before merging this.

I think we can let this PR sit for a little bit as there's no rush.

That sounds reasonable and good to me!

kevaundray · 2024-02-26T14:46:45Z

@TomAFrench should we rip this bandaid off?

TomAFrench · 2024-02-26T14:52:12Z

Yeah, I think so.

* master: (440 commits) chore: remove duplicate `parse_all` function in wasm compiler (#4411) chore(ci): prevent msrv checks from blocking PRs (#4414) feat: expose separate functions to compile programs vs contracts in `noir_wasm` (#4413) chore: do not panic when dividing by zero (#4424) chore: remove unwanted prints (#4419) fix: remove print from monomorphization pass (#4417) chore(ssa): Remove mem2reg run before flattening (#4415) feat: Add HashMap to the stdlib (#4242) fix!: Ban Fields in for loop indices and bitwise ops (#4376) chore: Add #[recursive] Explainer to Documentation (#4399) feat(ci): Use wasm-opt when compiling wasm packages (#4334) fix: add handling to `noir_wasm` for projects without dependencies (#4344) chore: rename parameter 'filter' to 'level' in 'init_log_level' (#4403) chore!: bump msrv to 1.73.0 (#4406) chore: fix docker test workflows (#4308) chore: Update Vec docs (#4400) feat: update error message when trying to load workspace as dependency (#4393) chore: bump webpack dependencies (#4346) fix: correct invalid brillig codegen for `EmbeddedCurvePoint.add` (#4382) chore: remove dependency on generational-arena (#4207) ...

TomAFrench · 2024-02-26T14:55:35Z

Let's do this after #4422 however so we can switch this repo over to using lowercase fields consistently though.

* master: feat: add poseidon2 opcode implementation for acvm/brillig, and Noir (#4398) fix: remove panic when generic array length is not resolvable (#4408) chore(ci): enforce formatting of noir code in CI (#4422)

* master: feat: Sync from aztec-packages (#4438) chore(docs): correct 'Edit this page' URL for dev docs (#4433) feat: Sync from aztec-packages (#4390) chore(docs): fix external contributor force push workflow (#4437) chore!: Remove empty value from bounded vec (#4431) chore: nargo fmt (#4434)

* master: (144 commits) fix: correct ICE panic messages in brillig `convert_black_box_call` (#4761) chore(docs): Fix link in the Data Types page (#4527) chore(doc): Fix broken docs links (#4606) chore: Remove the old Value struct from the oracle docs (#4738) feat(nargo): Multiple circuits info for binary programs (#4719) chore: update condition for clearing warning comment on release PRs (#4739) chore(ci): fix cutting new versions of the docs (#4737) chore(ci): replace `yarn build:js:only` script (#4735) chore: update JS publish workflow to upload build artifacts correctly. (#4734) feat: add `remove_enable_side_effects` SSA pass (#4224) chore: update from vulnerable version of h2 (#4714) chore(ci): stop updating version list before cutting new docs version (#4726) chore: remove `FunctionInput::dummy` (#4723) chore: remove docker CI flow (#4724) fix: unknown slice lengths coming from as_slice (#4725) chore: remove unused env vars from `Cross.toml` (#4717) feat: improve nargo check cli with --override flag and feedback for existing files (#4575) feat: Allow slices to brillig entry points (#4713) chore: simplify how `acvm_backend.wasm` is embedded (#4703) fix(acvm): Mark outputs of Opcode::Call solvable (#4708) ...

* master: (39 commits) feat(experimental): Add `comptime` keyword (#4840) feat: add `min` and `max` functions to the stdlib (#4839) feat: Allow numeric generics to non inlined ACIR functions (#4834) chore!: Add `as_array` and remove `_slice` variants of hash functions (#4675) feat!: reserve keyword `super` (#4836) feat: simplify `BoundedVec::eq` (#4838) feat: Add comptime Interpreter (#4821) feat: Sync from aztec-packages (#4833) feat: implement `Eq` trait on `BoundedVec` (#4830) chore: add benchmarks for serializing a dummy program (#4813) chore: remove unnecessary casts in `BoundedVec` (#4831) fix: issue 4682 and add solver for unconstrained bigintegers (#4729) chore(docs): fix wrong Nargo.toml workspace examples (#4822) chore: delete unnecessary Prover.toml file (#4829) chore: fix alerts on rust msrv (#4817) chore(ci): fix alerts on msrv issues (#4816) chore: run clippy (#4810) chore: optimize poseidon2 implementation (#4807) fix: catch panics from EC point creation (e.g. the point is at infinity) (#4790) feat: Sync from aztec-packages (#4792) ...

TomAFrench · 2024-04-20T15:05:10Z

We're not fully converting Field into field if it's in a struct definition, etc. currently. I don't think this is a blocker however.

github-actions · 2024-04-20T15:08:09Z

Changes to circuit sizes

Generated at commit: 7dfeb7b7be01e66563f908645f74784a18294e40, compared to commit: 4dfd7f03bc1b9cf57f5829c435a560bed53b7f46

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
main	+11 ❌	+1100.00%	+2,754 ❌	+39342.86%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
main	12 (+11)	+1100.00%	2,761 (+2,754)	+39342.86%

jfecher

I am thinking making "field" a reserved word would also help avoid errors from users using "field" as a variable name.

Or maybe we're fine with this breaking change? Not sure - we're still before 1.0 at least

jfecher · 2024-04-22T15:26:53Z

tooling/nargo_fmt/src/visitor/item.rs

                    self.last_position = span.end();
                }
+
+                // TODO: rewrite instances of "Field" into "field" in these items.


Can you make an issue for this once this PR is merged?

jfecher · 2024-04-22T15:31:14Z

noir_stdlib/src/hash/poseidon.nr

@@ -1,38 +1,38 @@
 mod bn254; // Instantiations of Poseidon for prime field of the same order as BN254
-use crate::field::modulus_num_bits;
+use crate::field_element::modulus_num_bits;


Changing the import paths is my main worry in this PR.

Maybe we can make "field" just a reserved word / pseudo-keyword like "self" instead? That way we can still allow it in import paths. Alternatively I suppose it can stay a keyword but we'd have to change the parser to allow paths with field in them.

allow lowercase field type

ee70cc4

kevaundray and others added 6 commits November 29, 2023 15:24

change field to field_element

06e5dda

chore: update import of bytes32_to_field

8746c28

chore: fix remaining instances of crate::field::

a9049bd

chore: handle std::field references

71abc61

chore: fix variable name collision with field

5fb072e

kevaundray and others added 6 commits February 26, 2024 16:30

Merge branch 'master' into kw/change-to-lowercase-field

48d498b

Merge branch 'master' into kw/change-to-lowercase-field

850565b

* master: feat: add poseidon2 opcode implementation for acvm/brillig, and Noir (#4398) fix: remove panic when generic array length is not resolvable (#4408) chore(ci): enforce formatting of noir code in CI (#4422)

chore: fix tests

b7d1d82

feat: format Field into field

b6689e4

chore: run formatter

a9f014b

chore: format stdlib

8190761

TomAFrench mentioned this pull request Feb 27, 2024

feat: add field keyword highlighting noir-lang/vscode-noir#71

Closed

2 tasks

TomAFrench added 8 commits February 27, 2024 13:58

chore: update outputs for formatter tests

dca0dbe

chore: format stdlib

f160797

chore: remove unwanted change

684139e

chore: fix stdlib

ccba71d

chore: update test programs

5eaf64a

TomAFrench added 4 commits April 20, 2024 15:39

chore: fix slices from merge

e91ecf5

chore: fix tests

f7305d3

chore: revert changes to trait name

5453a68

chore: update formatter tests

0445ec5

jfecher reviewed Apr 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: Allow lowercase field type#3631

chore!: Allow lowercase field type#3631
kevaundray wants to merge 26 commits intomasterfrom
kw/change-to-lowercase-field

kevaundray commented Nov 29, 2023

Uh oh!

kevaundray commented Nov 29, 2023

Uh oh!

kevaundray commented Nov 30, 2023

Uh oh!

TomAFrench commented Nov 30, 2023 •

edited

Loading

Uh oh!

kevaundray commented Nov 30, 2023

Uh oh!

kevaundray commented Feb 26, 2024

Uh oh!

TomAFrench commented Feb 26, 2024

Uh oh!

TomAFrench commented Feb 26, 2024

Uh oh!

TomAFrench commented Apr 20, 2024

Uh oh!

github-actions bot commented Apr 20, 2024

Uh oh!

jfecher left a comment

Uh oh!

jfecher Apr 22, 2024

Uh oh!

jfecher Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kevaundray commented Nov 29, 2023

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

Uh oh!

kevaundray commented Nov 29, 2023

Uh oh!

kevaundray commented Nov 30, 2023

Uh oh!

TomAFrench commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevaundray commented Nov 30, 2023

Uh oh!

kevaundray commented Feb 26, 2024

Uh oh!

TomAFrench commented Feb 26, 2024

Uh oh!

TomAFrench commented Feb 26, 2024

Uh oh!

TomAFrench commented Apr 20, 2024

Uh oh!

github-actions bot commented Apr 20, 2024

Changes to circuit sizes

🧾 Summary (10% most significant diffs)

Uh oh!

jfecher left a comment

Choose a reason for hiding this comment

Uh oh!

jfecher Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

jfecher Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TomAFrench commented Nov 30, 2023 •

edited

Loading