-
Notifications
You must be signed in to change notification settings - Fork 59
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
Rectify language fields #423
Conversation
@@ -64,7 +64,7 @@ fn end2end_benchmark(c: &mut Criterion) { | |||
|
|||
group.bench_with_input(benchmark_id, &size, |b, &s| { | |||
b.iter(|| { | |||
let ptr = go_base::<pasta_curves::pallas::Scalar>(&mut store, s.0, s.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can chuck the following in a clippy.toml
at the root of the repo to have the yelling done by tooling instead of you:
disallowed-methods = [
# we use strict naming conventions for fields of our workhorse curve
{ path = "pasta_curves::Fp", reason = "use pasta_curves::pallas::Scalar instead" },
...
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. However, please note that the code you provided here reproduces the error I sought to correct here. In fact, pasta_curves::Fq
is the one that corresponds to pallas::Scalar
— as noted in the added documentation.
I realize this isn't what you were intending to communicate, and I mention it only because:
- It's still wrong…
- It's a perfect and timely example of how allowing these wrong expressions into our source of truth creates ongoing confusion.
As always, I recognize that I did approve the PR(s) introducing these errors.
* Assign correct FIELD. * Use obviously-identifying Scalar type aliases. * Correct fields and use obvious aliases. * Document field names. --------- Co-authored-by: porcuquine <[email protected]>
* Assign correct FIELD. * Use obviously-identifying Scalar type aliases. * Correct fields and use obvious aliases. * Document field names. --------- Co-authored-by: porcuquine <[email protected]>
* separating Poseidon hashing into it's own module * move pointer types, traits, methods from store.rs to ptr.rs * move Expression/Continuation to expr.rs and cont.rs, respectively * refactor Ptr representation * update subcrates * fix ptr equality * remove no longer meaningful test in opaque_sym * fix benches * cargo clippy * rename LightData -> ZData * ZCont, ZExpr, ZPtr * Renamed ScalarPtr to ZExprPtr, etc * add new ZExpr variants with TODOs on implementation * Removing ScalarStore from z_data * ZStore now uses ZEntry instead of an Option * Removed ScalarStore from Store * ZExpr Encodable * ZCont and ZExpr Encodable * fix warnings * fix fcomm * z_store Encodable * fix merge src/ptr.rs * fix merge src/store.rs * fix test * warnings * add z_ptr hashing functions * stubbed get_z_expr * some progress in 'get_z_expr' * Small bug fixes * add some function stubs * Added intern_z_expr_ptr * Added intern_z_cont_ptr * remove scalar_store, refactor hashing, with stubs * fixup merge * put_z_strs and put_z_chars * interning strings and symbols * Progress on `intern_symcons` * Small bugfix * `to_store` finished * cleanup and document * cachemap * update Cargo * add ZCont::Call0 * refactor z modules * None case for continuations * implement conversions * nil_z_ptr and related changes * fix z_cont serialization * Expression SymCons/StrCons with stubs * Expression SymCons & StrCons progress * fill todos * reduce warnings * rest of the warnings * fmt; fix nil sym check; use poseidon cache to get nil ptr * implement is_keyword_sym * Jcb/nom and syntax (#383) * preparation for nom parser and syntax * syntax Display and Arbitrary * syntax interning * fix errors in src/eval/tests/mod.rs * fix lang.rs * fixing build; fixed 'sym_in_list' test * add syntax_macros.rs * fix macros * more fixes * added LurkSym enum for built-ins * more parser infrastructure * some fixes in the store tests * credit FrozenMap; remove lurk_sym.rs; add dummy LurkSym * populate z_expr_ptr_map when get_z_expr is called * fix infinite loop when fetching keys * remove writing test from eval tests file; add a writer test case * `fetch_syntax` completed * draft of parser * `fetch_syntax` update * Removed unused modules plus small fixes * Fixed fetch functions related to symbol * Finished `write_symbol` * syntax parser * Better `syntax.rs` tests * parser/printer now passing proptest roundtrip * Updated the repl to use new parser * solved conflicts * Static whitespace array and bug fix * syntax_full_roundtrip proptest * Fixed some opaque tests There was a reordering of matches in part of the evaluator. Instead of fetching the pointers and then checking the operator, the opposite is done so that we won't need to fetch opaque pointers in `equal` and `cons` cases * Serde refactor (WIP) * Fmt and clippy * Build the tests * fix opaque sym test * hunt down misplaced '.sym(' occurrences * fix some uppercasing * use cache maps for poseidon cache * Added parser for meta commands * fmt; try to mitigate sources of bugs * John's fix for 'get_sym' * Added old char syntax and changed root symbol syntax * ZData serialization * fix hash char parser/printer, and solve some failing tests * improve parse_list syntax, fix more tests * fix more tests * Refactor methods * fix char coercion * More test fixes * add minimal failing test for lambdas with empty lists of arguments * implement negative number and fraction syntax * fix number of iterations * Test serializer and create z_data nested modules * fix hashing for functions * Fixed `intern_z_cont_ptr` * fix Call and Call2 argument orders * Add deserializer (WIP) * Add compound deserialization types, rustfmt * Finish deserializer for compound types * Fixed repl * Fixed doc tests and `fetch_strcons` * Fix ZExpr deserialization, roundtrip tests * lowercase fcomm proof_tests * get clutch building with todos * Parser simplification * Clippy * Use ZExprPtr as Nova proof cache key * Revert to canonical FWrap serialization * Finish Nova proof key, fix proptest stack overflow * Remove Encodable instances in favor of ZData serde * Clippy * Format proof key as base32, fix clutch tests * resolve review comments * remove unused dependencies * Iterative version of `get_z_expr` * remove bad opaque dereference * disable makefile_tests which are broken on master * add TODOs for opaque dereference * small error message improvement * replace some unwraps with expects * some temporary debugs and printing shims * Add minimal chained fcomm unit tests * Proptest fcomm types into oblivion * Use latest Nova for serde fix * Fix chained fcomm test * Cleanup & fix Wasm build * fmt * docs: Improve pull request merge guidelines (#418) - Update CONTRIBUTING.md to enhance clarity on merge criteria - Revise workflow diagram with accurate naming * test: Add a test for enforce_implication_lc (#419) * test: Add a test for enforce_implication_lc - Add a new test for enforce_implication_lc function in constraints.rs * test: Improve test coverage for enforce_implication_lc - Add edge case test for enforce_implication_lc - documented behavior with non-boolean nums * chore: Less cloning (#420) * refactor: Avoid unnecessary cloning of lang parameter - Update multiframes creation to take a reference - Add derive traits to HashArity enum in hash.rs for better usability * refactor: Reduce cloning in LEM - Enhance duplicate key error message in interpreter.rs to not need a clone - Modify parameter type for `deconflict` function and its calls in mod.rs - modification of helper functions in Symbol inspired by [C-CALLER-CONTROL](https://rust-lang.github.io/api-guidelines/flexibility.html#caller-decides-where-to-copy-and-place-data-c-caller-control) * refactor: remove uneeded clones in circuit_frame - take by reference where possible (and easy) * Rectify language fields (#423) * Assign correct FIELD. * Use obviously-identifying Scalar type aliases. * Correct fields and use obvious aliases. * Document field names. --------- Co-authored-by: porcuquine <[email protected]> * Fix new_with_expr conversion to ZStore * Fix commitment roundtrip test * fix automatic conversion from string and string slices to symbol * solve issue of pointer not found on syntax full roundtrip * fix syntax full roundtrip test * Fix lurk_tests with updated lurk-lib * Rustfmt * Revert to Nova 0.21.0 * Fix license formatting (#439) * Fix branch comparison, remove unneeded permissions (#440) * ci: make checkouts recursive (#442) - Enable recursive submodule checkout for test, clippy, and rustfmt jobs * Issue426 (#432) * Include failing test for issue 426 * Minimum failing test * Fixed circuit eval * Issue 424 and 426 tests * Issue 426 solved * Issue 424 modified works Maybe we shouldn't support inner letrecs, and only a single top level letrec should be allowed. * Minimal example * fmt --------- Co-authored-by: Gabriel Barreto <[email protected]> Co-authored-by: Arthur Paulino <[email protected]> * More clippy tuning (#441) * Clippy 1.70 * style: Improve Clippy settings across multiple directories - Introduce new `.clippy.toml` files for clutch, fcomm, and lurk-macros directories - Create and configure a global `.clippy.toml` file with custom settings - Establish `disallowed-methods` for `pasta_curves::Fp` and `pasta_curves::Fq` * Fix head ref in benchmark action (#443) * update flake.nix * Add GPU test on PR merge (#444) * Add GPU test on PR merge * Add GPU options * cleaner fetch_syntax_list * comments * add rust-analyzer to flake * fetch_syntax_list comment * remove double path clone in Symbol::extend * small improvements on ZStore::get_expr and ZStore::new_with_expr * resolve TODO remove child_z_ptrs * fix unwrap on ZStore::insert_expr * add back z_expr_ptr_from_parts z_cont_ptr_from_parts convenience functions * fix cont.rs Call arg order typo * fix u32->char conversion to allow for u32 values which are not Unicode codepoints to be Lurk chars * clippy * lowercase package symbols * parser comments and fixes * rename StrCons/StrNil, SymCons/SymNil to Str/EmptyStr, Sym/RootSym * remove outdated comment * improve parser::base::is_digit * address LurkSym feedback * revove unused store arg * clippy * ci: add action for signaling rebases on upstream (#448) * `let` and `letrec` fix (#446) * Include failing test for issue 426 * Minimum failing test * Fixed circuit eval * Issue 424 and 426 tests * Issue 426 solved * Issue 424 modified works Maybe we shouldn't support inner letrecs, and only a single top level letrec should be allowed. * Fixed circuit bugs in both `let` and `letrec` cases * Fixed `let` and `letrec` conflicts * Removed unnecessary tests * cargo fmt * Fixed number of constraints * Fixed flake.nix * Added tests back * Addressed the change requests * Better tests --------- Co-authored-by: emmorais <[email protected]> * Optimize ci (#450) * ci: Add dependeabot config for GH Actions - Introduce new dependabot.yml configuration - Implement weekly updates for GitHub Actions * ci: fix issue creation in upstream_update - Improve accuracy of condition checks in upstream_update.yml workflow * ci: Refactor GPU tests and workflows in rust.yml - Consolidate test-gpu job by merging CUDA and OpenCL tests - Rename test-cuda job to test-gpu for clarity - Set EC_GPU_FRAMEWORK as an environment variable for GPU tests * chore(deps): bump actions/checkout from 2 to 3 (#454) Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v2...v3) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Clean unused actions + Updates (#457) * ci: remove unused epics-action * chore(deps): bump lycheeverse/lychee-action from 1.5.4 to 1.8.0 Bumps [lycheeverse/lychee-action](https://github.com/lycheeverse/lychee-action) from 1.5.4 to 1.8.0. - [Release notes](https://github.com/lycheeverse/lychee-action/releases) - [Commits](lycheeverse/lychee-action@v1.5.4...v1.8.0) --- updated-dependencies: - dependency-name: lycheeverse/lychee-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * chore(deps): bump peter-evans/create-or-update-comment from 1 to 3 Bumps [peter-evans/create-or-update-comment](https://github.com/peter-evans/create-or-update-comment) from 1 to 3. - [Release notes](https://github.com/peter-evans/create-or-update-comment/releases) - [Commits](peter-evans/create-or-update-comment@v1...v3) --- updated-dependencies: - dependency-name: peter-evans/create-or-update-comment dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> * chore(deps): bump peter-evans/close-issue from 2 to 3 Bumps [peter-evans/close-issue](https://github.com/peter-evans/close-issue) from 2 to 3. - [Release notes](https://github.com/peter-evans/close-issue/releases) - [Commits](peter-evans/close-issue@v2...v3) --- updated-dependencies: - dependency-name: peter-evans/close-issue dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * ci: clean up unused job (#461) * ci: merge rustfmt, clippy * ci: remove unused workflow * refactor parser to use ParseResult * change Keyword representation to keycons - also rename fetch_scalar in src/store * improve ZExpr::from_ptr * remove outdated comment * improve parser::error * clippy * cleanup fraction example * ptr comments * rewrite fetch_string * clippy * ZData docstrings & serde fixup * fix a symbol.rs docstring; avoid unnecessary clone on Symbol::path; recover Symbol::parent and Symbol::child APIs * symbol.rs tests cleanup * Update ZData tests * Cleanup & rename scalar_ptr to z_ptr * include minimized test case error * fmt * address some review suggestions * implement cache hits when interning strings * further optimize 'intern_string' * undo change on public parameters * add some documentation; fix failing proptests (thanks to Sam) * simplify symbol_cache * recover recursive expr hashing and add hashing caches * move None case to the bottom in 'get_z_expr' * Fix ZStore roundtrip in get_z_expr * refactor Symbol and Keyword representation * clippy * remove unused package.rs * some cosmetic changes; improve some docstrings * remove outdated tests for zdata * add some extra test cases for src/parser/syntar.rs * fmt --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Gabriel Barreto <[email protected]> Co-authored-by: Arthur Paulino <[email protected]> Co-authored-by: Samuel Burnham <[email protected]> Co-authored-by: Samuel Burnham <[email protected]> Co-authored-by: François Garillot <[email protected]> Co-authored-by: porcuquine <[email protected]> Co-authored-by: porcuquine <[email protected]> Co-authored-by: Eduardo Morais <[email protected]> Co-authored-by: emmorais <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR attempts to rectify confusion and inaccuracy wreaking havoc in our naming of fields. This is really important for multiple reasons. The worst-case situation is one in which we accidentally instantiate a Lurk language on the wrong field. This would lead to incompatible data and great confusion. The less bad cases involve conceptual incoherence, false labeling, and general mayhem in the vicinity of our efforts to produce an evidently correct code base.
Please see individual commits for details. Problems fixed here:
LanguageField
were used. This would be fine if the actual correspondences between the pasta_curves named fields (Fq
andFp
) were well-known, but apparently it's not. In any case, using the explicative aliases eliminates the need for it to (usually) be. The same goes for use ofnova::S1
andnova::S2
.pasta_curves
) were not explicitly documented outside of code.If you contribute to
lurk-rs
, please take note.