chore(noir): dedup within output also. Futhermore:

- reorder keywords (distinct first)
- add test case
- fix up comments & typos
- tidy up

crates/noirc_abi/src/lib.rs

@@ -87,13 +87,14 @@ impl std::fmt::Display for AbiVisibility {
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "lowercase")]
-/// Represents whether the return value should be encoded into witness indexes that are distinct
-/// from the input parameters.
+/// Represents whether the return value should compromise of unique witness indices such that no
+/// index occurs within the program's abi more than once.
 ///
-/// This is useful for apllication stacks that require an uniform abi across across multiple
-/// circuits. When duplication is allowed, the compiler may indentify that a public input reaches
-/// the output unaltered and is thus references directly, causing the input and output witness
-/// indcies to overlap.
+/// This is useful for application stacks that require an uniform abi across across multiple
+/// circuits. When index duplication is allowed, the compiler may identify that a public input
+/// reaches the output unaltered and is thus referenced directly, causing the input and output
+/// witness indices to overlap. Similarly, repetitions of copied values in the output may be
+/// optimized away.
 pub enum AbiDistinctness {
     Distinct,
     DuplicationAllowed,

crates/noirc_evaluator/src/lib.rs

@@ -54,8 +54,8 @@ pub struct Evaluator {
     // and increasing as for `public_parameters`. We then use a `Vec` rather
     // than a `BTreeSet` to preserve this order for the ABI.
     return_values: Vec<Witness>,
-    // If true, indicates that the resulting ACIR should prevent input and output witness indicies
-    // from overlapping by having extra contraints if necessary.
+    // If true, indicates that the resulting ACIR should enforce that all inputs and outputs are
+    // comprised of unique witness indices by having extra constraints if necessary.
     return_is_distinct: bool,
 
     opcodes: Vec<AcirOpcode>,
@@ -105,7 +105,7 @@ pub fn create_circuit(
 }
 
 impl Evaluator {
-    // Returns true if the `witnees_index` appears in the program's input parameters.
+    // Returns true if the `witness_index` appears in the program's input parameters.
     fn is_abi_input(&self, witness_index: Witness) -> bool {
         witness_index.as_usize() <= self.num_witnesses_abi_len
     }
@@ -125,8 +125,11 @@ impl Evaluator {
         self.is_abi_input(witness_index) && !is_public_input
     }
 
+    // True if the main function return has the `distinct` keyword and this particular witness
+    // index has already occurred elsewhere in the abi's inputs and outputs.
     fn should_proxy_witness_for_abi_output(&self, witness_index: Witness) -> bool {
-        self.return_is_distinct && self.is_abi_input(witness_index)
+        self.return_is_distinct
+            && (self.is_abi_input(witness_index) || self.return_values.contains(&witness_index))
     }
 
     // Creates a new Witness index

crates/noirc_evaluator/src/ssa/acir_gen/operations/return.rs

@@ -48,8 +48,8 @@ pub(crate) fn evaluate(
                     "we do not allow private ABI inputs to be returned as public outputs",
                 )));
             }
-            // Check if the outputted witness needs separating from an occurrence in the program's
-            // input. This behaviour stems from usage of the `distinct` keyword.
+            // Check if the outputted witness needs separating from an existing occurrence in the
+            // abi. This behavior stems from usage of the `distinct` keyword.
             let return_witness = if evaluator.should_proxy_witness_for_abi_output(witness) {
                 let proxy_constraint = Expression::from(witness);
                 evaluator.create_intermediate_variable(proxy_constraint)

crates/noirc_frontend/src/hir/resolution/errors.rs

@@ -187,7 +187,7 @@ impl From<ResolverError> for Diagnostic {
                     ident.0.span(),
                 );
 
-                diag.add_note("The `distinct` keyword is only valid when used on the main function of a program as it's only purpose is to prevent a program's parameter and return witness indexes from overlapping".to_owned());
+                diag.add_note("The `distinct` keyword is only valid when used on the main function of a program, as its only purpose is to ensure that all witness indices that occur in the abi are unique".to_owned());
                 diag
             }
             ResolverError::ExpectedComptimeVariable { name, span } => Diagnostic::simple_error(

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

@@ -681,9 +681,11 @@ impl<'a> Resolver<'a> {
         }
     }
 
-    /// True if the 'distinct' keyword is allowed on parameters in this function
+    /// True if the `distinct` keyword is allowed on function's return type
     fn distinct_allowed(&self, func: &NoirFunction) -> bool {
         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
         } else {
             func.name() == "main"

crates/noirc_frontend/src/monomorphization/ast.rs

@@ -221,9 +221,9 @@ impl Type {
 pub struct Program {
     pub functions: Vec<Function>,
     pub main_function_signature: FunctionSignature,
-    /// Indicates whether the resulting ACIR should keep input and output witness indcies separate.
+    /// Indicates whether witness indices are allowed to reoccur in the ABI of the resulting ACIR.
     ///
-    /// Note: this has no impact on monomorphisation, and is simply attached here for ease of
+    /// Note: this has no impact on monomorphization, and is simply attached here for ease of
     /// forwarding to the next phase.
     pub return_distinctness: noirc_abi::AbiDistinctness,
 }

crates/noirc_frontend/src/parser/parser.rs

@@ -162,7 +162,7 @@ fn function_definition(allow_self: bool) -> impl NoirParser<NoirFunction> {
             |(
                 (
                     ((((attribute, (is_unconstrained, is_open)), name), generics), parameters),
-                    (return_visibility, return_distinctness, return_type),
+                    ((return_distinctness, return_visibility), return_type),
                 ),
                 body,
             )| {
@@ -236,19 +236,15 @@ fn lambda_return_type() -> impl NoirParser<UnresolvedType> {
         .map(|ret| ret.unwrap_or(UnresolvedType::Unspecified))
 }
 
-fn function_return_type() -> impl NoirParser<(AbiVisibility, AbiDistinctness, UnresolvedType)> {
+fn function_return_type() -> impl NoirParser<((AbiDistinctness, AbiVisibility), UnresolvedType)> {
     just(Token::Arrow)
-        .ignore_then(optional_visibility())
-        .then(optional_distinctness())
+        .ignore_then(optional_distinctness())
+        .then(optional_visibility())
         .then(parse_type())
         .or_not()
         .map(|ret| {
-            ret.map(|((visibility, distinctness), return_type)| {
-                (visibility, distinctness, return_type)
-            })
-            .unwrap_or((
-                AbiVisibility::Private,
-                AbiDistinctness::DuplicationAllowed,
+            ret.unwrap_or((
+                (AbiDistinctness::DuplicationAllowed, AbiVisibility::Private),
                 UnresolvedType::Unit,
             ))
         })
@@ -1275,6 +1271,7 @@ mod test {
                 "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }",
                 "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}",
                 "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5])  {}",
+                "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }"
             ],
         );