Skip to content

Commit

Permalink
fix: Prevent overlapping associated types impls (noir-lang/noir#7047)
Browse files Browse the repository at this point in the history
feat: unconstrained optimizations for BoundedVec (noir-lang/noir#7119)
chore: mark libs good (noir-lang/noir#7123)
chore: remove comments for time/memory benchmarks (noir-lang/noir#7121)
fix: don't always use an exclusive lock in `nargo check` (noir-lang/noir#7120)
feat(ssa): Pass to preprocess functions (noir-lang/noir#7072)
chore: Formatting issues / minor errors in the docs (noir-lang/noir#7105)
fix: defunctionalize pass on the caller runtime to apply (noir-lang/noir#7100)
feat: Parser and formatter support for `enum`s (noir-lang/noir#7110)
feat(brillig): SSA globals code gen (noir-lang/noir#7021)
feat: `loop` keyword in runtime and comptime code (noir-lang/noir#7096)
chore: Add benchmarking dashboard (noir-lang/noir#7068)
feat(experimental): try to infer lambda argument types inside calls (noir-lang/noir#7088)
feat(ssa): Add flag to DIE pass to be able to keep `store` instructions (noir-lang/noir#7106)
chore: Cookbook Onboard integration (noir-lang/noir#7044)
chore: lock to ubuntu 22.04 (noir-lang/noir#7098)
fix: Remove unused brillig functions (noir-lang/noir#7102)
chore(ssa): Use correct prefix when printing array values in global space (noir-lang/noir#7095)
  • Loading branch information
AztecBot committed Jan 21, 2025
2 parents a5c1b5f + 053fc55 commit 57ac507
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 127 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
bbdb937d44ddfbcd02a746dba9ad73ac704fcdd3
ed9977a57e0015ed653f54ce9377225434a947df
158 changes: 50 additions & 108 deletions noir/noir-repo/.github/workflows/reports.yml
Original file line number Diff line number Diff line change
Expand Up @@ -421,54 +421,6 @@ jobs:
retention-days: 3
overwrite: true

upload_compilation_report:
name: Upload compilation report
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
runs-on: ubuntu-22.04
permissions:
pull-requests: write
# deployments permission to deploy GitHub pages website
deployments: write
# contents permission to update benchmark contents in gh-pages branch
contents: write

steps:
- uses: actions/checkout@v4

- name: Download initial compilation report
uses: actions/download-artifact@v4
with:
name: in_progress_compilation_report

- name: Download matrix compilation reports
uses: actions/download-artifact@v4
with:
pattern: compilation_report_*
path: ./reports

- name: Merge compilation reports using jq
run: |
mv ./.github/scripts/merge-bench-reports.sh merge-bench-reports.sh
./merge-bench-reports.sh compilation_report
- name: Parse compilation report
id: compilation_report
uses: noir-lang/noir-bench-report@6ba151d7795042c4ff51864fbeb13c0a6a79246c
with:
report: compilation_report.json
header: |
Compilation Report
memory_report: false

- name: Add memory report to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: compilation
message: ${{ steps.compilation_report.outputs.markdown }}

- name: Convert to `benchmark-action` format
run: |
jq ".compilation_reports | map({name: .artifact_name, value: (.time[:-1] | tonumber), unit: \"s\"}) " ./compilation_report.json > time_bench.json
Expand Down Expand Up @@ -594,6 +546,55 @@ jobs:
retention-days: 3
overwrite: true

upload_compilation_report:
name: Upload compilation report
needs: [generate_compilation_and_execution_report, external_repo_compilation_and_execution_report]
# We want this job to run even if one variation of the matrix in `external_repo_compilation_and_execution_report` fails
if: always()
runs-on: ubuntu-22.04
permissions:
pull-requests: write
# deployments permission to deploy GitHub pages website
deployments: write
# contents permission to update benchmark contents in gh-pages branch
contents: write

steps:
- uses: actions/checkout@v4

- name: Download initial compilation report
uses: actions/download-artifact@v4
with:
name: in_progress_compilation_report

- name: Download matrix compilation reports
uses: actions/download-artifact@v4
with:
pattern: compilation_report_*
path: ./reports

- name: Merge compilation reports using jq
run: |
mv ./.github/scripts/merge-bench-reports.sh merge-bench-reports.sh
./merge-bench-reports.sh compilation_report
jq ".compilation_reports | map({name: .artifact_name, value: (.time[:-1] | tonumber), unit: \"s\"}) " ./compilation_report.json > time_bench.json
- name: Store benchmark result
continue-on-error: true
uses: benchmark-action/github-action-benchmark@4de1bed97a47495fc4c5404952da0499e31f5c29
with:
name: "Compilation Time"
tool: "customSmallerIsBetter"
output-file-path: ./time_bench.json
github-token: ${{ secrets.GITHUB_TOKEN }}
# We want this to only run on master to avoid garbage data from PRs being added.
auto-push: ${{ github.ref == 'refs/heads/master' }}
alert-threshold: "110%"
comment-on-alert: true
fail-on-alert: false
alert-comment-cc-users: "@TomAFrench"
max-items-in-chart: 50

upload_compilation_memory_report:
name: Upload compilation memory report
needs: [generate_memory_report, external_repo_memory_report]
Expand Down Expand Up @@ -625,27 +626,6 @@ jobs:
run: |
mv ./.github/scripts/merge-bench-reports.sh merge-bench-reports.sh
./merge-bench-reports.sh memory_report
# Rename the memory report as to not clash with the compilation memory report file name
cp memory_report.json execution_memory_report.json
- name: Parse compilation memory report
id: compilation_mem_report
uses: noir-lang/noir-bench-report@6ba151d7795042c4ff51864fbeb13c0a6a79246c
with:
report: execution_memory_report.json
header: |
Compilation Memory Report
memory_report: true

- name: Add execution memory report to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: compilation_memory
message: ${{ steps.compilation_mem_report.outputs.markdown }}

- name: Convert to `benchmark-action` format
run: |
jq ".memory_reports | map({name: .artifact_name, value: (.peak_memory | tonumber), unit: \"MB\"}) " ./memory_report.json > memory_bench.json
- name: Store benchmark result
Expand Down Expand Up @@ -697,26 +677,7 @@ jobs:
./merge-bench-reports.sh memory_report
# Rename the memory report as to not clash with the compilation memory report file name
cp memory_report.json execution_memory_report.json
- name: Parse execution memory report
id: execution_mem_report
uses: noir-lang/noir-bench-report@6ba151d7795042c4ff51864fbeb13c0a6a79246c
with:
report: execution_memory_report.json
header: |
Execution Memory Report
memory_report: true

- name: Add execution memory report to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: execution_memory
message: ${{ steps.execution_mem_report.outputs.markdown }}

- name: Convert to `benchmark-action` format
run: |
jq ".memory_reports | map({name: .artifact_name, value: (.peak_memory | tonumber), unit: \"MB\"}) " ./execution_memory_report.json > memory_bench.json
jq ".memory_reports | map({name: .artifact_name, value: (.peak_memory | tonumber), unit: \"MB\"}) " ./execution_memory_report.json > memory_bench.json
- name: Store benchmark result
continue-on-error: true
Expand Down Expand Up @@ -766,25 +727,6 @@ jobs:
run: |
mv ./.github/scripts/merge-bench-reports.sh merge-bench-reports.sh
./merge-bench-reports.sh execution_report
- name: Parse execution report
id: execution_report
uses: noir-lang/noir-bench-report@6ba151d7795042c4ff51864fbeb13c0a6a79246c
with:
report: execution_report.json
header: |
Execution Report
execution_report: true

- name: Add memory report to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
header: execution_time
message: ${{ steps.execution_report.outputs.markdown }}

- name: Convert to `benchmark-action` format
run: |
jq ".execution_reports | map({name: .artifact_name, value: (.time[:-1] | tonumber), unit: \"s\"}) " ./execution_report.json > time_bench.json
- name: Store benchmark result
Expand Down
12 changes: 11 additions & 1 deletion noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1722,8 +1722,18 @@ impl NodeInterner {
let instantiated_object_type = object_type.substitute(&substitutions);

let trait_generics = &trait_impl.borrow().trait_generics;

// Replace any associated types with fresh type variables so that we match
// any existing impl regardless of associated types if one already exists.
// E.g. if we already have an `impl Foo<Bar = i32> for Baz`, we should
// reject `impl Foo<Bar = u32> for Baz` if it were to be added.
let associated_types = self.get_associated_types_for_impl(impl_id);

let associated_types = vecmap(associated_types, |named| {
let typ = self.next_type_variable();
NamedType { name: named.name.clone(), typ }
});

// Ignoring overlapping `TraitImplKind::Assumed` impls here is perfectly fine.
// It should never happen since impls are defined at global scope, but even
// if they were, we should never prevent defining a new impl because a 'where'
Expand All @@ -1732,7 +1742,7 @@ impl NodeInterner {
&instantiated_object_type,
trait_id,
trait_generics,
associated_types,
&associated_types,
) {
let existing_impl = self.get_trait_implementation(existing);
let existing_impl = existing_impl.borrow();
Expand Down
59 changes: 59 additions & 0 deletions noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::hir::def_collector::dc_crate::CompilationError;
use crate::hir::def_collector::errors::DefCollectorErrorKind;
use crate::hir::resolution::errors::ResolverError;
use crate::hir::resolution::import::PathResolutionError;
use crate::hir::type_check::TypeCheckError;
Expand Down Expand Up @@ -1237,6 +1238,64 @@ fn warns_if_trait_is_not_in_scope_for_generic_function_call_and_there_is_only_on
assert_eq!(trait_name, "private_mod::Foo");
}

#[test]
fn error_on_duplicate_impl_with_associated_type() {
let src = r#"
trait Foo {
type Bar;
}
impl Foo for i32 {
type Bar = u32;
}
impl Foo for i32 {
type Bar = u8;
}
fn main() {}
"#;

// Expect "Impl for type `i32` overlaps with existing impl"
// and "Previous impl defined here"
let errors = get_program_errors(src);
assert_eq!(errors.len(), 2);

use CompilationError::DefinitionError;
use DefCollectorErrorKind::*;
assert!(matches!(&errors[0].0, DefinitionError(OverlappingImpl { .. })));
assert!(matches!(&errors[1].0, DefinitionError(OverlappingImplNote { .. })));
}

#[test]
fn error_on_duplicate_impl_with_associated_constant() {
let src = r#"
trait Foo {
let Bar: u32;
}
impl Foo for i32 {
let Bar = 5;
}
impl Foo for i32 {
let Bar = 6;
}
fn main() {}
"#;

// Expect "Impl for type `i32` overlaps with existing impl"
// and "Previous impl defined here"
let errors = get_program_errors(src);
assert_eq!(errors.len(), 2);

use CompilationError::DefinitionError;
use DefCollectorErrorKind::*;
assert!(matches!(&errors[0].0, DefinitionError(OverlappingImpl { .. })));
assert!(matches!(&errors[1].0, DefinitionError(OverlappingImplNote { .. })));
}

// See https://github.com/noir-lang/noir/issues/6530
#[test]
fn regression_6530() {
Expand Down
55 changes: 42 additions & 13 deletions noir/noir-repo/noir_stdlib/src/collections/bounded_vec.nr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{cmp::Eq, convert::From};
use crate::{cmp::Eq, convert::From, runtime::is_unconstrained};

/// A `BoundedVec<T, MaxLen>` is a growable storage similar to a `Vec<T>` except that it
/// is bounded with a maximum possible length. Unlike `Vec`, `BoundedVec` is not implemented
Expand Down Expand Up @@ -320,12 +320,18 @@ impl<T, let MaxLen: u32> BoundedVec<T, MaxLen> {
let new_len = self.len + append_len;
assert(new_len <= MaxLen, "extend_from_bounded_vec out of bounds");

let mut exceeded_len = false;
for i in 0..Len {
exceeded_len |= i == append_len;
if !exceeded_len {
if is_unconstrained() {
for i in 0..append_len {
self.storage[self.len + i] = vec.get_unchecked(i);
}
} else {
let mut exceeded_len = false;
for i in 0..Len {
exceeded_len |= i == append_len;
if !exceeded_len {
self.storage[self.len + i] = vec.get_unchecked(i);
}
}
}
self.len = new_len;
}
Expand Down Expand Up @@ -389,12 +395,19 @@ impl<T, let MaxLen: u32> BoundedVec<T, MaxLen> {
/// ```
pub fn any<Env>(self, predicate: fn[Env](T) -> bool) -> bool {
let mut ret = false;
let mut exceeded_len = false;
for i in 0..MaxLen {
exceeded_len |= i == self.len;
if !exceeded_len {
if is_unconstrained() {
for i in 0..self.len {
ret |= predicate(self.storage[i]);
}
} else {
let mut ret = false;
let mut exceeded_len = false;
for i in 0..MaxLen {
exceeded_len |= i == self.len;
if !exceeded_len {
ret |= predicate(self.storage[i]);
}
}
}
ret
}
Expand All @@ -413,11 +426,19 @@ impl<T, let MaxLen: u32> BoundedVec<T, MaxLen> {
pub fn map<U, Env>(self, f: fn[Env](T) -> U) -> BoundedVec<U, MaxLen> {
let mut ret = BoundedVec::new();
ret.len = self.len();
for i in 0..MaxLen {
if i < self.len() {

if is_unconstrained() {
for i in 0..self.len() {
ret.storage[i] = f(self.get_unchecked(i));
}
} else {
for i in 0..MaxLen {
if i < self.len() {
ret.storage[i] = f(self.get_unchecked(i));
}
}
}

ret
}

Expand All @@ -437,11 +458,19 @@ impl<T, let MaxLen: u32> BoundedVec<T, MaxLen> {
pub fn from_parts(mut array: [T; MaxLen], len: u32) -> Self {
assert(len <= MaxLen);
let zeroed = crate::mem::zeroed();
for i in 0..MaxLen {
if i >= len {

if is_unconstrained() {
for i in len..MaxLen {
array[i] = zeroed;
}
} else {
for i in 0..MaxLen {
if i >= len {
array[i] = zeroed;
}
}
}

BoundedVec { storage: array, len }
}

Expand Down
Loading

0 comments on commit 57ac507

Please sign in to comment.