Merge remote-tracking branch 'origin/master' into kw/switch-backend

.github/workflows/gates_report.yml

@@ -0,0 +1,90 @@
+name: Report gates diff
+
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+
+jobs:
+  build-nargo:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        target: [x86_64-unknown-linux-gnu]
+
+    steps:
+      - name: Checkout Noir repo
+        uses: actions/checkout@v4
+
+      - name: Setup toolchain
+        uses: dtolnay/[email protected]
+
+      - uses: Swatinem/rust-cache@v2
+        with:
+          key: ${{ matrix.target }}
+          cache-on-failure: true
+          save-if: ${{ github.event_name != 'merge_group' }}
+
+      - name: Build Nargo
+        run: cargo build --package nargo_cli --release
+
+      - name: Package artifacts
+        run: |
+          mkdir dist
+          cp ./target/release/nargo ./dist/nargo
+          7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz
+
+      - name: Upload artifact
+        uses: actions/upload-artifact@v3
+        with:
+          name: nargo
+          path: ./dist/*
+          retention-days: 3
+
+
+  compare_gas_reports:
+    needs: [build-nargo]
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          submodules: recursive
+
+      - name: Download nargo binary
+        uses: actions/download-artifact@v3
+        with:
+          name: nargo
+          path: ./nargo
+
+      - name: Set nargo on PATH
+        run: |
+          nargo_binary="${{ github.workspace }}/nargo/nargo"
+          chmod +x $nargo_binary
+          echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
+          export PATH="$PATH:$(dirname $nargo_binary)"
+          nargo -V
+
+      - name: Generate gates report
+        working-directory: ./tooling/nargo_cli/tests
+        run: |
+          ./gates_report.sh
+          mv gates_report.json ../../../gates_report.json
+       
+      - name: Compare gates reports
+        id: gates_diff
+        uses: TomAFrench/noir-gates-diff@e7cf131b7e7f044c01615f93f0b855f65ddc02d4
+        with:
+          report: gates_report.json
+          summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
+
+      - name: Add gates diff to sticky comment
+        if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
+        uses: marocchino/sticky-pull-request-comment@v2
+        with:
+          # delete the comment in case changes no longer impact circuit sizes
+          delete: ${{ !steps.gates_diff.outputs.markdown }}
+          message: ${{ steps.gates_diff.outputs.markdown }}

.gitignore

@@ -40,6 +40,9 @@ result
 !tooling/nargo_cli/tests/acir_artifacts/*/target
 !tooling/nargo_cli/tests/acir_artifacts/*/target/witness.gz
 !compiler/wasm/noir-script/target
+
+gates_report.json
+
 # Github Actions scratch space
 # This gives a location to download artifacts into the repository in CI without making git dirty.
 libbarretenberg-wasm32

compiler/noirc_driver/Cargo.toml

@@ -7,6 +7,9 @@ license.workspace = true
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
+[build-dependencies]
+build-data = "0.1.3"
+
 [dependencies]
 clap.workspace = true
 noirc_errors.workspace = true

compiler/noirc_driver/build.rs

@@ -0,0 +1,14 @@
+const GIT_COMMIT: &&str = &"GIT_COMMIT";
+
+fn main() {
+    // Rebuild if the tests have changed
+    println!("cargo:rerun-if-changed=tests");
+
+    // Only use build_data if the environment variable isn't set
+    // The environment variable is always set when working via Nix
+    if std::env::var(GIT_COMMIT).is_err() {
+        build_data::set_GIT_COMMIT();
+        build_data::set_GIT_DIRTY();
+        build_data::no_debug_rebuilds();
+    }
+}

compiler/noirc_driver/src/contract.rs

@@ -28,6 +28,8 @@ pub enum ContractFunctionType {
 
 #[derive(Serialize, Deserialize)]
 pub struct CompiledContract {
+    pub noir_version: String,
+
     /// The name of the contract.
     pub name: String,
     /// Each of the contract's functions are compiled into a separate `CompiledProgram`

compiler/noirc_driver/src/lib.rs

@@ -28,6 +28,15 @@ pub use program::CompiledProgram;
 
 const STD_CRATE_NAME: &str = "std";
 
+pub const GIT_COMMIT: &str = env!("GIT_COMMIT");
+pub const GIT_DIRTY: &str = env!("GIT_DIRTY");
+pub const NOIRC_VERSION: &str = env!("CARGO_PKG_VERSION");
+
+/// Version string that gets placed in artifacts that Noir builds. This is semver compatible.
+/// Note: You can't directly use the value of a constant produced with env! inside a concat! macro.
+pub const NOIR_ARTIFACT_VERSION_STRING: &str =
+    concat!(env!("CARGO_PKG_VERSION"), "+", env!("GIT_COMMIT"));
+
 #[derive(Args, Clone, Debug, Default, Serialize, Deserialize)]
 pub struct CompileOptions {
     /// Emit debug information for the intermediate SSA IR
@@ -305,6 +314,7 @@ fn compile_contract_inner(
                 .collect(),
             functions,
             file_map,
+            noir_version: NOIR_ARTIFACT_VERSION_STRING.to_string(),
         })
     } else {
         Err(errors)
@@ -342,5 +352,12 @@ pub fn compile_no_check(
 
     let file_map = filter_relevant_files(&[debug.clone()], &context.file_manager);
 
-    Ok(CompiledProgram { hash, circuit, debug, abi, file_map })
+    Ok(CompiledProgram {
+        hash,
+        circuit,
+        debug,
+        abi,
+        file_map,
+        noir_version: NOIR_ARTIFACT_VERSION_STRING.to_string(),
+    })
 }

compiler/noirc_driver/src/program.rs

@@ -12,6 +12,7 @@ use super::debug::DebugFile;
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
 pub struct CompiledProgram {
+    pub noir_version: String,
     /// Hash of the [`Program`][noirc_frontend::monomorphization::ast::Program] from which this [`CompiledProgram`]
     /// was compiled.
     ///

compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

@@ -427,20 +427,10 @@ impl AcirContext {
         let diff_expr = &lhs_expr - &rhs_expr;
 
         // Check to see if equality can be determined at compile-time.
-        if diff_expr.is_const() {
-            if diff_expr.is_zero() {
-                // Constraint is always true - assertion is unnecessary.
-                self.mark_variables_equivalent(lhs, rhs)?;
-                return Ok(());
-            } else {
-                // Constraint is always false - this program is unprovable.
-                return Err(RuntimeError::FailedConstraint {
-                    lhs: Box::new(lhs_expr),
-                    rhs: Box::new(rhs_expr),
-                    call_stack: self.get_call_stack(),
-                    assert_message,
-                });
-            };
+        if diff_expr.is_zero() {
+            // Constraint is always true - assertion is unnecessary.
+            self.mark_variables_equivalent(lhs, rhs)?;
+            return Ok(());
         }
 
         self.acir_ir.assert_is_zero(diff_expr);

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -1173,6 +1173,11 @@ fn resolve_function_set(
         resolver.set_self_type(self_type.clone());
         resolver.set_trait_id(unresolved_functions.trait_id);
 
+        // Without this, impl methods can accidentally be placed in contracts. See #3254
+        if self_type.is_some() {
+            resolver.set_in_contract(false);
+        }
+
         let (hir_func, func_meta, errs) = resolver.resolve_function(func, func_id);
         interner.push_fn_meta(func_meta, func_id);
         interner.update_fn(func_id, hir_func);

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -87,6 +87,14 @@ pub struct Resolver<'a> {
     /// Set to the current type if we're resolving an impl
     self_type: Option<Type>,
 
+    /// True if the current module is a contract.
+    /// This is usually determined by self.path_resolver.module_id(), but it can
+    /// be overriden for impls. Impls are an odd case since the methods within resolve
+    /// as if they're in the parent module, but should be placed in a child module.
+    /// Since they should be within a child module, in_contract is manually set to false
+    /// for these so we can still resolve them in the parent module without them being in a contract.
+    in_contract: bool,
+
     /// Contains a mapping of the current struct or functions's generics to
     /// unique type variables if we're resolving a struct. Empty otherwise.
     /// This is a Vec rather than a map to preserve the order a functions generics
@@ -119,6 +127,9 @@ impl<'a> Resolver<'a> {
         def_maps: &'a BTreeMap<CrateId, CrateDefMap>,
         file: FileId,
     ) -> Resolver<'a> {
+        let module_id = path_resolver.module_id();
+        let in_contract = module_id.module(def_maps).is_contract;
+
         Self {
             path_resolver,
             def_maps,
@@ -131,6 +142,7 @@ impl<'a> Resolver<'a> {
             errors: Vec::new(),
             lambda_stack: Vec::new(),
             file,
+            in_contract,
         }
     }
 
@@ -805,17 +817,24 @@ impl<'a> Resolver<'a> {
         }
     }
 
+    /// Override whether this name resolver is within a contract or not.
+    /// This will affect which types are allowed as parameters to methods as well
+    /// as which modifiers are allowed on a function.
+    pub(crate) fn set_in_contract(&mut self, in_contract: bool) {
+        self.in_contract = in_contract;
+    }
+
     /// True if the 'pub' keyword is allowed on parameters in this function
     fn pub_allowed(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             !func.def.is_unconstrained
         } else {
             func.name() == MAIN_FUNCTION
         }
     }
 
     fn is_entry_point_function(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             func.attributes().is_contract_entry_point()
         } else {
             func.name() == MAIN_FUNCTION
@@ -824,7 +843,7 @@ impl<'a> Resolver<'a> {
 
     /// True if the `distinct` keyword is allowed on a function's return type
     fn distinct_allowed(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             // "open" and "unconstrained" functions are compiled to brillig and thus duplication of
             // witness indices in their abis is not a concern.
             !func.def.is_unconstrained && !func.def.is_open
@@ -836,15 +855,15 @@ impl<'a> Resolver<'a> {
     fn handle_function_type(&mut self, function: &FuncId) {
         let function_type = self.interner.function_modifiers(function).contract_function_type;
 
-        if !self.in_contract() && function_type == Some(ContractFunctionType::Open) {
+        if !self.in_contract && function_type == Some(ContractFunctionType::Open) {
             let span = self.interner.function_ident(function).span();
             self.errors.push(ResolverError::ContractFunctionTypeInNormalFunction { span });
             self.interner.function_modifiers_mut(function).contract_function_type = None;
         }
     }
 
     fn handle_is_function_internal(&mut self, function: &FuncId) {
-        if !self.in_contract() {
+        if !self.in_contract {
             if self.interner.function_modifiers(function).is_internal == Some(true) {
                 let span = self.interner.function_ident(function).span();
                 self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { span });
@@ -1605,11 +1624,6 @@ impl<'a> Resolver<'a> {
         }
     }
 
-    fn in_contract(&self) -> bool {
-        let module_id = self.path_resolver.module_id();
-        module_id.module(self.def_maps).is_contract
-    }
-
     fn resolve_fmt_str_literal(&mut self, str: String, call_expr_span: Span) -> HirLiteral {
         let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}")
             .expect("ICE: an invalid regex pattern was used for checking format strings");

compiler/wasm/build.rs

@@ -1,14 +1,4 @@
-const GIT_COMMIT: &&str = &"GIT_COMMIT";
-
 fn main() {
-    // Only use build_data if the environment variable isn't set
-    // The environment variable is always set when working via Nix
-    if std::env::var(GIT_COMMIT).is_err() {
-        build_data::set_GIT_COMMIT();
-        build_data::set_GIT_DIRTY();
-        build_data::no_debug_rebuilds();
-    }
-
     build_data::set_SOURCE_TIMESTAMP();
     build_data::no_debug_rebuilds();
 }