diff --git a/.noir-sync-commit b/.noir-sync-commit index acaab867910..b64f398c068 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -db28cb9ffb710c286b54dbfcf57292ae3dffb03d +9471e28ad6f02bf2fae3782c3db68106b615595f diff --git a/noir-projects/aztec-nr/aztec/src/macros/dispatch/mod.nr b/noir-projects/aztec-nr/aztec/src/macros/dispatch/mod.nr index 956f518c19f..c8c1deeb0cc 100644 --- a/noir-projects/aztec-nr/aztec/src/macros/dispatch/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/macros/dispatch/mod.nr @@ -159,8 +159,9 @@ comptime fn str_size_in_fields(typ: Type) -> Option { comptime fn struct_size_in_fields(typ: Type) -> Option { typ.as_struct().map(|typ: (StructDefinition, [Type])| { let struct_type = typ.0; + let generics = typ.1; let mut size = 0; - for field in struct_type.fields() { + for field in struct_type.fields(generics) { size += size_in_fields(field.1); } size diff --git a/noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr b/noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr index 3fb417f3423..53efed5e62e 100644 --- a/noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr @@ -203,7 +203,8 @@ comptime fn generate_note_properties(s: StructDefinition) -> Quoted { let property_selector_type = type_of(PropertySelector { index: 0, offset: 0, length: 0 }); let note_header_type: Type = type_of(NoteHeader::empty()); - let non_header_fields = s.fields().filter(|(_, typ): (Quoted, Type)| typ != note_header_type); + let non_header_fields = + s.fields_as_written().filter(|(_, typ): (Quoted, Type)| typ != note_header_type); let properties_types = non_header_fields .map(|(name, _): (Quoted, Type)| quote { pub $name: $property_selector_type }) @@ -827,7 +828,7 @@ comptime fn index_note_fields( let mut indexed_fixed_fields: [(Quoted, Type, u32)] = &[]; let mut indexed_nullable_fields = &[]; let mut counter: u32 = 0; - for field in s.fields() { + for field in s.fields_as_written() { let (name, typ) = field; if (typ != NOTE_HEADER_TYPE) { if nullable_fields.all(|field| field != name) { @@ -845,9 +846,10 @@ comptime fn index_note_fields( /// Injects `NoteHeader` to the note struct if not present. comptime fn inject_note_header(s: StructDefinition) { - let filtered_header = s.fields().filter(|(_, typ): (Quoted, Type)| typ == NOTE_HEADER_TYPE); + let filtered_header = + s.fields_as_written().filter(|(_, typ): (Quoted, Type)| typ == NOTE_HEADER_TYPE); if (filtered_header.len() == 0) { - let new_fields = s.fields().push_back((quote { header }, NOTE_HEADER_TYPE)); + let new_fields = s.fields_as_written().push_back((quote { header }, NOTE_HEADER_TYPE)); s.set_fields(new_fields); } } diff --git a/noir-projects/aztec-nr/aztec/src/macros/storage/mod.nr b/noir-projects/aztec-nr/aztec/src/macros/storage/mod.nr index 40c22dae0a5..857517210ce 100644 --- a/noir-projects/aztec-nr/aztec/src/macros/storage/mod.nr +++ b/noir-projects/aztec-nr/aztec/src/macros/storage/mod.nr @@ -27,7 +27,8 @@ pub comptime fn storage(s: StructDefinition) -> Quoted { // TODO(#8658): uncomment the code below to inject the Context type parameter. //let mut new_storage_fields = &[]; //let context_generic = s.add_generic("Context"); - for field in s.fields() { + for field in s.fields_as_written() { + // FIXME: This doesn't handle field types with generics let (name, typ) = field; let (storage_field_constructor, serialized_size) = generate_storage_field_constructor(typ, quote { $slot }, false); diff --git a/noir-projects/aztec-nr/aztec/src/macros/utils.nr b/noir-projects/aztec-nr/aztec/src/macros/utils.nr index 4160022fd14..04157ef53ed 100644 --- a/noir-projects/aztec-nr/aztec/src/macros/utils.nr +++ b/noir-projects/aztec-nr/aztec/src/macros/utils.nr @@ -128,9 +128,11 @@ comptime fn signature_of_type(typ: Type) -> Quoted { let element_typ_quote = signature_of_type(element_type); f"[{element_typ_quote};{array_len}]".quoted_contents() } else if typ.as_struct().is_some() { - let (s, _) = typ.as_struct().unwrap(); - let field_signatures = - s.fields().map(|(_, typ): (Quoted, Type)| signature_of_type(typ)).join(quote {,}); + let (s, generics) = typ.as_struct().unwrap(); + let field_signatures = s + .fields(generics) + .map(|(_, typ): (Quoted, Type)| signature_of_type(typ)) + .join(quote {,}); f"({field_signatures})".quoted_contents() } else if typ.as_tuple().is_some() { // Note that tuples are handled the same way as structs @@ -201,9 +203,11 @@ pub(crate) comptime fn compute_event_selector(s: StructDefinition) -> Field { // The signature will be "Foo(Field,AztecAddress)". let event_name = s.name(); let args_signatures = s - .fields() + .fields_as_written() .map(|(_, typ): (Quoted, Type)| { - signature_of_type(typ) // signature_of_type can handle structs, so this supports nested structs + // signature_of_type can handle structs, so this supports nested structs + // FIXME: Field generics are not handled here! + signature_of_type(typ) }) .join(quote {,}); let signature_quote = quote { $event_name($args_signatures) }; diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/blob.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/blob.nr index 30b9584a927..a4da452fe44 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/blob.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/blob.nr @@ -4,7 +4,7 @@ use crate::{ unconstrained_config::{compute_roots_of_unity, D_INV, F, LOG_FIELDS_PER_BLOB}, }; -use bigint::BigNum; +use bigint::{BigNum, BigNumTrait}; // Fixed hash method: use types::hash::poseidon2_hash_subarray; // Variable hash method: diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/mock_blob_oracle.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/mock_blob_oracle.nr index e25468ded05..8dfda494bcf 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/mock_blob_oracle.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/mock_blob_oracle.nr @@ -1,8 +1,10 @@ use crate::blob_public_inputs::{BlobCommitment, BlockBlobPublicInputs}; +use super::blob_public_inputs::Deserialize; use types::{ abis::sponge_blob::SpongeBlob, constants::{BLOB_PUBLIC_INPUTS, BLOBS_PER_BLOCK, FIELDS_PER_BLOB}, }; + // TODO(#10323): this was added to save simulation time (~1min in ACVM, ~3mins in wasm -> 500ms). // The use of bignum adds a lot of unconstrained code which overloads limits when simulating. // If/when simulation times of unconstrained are improved, remove this. diff --git a/noir-projects/noir-protocol-circuits/crates/blob/src/unconstrained_config.nr b/noir-projects/noir-protocol-circuits/crates/blob/src/unconstrained_config.nr index 0a794453d0a..8d8992e4508 100644 --- a/noir-projects/noir-protocol-circuits/crates/blob/src/unconstrained_config.nr +++ b/noir-projects/noir-protocol-circuits/crates/blob/src/unconstrained_config.nr @@ -1,4 +1,4 @@ -use bigint::{BigNum, fields::bls12_381Fr::BLS12_381_Fr_Params}; +use bigint::{bignum::{BigNum, BigNumTrait}, fields::bls12_381Fr::BLS12_381_Fr_Params}; use types::constants::FIELDS_PER_BLOB; // TODO(#9982): Delete this file and go back to using config.nr - calculating ROOTS in unconstrained is insecure. diff --git a/noir-projects/noir-protocol-circuits/crates/types/src/meta/mod.nr b/noir-projects/noir-protocol-circuits/crates/types/src/meta/mod.nr index 156d1c2c414..5176086a2e1 100644 --- a/noir-projects/noir-protocol-circuits/crates/types/src/meta/mod.nr +++ b/noir-projects/noir-protocol-circuits/crates/types/src/meta/mod.nr @@ -23,10 +23,10 @@ pub comptime fn pack_from_fields( result = quote { $buffer[$already_consumed] as $typ }; consumed = 1; } else if typ.as_struct().is_some() { - let (nested_def, _) = typ.as_struct().unwrap(); + let (nested_def, generics) = typ.as_struct().unwrap(); let nested_name = nested_def.name(); let mut deserialized_fields_list = &[]; - for field in nested_def.fields() { + for field in nested_def.fields(generics) { let (field_name, field_type) = field; let (deserialized_field, consumed_by_field) = pack_from_fields( quote { $field_name }, @@ -106,7 +106,7 @@ pub comptime fn flatten_to_fields(name: Quoted, typ: Type, omit: [Quoted]) -> ([ } else if typ.as_struct().is_some() { // For struct we pref let nested_struct = typ.as_struct().unwrap(); - let params = nested_struct.0.fields(); + let params = nested_struct.0.fields(nested_struct.1); let struct_flattened = params.map(|(param_name, param_type): (Quoted, Type)| { let maybe_prefixed_name = if name == quote {} { // Triggered when the param name is of a value available in the current scope (e.g. a function diff --git a/noir/bootstrap.sh b/noir/bootstrap.sh index b8fe93766b2..5554e6b9b5a 100755 --- a/noir/bootstrap.sh +++ b/noir/bootstrap.sh @@ -10,7 +10,8 @@ function build { if ! cache_download noir-$hash.tar.gz; then # Fake this so artifacts have a consistent hash in the cache and not git hash dependent export COMMIT_HASH="$(echo "$hash" | sed 's/-.*//g')" - parallel denoise ::: ./scripts/bootstrap_native.sh ./scripts/bootstrap_packages.sh + denoise ./scripts/bootstrap_native.sh + denoise ./scripts/bootstrap_packages.sh cache_upload noir-$hash.tar.gz noir-repo/target/release/nargo noir-repo/target/release/acvm packages fi github_endgroup diff --git a/noir/noir-repo/.github/critical_libraries_status/.gitignore b/noir/noir-repo/.github/critical_libraries_status/.gitignore new file mode 100644 index 00000000000..38a3cf9caf1 --- /dev/null +++ b/noir/noir-repo/.github/critical_libraries_status/.gitignore @@ -0,0 +1,3 @@ +.actual.jsonl +.actual.jsonl.jq +.failures.jsonl.jq \ No newline at end of file diff --git a/noir/noir-repo/.github/critical_libraries_status/noir-lang/noir_check_shuffle/.failures.jsonl b/noir/noir-repo/.github/critical_libraries_status/noir-lang/noir_check_shuffle/.failures.jsonl new file mode 100644 index 00000000000..e69de29bb2d diff --git a/noir/noir-repo/.github/workflows/formatting.yml b/noir/noir-repo/.github/workflows/formatting.yml index b132ba6f938..4e836ef2493 100644 --- a/noir/noir-repo/.github/workflows/formatting.yml +++ b/noir/noir-repo/.github/workflows/formatting.yml @@ -143,3 +143,29 @@ jobs: - name: Format test suite working-directory: ./test_programs run: ./format.sh check + + # This is a job which depends on all test jobs and reports the overall status. + # This allows us to add/remove test jobs without having to update the required workflows. + formatting-end: + name: Formatting End + runs-on: ubuntu-22.04 + # We want this job to always run (even if the dependant jobs fail) as we want this job to fail rather than skipping. + if: ${{ always() }} + needs: + - clippy + - rustfmt + - eslint + - nargo_fmt + + steps: + - name: Report overall success + run: | + if [[ $FAIL == true ]]; then + exit 1 + else + exit 0 + fi + env: + # We treat any skipped or failing jobs as a failure for the workflow as a whole. + FAIL: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped') }} + diff --git a/noir/noir-repo/.github/workflows/reports.yml b/noir/noir-repo/.github/workflows/reports.yml index ce9bce774a6..ccf86b83200 100644 --- a/noir/noir-repo/.github/workflows/reports.yml +++ b/noir/noir-repo/.github/workflows/reports.yml @@ -116,8 +116,19 @@ jobs: - name: Generate Brillig bytecode size report working-directory: ./test_programs run: | - ./gates_report_brillig.sh - mv gates_report_brillig.json ../gates_report_brillig.json + mkdir ./reports + + ./gates_report_brillig.sh 9223372036854775807 + jq '.programs |= map(.package_name |= (. + "_inliner_max"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_max.json + + ./gates_report_brillig.sh 0 + jq '.programs |= map(.package_name |= (. + "_inliner_zero"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_zero.json + + ./gates_report_brillig.sh -9223372036854775808 + jq '.programs |= map(.package_name |= (. + "_inliner_min"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_min.json + + # Merge all reports + jq -s '{ programs: map(.programs) | add }' ./reports/* > ../gates_report_brillig.json - name: Compare Brillig bytecode size reports id: brillig_bytecode_diff @@ -165,8 +176,19 @@ jobs: - name: Generate Brillig execution report working-directory: ./test_programs run: | - ./gates_report_brillig_execution.sh - mv gates_report_brillig_execution.json ../gates_report_brillig_execution.json + mkdir ./reports + + ./gates_report_brillig_execution.sh 9223372036854775807 + jq '.programs |= map(.package_name |= (. + "_inliner_max"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_max.json + + ./gates_report_brillig_execution.sh 0 + jq '.programs |= map(.package_name |= (. + "_inliner_zero"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_zero.json + + ./gates_report_brillig_execution.sh -9223372036854775808 + jq '.programs |= map(.package_name |= (. + "_inliner_min"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_min.json + + # Merge all reports + jq -s '{ programs: map(.programs) | add }' ./reports/* > ../gates_report_brillig_execution.json - name: Compare Brillig execution reports id: brillig_execution_diff @@ -299,15 +321,18 @@ jobs: fail-fast: false matrix: include: - # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, is_library: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, take_average: true } - #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, take_average: true } + # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, num_runs: 5, cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge, num_runs: 5 } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, num_runs: 5 } name: External repo compilation and execution reports - ${{ matrix.project.repo }}/${{ matrix.project.path }} steps: @@ -349,33 +374,17 @@ jobs: touch parse_time.sh chmod +x parse_time.sh cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh - ./compilation_report.sh 1 - - - name: Generate compilation report with averages - working-directory: ./test-repo/${{ matrix.project.path }} - if: ${{ matrix.project.take_average }} - run: | - mv /home/runner/work/noir/noir/scripts/test_programs/compilation_report.sh ./compilation_report.sh - touch parse_time.sh - chmod +x parse_time.sh - cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh - ./compilation_report.sh 1 1 - - - name: Generate execution report without averages - working-directory: ./test-repo/${{ matrix.project.path }} - if: ${{ !matrix.project.is_library && !matrix.project.take_average }} - run: | - mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh - mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh - ./execution_report.sh 1 + ./compilation_report.sh 1 ${{ matrix.project.num_runs }} + env: + FLAGS: ${{ matrix.project.flags }} - - name: Generate execution report with averages + - name: Generate execution report working-directory: ./test-repo/${{ matrix.project.path }} - if: ${{ !matrix.project.is_library && matrix.project.take_average }} + if: ${{ !matrix.project.cannot_execute }} run: | mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh - ./execution_report.sh 1 1 + ./execution_report.sh 1 ${{ matrix.project.num_runs }} - name: Move compilation report id: compilation_report @@ -389,7 +398,7 @@ jobs: - name: Move execution report id: execution_report shell: bash - if: ${{ !matrix.project.is_library }} + if: ${{ !matrix.project.cannot_execute }} run: | PACKAGE_NAME=${{ matrix.project.path }} PACKAGE_NAME=$(basename $PACKAGE_NAME) @@ -467,10 +476,16 @@ jobs: # TODO: Bring this report back under a flag. The `noir-contracts` report takes just under 30 minutes. # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root } name: External repo memory report - ${{ matrix.project.repo }}/${{ matrix.project.path }} steps: @@ -509,9 +524,12 @@ jobs: ./memory_report.sh 1 # Rename the memory report as the execution report is about to write to the same file cp memory_report.json compilation_memory_report.json + env: + FLAGS: ${{ matrix.project.flags }} - name: Generate execution memory report working-directory: ./test-repo/${{ matrix.project.path }} + if: ${{ !matrix.project.cannot_execute }} run: | ./memory_report.sh 1 1 @@ -534,6 +552,7 @@ jobs: - name: Move execution report id: execution_mem_report + if: ${{ !matrix.project.cannot_execute }} shell: bash run: | PACKAGE_NAME=${{ matrix.project.path }} diff --git a/noir/noir-repo/CRITICAL_NOIR_LIBRARIES b/noir/noir-repo/CRITICAL_NOIR_LIBRARIES index c753b76a4fc..7637d9ac6df 100644 --- a/noir/noir-repo/CRITICAL_NOIR_LIBRARIES +++ b/noir/noir-repo/CRITICAL_NOIR_LIBRARIES @@ -1,3 +1,4 @@ +https://github.com/noir-lang/noir_check_shuffle https://github.com/noir-lang/ec https://github.com/noir-lang/eddsa https://github.com/noir-lang/mimc diff --git a/noir/noir-repo/Cargo.lock b/noir/noir-repo/Cargo.lock index 89287609a2b..e82d47d690a 100644 --- a/noir/noir-repo/Cargo.lock +++ b/noir/noir-repo/Cargo.lock @@ -644,6 +644,12 @@ version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8c3c1a368f70d6cf7302d78f8f7093da241fb8e8807c05cc9e51a125895a6d5b" +[[package]] +name = "binary-merge" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "597bb81c80a54b6a4381b23faba8d7774b144c94cbd1d6fe3f1329bd776554ab" + [[package]] name = "bincode" version = "1.3.3" @@ -2519,6 +2525,15 @@ dependencies = [ "libc", ] +[[package]] +name = "inplace-vec-builder" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf64c2edc8226891a71f127587a2861b132d2b942310843814d5001d99a1d307" +dependencies = [ + "smallvec", +] + [[package]] name = "is-terminal" version = "0.4.13" @@ -3366,6 +3381,7 @@ dependencies = [ "thiserror", "tracing", "tracing-test", + "vec-collections", ] [[package]] @@ -4807,6 +4823,12 @@ dependencies = [ "sha1", ] +[[package]] +name = "sorted-iter" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bceb57dc07c92cdae60f5b27b3fa92ecaaa42fe36c55e22dbfb0b44893e0b1f7" + [[package]] name = "spin" version = "0.9.8" @@ -5497,6 +5519,21 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" +[[package]] +name = "vec-collections" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c9965c8f2ffed1dbcd16cafe18a009642f540fa22661c6cfd6309ddb02e4982" +dependencies = [ + "binary-merge", + "inplace-vec-builder", + "lazy_static", + "num-traits", + "serde", + "smallvec", + "sorted-iter", +] + [[package]] name = "version_check" version = "0.9.5" diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/mod.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/mod.rs index daedd77c4a0..ee84f7bf60b 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/mod.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/mod.rs @@ -16,10 +16,6 @@ pub use simulator::CircuitSimulator; use transformers::transform_internal; pub use transformers::{transform, MIN_EXPRESSION_WIDTH}; -/// We need multiple passes to stabilize the output. -/// The value was determined by running tests. -const MAX_OPTIMIZER_PASSES: usize = 3; - /// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map /// metadata they had about the opcodes to the new opcode structure generated after the transformation. #[derive(Debug)] @@ -84,41 +80,10 @@ pub fn compile( ) -> (Circuit, AcirTransformationMap) { let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - if MAX_OPTIMIZER_PASSES == 0 { - return (acir, AcirTransformationMap::new(&acir_opcode_positions)); - } - - let mut pass = 0; - let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); - let mut prev_acir = acir; - let mut prev_acir_opcode_positions = acir_opcode_positions; - - // For most test programs it would be enough to only loop `transform_internal`, - // but some of them don't stabilize unless we also repeat the backend agnostic optimizations. - let (mut acir, acir_opcode_positions) = loop { - let (acir, acir_opcode_positions) = - optimize_internal(prev_acir, prev_acir_opcode_positions); - - // Stop if we have already done at least one transform and an extra optimization changed nothing. - if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) { - break (acir, acir_opcode_positions); - } - - let (acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions); - - let opcodes_hash = fxhash::hash64(&acir.opcodes); - - // Stop if the output hasn't change in this loop or we went too long. - if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { - break (acir, acir_opcode_positions); - } + let (acir, acir_opcode_positions) = optimize_internal(acir, acir_opcode_positions); - pass += 1; - prev_acir = acir; - prev_opcodes_hash = opcodes_hash; - prev_acir_opcode_positions = acir_opcode_positions; - }; + let (mut acir, acir_opcode_positions) = + transform_internal(acir, expression_width, acir_opcode_positions); let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/transformers/mod.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/transformers/mod.rs index a499aec1b30..77a5d2da34e 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -14,12 +14,17 @@ mod csat; pub(crate) use csat::CSatTransformer; pub use csat::MIN_EXPRESSION_WIDTH; +use tracing::info; use super::{ - optimizers::MergeExpressionsOptimizer, transform_assert_messages, AcirTransformationMap, - MAX_OPTIMIZER_PASSES, + optimizers::{MergeExpressionsOptimizer, RangeOptimizer}, + transform_assert_messages, AcirTransformationMap, }; +/// We need multiple passes to stabilize the output. +/// The value was determined by running tests. +const MAX_TRANSFORMER_PASSES: usize = 3; + /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. pub fn transform( acir: Circuit, @@ -50,12 +55,18 @@ pub(super) fn transform_internal( expression_width: ExpressionWidth, mut acir_opcode_positions: Vec, ) -> (Circuit, Vec) { + if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) { + info!("Program is fully unconstrained, skipping transformation pass"); + return (acir, acir_opcode_positions); + } + // Allow multiple passes until we have stable output. let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); // For most test programs it would be enough to loop here, but some of them // don't stabilize unless we also repeat the backend agnostic optimizations. - for _ in 0..MAX_OPTIMIZER_PASSES { + for _ in 0..MAX_TRANSFORMER_PASSES { + info!("Number of opcodes {}", acir.opcodes.len()); let (new_acir, new_acir_opcode_positions) = transform_internal_once(acir, expression_width, acir_opcode_positions); @@ -217,6 +228,12 @@ fn transform_internal_once( ..acir }; + // The `MergeOptimizer` can merge two witnesses which have range opcodes applied to them + // so we run the `RangeOptimizer` afterwards to clear these up. + let range_optimizer = RangeOptimizer::new(acir); + let (acir, new_acir_opcode_positions) = + range_optimizer.replace_redundant_ranges(new_acir_opcode_positions); + (acir, new_acir_opcode_positions) } diff --git a/noir/noir-repo/compiler/noirc_evaluator/Cargo.toml b/noir/noir-repo/compiler/noirc_evaluator/Cargo.toml index 15531fafff7..3e30fa6673d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/Cargo.toml +++ b/noir/noir-repo/compiler/noirc_evaluator/Cargo.toml @@ -29,6 +29,7 @@ chrono = "0.4.37" rayon.workspace = true cfg-if.workspace = true smallvec = { version = "1.13.2", features = ["serde"] } +vec-collections = "0.4.3" [dev-dependencies] proptest.workspace = true diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/generated_acir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/generated_acir.rs index a2b161688c0..14ceac62461 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/generated_acir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/generated_acir.rs @@ -479,7 +479,7 @@ impl GeneratedAcir { pub(crate) fn is_equal(&mut self, lhs: &Expression, rhs: &Expression) -> Witness { let t = lhs - rhs; - self.is_zero(&t) + self.is_zero(t) } /// Returns a `Witness` that is constrained to be: @@ -534,36 +534,32 @@ impl GeneratedAcir { /// By setting `z` to be `0`, we can make `y` equal to `1`. /// This is easily observed: `y = 1 - t * 0` /// Now since `y` is one, this means that `t` needs to be zero, or else `y * t == 0` will fail. - fn is_zero(&mut self, t_expr: &Expression) -> Witness { - // We're checking for equality with zero so we can negate the expression without changing the result. - // This is useful as it will sometimes allow us to simplify an expression down to a witness. - let t_witness = if let Some(witness) = t_expr.to_witness() { - witness + fn is_zero(&mut self, t_expr: Expression) -> Witness { + // We're going to be multiplying this expression by two different witnesses in a second so we want to + // ensure that this expression only contains a single witness. We can tolerate coefficients and constant terms however. + let linear_t = if t_expr.is_degree_one_univariate() { + t_expr } else { - let negated_expr = t_expr * -F::one(); - self.get_or_create_witness(&negated_expr) + Expression::::from(self.get_or_create_witness(&t_expr)) }; // Call the inversion directive, since we do not apply a constraint // the prover can choose anything here. - let z = self.brillig_inverse(t_witness.into()); + let z = self.brillig_inverse(linear_t.clone()); + let z_expr = Expression::::from(z); let y = self.next_witness_index(); + let y_expr = Expression::::from(y); // Add constraint y == 1 - tz => y + tz - 1 == 0 - let y_is_boolean_constraint = Expression { - mul_terms: vec![(F::one(), t_witness, z)], - linear_combinations: vec![(F::one(), y)], - q_c: -F::one(), - }; + let mut y_is_boolean_constraint = + (&z_expr * &linear_t).expect("multiplying two linear expressions"); + y_is_boolean_constraint.push_addition_term(F::one(), y); + let y_is_boolean_constraint = y_is_boolean_constraint - F::one(); self.assert_is_zero(y_is_boolean_constraint); // Add constraint that y * t == 0; - let ty_zero_constraint = Expression { - mul_terms: vec![(F::one(), t_witness, y)], - linear_combinations: vec![], - q_c: F::zero(), - }; + let ty_zero_constraint = (&y_expr * &linear_t).expect("multiplying two linear expressions"); self.assert_is_zero(ty_zero_constraint); y diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index ddc9814a40d..6eae291ca47 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -20,7 +20,6 @@ use iter_extended::vecmap; use serde::{Deserialize, Serialize}; use serde_with::serde_as; use serde_with::DisplayFromStr; -use tracing::warn; /// The DataFlowGraph contains most of the actual data in a function including /// its blocks, instructions, and values. This struct is largely responsible for @@ -238,8 +237,7 @@ impl DataFlowGraph { call_stack: CallStackId, ) -> InsertInstructionResult { if !self.is_handled_by_runtime(&instruction) { - warn!("Attempted to insert instruction not handled by runtime: {instruction:?}"); - return InsertInstructionResult::InstructionRemoved; + panic!("Attempted to insert instruction not handled by runtime: {instruction:?}"); } match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 2c7d6b4f1ee..5b75055c09a 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -307,6 +307,7 @@ impl<'brillig> Context<'brillig> { let old_results = dfg.instruction_results(id).to_vec(); // If a copy of this instruction exists earlier in the block, then reuse the previous results. + let runtime_is_brillig = dfg.runtime().is_brillig(); if let Some(cache_result) = self.get_cached(dfg, dom, &instruction, *side_effects_enabled_var, block) { @@ -314,7 +315,7 @@ impl<'brillig> Context<'brillig> { CacheResult::Cached(cached) => { // We track whether we may mutate MakeArray instructions before we deduplicate // them but we still need to issue an extra inc_rc in case they're mutated afterward. - if matches!(instruction, Instruction::MakeArray { .. }) { + if runtime_is_brillig && matches!(instruction, Instruction::MakeArray { .. }) { let value = *cached.last().unwrap(); let inc_rc = Instruction::IncrementRc { value }; let call_stack = dfg.get_instruction_call_stack_id(id); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index f0f677e5db9..125cf3a12ca 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -112,10 +112,12 @@ impl<'f> LoopInvariantContext<'f> { // If we are hoisting a MakeArray instruction, // we need to issue an extra inc_rc in case they are mutated afterward. - if matches!( - self.inserter.function.dfg[instruction_id], - Instruction::MakeArray { .. } - ) { + if self.inserter.function.runtime().is_brillig() + && matches!( + self.inserter.function.dfg[instruction_id], + Instruction::MakeArray { .. } + ) + { let result = self.inserter.function.dfg.instruction_results(instruction_id)[0]; let inc_rc = Instruction::IncrementRc { value: result }; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 96a24583837..ce76825877a 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -79,6 +79,7 @@ mod block; use std::collections::{BTreeMap, BTreeSet}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use vec_collections::VecSet; use crate::ssa::{ ir::{ @@ -619,7 +620,7 @@ impl<'f> PerFunctionContext<'f> { // then those parameters also alias each other. // We save parameters with repeat arguments to later mark those // parameters as aliasing one another. - let mut arg_set: HashMap> = HashMap::default(); + let mut arg_set = HashMap::default(); // Add an alias for each reference parameter for (parameter, argument) in destination_parameters.iter().zip(arguments) { @@ -632,7 +633,8 @@ impl<'f> PerFunctionContext<'f> { aliases.insert(*parameter); // Check if we have seen the same argument - let seen_parameters = arg_set.entry(argument).or_default(); + let seen_parameters = + arg_set.entry(argument).or_insert_with(VecSet::empty); // Add the current parameter to the parameters we have seen for this argument. // The previous parameters and the current one alias one another. seen_parameters.insert(*parameter); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs index e32eaa70186..443b21cfd15 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeSet; +use vec_collections::{AbstractVecSet, VecSet}; use crate::ssa::ir::value::ValueId; @@ -10,7 +10,7 @@ use crate::ssa::ir::value::ValueId; /// "unknown which aliases this may refer to" - `None`. #[derive(Debug, Default, Clone)] pub(super) struct AliasSet { - aliases: Option>, + aliases: Option>, } impl AliasSet { @@ -19,12 +19,10 @@ impl AliasSet { } pub(super) fn known(value: ValueId) -> AliasSet { - let mut aliases = BTreeSet::new(); - aliases.insert(value); - Self { aliases: Some(aliases) } + Self { aliases: Some(VecSet::single(value)) } } - pub(super) fn known_multiple(values: BTreeSet) -> AliasSet { + pub(super) fn known_multiple(values: VecSet<[ValueId; 1]>) -> AliasSet { Self { aliases: Some(values) } } @@ -32,7 +30,7 @@ impl AliasSet { /// particular value will be known to be zero, which is distinct from being unknown and /// possibly referring to any alias. pub(super) fn known_empty() -> AliasSet { - Self { aliases: Some(BTreeSet::new()) } + Self { aliases: Some(VecSet::empty()) } } pub(super) fn is_unknown(&self) -> bool { @@ -44,19 +42,33 @@ impl AliasSet { pub(super) fn single_alias(&self) -> Option { self.aliases .as_ref() - .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.first().unwrap())) + .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.iter().next().unwrap())) } /// Unify this alias set with another. The result of this set is empty if either set is empty. /// Otherwise, it is the union of both alias sets. pub(super) fn unify(&mut self, other: &Self) { if let (Some(self_aliases), Some(other_aliases)) = (&mut self.aliases, &other.aliases) { - self_aliases.extend(other_aliases); + self_aliases.extend(other_aliases.iter().cloned()); } else { self.aliases = None; } } + /// Returns true if calling `unify` would change something in this alias set. + /// + /// This is an optimization to avoid having to look up an entry ready to be modified in the [Block](crate::ssa::opt::mem2reg::block::Block), + /// because doing so would involve calling `Arc::make_mut` which clones the entry, ready for modification. + pub(super) fn should_unify(&self, other: &Self) -> bool { + if let (Some(self_aliases), Some(other_aliases)) = (&self.aliases, &other.aliases) { + // `unify` would extend `self_aliases` with `other_aliases`, so if `other_aliases` is a subset, then nothing would happen. + !other_aliases.is_subset(self_aliases) + } else { + // `unify` would set `aliases` to `None`, so if it's not `Some`, then nothing would happen. + self.aliases.is_some() + } + } + /// Inserts a new alias into this set if it is not unknown pub(super) fn insert(&mut self, new_alias: ValueId) { if let Some(aliases) = &mut self.aliases { @@ -82,6 +94,6 @@ impl AliasSet { /// The ordering is arbitrary (by lowest ValueId) so this method should only be /// used when you need an arbitrary ValueId from the alias set. pub(super) fn first(&self) -> Option { - self.aliases.as_ref().and_then(|aliases| aliases.first().copied()) + self.aliases.as_ref().and_then(|aliases| aliases.iter().next().copied()) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index f4265b2466d..91e27f07b8e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -128,6 +128,12 @@ impl Block { } for (expression, new_aliases) in &other.aliases { + // If nothing would change, then don't call `.entry(...).and_modify(...)` as it involves creating more `Arc`s. + if let Some(aliases) = self.aliases.get(expression) { + if !aliases.should_unify(new_aliases) { + continue; + } + } let expression = expression.clone(); self.aliases diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 16b7b3ea01b..529c207a4ad 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -528,6 +528,22 @@ impl<'a> FunctionContext<'a> { /// ... This is the current insert point after codegen_for finishes ... /// ``` fn codegen_for(&mut self, for_expr: &ast::For) -> Result { + self.builder.set_location(for_expr.start_range_location); + let start_index = self.codegen_non_tuple_expression(&for_expr.start_range)?; + + self.builder.set_location(for_expr.end_range_location); + let end_index = self.codegen_non_tuple_expression(&for_expr.end_range)?; + + if let (Some(start_constant), Some(end_constant)) = ( + self.builder.current_function.dfg.get_numeric_constant(start_index), + self.builder.current_function.dfg.get_numeric_constant(end_index), + ) { + // If we can determine that the loop contains zero iterations then there's no need to codegen the loop. + if start_constant >= end_constant { + return Ok(Self::unit_value()); + } + } + let loop_entry = self.builder.insert_block(); let loop_body = self.builder.insert_block(); let loop_end = self.builder.insert_block(); @@ -540,12 +556,6 @@ impl<'a> FunctionContext<'a> { // within the loop which need to jump to them. self.enter_loop(loop_entry, loop_index, loop_end); - self.builder.set_location(for_expr.start_range_location); - let start_index = self.codegen_non_tuple_expression(&for_expr.start_range)?; - - self.builder.set_location(for_expr.end_range_location); - let end_index = self.codegen_non_tuple_expression(&for_expr.end_range)?; - // Set the location of the initial jmp instruction to the start range. This is the location // used to issue an error if the start range cannot be determined at compile-time. self.builder.set_location(for_expr.start_range_location); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs index 1b3bc645cbc..33af075aebd 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -775,7 +775,10 @@ impl<'context> Elaborator<'context> { span, }, }; - self.push_trait_constraint(constraint, expr_id); + self.push_trait_constraint( + constraint, expr_id, + true, // this constraint should lead to choosing a trait impl + ); self.type_check_operator_method(expr_id, trait_id, operand_type, span); } typ diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs index aeee417789e..10e56364ed3 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -197,7 +197,11 @@ struct FunctionContext { /// verified at the end of a function. This is because constraints arise /// on each variable, but it is only until function calls when the types /// needed for the trait constraint may become known. - trait_constraints: Vec<(TraitConstraint, ExprId)>, + /// The `select impl` bool indicates whether, after verifying the trait constraint, + /// the resulting trait impl should be the one used for a call (sometimes trait + /// constraints are verified but there's no call associated with them, like in the + /// case of checking generic arguments) + trait_constraints: Vec<(TraitConstraint, ExprId, bool /* select impl */)>, } impl<'context> Elaborator<'context> { @@ -315,7 +319,7 @@ impl<'context> Elaborator<'context> { self.define_function_metas(&mut items.functions, &mut items.impls, &mut items.trait_impls); - self.collect_traits(&items.traits); + self.collect_traits(&mut items.traits); // Before we resolve any function symbols we must go through our impls and // re-collect the methods within into their proper module. This cannot be @@ -354,6 +358,7 @@ impl<'context> Elaborator<'context> { self.current_trait = Some(trait_id); self.elaborate_functions(unresolved_trait.fns_with_default_impl); } + self.current_trait = None; for impls in items.impls.into_values() { @@ -475,7 +480,7 @@ impl<'context> Elaborator<'context> { self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused); } - self.add_trait_constraints_to_scope(&func_meta); + self.add_trait_constraints_to_scope(&func_meta.trait_constraints, func_meta.location.span); let (hir_func, body_type) = match kind { FunctionKind::Builtin @@ -501,7 +506,7 @@ impl<'context> Elaborator<'context> { // when multiple impls are available. Instead we default first to choose the Field or u64 impl. self.check_and_pop_function_context(); - self.remove_trait_constraints_from_scope(&func_meta); + self.remove_trait_constraints_from_scope(&func_meta.trait_constraints); let func_scope_tree = self.scopes.end_function(); @@ -549,7 +554,7 @@ impl<'context> Elaborator<'context> { } } - for (mut constraint, expr_id) in context.trait_constraints { + for (mut constraint, expr_id, select_impl) in context.trait_constraints { let span = self.interner.expr_span(&expr_id); if matches!(&constraint.typ, Type::MutableReference(_)) { @@ -564,6 +569,7 @@ impl<'context> Elaborator<'context> { &constraint.trait_bound.trait_generics.ordered, &constraint.trait_bound.trait_generics.named, expr_id, + select_impl, span, ); } @@ -733,8 +739,13 @@ impl<'context> Elaborator<'context> { None } - /// TODO: This is currently only respected for generic free functions - /// there's a bunch of other places where trait constraints can pop up + /// Resolve the given trait constraints and add them to scope as we go. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. + /// + /// If these constraints are unwanted afterward they should be manually + /// removed from the interner. fn resolve_trait_constraints( &mut self, where_clause: &[UnresolvedTraitConstraint], @@ -745,12 +756,92 @@ impl<'context> Elaborator<'context> { .collect() } - pub fn resolve_trait_constraint( + /// Expands any traits in a where clause to mention all associated types if they were + /// elided by the user. See `add_missing_named_generics` for more detail. + /// + /// Returns all newly created generics to be added to this function/trait/impl. + fn desugar_trait_constraints( + &mut self, + where_clause: &mut [UnresolvedTraitConstraint], + ) -> Vec { + where_clause + .iter_mut() + .flat_map(|constraint| self.add_missing_named_generics(&mut constraint.trait_bound)) + .collect() + } + + /// For each associated type that isn't mentioned in a trait bound, this adds + /// the type as an implicit generic to the where clause and returns the newly + /// created generics in a vector to add to the function/trait/impl later. + /// For example, this will turn a function using a trait with 2 associated types: + /// + /// `fn foo() where T: Foo { ... }` + /// + /// into: + /// `fn foo() where T: Foo { ... }` + /// + /// with a vector of `` returned so that the caller can then modify the function to: + /// `fn foo() where T: Foo { ... }` + fn add_missing_named_generics(&mut self, bound: &mut TraitBound) -> Vec { + let mut added_generics = Vec::new(); + + let Ok(item) = self.resolve_path_or_error(bound.trait_path.clone()) else { + return Vec::new(); + }; + + let PathResolutionItem::Trait(trait_id) = item else { + return Vec::new(); + }; + + let the_trait = self.get_trait_mut(trait_id); + + if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { + for associated_type in &the_trait.associated_types.clone() { + if !bound + .trait_generics + .named_args + .iter() + .any(|(name, _)| name.0.contents == *associated_type.name.as_ref()) + { + // This generic isn't contained in the bound's named arguments, + // so add it by creating a fresh type variable. + let new_generic_id = self.interner.next_type_variable_id(); + let type_var = TypeVariable::unbound(new_generic_id, Kind::Normal); + + let span = bound.trait_path.span; + let name = associated_type.name.clone(); + let typ = Type::NamedGeneric(type_var.clone(), name.clone()); + let typ = self.interner.push_quoted_type(typ); + let typ = UnresolvedTypeData::Resolved(typ).with_span(span); + let ident = Ident::new(associated_type.name.as_ref().clone(), span); + + bound.trait_generics.named_args.push((ident, typ)); + added_generics.push(ResolvedGeneric { name, span, type_var }); + } + } + } + + added_generics + } + + /// Resolves a trait constraint and adds it to scope as an assumed impl. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. + fn resolve_trait_constraint( &mut self, constraint: &UnresolvedTraitConstraint, ) -> Option { let typ = self.resolve_type(constraint.typ.clone()); let trait_bound = self.resolve_trait_bound(&constraint.trait_bound)?; + + self.add_trait_bound_to_scope( + constraint.trait_bound.trait_path.span, + &typ, + &trait_bound, + trait_bound.trait_id, + ); + Some(TraitConstraint { typ, trait_bound }) } @@ -800,10 +891,13 @@ impl<'context> Elaborator<'context> { let has_inline_attribute = has_no_predicates_attribute || should_fold; let is_pub_allowed = self.pub_allowed(func, in_contract); self.add_generics(&func.def.generics); + let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); + + let new_generics = self.desugar_trait_constraints(&mut func.def.where_clause); + generics.extend(new_generics.into_iter().map(|generic| generic.type_var)); let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); - let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); let mut parameters = Vec::new(); let mut parameter_types = Vec::new(); let mut parameter_idents = Vec::new(); @@ -874,6 +968,9 @@ impl<'context> Elaborator<'context> { None }; + // Remove the traits assumed by `resolve_trait_constraints` from scope + self.remove_trait_constraints_from_scope(&trait_constraints); + let meta = FuncMeta { name: name_ident, kind: func.kind, @@ -1013,10 +1110,10 @@ impl<'context> Elaborator<'context> { } } - fn add_trait_constraints_to_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn add_trait_constraints_to_scope(&mut self, constraints: &[TraitConstraint], span: Span) { + for constraint in constraints { self.add_trait_bound_to_scope( - func_meta, + span, &constraint.typ, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1030,7 +1127,7 @@ impl<'context> Elaborator<'context> { let self_type = self.self_type.clone().expect("Expected a self type if there's a current trait"); self.add_trait_bound_to_scope( - func_meta, + span, &self_type, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1038,8 +1135,8 @@ impl<'context> Elaborator<'context> { } } - fn remove_trait_constraints_from_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn remove_trait_constraints_from_scope(&mut self, constraints: &[TraitConstraint]) { + for constraint in constraints { self.interner .remove_assumed_trait_implementations_for_trait(constraint.trait_bound.trait_id); } @@ -1052,7 +1149,7 @@ impl<'context> Elaborator<'context> { fn add_trait_bound_to_scope( &mut self, - func_meta: &FuncMeta, + span: Span, object: &Type, trait_bound: &ResolvedTraitBound, starting_trait_id: TraitId, @@ -1064,7 +1161,6 @@ impl<'context> Elaborator<'context> { if let Some(the_trait) = self.interner.try_get_trait(trait_id) { let trait_name = the_trait.name.to_string(); let typ = object.clone(); - let span = func_meta.location.span; self.push_err(TypeCheckError::UnneededTraitConstraint { trait_name, typ, span }); } } @@ -1081,12 +1177,7 @@ impl<'context> Elaborator<'context> { let parent_trait_bound = self.instantiate_parent_trait_bound(trait_bound, &parent_trait_bound); - self.add_trait_bound_to_scope( - func_meta, - object, - &parent_trait_bound, - starting_trait_id, - ); + self.add_trait_bound_to_scope(span, object, &parent_trait_bound, starting_trait_id); } } } @@ -1316,15 +1407,22 @@ impl<'context> Elaborator<'context> { self.generics = trait_impl.resolved_generics.clone(); let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause); + self.remove_trait_constraints_from_scope(&where_clause); self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); let span = trait_impl.object_type.span; self.declare_methods_on_struct(Some(trait_id), &mut trait_impl.methods, span); + let trait_visibility = self.interner.get_trait(trait_id).visibility; + let methods = trait_impl.methods.function_ids(); for func_id in &methods { self.interner.set_function_trait(*func_id, self_type.clone(), trait_id); + + // A trait impl method has the same visibility as its trait + let modifiers = self.interner.function_modifiers_mut(func_id); + modifiers.visibility = trait_visibility; } let trait_generics = trait_impl.resolved_trait_generics.clone(); @@ -1805,6 +1903,17 @@ impl<'context> Elaborator<'context> { self.add_generics(&trait_impl.generics); trait_impl.resolved_generics = self.generics.clone(); + let new_generics = self.desugar_trait_constraints(&mut trait_impl.where_clause); + for new_generic in new_generics { + trait_impl.resolved_generics.push(new_generic.clone()); + self.generics.push(new_generic); + } + + // We need to resolve the where clause before any associated types to be + // able to resolve trait as type syntax, eg. `` in case there + // is a where constraint for `T: Foo`. + let constraints = self.resolve_trait_constraints(&trait_impl.where_clause); + for (_, _, method) in trait_impl.methods.functions.iter_mut() { // Attach any trait constraints on the impl to the function method.def.where_clause.append(&mut trait_impl.where_clause.clone()); @@ -1817,17 +1926,20 @@ impl<'context> Elaborator<'context> { let impl_id = self.interner.next_trait_impl_id(); self.current_trait_impl = Some(impl_id); - // Fetch trait constraints here + let path_span = trait_impl.trait_path.span; let (ordered_generics, named_generics) = trait_impl .trait_id .map(|trait_id| { - self.resolve_type_args(trait_generics, trait_id, trait_impl.trait_path.span) + // Check for missing generics & associated types for the trait being implemented + self.resolve_trait_args_from_trait_impl(trait_generics, trait_id, path_span) }) .unwrap_or_default(); trait_impl.resolved_trait_generics = ordered_generics; self.interner.set_associated_types_for_impl(impl_id, named_generics); + self.remove_trait_constraints_from_scope(&constraints); + let self_type = self.resolve_type(unresolved_type); self.self_type = Some(self_type.clone()); trait_impl.methods.self_type = Some(self_type); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs index 242f5f0b496..6a672866d7e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -751,7 +751,11 @@ impl<'context> Elaborator<'context> { let function = self.interner.function_meta(&function); for mut constraint in function.trait_constraints.clone() { constraint.apply_bindings(&bindings); - self.push_trait_constraint(constraint, expr_id); + + self.push_trait_constraint( + constraint, expr_id, + false, // This constraint shouldn't lead to choosing a trait impl method + ); } } } @@ -767,7 +771,11 @@ impl<'context> Elaborator<'context> { // Currently only one impl can be selected per expr_id, so this // constraint needs to be pushed after any other constraints so // that monomorphization can resolve this trait method to the correct impl. - self.push_trait_constraint(method.constraint, expr_id); + self.push_trait_constraint( + method.constraint, + expr_id, + true, // this constraint should lead to choosing a trait impl method + ); } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/traits.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/traits.rs index a10939dde16..bbc1214de9e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/traits.rs @@ -21,7 +21,7 @@ use crate::{ use super::Elaborator; impl<'context> Elaborator<'context> { - pub fn collect_traits(&mut self, traits: &BTreeMap) { + pub fn collect_traits(&mut self, traits: &mut BTreeMap) { for (trait_id, unresolved_trait) in traits { self.local_module = unresolved_trait.module_id; @@ -39,8 +39,13 @@ impl<'context> Elaborator<'context> { &resolved_generics, ); + let new_generics = + this.desugar_trait_constraints(&mut unresolved_trait.trait_def.where_clause); + this.generics.extend(new_generics); + let where_clause = this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause); + this.remove_trait_constraints_from_scope(&where_clause); // Each associated type in this trait is also an implicit generic for associated_type in &this.interner.get_trait(*trait_id).associated_types { @@ -56,6 +61,7 @@ impl<'context> Elaborator<'context> { this.interner.update_trait(*trait_id, |trait_def| { trait_def.set_trait_bounds(resolved_trait_bounds); trait_def.set_where_clause(where_clause); + trait_def.set_visibility(unresolved_trait.trait_def.visibility); }); let methods = this.resolve_trait_methods(*trait_id, unresolved_trait); @@ -126,7 +132,7 @@ impl<'context> Elaborator<'context> { let func_id = unresolved_trait.method_ids[&name.0.contents]; let mut where_clause = where_clause.to_vec(); - // Attach any trait constraints on the trait to the function + // Attach any trait constraints on the trait to the function, where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); this.resolve_trait_function( diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs index 72e46d36c2c..8fa0b210605 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs @@ -310,11 +310,32 @@ impl<'context> Elaborator<'context> { } } + /// Identical to `resolve_type_args` but does not allow + /// associated types to be elided since trait impls must specify them. + pub(super) fn resolve_trait_args_from_trait_impl( + &mut self, + args: GenericTypeArgs, + item: TraitId, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, false) + } + pub(super) fn resolve_type_args( + &mut self, + args: GenericTypeArgs, + item: impl Generic, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, true) + } + + pub(super) fn resolve_type_args_inner( &mut self, mut args: GenericTypeArgs, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> (Vec, Vec) { let expected_kinds = item.generics(self.interner); @@ -336,7 +357,12 @@ impl<'context> Elaborator<'context> { let mut associated = Vec::new(); if item.accepts_named_type_args() { - associated = self.resolve_associated_type_args(args.named_args, item, span); + associated = self.resolve_associated_type_args( + args.named_args, + item, + span, + allow_implicit_named_args, + ); } else if !args.named_args.is_empty() { let item_kind = item.item_kind(); self.push_err(ResolverError::NamedTypeArgs { span, item_kind }); @@ -350,6 +376,7 @@ impl<'context> Elaborator<'context> { args: Vec<(Ident, UnresolvedType)>, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> Vec { let mut seen_args = HashMap::default(); let mut required_args = item.named_generics(self.interner); @@ -379,11 +406,19 @@ impl<'context> Elaborator<'context> { resolved.push(NamedType { name, typ }); } - // Anything that hasn't been removed yet is missing + // Anything that hasn't been removed yet is missing. + // Fill it in to avoid a panic if we allow named args to be elided, otherwise error. for generic in required_args { - let item = item.item_name(self.interner); let name = generic.name.clone(); - self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + + if allow_implicit_named_args { + let name = Ident::new(name.as_ref().clone(), span); + let typ = self.interner.next_type_variable(); + resolved.push(NamedType { name, typ }); + } else { + let item = item.item_name(self.interner); + self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + } } resolved @@ -1434,12 +1469,8 @@ impl<'context> Elaborator<'context> { .filter_map(|trait_id| { let trait_ = self.interner.get_trait(*trait_id); let trait_name = &trait_.name; - let Some(map) = module_data.scope().types().get(trait_name) else { - return None; - }; - let Some(imported_item) = map.get(&None) else { - return None; - }; + let map = module_data.scope().types().get(trait_name)?; + let imported_item = map.get(&None)?; if imported_item.0 == ModuleDefId::TraitId(*trait_id) { Some((*trait_id, trait_name)) } else { @@ -1814,6 +1845,7 @@ impl<'context> Elaborator<'context> { (expr_span, empty_function) } + #[allow(clippy::too_many_arguments)] pub fn verify_trait_constraint( &mut self, object_type: &Type, @@ -1821,6 +1853,7 @@ impl<'context> Elaborator<'context> { trait_generics: &[Type], associated_types: &[NamedType], function_ident_id: ExprId, + select_impl: bool, span: Span, ) { match self.interner.lookup_trait_implementation( @@ -1830,7 +1863,9 @@ impl<'context> Elaborator<'context> { associated_types, ) { Ok(impl_kind) => { - self.interner.select_impl_for_expression(function_ident_id, impl_kind); + if select_impl { + self.interner.select_impl_for_expression(function_ident_id, impl_kind); + } } Err(error) => self.push_trait_constraint_error(object_type, error, span), } @@ -1904,10 +1939,15 @@ impl<'context> Elaborator<'context> { /// Push a trait constraint into the current FunctionContext to be solved if needed /// at the end of the earlier of either the current function or the current comptime scope. - pub fn push_trait_constraint(&mut self, constraint: TraitConstraint, expr_id: ExprId) { + pub fn push_trait_constraint( + &mut self, + constraint: TraitConstraint, + expr_id: ExprId, + select_impl: bool, + ) { let context = self.function_context.last_mut(); let context = context.expect("The function_context stack should always be non-empty"); - context.trait_constraints.push((constraint, expr_id)); + context.trait_constraints.push((constraint, expr_id, select_impl)); } pub fn bind_generics_from_trait_constraint( @@ -1977,7 +2017,7 @@ pub(crate) fn bind_named_generics( args: &[NamedType], bindings: &mut TypeBindings, ) { - assert_eq!(params.len(), args.len()); + assert!(args.len() <= params.len()); for arg in args { let i = params @@ -1988,6 +2028,10 @@ pub(crate) fn bind_named_generics( let param = params.swap_remove(i); bind_generic(¶m, &arg.typ, bindings); } + + for unbound_param in params { + bind_generic(&unbound_param, &Type::Error, bindings); + } } fn bind_generic(param: &ResolvedGeneric, arg: &Type, bindings: &mut TypeBindings) { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 6af356d8c47..ae0da5ddf54 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -183,7 +183,10 @@ impl<'local, 'context> Interpreter<'local, 'context> { "struct_def_add_generic" => struct_def_add_generic(interner, arguments, location), "struct_def_as_type" => struct_def_as_type(interner, arguments, location), "struct_def_eq" => struct_def_eq(arguments, location), - "struct_def_fields" => struct_def_fields(interner, arguments, location), + "struct_def_fields" => struct_def_fields(interner, arguments, location, call_stack), + "struct_def_fields_as_written" => { + struct_def_fields_as_written(interner, arguments, location) + } "struct_def_generics" => struct_def_generics(interner, arguments, location), "struct_def_has_named_attribute" => { struct_def_has_named_attribute(interner, arguments, location) @@ -494,12 +497,57 @@ fn struct_def_has_named_attribute( Ok(Value::Bool(has_named_attribute(&name, interner.struct_attributes(&struct_id)))) } -/// fn fields(self) -> [(Quoted, Type)] -/// Returns (name, type) pairs of each field of this StructDefinition +/// fn fields(self, generic_args: [Type]) -> [(Quoted, Type)] +/// Returns (name, type) pairs of each field of this StructDefinition. +/// Applies the given generic arguments to each field. fn struct_def_fields( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, location: Location, + call_stack: &im::Vector, +) -> IResult { + let (typ, generic_args) = check_two_arguments(arguments, location)?; + let struct_id = get_struct(typ)?; + let struct_def = interner.get_struct(struct_id); + let struct_def = struct_def.borrow(); + + let args_location = generic_args.1; + let generic_args = get_slice(interner, generic_args)?.0; + let generic_args = try_vecmap(generic_args, |arg| get_type((arg, args_location)))?; + + let actual = generic_args.len(); + let expected = struct_def.generics.len(); + if actual != expected { + let s = if expected == 1 { "" } else { "s" }; + let was_were = if actual == 1 { "was" } else { "were" }; + let message = Some(format!("`StructDefinition::fields` expected {expected} generic{s} for `{}` but {actual} {was_were} given", struct_def.name)); + let location = args_location; + let call_stack = call_stack.clone(); + return Err(InterpreterError::FailingConstraint { message, location, call_stack }); + } + + let mut fields = im::Vector::new(); + + for (field_name, field_type) in struct_def.get_fields(&generic_args) { + let name = Value::Quoted(Rc::new(vec![Token::Ident(field_name)])); + fields.push_back(Value::Tuple(vec![name, Value::Type(field_type)])); + } + + let typ = Type::Slice(Box::new(Type::Tuple(vec![ + Type::Quoted(QuotedType::Quoted), + Type::Quoted(QuotedType::Type), + ]))); + Ok(Value::Slice(fields, typ)) +} + +/// fn fields_as_written(self) -> [(Quoted, Type)] +/// Returns (name, type) pairs of each field of this StructDefinition. +/// +/// Note that any generic arguments won't be applied: if you need them to be, use `fields`. +fn struct_def_fields_as_written( + interner: &mut NodeInterner, + arguments: Vec<(Value, Location)>, + location: Location, ) -> IResult { let argument = check_one_argument(arguments, location)?; let struct_id = get_struct(argument)?; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index ec13cb2db15..ead6a801ba7 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -1228,9 +1228,10 @@ pub(crate) fn collect_trait_impl_items( for item in std::mem::take(&mut trait_impl.items) { match item.item.kind { TraitImplItemKind::Function(mut impl_method) => { - // Regardless of what visibility was on the source code, treat it as public - // (a warning is produced during parsing for this) - impl_method.def.visibility = ItemVisibility::Public; + // Set the impl method visibility as temporarily private. + // Eventually when we find out what trait is this impl for we'll set it + // to the trait's visibility. + impl_method.def.visibility = ItemVisibility::Private; let func_id = interner.push_empty_fn(); let location = Location::new(impl_method.span(), file_id); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/visibility.rs index 5e6dffa56e4..557f799df89 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/visibility.rs @@ -138,18 +138,29 @@ pub fn method_call_is_visible( ItemVisibility::Public => true, ItemVisibility::PublicCrate | ItemVisibility::Private => { let func_meta = interner.function_meta(&func_id); - if let Some(struct_id) = func_meta.struct_id { - return struct_member_is_visible( - struct_id, + + if let Some(trait_id) = func_meta.trait_id { + return trait_member_is_visible( + trait_id, modifiers.visibility, current_module, def_maps, ); } - if let Some(trait_id) = func_meta.trait_id { + if let Some(trait_impl_id) = func_meta.trait_impl { + let trait_impl = interner.get_trait_implementation(trait_impl_id); return trait_member_is_visible( - trait_id, + trait_impl.borrow().trait_id, + modifiers.visibility, + current_module, + def_maps, + ); + } + + if let Some(struct_id) = func_meta.struct_id { + return struct_member_is_visible( + struct_id, modifiers.visibility, current_module, def_maps, diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/traits.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir_def/traits.rs index 6fd3c4f7a24..ff0cac027b1 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir_def/traits.rs @@ -1,7 +1,7 @@ use iter_extended::vecmap; use rustc_hash::FxHashMap as HashMap; -use crate::ast::{Ident, NoirFunction}; +use crate::ast::{Ident, ItemVisibility, NoirFunction}; use crate::hir::type_check::generics::TraitGenerics; use crate::ResolvedGeneric; use crate::{ @@ -66,6 +66,7 @@ pub struct Trait { pub name: Ident, pub generics: Generics, pub location: Location, + pub visibility: ItemVisibility, /// When resolving the types of Trait elements, all references to `Self` resolve /// to this TypeVariable. Then when we check if the types of trait impl elements @@ -160,6 +161,10 @@ impl Trait { self.where_clause = where_clause; } + pub fn set_visibility(&mut self, visibility: ItemVisibility) { + self.visibility = visibility; + } + pub fn find_method(&self, name: &str) -> Option { for (idx, method) in self.methods.iter().enumerate() { if &method.name == name { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs index a2b1d7fc8f0..0ec033a399b 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -729,6 +729,7 @@ impl NodeInterner { crate_id: unresolved_trait.crate_id, location: Location::new(unresolved_trait.trait_def.span, unresolved_trait.file_id), generics, + visibility: ItemVisibility::Private, self_type_typevar: TypeVariable::unbound(self.next_type_variable_id(), Kind::Normal), methods: Vec::new(), method_ids: unresolved_trait.method_ids.clone(), @@ -2268,31 +2269,26 @@ impl Methods { } /// Iterate through each method, starting with the direct methods - pub fn iter(&self) -> impl Iterator)> + '_ { - let trait_impl_methods = self.trait_impl_methods.iter().map(|m| (m.method, &m.typ)); - let direct = self.direct.iter().copied().map(|func_id| { - let typ: &Option = &None; - (func_id, typ) - }); + pub fn iter(&self) -> impl Iterator, Option)> { + let trait_impl_methods = + self.trait_impl_methods.iter().map(|m| (m.method, m.typ.as_ref(), Some(m.trait_id))); + let direct = self.direct.iter().copied().map(|func_id| (func_id, None, None)); direct.chain(trait_impl_methods) } - /// Select the 1 matching method with an object type matching `typ` - pub fn find_matching_method( - &self, - typ: &Type, + pub fn find_matching_methods<'a>( + &'a self, + typ: &'a Type, has_self_param: bool, - interner: &NodeInterner, - ) -> Option { - // When adding methods we always check they do not overlap, so there should be - // at most 1 matching method in this list. - for (method, method_type) in self.iter() { + interner: &'a NodeInterner, + ) -> impl Iterator)> + 'a { + self.iter().filter_map(move |(method, method_type, trait_id)| { if Self::method_matches(typ, has_self_param, method, method_type, interner) { - return Some(method); + Some((method, trait_id)) + } else { + None } - } - - None + }) } pub fn find_direct_method( @@ -2302,7 +2298,7 @@ impl Methods { interner: &NodeInterner, ) -> Option { for method in &self.direct { - if Self::method_matches(typ, has_self_param, *method, &None, interner) { + if Self::method_matches(typ, has_self_param, *method, None, interner) { return Some(*method); } } @@ -2320,7 +2316,7 @@ impl Methods { for trait_impl_method in &self.trait_impl_methods { let method = trait_impl_method.method; - let method_type = &trait_impl_method.typ; + let method_type = trait_impl_method.typ.as_ref(); let trait_id = trait_impl_method.trait_id; if Self::method_matches(typ, has_self_param, method, method_type, interner) { @@ -2335,7 +2331,7 @@ impl Methods { typ: &Type, has_self_param: bool, method: FuncId, - method_type: &Option, + method_type: Option<&Type>, interner: &NodeInterner, ) -> bool { match interner.function_meta(&method).typ.instantiate(interner).0 { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index c1b86b5dcf7..637b15e7197 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -3955,3 +3955,26 @@ fn mutable_self_call() { "#; assert_no_errors(src); } + +#[test] +fn checks_visibility_of_trait_related_to_trait_impl_on_method_call() { + let src = r#" + mod moo { + pub struct Bar {} + } + + trait Foo { + fn foo(self); + } + + impl Foo for moo::Bar { + fn foo(self) {} + } + + fn main() { + let bar = moo::Bar {}; + bar.foo(); + } + "#; + assert_no_errors(src); +} diff --git a/noir/noir-repo/docs/docs/noir/concepts/traits.md b/noir/noir-repo/docs/docs/noir/concepts/traits.md index 1b232dcf8ab..13818ecffac 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/traits.md +++ b/noir/noir-repo/docs/docs/noir/concepts/traits.md @@ -252,36 +252,33 @@ impl MyTrait for Field { Since associated constants can also be used in a type position, its values are limited to only other expression kinds allowed in numeric generics. -Note that currently all associated types and constants must be explicitly specified in a trait constraint. -If we leave out any, we'll get an error that we're missing one: +When writing a trait constraint, you can specify all associated types and constants explicitly if +you wish: ```rust -// Error! Constraint is missing associated constant for `Bar` -fn foo(x: T) where T: MyTrait { +fn foo(x: T) where T: MyTrait { ... } ``` -Because all associated types and constants must be explicitly specified, they are essentially named generics, -although this is set to change in the future. Future versions of Noir will allow users to elide associated types -in trait constraints similar to Rust. When this is done, you may still refer to their value with the `::AssociatedType` -syntax: +Or you can also elide them since there should only be one `Foo` and `Bar` for a given implementation +of `MyTrait` for a type: ```rust -// Only valid in future versions of Noir: fn foo(x: T) where T: MyTrait { - let _: ::Foo = ...; + ... } ``` -The type as trait syntax is possible in Noir today but is less useful when each type must be explicitly specified anyway: +If you elide associated types, you can still refer to them via the type as trait syntax ``: ```rust -fn foo(x: T) where T: MyTrait { - // Works, but could just use F directly - let _: >::Foo = ...; - - let _: F = ...; +fn foo(x: T) where + T: MyTrait, + ::Foo: Default + Eq +{ + let foo_value: ::Foo = Default::default(); + assert_eq(foo_value, foo_value); } ``` diff --git a/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md b/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md index 535708e0353..8c6e50ba23a 100644 --- a/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md +++ b/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md @@ -66,7 +66,18 @@ comptime fn example(foo: StructDefinition) { #include_code fields noir_stdlib/src/meta/struct_def.nr rust -Returns each field of this struct as a pair of (field name, field type). +Returns (name, type) pairs of each field in this struct. +Any generic types used in each field type is automatically substituted with the +provided generic arguments. + +### fields_as_written + +#include_code fields_as_written noir_stdlib/src/meta/struct_def.nr rust + +Returns (name, type) pairs of each field in this struct. Each type is as-is +with any generic arguments unchanged. Unless the field types are not needed, +users should generally prefer to use `StructDefinition::fields` over this +function if possible. ### has_named_attribute diff --git a/noir/noir-repo/noir_stdlib/src/cmp.nr b/noir/noir-repo/noir_stdlib/src/cmp.nr index ae515150a4d..7f19594c30e 100644 --- a/noir/noir-repo/noir_stdlib/src/cmp.nr +++ b/noir/noir-repo/noir_stdlib/src/cmp.nr @@ -12,7 +12,7 @@ comptime fn derive_eq(s: StructDefinition) -> Quoted { let signature = quote { fn eq(_self: Self, _other: Self) -> bool }; let for_each_field = |name| quote { (_self.$name == _other.$name) }; let body = |fields| { - if s.fields().len() == 0 { + if s.fields_as_written().len() == 0 { quote { true } } else { fields diff --git a/noir/noir-repo/noir_stdlib/src/collections/umap.nr b/noir/noir-repo/noir_stdlib/src/collections/umap.nr index fb15d532bc5..bcb9759b4db 100644 --- a/noir/noir-repo/noir_stdlib/src/collections/umap.nr +++ b/noir/noir-repo/noir_stdlib/src/collections/umap.nr @@ -114,8 +114,7 @@ impl UHashMap { { // docs:end:contains_key /// Safety: unconstrained context - unsafe { self.get(key) } - .is_some() + unsafe { self.get(key) }.is_some() } // Returns true if the map contains no elements. diff --git a/noir/noir-repo/noir_stdlib/src/meta/mod.nr b/noir/noir-repo/noir_stdlib/src/meta/mod.nr index f6a1f4838ee..21c1b14cc96 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/mod.nr @@ -101,7 +101,7 @@ pub comptime fn make_trait_impl( let where_clause = s.generics().map(|name| quote { $name: $trait_name }).join(quote {,}); // `for_each_field(field1) $join_fields_with for_each_field(field2) $join_fields_with ...` - let fields = s.fields().map(|f: (Quoted, Type)| { + let fields = s.fields_as_written().map(|f: (Quoted, Type)| { let name = f.0; for_each_field(name) }); @@ -155,7 +155,7 @@ mod tests { comptime fn derive_field_count(s: StructDefinition) -> Quoted { let typ = s.as_type(); - let field_count = s.fields().len(); + let field_count = s.fields_as_written().len(); quote { impl FieldCount for $typ { fn field_count() -> u32 { @@ -174,7 +174,7 @@ mod tests { comptime fn assert_field_is_type(s: StructDefinition, typ: Type) { // Assert the first field in `s` has type `typ` - let fields = s.fields(); + let fields = s.fields([]); assert_eq(fields[0].1, typ); } // docs:end:annotation-arguments-example diff --git a/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr b/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr index ba5d0289e73..0e64a537f1f 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr @@ -27,13 +27,23 @@ impl StructDefinition { pub comptime fn generics(self) -> [Type] {} // docs:end:generics - /// Returns (name, type) pairs of each field in this struct. Each type is as-is - /// with any generic arguments unchanged. + /// Returns (name, type) pairs of each field in this struct. + /// Any generic types used in each field type is automatically substituted with the + /// provided generic arguments. #[builtin(struct_def_fields)] // docs:start:fields - pub comptime fn fields(self) -> [(Quoted, Type)] {} + pub comptime fn fields(self, generic_args: [Type]) -> [(Quoted, Type)] {} // docs:end:fields + /// Returns (name, type) pairs of each field in this struct. Each type is as-is + /// with any generic arguments unchanged. Unless the field types are not needed, + /// users should generally prefer to use `StructDefinition::fields` over this + /// function if possible. + #[builtin(struct_def_fields_as_written)] + // docs:start:fields_as_written + pub comptime fn fields_as_written(self) -> [(Quoted, Type)] {} + // docs:end:fields_as_written + #[builtin(struct_def_module)] // docs:start:module pub comptime fn module(self) -> Module {} diff --git a/noir/noir-repo/test_programs/compilation_report.sh b/noir/noir-repo/test_programs/compilation_report.sh index df15ef00008..786dbd75fe8 100755 --- a/noir/noir-repo/test_programs/compilation_report.sh +++ b/noir/noir-repo/test_programs/compilation_report.sh @@ -18,6 +18,7 @@ fi ITER="1" NUM_ARTIFACTS=${#tests_to_profile[@]} +FLAGS=${FLAGS:- ""} for dir in ${tests_to_profile[@]}; do if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]]; then @@ -37,17 +38,11 @@ for dir in ${tests_to_profile[@]}; do PACKAGE_NAME=$(basename $current_dir) fi - NUM_RUNS=1 + NUM_RUNS=$2 TOTAL_TIME=0 - # Passing a second argument will take an average of five runs - # rather than - if [ "$2" == "1" ]; then - NUM_RUNS=5 - fi - for ((i = 1; i <= NUM_RUNS; i++)); do - NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo compile --force --silence-warnings + NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo compile --force --silence-warnings $FLAGS done TIMES=($(jq -r '. | select(.target == "nargo::cli" and .fields.message == "close") | .fields."time.busy"' ./tmp/*)) diff --git a/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml new file mode 100644 index 00000000000..6b54cbfca6a --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "associated_types_implicit" +type = "bin" +authors = [""] + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Prover.toml b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Prover.toml new file mode 100644 index 00000000000..cab679b4b66 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/Prover.toml @@ -0,0 +1 @@ +a = [[0, 1], [2, 3]] diff --git a/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/src/main.nr new file mode 100644 index 00000000000..d04cef51868 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/associated_types_implicit/src/main.nr @@ -0,0 +1,60 @@ +trait Foo { + type Bar; + + fn foo(self) -> Self::Bar; +} + +impl Foo for u64 { + type Bar = u8; + + fn foo(self) -> Self::Bar { + self as u8 + } +} + +fn main() { + // This currently requires a type annotation to find the impl + let three: u64 = 3; + call_foo(three); + + let x: Option> = Option::some(Option::some(0)); + let x_foo = x.foo(); + assert_eq(x_foo, x_foo); // ensure we don't need an additional type annotation for Bar here + + // The `as u8` is still necessary even though we know the object type, + // otherwise we try to search for `u64: Foo`. + // It seems the `Bar = u8` constraint is still there & checked for, but + // the defaulting of the polymorphic integer occurs first. + assert_eq(x.foo(), 0 as u8); +} + +// Ensure we can use `::Bar: Eq` in a function's where clause +fn call_foo(x: T) +where + T: Foo, + ::Bar: Eq, +{ + let y = x.foo(); + assert_eq(y, y); +} + +// Ensure we can use `::Bar: Eq` in a trait impl's where clause +impl Foo for Option +where + T: Foo, + ::Bar: Eq, +{ + type Bar = ::Bar; + + fn foo(self) -> Self::Bar { + self.unwrap().foo() + } +} + +// Ensure we can use `::Bar: Eq` in a trait's where clause +// TODO: Not working, see issue #7024 +// trait Baz where +// T: Foo, +// ::Bar: Eq {} +// +// impl Baz for u32 {} diff --git a/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr index 686ac26025d..0b71d44f016 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr @@ -12,7 +12,7 @@ pub struct I32AndField { comptime fn my_comptime_fn(typ: StructDefinition) { let _ = typ.as_type(); assert_eq(typ.generics().len(), 3); - assert_eq(typ.fields().len(), 2); + assert_eq(typ.fields_as_written().len(), 2); assert_eq(typ.name(), quote { MyType }); } @@ -44,4 +44,22 @@ mod foo { // docs:end:add-generic-example } -fn main() {} +fn main() { + comptime { + let typ = quote { MyType }.as_type(); + let (struct_def, generics) = typ.as_struct().unwrap(); + + let fields = struct_def.fields(generics); + assert_eq(fields.len(), 2); + + let (field1_name, field1_type) = fields[0]; + let (field2_name, field2_type) = fields[1]; + + assert_eq(field1_name, quote { field1 }); + assert_eq(field2_name, quote { field2 }); + + // Ensure .fields(generics) actually performs substitutions on generics + assert_eq(field1_type, quote { [i8; 10] }.as_type()); + assert_eq(field2_type, quote { (i16, i32) }.as_type()); + } +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/comptime_type/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/comptime_type/src/main.nr index 887690cb6cb..6a2453ff0f2 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/comptime_type/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/comptime_type/src/main.nr @@ -100,7 +100,7 @@ fn main() { let foo = Foo { x: 0 }; let foo_type = type_of(foo); let (struct_definition, generics) = foo_type.as_struct().unwrap(); - let fields = struct_definition.fields(); + let fields = struct_definition.fields(generics); assert_eq(fields.len(), 1); assert_eq(generics.len(), 1); diff --git a/noir/noir-repo/test_programs/compile_success_empty/derive_impl/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/derive_impl/src/main.nr index 4396169235d..fe7a7140280 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/derive_impl/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/derive_impl/src/main.nr @@ -7,7 +7,7 @@ comptime fn derive_default(typ: StructDefinition) -> Quoted { ); let type_name = typ.as_type(); - let fields = typ.fields(); + let fields = typ.fields_as_written(); let fields = join(make_field_exprs(fields)); diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/regression_7038/Nargo.toml new file mode 100644 index 00000000000..3c874d4b6e8 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/regression_7038/src/main.nr new file mode 100644 index 00000000000..793a3f60807 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + fn one(); +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/Nargo.toml new file mode 100644 index 00000000000..f4f23683eb8 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_2" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/src/main.nr new file mode 100644 index 00000000000..6a116bb0722 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_2/src/main.nr @@ -0,0 +1,39 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + // The difference between this and regression_7083 is that here + // this is a default method. + fn one() {} +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params {} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/Nargo.toml new file mode 100644 index 00000000000..65bc946c559 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_3" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/src/main.nr new file mode 100644 index 00000000000..1b6bf0b72d5 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_3/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait { + // The difference between this and regression_7038 and regression_7038_2 is that + // here the where clause is on the method, not the trait + fn one() + where + BigNum: BigNumTrait; +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/Nargo.toml new file mode 100644 index 00000000000..435c8094c70 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_4" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/src/main.nr new file mode 100644 index 00000000000..524f05a5022 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_7038_4/src/main.nr @@ -0,0 +1,29 @@ +// This program is a reduction of regression_7038_3 that led to a monomorphizer crash +trait BigNumTrait {} + +trait CurveParamsTrait { + fn one() + where + BigNum: BigNumTrait; +} + +pub struct MyBigNum; + +impl BigNumTrait for MyBigNum {} + +pub struct Params; +impl CurveParamsTrait for Params { + + fn one() {} +} + +fn foo() +where + C: CurveParamsTrait, +{ + let _ = C::one(); +} + +fn main() { + foo::(); +} diff --git a/noir/noir-repo/test_programs/execution_report.sh b/noir/noir-repo/test_programs/execution_report.sh index b929c367f7d..827b7806d37 100755 --- a/noir/noir-repo/test_programs/execution_report.sh +++ b/noir/noir-repo/test_programs/execution_report.sh @@ -46,12 +46,8 @@ for dir in ${tests_to_profile[@]}; do fi - NUM_RUNS=1 + NUM_RUNS=$2 TOTAL_TIME=0 - - if [ "$2" == "1" ]; then - NUM_RUNS=5 - fi for ((i = 1; i <= NUM_RUNS; i++)); do NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo execute --silence-warnings diff --git a/noir/noir-repo/test_programs/gates_report_brillig.sh b/noir/noir-repo/test_programs/gates_report_brillig.sh index 7343857a3c5..3f2a4542735 100755 --- a/noir/noir-repo/test_programs/gates_report_brillig.sh +++ b/noir/noir-repo/test_programs/gates_report_brillig.sh @@ -28,6 +28,6 @@ done echo "]" >> Nargo.toml -nargo info --force-brillig --json | jq -r ".programs[].functions = []" > gates_report_brillig.json +nargo info --silence-warnings --force-brillig --json --inliner-aggressiveness $1 | jq -r ".programs[].functions = []" > gates_report_brillig.json rm Nargo.toml diff --git a/noir/noir-repo/test_programs/gates_report_brillig_execution.sh b/noir/noir-repo/test_programs/gates_report_brillig_execution.sh index 219fbb5c11a..c6a904f8b93 100755 --- a/noir/noir-repo/test_programs/gates_report_brillig_execution.sh +++ b/noir/noir-repo/test_programs/gates_report_brillig_execution.sh @@ -42,6 +42,6 @@ done echo "]" >> Nargo.toml -nargo info --profile-execution --json | jq -r ".programs[].functions = []" > gates_report_brillig_execution.json +nargo info --silence-warnings --profile-execution --json --inliner-aggressiveness $1 | jq -r ".programs[].functions = []" > gates_report_brillig_execution.json rm Nargo.toml diff --git a/noir/noir-repo/test_programs/memory_report.sh b/noir/noir-repo/test_programs/memory_report.sh index e501464c198..a5c13f58b0f 100755 --- a/noir/noir-repo/test_programs/memory_report.sh +++ b/noir/noir-repo/test_programs/memory_report.sh @@ -19,6 +19,7 @@ fi FIRST="1" +FLAGS=${FLAGS:- ""} echo "{\"memory_reports\": [ " > memory_report.json for test_name in ${tests_to_profile[@]}; do @@ -35,12 +36,12 @@ for test_name in ${tests_to_profile[@]}; do test_name=$(basename $current_dir) fi - COMMAND="compile --force --silence-warnings" + COMMAND="compile --force --silence-warnings $FLAGS" if [ "$2" == "1" ]; then COMMAND="execute --silence-warnings" fi - heaptrack --output $current_dir/$test_name"_heap" $NARGO $COMMAND + heaptrack --output $current_dir/$test_name"_heap" $NARGO $COMMAND if test -f $current_dir/$test_name"_heap.gz"; then heaptrack --analyze $current_dir/$test_name"_heap.gz" > $current_dir/$test_name"_heap_analysis.txt" diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs index 38cc6bddf64..24ed327393d 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs @@ -33,6 +33,7 @@ use super::{process_request, to_lsp_location}; mod fill_struct_fields; mod implement_missing_members; mod import_or_qualify; +mod import_trait; mod remove_bang_from_call; mod remove_unused_import; mod tests; @@ -285,6 +286,8 @@ impl<'a> Visitor for CodeActionFinder<'a> { self.remove_bang_from_call(method_call.method_name.span()); } + self.import_trait_in_method_call(method_call); + true } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs new file mode 100644 index 00000000000..1e731aa563b --- /dev/null +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs @@ -0,0 +1,252 @@ +use noirc_errors::Location; +use noirc_frontend::{ + ast::MethodCallExpression, + hir::{def_map::ModuleDefId, resolution::visibility::trait_member_is_visible}, + hir_def::traits::Trait, + node_interner::ReferenceId, +}; + +use crate::{ + modules::relative_module_full_path, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, +}; + +use super::CodeActionFinder; + +impl<'a> CodeActionFinder<'a> { + pub(super) fn import_trait_in_method_call(&mut self, method_call: &MethodCallExpression) { + // First see if the method name already points to a function. + let name_location = Location::new(method_call.method_name.span(), self.file); + if let Some(ReferenceId::Function(func_id)) = self.interner.find_referenced(name_location) { + // If yes, it could be that the compiler is issuing a warning because there's + // only one possible trait that the method could be coming from, but it's not imported + let func_meta = self.interner.function_meta(&func_id); + let Some(trait_impl_id) = func_meta.trait_impl else { + return; + }; + + let trait_impl = self.interner.get_trait_implementation(trait_impl_id); + let trait_id = trait_impl.borrow().trait_id; + let trait_ = self.interner.get_trait(trait_id); + + // Check if the trait is currently imported. If so, no need to suggest anything + let module_data = + &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; + if !module_data.scope().find_name(&trait_.name).is_none() { + return; + } + + self.push_import_trait_code_action(trait_); + return; + } + + // Find out the type of the object + let object_location = Location::new(method_call.object.span, self.file); + let Some(typ) = self.interner.type_at_location(object_location) else { + return; + }; + + for (func_id, trait_id) in + self.interner.lookup_trait_methods(&typ, &method_call.method_name.0.contents, true) + { + let visibility = self.interner.function_modifiers(&func_id).visibility; + if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) { + continue; + } + + let trait_ = self.interner.get_trait(trait_id); + self.push_import_trait_code_action(trait_); + } + } + + fn push_import_trait_code_action(&mut self, trait_: &Trait) { + let trait_id = trait_.id; + + let module_def_id = ModuleDefId::TraitId(trait_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); + let Some(module_full_path) = relative_module_full_path( + module_def_id, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + return; + }; + let full_path = format!("{}::{}", module_full_path, trait_.name); + + let title = format!("Import {}", full_path); + + let text_edits = use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + full_path: &full_path, + files: self.files, + file: self.file, + lines: &self.lines, + nesting: self.nesting, + auto_import_line: self.auto_import_line, + }, + &self.use_segment_positions, + ); + + let code_action = self.new_quick_fix_multiple_edits(title, text_edits); + self.code_actions.push(code_action); + } +} + +#[cfg(test)] +mod tests { + use tokio::test; + + use crate::requests::code_action::tests::assert_code_action; + + #[test] + async fn test_import_trait_in_method_call_when_one_option_but_not_in_scope() { + let title = "Import moo::Foo"; + + let src = r#"mod moo { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } +} + +fn main() { + let x: Field = 1; + x.foo>||| CodeActionResponse { ) .await .expect("Could not execute on_code_action_request") - .unwrap() + .expect("Expected to get a CodeActionResponse, got None") } pub(crate) async fn assert_code_action(title: &str, src: &str, expected: &str) { diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion.rs b/noir/noir-repo/tooling/lsp/src/requests/completion.rs index fd6ef60af82..219103388bc 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion.rs @@ -28,6 +28,7 @@ use noirc_frontend::{ def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}, resolution::visibility::{ item_in_module_is_visible, method_call_is_visible, struct_member_is_visible, + trait_member_is_visible, }, }, hir_def::traits::Trait, @@ -245,7 +246,7 @@ impl<'a> NodeFinder<'a> { let mut idents: Vec = Vec::new(); // Find in which ident we are in, and in which part of it - // (it could be that we are completting in the middle of an ident) + // (it could be that we are completing in the middle of an ident) for segment in &path.segments { let ident = &segment.ident; @@ -658,45 +659,54 @@ impl<'a> NodeFinder<'a> { let has_self_param = matches!(function_kind, FunctionKind::SelfType(..)); for (name, methods) in methods_by_name { - let Some(func_id) = - methods.find_matching_method(typ, has_self_param, self.interner).or_else(|| { - // Also try to find a method assuming typ is `&mut typ`: - // we want to suggest methods that take `&mut self` even though a variable might not - // be mutable, so a user can know they need to mark it as mutable. - let typ = Type::MutableReference(Box::new(typ.clone())); - methods.find_matching_method(&typ, has_self_param, self.interner) - }) - else { + if !name_matches(name, prefix) { continue; - }; - - if let Some(struct_id) = struct_id { - let modifiers = self.interner.function_modifiers(&func_id); - let visibility = modifiers.visibility; - if !struct_member_is_visible(struct_id, visibility, self.module_id, self.def_maps) { - continue; - } } - if is_primitive - && !method_call_is_visible( - typ, - func_id, - self.module_id, - self.interner, - self.def_maps, - ) + for (func_id, trait_id) in + methods.find_matching_methods(typ, has_self_param, self.interner) { - continue; - } + if let Some(struct_id) = struct_id { + let modifiers = self.interner.function_modifiers(&func_id); + let visibility = modifiers.visibility; + if !struct_member_is_visible( + struct_id, + visibility, + self.module_id, + self.def_maps, + ) { + continue; + } + } + + if let Some(trait_id) = trait_id { + let modifiers = self.interner.function_modifiers(&func_id); + let visibility = modifiers.visibility; + if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) + { + continue; + } + } + + if is_primitive + && !method_call_is_visible( + typ, + func_id, + self.module_id, + self.interner, + self.def_maps, + ) + { + continue; + } - if name_matches(name, prefix) { let completion_items = self.function_completion_items( name, func_id, function_completion_kind, function_kind, None, // attribute first type + trait_id, self_prefix, ); if !completion_items.is_empty() { @@ -749,6 +759,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind, function_kind, None, // attribute first type + None, // trait_id (we are suggesting methods for `Trait::>|<` so no need to auto-import it) self_prefix, ); if !completion_items.is_empty() { diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs index 49e4474738e..86bb308ee39 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs @@ -10,6 +10,13 @@ use noirc_frontend::{ QuotedType, Type, }; +use crate::{ + modules::relative_module_full_path, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, +}; + use super::{ sort_text::{ crate_or_module_sort_text, default_sort_text, new_sort_text, operator_sort_text, @@ -75,6 +82,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind, function_kind, attribute_first_type.as_ref(), + None, // trait_id false, // self_prefix ), ModuleDefId::TypeId(struct_id) => vec![self.struct_completion_item(name, struct_id)], @@ -144,6 +152,7 @@ impl<'a> NodeFinder<'a> { self.completion_item_with_doc_comments(ReferenceId::Global(global_id), completion_item) } + #[allow(clippy::too_many_arguments)] pub(super) fn function_completion_items( &self, name: &String, @@ -151,6 +160,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, attribute_first_type: Option<&Type>, + trait_id: Option, self_prefix: bool, ) -> Vec { let func_meta = self.interner.function_meta(&func_id); @@ -223,6 +233,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind, function_kind, attribute_first_type, + trait_id, self_prefix, is_macro_call, ) @@ -265,6 +276,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, attribute_first_type: Option<&Type>, + trait_id: Option, self_prefix: bool, is_macro_call: bool, ) -> CompletionItem { @@ -325,7 +337,7 @@ impl<'a> NodeFinder<'a> { completion_item }; - let completion_item = match function_completion_kind { + let mut completion_item = match function_completion_kind { FunctionCompletionKind::Name => completion_item, FunctionCompletionKind::NameAndParameters => { if has_arguments { @@ -336,9 +348,51 @@ impl<'a> NodeFinder<'a> { } }; + self.auto_import_trait_if_trait_method(func_id, trait_id, &mut completion_item); + self.completion_item_with_doc_comments(ReferenceId::Function(func_id), completion_item) } + fn auto_import_trait_if_trait_method( + &self, + func_id: FuncId, + trait_id: Option, + completion_item: &mut CompletionItem, + ) -> Option<()> { + // If this is a trait method, check if the trait is in scope + let trait_ = self.interner.get_trait(trait_id?); + let module_data = + &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; + if !module_data.scope().find_name(&trait_.name).is_none() { + return None; + } + // If not, automatically import it + let current_module_parent_id = self.module_id.parent(self.def_maps); + let module_full_path = relative_module_full_path( + ModuleDefId::FunctionId(func_id), + self.module_id, + current_module_parent_id, + self.interner, + )?; + let full_path = format!("{}::{}", module_full_path, trait_.name); + let mut label_details = completion_item.label_details.clone().unwrap(); + label_details.detail = Some(format!("(use {})", full_path)); + completion_item.label_details = Some(label_details); + completion_item.additional_text_edits = Some(use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + full_path: &full_path, + files: self.files, + file: self.file, + lines: &self.lines, + nesting: self.nesting, + auto_import_line: self.auto_import_line, + }, + &self.use_segment_positions, + )); + + None + } + fn compute_function_insert_text( &self, func_meta: &FuncMeta, diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs index 65157090767..e6cfebddb0c 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs @@ -2879,4 +2879,147 @@ fn main() { let items = get_completions(src).await; assert_eq!(items.len(), 1); } + + #[test] + async fn test_does_not_suggest_trait_function_not_visible() { + let src = r#" + mod moo { + trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } + } + + fn main() { + Field::fooba>|< + } + + "#; + assert_completion(src, vec![]).await; + } + + #[test] + async fn test_suggests_multiple_trait_methods() { + let src = r#" + mod moo { + pub trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } + + pub trait Bar { + fn foobar(); + } + + impl Bar for Field { + fn foobar() {} + } + } + + fn main() { + Field::fooba>|< + } + + "#; + let items = get_completions(src).await; + assert_eq!(items.len(), 2); + } + + #[test] + async fn test_suggests_and_imports_trait_method_without_self() { + let src = r#" +mod moo { + pub trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } +} + +fn main() { + Field::fooba>|< +} + "#; + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + assert_eq!(item.label_details.unwrap().detail, Some("(use moo::Foo)".to_string())); + + let new_code = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + + let expected = r#"use moo::Foo; + +mod moo { + pub trait Foo { + fn foobar(); + } + + impl Foo for Field { + fn foobar() {} + } +} + +fn main() { + Field::fooba +} + "#; + assert_eq!(new_code, expected); + } + + #[test] + async fn test_suggests_and_imports_trait_method_with_self() { + let src = r#" +mod moo { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } +} + +fn main() { + let x: Field = 1; + x.fooba>|< +} + "#; + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + assert_eq!(item.label_details.unwrap().detail, Some("(use moo::Foo)".to_string())); + + let new_code = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + + let expected = r#"use moo::Foo; + +mod moo { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } +} + +fn main() { + let x: Field = 1; + x.fooba +} + "#; + assert_eq!(new_code, expected); + } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/hover.rs b/noir/noir-repo/tooling/lsp/src/requests/hover.rs index 9da24fd1c8a..ef1246d752d 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/hover.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/hover.rs @@ -399,7 +399,10 @@ fn format_function(id: FuncId, args: &ProcessRequestCallbackArgs) -> String { } string.push_str(" "); - if func_modifiers.visibility != ItemVisibility::Private { + if func_modifiers.visibility != ItemVisibility::Private + && func_meta.trait_id.is_none() + && func_meta.trait_impl.is_none() + { string.push_str(&func_modifiers.visibility.to_string()); string.push(' '); } @@ -1154,7 +1157,7 @@ mod hover_tests { assert!(hover_text.starts_with( " two impl Bar for Foo - pub fn bar_stuff(self)" + fn bar_stuff(self)" )); } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs index 5d30b2aeca3..77c3c2a396b 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs @@ -23,10 +23,17 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { // Now write any leading comment respecting multiple newlines after them group.leading_comment(self.chunk(|formatter| { + // Doc comments for a let statement could come before a potential non-doc comment if formatter.token.kind() == TokenKind::OuterDocComment { formatter.format_outer_doc_comments(); } + formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); + + // Or doc comments could come after a potential non-doc comment + if formatter.token.kind() == TokenKind::OuterDocComment { + formatter.format_outer_doc_comments(); + } })); ignore_next |= self.ignore_next; @@ -390,6 +397,21 @@ mod tests { assert_format(src, expected); } + #[test] + fn format_let_statement_with_unsafe_and_comment_before_it() { + let src = " fn foo() { + // Some comment + /// Safety: some doc + let x = unsafe { 1 } ; } "; + let expected = "fn foo() { + // Some comment + /// Safety: some doc + let x = unsafe { 1 }; +} +"; + assert_format(src, expected); + } + #[test] fn format_assign() { let src = " fn foo() { x = 2 ; } "; diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/use_tree_merge.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/use_tree_merge.rs index 76fdcd0d0f8..a679e026435 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/use_tree_merge.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/use_tree_merge.rs @@ -179,7 +179,12 @@ impl Ord for Segment { if let (Segment::Plain(self_string), Segment::Plain(other_string)) = (self, other) { // Case-insensitive comparison for plain segments - self_string.to_lowercase().cmp(&other_string.to_lowercase()) + let ordering = self_string.to_lowercase().cmp(&other_string.to_lowercase()); + if ordering == Ordering::Equal { + self_string.cmp(other_string) + } else { + ordering + } } else { order_number_ordering } @@ -620,4 +625,10 @@ use std::merkle::compute_merkle_root; let expected = "use std::{as_witness, merkle::compute_merkle_root};\n"; assert_format(src, expected); } + + #[test] + fn does_not_merge_same_identifiers_if_equal_case_insensitive() { + let src = "use bigint::{BigNum, bignum::BigNumTrait};\n"; + assert_format(src, src); + } }