feat!: use distinct return value witnesses by default (#4951)

# Description

## Problem\*

Resolves #4914
Resolves #4443

## Summary\*

We're starting to need various workarounds for non-distinct witnesses to
the point where it's more of a hindrance than it's worth. This PR makes
the `distinct` behaviour the default and raises a parser error when
using the `distinct` keyword.

I've gone for a hard error rather than a warning as the only place where
`distinct` can be used is on `main` and so there's no risk of breaking
libraries. Affected users can simply modify their own binary project to
fix the issue.

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

.github/workflows/test-rust-workspace-msrv.yml

@@ -88,7 +88,8 @@ jobs:
       - name: Run tests
         run: |
           cargo nextest run --archive-file nextest-archive.tar.zst \
-            --partition count:${{ matrix.partition }}/4
+            --partition count:${{ matrix.partition }}/4 \
+            --no-fail-fast
 
   # 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.

.github/workflows/test-rust-workspace.yml

@@ -75,7 +75,8 @@ jobs:
       - name: Run tests
         run: |
           cargo nextest run --archive-file nextest-archive.tar.zst \
-            --partition count:${{ matrix.partition }}/4
+            --partition count:${{ matrix.partition }}/4 \
+            --no-fail-fast
 
   # 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.

aztec_macros/src/transforms/functions.rs

@@ -2,7 +2,7 @@ use convert_case::{Case, Casing};
 use noirc_errors::Span;
 use noirc_frontend::ast;
 use noirc_frontend::ast::{
-    BlockExpression, ConstrainKind, ConstrainStatement, Distinctness, Expression, ExpressionKind,
+    BlockExpression, ConstrainKind, ConstrainStatement, Expression, ExpressionKind,
     ForLoopStatement, ForRange, FunctionReturnType, Ident, Literal, NoirFunction, NoirStruct,
     Param, PathKind, Pattern, Signedness, Statement, StatementKind, UnresolvedType,
     UnresolvedTypeData, Visibility,
@@ -104,13 +104,8 @@ pub fn transform_function(
         func.def.return_visibility = Visibility::Public;
     }
 
-    // Distinct return types are only required for private functions
     // Public functions should have unconstrained auto-inferred
-    match ty {
-        "Private" => func.def.return_distinctness = Distinctness::Distinct,
-        "Public" | "Avm" => func.def.is_unconstrained = true,
-        _ => (),
-    }
+    func.def.is_unconstrained = matches!(ty, "Public" | "Avm");
 
     Ok(())
 }

compiler/noirc_evaluator/src/ssa.rs

@@ -43,8 +43,6 @@ pub(crate) fn optimize_into_acir(
     force_brillig_output: bool,
     print_timings: bool,
 ) -> Result<(Vec<GeneratedAcir>, Vec<BrilligBytecode>), RuntimeError> {
-    let abi_distinctness = program.return_distinctness;
-
     let ssa_gen_span = span!(Level::TRACE, "ssa_generation");
     let ssa_gen_span_guard = ssa_gen_span.enter();
     let ssa = SsaBuilder::new(program, print_passes, force_brillig_output, print_timings)?
@@ -75,7 +73,7 @@ pub(crate) fn optimize_into_acir(
 
     drop(ssa_gen_span_guard);
 
-    time("SSA to ACIR", print_timings, || ssa.into_acir(&brillig, abi_distinctness))
+    time("SSA to ACIR", print_timings, || ssa.into_acir(&brillig))
 }
 
 // Helper to time SSA passes

compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs

@@ -41,7 +41,6 @@ use acvm::{
 use fxhash::FxHashMap as HashMap;
 use im::Vector;
 use iter_extended::{try_vecmap, vecmap};
-use noirc_frontend::ast::Distinctness;
 
 #[derive(Default)]
 struct SharedContext {
@@ -281,7 +280,6 @@ impl Ssa {
     pub(crate) fn into_acir(
         self,
         brillig: &Brillig,
-        abi_distinctness: Distinctness,
     ) -> Result<(Vec<GeneratedAcir>, Vec<BrilligBytecode>), RuntimeError> {
         let mut acirs = Vec::new();
         // TODO: can we parallelise this?
@@ -335,28 +333,33 @@ impl Ssa {
         // Also at the moment we specify Distinctness as part of the ABI exclusively rather than the function itself
         // so this will need to be updated.
         let main_func_acir = &mut acirs[0];
-        match abi_distinctness {
-            Distinctness::Distinct => {
-                // Create a witness for each return witness we have
-                // to guarantee that the return witnesses are distinct
-                let distinct_return_witness: Vec<_> = main_func_acir
-                    .return_witnesses
-                    .clone()
-                    .into_iter()
-                    .map(|return_witness| {
-                        main_func_acir
-                            .create_witness_for_expression(&Expression::from(return_witness))
-                    })
-                    .collect();
+        generate_distinct_return_witnesses(main_func_acir);
 
-                main_func_acir.return_witnesses = distinct_return_witness;
-            }
-            Distinctness::DuplicationAllowed => {}
-        }
         Ok((acirs, brillig))
     }
 }
 
+fn generate_distinct_return_witnesses(acir: &mut GeneratedAcir) {
+    // Create a witness for each return witness we have to guarantee that the return witnesses match the standard
+    // layout for serializing those types as if they were being passed as inputs.
+    //
+    // This is required for recursion as otherwise in situations where we cannot make use of the program's ABI
+    // (e.g. for `std::verify_proof` or the solidity verifier), we need extra knowledge about the program we're
+    // working with rather than following the standard ABI encoding rules.
+    //
+    // TODO: We're being conservative here by generating a new witness for every expression.
+    // This means that we're likely to get a number of constraints which are just renumbering witnesses.
+    // This can be tackled by:
+    // - Tracking the last assigned public input witness and only renumbering a witness if it is below this value.
+    // - Modifying existing constraints to rearrange their outputs so they are suitable
+    //   - See: https://github.com/noir-lang/noir/pull/4467
+    let distinct_return_witness = vecmap(acir.return_witnesses.clone(), |return_witness| {
+        acir.create_witness_for_expression(&Expression::from(return_witness))
+    });
+
+    acir.return_witnesses = distinct_return_witness;
+}
+
 impl<'a> Context<'a> {
     fn new(shared_context: &'a mut SharedContext) -> Context<'a> {
         let mut acir_context = AcirContext::default();
@@ -2705,7 +2708,7 @@ mod test {
         let ssa = builder.finish();
 
         let (acir_functions, _) = ssa
-            .into_acir(&Brillig::default(), noirc_frontend::ast::Distinctness::Distinct)
+            .into_acir(&Brillig::default())
             .expect("Should compile manually written SSA into ACIR");
         // Expected result:
         // main f0
@@ -2800,7 +2803,7 @@ mod test {
         let ssa = builder.finish();
 
         let (acir_functions, _) = ssa
-            .into_acir(&Brillig::default(), noirc_frontend::ast::Distinctness::Distinct)
+            .into_acir(&Brillig::default())
             .expect("Should compile manually written SSA into ACIR");
         // The expected result should look very similar to the above test expect that the input witnesses of the `Call`
         // opcodes will be different. The changes can discerned from the checks below.
@@ -2890,7 +2893,7 @@ mod test {
         let ssa = builder.finish();
 
         let (acir_functions, _) = ssa
-            .into_acir(&Brillig::default(), noirc_frontend::ast::Distinctness::Distinct)
+            .into_acir(&Brillig::default())
             .expect("Should compile manually written SSA into ACIR");
 
         assert_eq!(acir_functions.len(), 3, "Should have three ACIR functions");
@@ -3003,9 +3006,8 @@ mod test {
         let brillig = ssa.to_brillig(false);
         println!("{}", ssa);
 
-        let (acir_functions, brillig_functions) = ssa
-            .into_acir(&brillig, noirc_frontend::ast::Distinctness::Distinct)
-            .expect("Should compile manually written SSA into ACIR");
+        let (acir_functions, brillig_functions) =
+            ssa.into_acir(&brillig).expect("Should compile manually written SSA into ACIR");
 
         assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
         assert_eq!(brillig_functions.len(), 2, "Should only have generated two Brillig functions");
@@ -3061,7 +3063,7 @@ mod test {
         // The Brillig bytecode we insert for the stdlib is hardcoded so we do not need to provide any
         // Brillig artifacts to the ACIR gen pass.
         let (acir_functions, brillig_functions) = ssa
-            .into_acir(&Brillig::default(), noirc_frontend::ast::Distinctness::Distinct)
+            .into_acir(&Brillig::default())
             .expect("Should compile manually written SSA into ACIR");
 
         assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
@@ -3132,9 +3134,8 @@ mod test {
         let brillig = ssa.to_brillig(false);
         println!("{}", ssa);
 
-        let (acir_functions, brillig_functions) = ssa
-            .into_acir(&brillig, noirc_frontend::ast::Distinctness::Distinct)
-            .expect("Should compile manually written SSA into ACIR");
+        let (acir_functions, brillig_functions) =
+            ssa.into_acir(&brillig).expect("Should compile manually written SSA into ACIR");
 
         assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function");
         // We expect 3 brillig functions:
@@ -3221,9 +3222,8 @@ mod test {
         let brillig = ssa.to_brillig(false);
         println!("{}", ssa);
 
-        let (acir_functions, brillig_functions) = ssa
-            .into_acir(&brillig, noirc_frontend::ast::Distinctness::Distinct)
-            .expect("Should compile manually written SSA into ACIR");
+        let (acir_functions, brillig_functions) =
+            ssa.into_acir(&brillig).expect("Should compile manually written SSA into ACIR");
 
         assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions");
         // We expect 3 brillig functions:

compiler/noirc_frontend/src/ast/expression.rs

@@ -2,7 +2,7 @@ use std::borrow::Cow;
 use std::fmt::Display;
 
 use crate::ast::{
-    Distinctness, Ident, ItemVisibility, Path, Pattern, Recoverable, Statement, StatementKind,
+    Ident, ItemVisibility, Path, Pattern, Recoverable, Statement, StatementKind,
     UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, Visibility,
 };
 use crate::token::{Attributes, Token};
@@ -401,7 +401,6 @@ pub struct FunctionDefinition {
     pub where_clause: Vec<UnresolvedTraitConstraint>,
     pub return_type: FunctionReturnType,
     pub return_visibility: Visibility,
-    pub return_distinctness: Distinctness,
 }
 
 #[derive(Debug, PartialEq, Eq, Clone)]
@@ -698,7 +697,6 @@ impl FunctionDefinition {
             where_clause: where_clause.to_vec(),
             return_type: return_type.clone(),
             return_visibility: Visibility::Private,
-            return_distinctness: Distinctness::DuplicationAllowed,
         }
     }
 }

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

@@ -26,8 +26,8 @@ use std::collections::{BTreeMap, BTreeSet, HashSet};
 use std::rc::Rc;
 
 use crate::ast::{
-    ArrayLiteral, BinaryOpKind, BlockExpression, Distinctness, Expression, ExpressionKind,
-    ForRange, FunctionDefinition, FunctionKind, FunctionReturnType, Ident, ItemVisibility, LValue,
+    ArrayLiteral, BinaryOpKind, BlockExpression, Expression, ExpressionKind, ForRange,
+    FunctionDefinition, FunctionKind, FunctionReturnType, Ident, ItemVisibility, LValue,
     LetStatement, Literal, NoirFunction, NoirStruct, NoirTypeAlias, Param, Path, PathKind, Pattern,
     Statement, StatementKind, TraitBound, UnaryOp, UnresolvedGenerics, UnresolvedTraitConstraint,
     UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, Visibility, ERROR_IDENT,
@@ -320,7 +320,6 @@ impl<'a> Resolver<'a> {
             where_clause: where_clause.to_vec(),
             return_type: return_type.clone(),
             return_visibility: Visibility::Private,
-            return_distinctness: Distinctness::DuplicationAllowed,
         };
 
         let (hir_func, func_meta) = self.intern_function(NoirFunction { kind, def }, func_id);
@@ -1069,12 +1068,6 @@ impl<'a> Resolver<'a> {
             });
         }
 
-        if !self.distinct_allowed(func)
-            && func.def.return_distinctness != Distinctness::DuplicationAllowed
-        {
-            self.push_err(ResolverError::DistinctNotAllowed { ident: func.name_ident().clone() });
-        }
-
         if matches!(attributes.function, Some(FunctionAttribute::Test { .. }))
             && !parameters.is_empty()
         {
@@ -1107,7 +1100,6 @@ impl<'a> Resolver<'a> {
             parameters: parameters.into(),
             return_type: func.def.return_type.clone(),
             return_visibility: func.def.return_visibility,
-            return_distinctness: func.def.return_distinctness,
             has_body: !func.def.body.is_empty(),
             trait_constraints: self.resolve_trait_constraints(&func.def.where_clause),
             is_entry_point: self.is_entry_point_function(func),
@@ -1136,17 +1128,6 @@ 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 {
-            // "unconstrained" functions are compiled to brillig and thus duplication of
-            // witness indices in their abis is not a concern.
-            !func.def.is_unconstrained
-        } else {
-            func.name() == MAIN_FUNCTION
-        }
-    }
-
     fn inline_attribute_allowed(&self, func: &NoirFunction) -> bool {
         // Inline attributes are only relevant for constrained functions
         // as all unconstrained functions are not inlined

compiler/noirc_frontend/src/hir/type_check/mod.rs

@@ -433,9 +433,7 @@ pub mod test {
     use iter_extended::btree_map;
     use noirc_errors::{Location, Span};
 
-    use crate::ast::{
-        BinaryOpKind, Distinctness, FunctionKind, FunctionReturnType, Path, Visibility,
-    };
+    use crate::ast::{BinaryOpKind, FunctionKind, FunctionReturnType, Path, Visibility};
     use crate::graph::CrateId;
     use crate::hir::def_map::{ModuleData, ModuleId};
     use crate::hir::resolution::import::{
@@ -539,7 +537,6 @@ pub mod test {
             ]
             .into(),
             return_visibility: Visibility::Private,
-            return_distinctness: Distinctness::DuplicationAllowed,
             has_body: true,
             trait_impl: None,
             return_type: FunctionReturnType::Default(Span::default()),

compiler/noirc_frontend/src/hir_def/function.rs

@@ -6,7 +6,7 @@ use std::rc::Rc;
 use super::expr::{HirBlockExpression, HirExpression, HirIdent};
 use super::stmt::HirPattern;
 use super::traits::TraitConstraint;
-use crate::ast::{Distinctness, FunctionKind, FunctionReturnType, Visibility};
+use crate::ast::{FunctionKind, FunctionReturnType, Visibility};
 use crate::node_interner::{ExprId, NodeInterner, TraitImplId};
 use crate::{Type, TypeVariable};
 
@@ -99,8 +99,6 @@ pub struct FuncMeta {
 
     pub return_visibility: Visibility,
 
-    pub return_distinctness: Distinctness,
-
     /// The type of this function. Either a Type::Function
     /// or a Type::Forall for generic functions.
     pub typ: Type,

compiler/noirc_frontend/src/lib.rs

@@ -50,7 +50,7 @@ pub mod macros_api {
     pub use crate::token::SecondaryAttribute;
 
     pub use crate::ast::{
-        BlockExpression, CallExpression, CastExpression, Distinctness, Expression, ExpressionKind,
+        BlockExpression, CallExpression, CastExpression, Expression, ExpressionKind,
         FunctionReturnType, Ident, IndexExpression, ItemVisibility, LetStatement, Literal,
         MemberAccessExpression, MethodCallExpression, NoirFunction, Path, PathKind, Pattern,
         Statement, UnresolvedType, UnresolvedTypeData, Visibility,

compiler/noirc_frontend/src/monomorphization/ast.rs

@@ -7,7 +7,7 @@ use noirc_errors::{
 
 use crate::hir_def::function::FunctionSignature;
 use crate::{
-    ast::{BinaryOpKind, Distinctness, IntegerBitSize, Signedness, Visibility},
+    ast::{BinaryOpKind, IntegerBitSize, Signedness, Visibility},
     token::{Attributes, FunctionAttribute},
 };
 
@@ -302,11 +302,6 @@ pub struct Program {
     pub functions: Vec<Function>,
     pub function_signatures: Vec<FunctionSignature>,
     pub main_function_signature: FunctionSignature,
-    /// Indicates whether witness indices are allowed to reoccur in the ABI of the resulting ACIR.
-    ///
-    /// Note: this has no impact on monomorphization, and is simply attached here for ease of
-    /// forwarding to the next phase.
-    pub return_distinctness: Distinctness,
     pub return_location: Option<Location>,
     pub return_visibility: Visibility,
     /// Indicates to a backend whether a SNARK-friendly prover should be used.  
@@ -322,7 +317,6 @@ impl Program {
         functions: Vec<Function>,
         function_signatures: Vec<FunctionSignature>,
         main_function_signature: FunctionSignature,
-        return_distinctness: Distinctness,
         return_location: Option<Location>,
         return_visibility: Visibility,
         recursive: bool,
@@ -334,7 +328,6 @@ impl Program {
             functions,
             function_signatures,
             main_function_signature,
-            return_distinctness,
             return_location,
             return_visibility,
             recursive,

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -142,16 +142,14 @@ pub fn monomorphize_debug(
         .collect();
 
     let functions = vecmap(monomorphizer.finished_functions, |(_, f)| f);
-    let FuncMeta { return_distinctness, return_visibility, kind, .. } =
-        monomorphizer.interner.function_meta(&main);
+    let FuncMeta { return_visibility, kind, .. } = monomorphizer.interner.function_meta(&main);
 
     let (debug_variables, debug_functions, debug_types) =
         monomorphizer.debug_type_tracker.extract_vars_and_types();
     let program = Program::new(
         functions,
         func_sigs,
         function_sig,
-        *return_distinctness,
         monomorphizer.return_location,
         *return_visibility,
         *kind == FunctionKind::Recursive,

compiler/noirc_frontend/src/parser/errors.rs

@@ -30,6 +30,8 @@ pub enum ParserErrorReason {
     TraitImplFunctionModifiers,
     #[error("comptime keyword is deprecated")]
     ComptimeDeprecated,
+    #[error("distinct keyword is deprecated. The `distinct` behavior is now the default.")]
+    DistinctDeprecated,
     #[error("{0} are experimental and aren't fully supported yet")]
     ExperimentalFeature(&'static str),
     #[error(