From 5f1b58470779e977293323d10ab9a8f0857ea29e Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 1 May 2024 16:18:05 +0100 Subject: [PATCH] feat!: use `distinct` return value witnesses by default (#4951) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/4914 Resolves https://github.com/noir-lang/noir/issues/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. --- .../workflows/test-rust-workspace-msrv.yml | 3 +- .github/workflows/test-rust-workspace.yml | 3 +- aztec_macros/src/transforms/functions.rs | 9 +-- compiler/noirc_evaluator/src/ssa.rs | 4 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 64 +++++++++---------- compiler/noirc_frontend/src/ast/expression.rs | 4 +- .../src/hir/resolution/resolver.rs | 23 +------ .../noirc_frontend/src/hir/type_check/mod.rs | 5 +- .../noirc_frontend/src/hir_def/function.rs | 4 +- compiler/noirc_frontend/src/lib.rs | 2 +- .../src/monomorphization/ast.rs | 9 +-- .../src/monomorphization/mod.rs | 4 +- compiler/noirc_frontend/src/parser/errors.rs | 2 + compiler/noirc_frontend/src/parser/parser.rs | 30 ++------- .../src/parser/parser/function.rs | 46 ++++++++----- .../src/parser/parser/traits.rs | 6 +- docs/docs/noir/concepts/distinct.md | 64 ------------------- .../conditional_regression_547/Nargo.toml | 0 .../conditional_regression_547/Prover.toml | 0 .../conditional_regression_547/src/main.nr | 0 .../distinct_keyword/Nargo.toml | 6 -- .../distinct_keyword/Prover.toml | 1 - .../distinct_keyword/src/main.nr | 4 -- .../main_return/Nargo.toml | 0 .../main_return/Prover.toml | 0 .../main_return/src/main.nr | 0 .../simple_array_param/Nargo.toml | 0 .../simple_array_param/Prover.toml | 0 .../simple_array_param/src/main.nr | 0 tooling/nargo_fmt/src/visitor/item.rs | 6 +- tooling/nargo_fmt/tests/expected/fn.nr | 2 +- tooling/nargo_fmt/tests/input/fn.nr | 2 +- 32 files changed, 87 insertions(+), 216 deletions(-) delete mode 100644 docs/docs/noir/concepts/distinct.md rename test_programs/{compile_success_empty => execution_success}/conditional_regression_547/Nargo.toml (100%) rename test_programs/{compile_success_empty => execution_success}/conditional_regression_547/Prover.toml (100%) rename test_programs/{compile_success_empty => execution_success}/conditional_regression_547/src/main.nr (100%) delete mode 100644 test_programs/execution_success/distinct_keyword/Nargo.toml delete mode 100644 test_programs/execution_success/distinct_keyword/Prover.toml delete mode 100644 test_programs/execution_success/distinct_keyword/src/main.nr rename test_programs/{compile_success_empty => execution_success}/main_return/Nargo.toml (100%) rename test_programs/{compile_success_empty => execution_success}/main_return/Prover.toml (100%) rename test_programs/{compile_success_empty => execution_success}/main_return/src/main.nr (100%) rename test_programs/{compile_success_empty => execution_success}/simple_array_param/Nargo.toml (100%) rename test_programs/{compile_success_empty => execution_success}/simple_array_param/Prover.toml (100%) rename test_programs/{compile_success_empty => execution_success}/simple_array_param/src/main.nr (100%) diff --git a/.github/workflows/test-rust-workspace-msrv.yml b/.github/workflows/test-rust-workspace-msrv.yml index c13b0815d94..ccd431ea879 100644 --- a/.github/workflows/test-rust-workspace-msrv.yml +++ b/.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. diff --git a/.github/workflows/test-rust-workspace.yml b/.github/workflows/test-rust-workspace.yml index cf35950fa3e..1f3ee5e2268 100644 --- a/.github/workflows/test-rust-workspace.yml +++ b/.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. diff --git a/aztec_macros/src/transforms/functions.rs b/aztec_macros/src/transforms/functions.rs index 24cad05b866..b5803d8ac9e 100644 --- a/aztec_macros/src/transforms/functions.rs +++ b/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(()) } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index edc32fddeaf..89ef74c7406 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/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, Vec), 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 diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 16680c2b7eb..4a513c7b804 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/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, Vec), 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: diff --git a/compiler/noirc_frontend/src/ast/expression.rs b/compiler/noirc_frontend/src/ast/expression.rs index 5659de46588..59c04218e2e 100644 --- a/compiler/noirc_frontend/src/ast/expression.rs +++ b/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, 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, } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index bfdc2fe37cc..6c07957b27f 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/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 diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index ef5692abdd3..0f8131d6ebb 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/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()), diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index db54a40c8e7..c38dd41fd3d 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/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, diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index 6c77e3d0545..958a18ac2fb 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/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, diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index 721c7d5f0b1..97a71f800a8 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/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, pub function_signatures: Vec, 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, 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_signatures: Vec, main_function_signature: FunctionSignature, - return_distinctness: Distinctness, return_location: Option, return_visibility: Visibility, recursive: bool, @@ -334,7 +328,6 @@ impl Program { functions, function_signatures, main_function_signature, - return_distinctness, return_location, return_visibility, recursive, diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index b5754897d3f..94cc6bcd849 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -142,8 +142,7 @@ 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(); @@ -151,7 +150,6 @@ pub fn monomorphize_debug( functions, func_sigs, function_sig, - *return_distinctness, monomorphizer.return_location, *return_visibility, *kind == FunctionKind::Recursive, diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index af3d4caa145..9f9a8200954 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/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( diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 4a85f6cb18f..b627714d2a6 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -33,10 +33,10 @@ use super::{ }; use super::{spanned, Item, ItemKind}; use crate::ast::{ - BinaryOp, BinaryOpKind, BlockExpression, Distinctness, ForLoopStatement, ForRange, - FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Literal, ModuleDeclaration, - NoirTypeAlias, Param, Path, Pattern, Recoverable, Statement, TraitBound, TypeImpl, - UnresolvedTraitConstraint, UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, + BinaryOp, BinaryOpKind, BlockExpression, ForLoopStatement, ForRange, Ident, IfExpression, + InfixExpression, LValue, Literal, ModuleDeclaration, NoirTypeAlias, Param, Path, Pattern, + Recoverable, Statement, TraitBound, TypeImpl, UnresolvedTraitConstraint, + UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, }; use crate::ast::{ Expression, ExpressionKind, LetStatement, StatementKind, UnresolvedType, UnresolvedTypeData, @@ -296,21 +296,6 @@ fn type_alias_definition() -> impl NoirParser { }) } -fn function_return_type() -> impl NoirParser<((Distinctness, Visibility), FunctionReturnType)> { - just(Token::Arrow) - .ignore_then(optional_distinctness()) - .then(optional_visibility()) - .then(spanned(parse_type())) - .or_not() - .map_with_span(|ret, span| match ret { - Some((head, (ty, _))) => (head, FunctionReturnType::Ty(ty)), - None => ( - (Distinctness::DuplicationAllowed, Visibility::Private), - FunctionReturnType::Default(span), - ), - }) -} - fn self_parameter() -> impl NoirParser { let mut_ref_pattern = just(Token::Ampersand).then_ignore(keyword(Keyword::Mut)); let mut_pattern = keyword(Keyword::Mut); @@ -737,13 +722,6 @@ fn optional_visibility() -> impl NoirParser { }) } -fn optional_distinctness() -> impl NoirParser { - keyword(Keyword::Distinct).or_not().map(|opt| match opt { - Some(_) => Distinctness::Distinct, - None => Distinctness::DuplicationAllowed, - }) -} - fn maybe_comp_time() -> impl NoirParser { keyword(Keyword::Comptime).or_not().validate(|opt, span, emit| { if opt.is_some() { diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index ec4728fba4f..40180a9f9ac 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -1,16 +1,19 @@ use super::{ attributes::{attributes, validate_attributes}, - block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_distinctness, - optional_visibility, parameter_name_recovery, parameter_recovery, parenthesized, parse_type, - pattern, self_parameter, where_clause, NoirParser, -}; -use crate::ast::{ - Distinctness, FunctionDefinition, FunctionReturnType, Ident, ItemVisibility, NoirFunction, - Param, Visibility, + block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility, + parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, + self_parameter, where_clause, NoirParser, }; use crate::parser::labels::ParsingRuleLabel; use crate::parser::spanned; use crate::token::{Keyword, Token}; +use crate::{ + ast::{ + FunctionDefinition, FunctionReturnType, Ident, ItemVisibility, NoirFunction, Param, + Visibility, + }, + parser::{ParserError, ParserErrorReason}, +}; use chumsky::prelude::*; @@ -43,8 +46,7 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser impl NoirParser> { .map(|opt| opt.unwrap_or_default()) } -fn function_return_type() -> impl NoirParser<((Distinctness, Visibility), FunctionReturnType)> { +#[deprecated = "Distinct keyword is now deprecated. Remove this function after the 0.30.0 release"] +fn optional_distinctness() -> impl NoirParser { + keyword(Keyword::Distinct).or_not().validate(|opt, span, emit| { + if opt.is_some() { + emit(ParserError::with_reason(ParserErrorReason::DistinctDeprecated, span)); + } + opt.is_some() + }) +} + +pub(super) fn function_return_type() -> impl NoirParser<(Visibility, FunctionReturnType)> { + #[allow(deprecated)] just(Token::Arrow) .ignore_then(optional_distinctness()) - .then(optional_visibility()) + .ignore_then(optional_visibility()) .then(spanned(parse_type())) .or_not() .map_with_span(|ret, span| match ret { - Some((head, (ty, _))) => (head, FunctionReturnType::Ty(ty)), - None => ( - (Distinctness::DuplicationAllowed, Visibility::Private), - FunctionReturnType::Default(span), - ), + Some((visibility, (ty, _))) => (visibility, FunctionReturnType::Ty(ty)), + None => (Visibility::Private, FunctionReturnType::Default(span)), }) } @@ -174,7 +184,7 @@ mod test { "fn f(f: pub Field, y : Field, z : Field) -> u8 { x + a }", "fn f(f: pub Field, y : T, z : Field) -> u8 { x + a }", "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] }", + "fn main(x: pub u8, y: pub u8) -> pub [u8; 2] { [x, y] }", "fn f(f: pub Field, y : Field, z : Field) -> u8 { x + a }", "fn f(f: pub Field, y : T, z : Field) -> u8 { x + a }", "fn func_name(f: Field, y : T) where T: SomeTrait {}", @@ -214,6 +224,8 @@ mod test { // A leading plus is not allowed. "fn func_name(f: Field, y : T) where T: + SomeTrait {}", "fn func_name(f: Field, y : T) where T: TraitX + {}", + // `distinct` is deprecated + "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }", ], ); } diff --git a/compiler/noirc_frontend/src/parser/parser/traits.rs b/compiler/noirc_frontend/src/parser/parser/traits.rs index 0507dbdbc71..1aec57c8e41 100644 --- a/compiler/noirc_frontend/src/parser/parser/traits.rs +++ b/compiler/noirc_frontend/src/parser/parser/traits.rs @@ -1,9 +1,7 @@ use chumsky::prelude::*; -use super::{ - block, expression, fresh_statement, function, function_declaration_parameters, - function_return_type, -}; +use super::function::function_return_type; +use super::{block, expression, fresh_statement, function, function_declaration_parameters}; use crate::ast::{ Expression, ItemVisibility, NoirTrait, NoirTraitImpl, TraitBound, TraitImplItem, TraitItem, diff --git a/docs/docs/noir/concepts/distinct.md b/docs/docs/noir/concepts/distinct.md deleted file mode 100644 index 6c993b8b5e0..00000000000 --- a/docs/docs/noir/concepts/distinct.md +++ /dev/null @@ -1,64 +0,0 @@ ---- -title: Distinct Witnesses -sidebar_position: 11 ---- - -The `distinct` keyword prevents repetitions of witness indices in the program's ABI. This ensures -that the witnesses being returned as public inputs are all unique. - -The `distinct` keyword is only used for return values on program entry points (usually the `main()` -function). - -When using `distinct` and `pub` simultaneously, `distinct` comes first. See the example below. - -You can read more about the problem this solves -[here](https://github.com/noir-lang/noir/issues/1183). - -## Example - -Without the `distinct` keyword, the following program - -```rust -fn main(x : pub Field, y : pub Field) -> pub [Field; 4] { - let a = 1; - let b = 1; - [x + 1, y, a, b] -} -``` - -compiles to - -```json -{ - //... - "abi": { - //... - "param_witnesses": { "x": [1], "y": [2] }, - "return_witnesses": [3, 2, 4, 4] - } -} -``` - -Whereas (with the `distinct` keyword) - -```rust -fn main(x : pub Field, y : pub Field) -> distinct pub [Field; 4] { - let a = 1; - let b = 1; - [x + 1, y, a, b] -} -``` - -compiles to - -```json -{ - //... - "abi": { - //... - "param_witnesses": { "x": [1], "y": [2] }, - //... - "return_witnesses": [3, 4, 5, 6] - } -} -``` diff --git a/test_programs/compile_success_empty/conditional_regression_547/Nargo.toml b/test_programs/execution_success/conditional_regression_547/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/conditional_regression_547/Nargo.toml rename to test_programs/execution_success/conditional_regression_547/Nargo.toml diff --git a/test_programs/compile_success_empty/conditional_regression_547/Prover.toml b/test_programs/execution_success/conditional_regression_547/Prover.toml similarity index 100% rename from test_programs/compile_success_empty/conditional_regression_547/Prover.toml rename to test_programs/execution_success/conditional_regression_547/Prover.toml diff --git a/test_programs/compile_success_empty/conditional_regression_547/src/main.nr b/test_programs/execution_success/conditional_regression_547/src/main.nr similarity index 100% rename from test_programs/compile_success_empty/conditional_regression_547/src/main.nr rename to test_programs/execution_success/conditional_regression_547/src/main.nr diff --git a/test_programs/execution_success/distinct_keyword/Nargo.toml b/test_programs/execution_success/distinct_keyword/Nargo.toml deleted file mode 100644 index 3f1b1386ba7..00000000000 --- a/test_programs/execution_success/distinct_keyword/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "distinct_keyword" -type = "bin" -authors = [""] - -[dependencies] diff --git a/test_programs/execution_success/distinct_keyword/Prover.toml b/test_programs/execution_success/distinct_keyword/Prover.toml deleted file mode 100644 index 07890234a19..00000000000 --- a/test_programs/execution_success/distinct_keyword/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -x = "3" diff --git a/test_programs/execution_success/distinct_keyword/src/main.nr b/test_programs/execution_success/distinct_keyword/src/main.nr deleted file mode 100644 index 8e9b5c008ed..00000000000 --- a/test_programs/execution_success/distinct_keyword/src/main.nr +++ /dev/null @@ -1,4 +0,0 @@ -// Example that uses the distinct keyword -fn main(x: pub Field) -> distinct pub [Field; 2] { - [x + 1, x] -} diff --git a/test_programs/compile_success_empty/main_return/Nargo.toml b/test_programs/execution_success/main_return/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/main_return/Nargo.toml rename to test_programs/execution_success/main_return/Nargo.toml diff --git a/test_programs/compile_success_empty/main_return/Prover.toml b/test_programs/execution_success/main_return/Prover.toml similarity index 100% rename from test_programs/compile_success_empty/main_return/Prover.toml rename to test_programs/execution_success/main_return/Prover.toml diff --git a/test_programs/compile_success_empty/main_return/src/main.nr b/test_programs/execution_success/main_return/src/main.nr similarity index 100% rename from test_programs/compile_success_empty/main_return/src/main.nr rename to test_programs/execution_success/main_return/src/main.nr diff --git a/test_programs/compile_success_empty/simple_array_param/Nargo.toml b/test_programs/execution_success/simple_array_param/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/simple_array_param/Nargo.toml rename to test_programs/execution_success/simple_array_param/Nargo.toml diff --git a/test_programs/compile_success_empty/simple_array_param/Prover.toml b/test_programs/execution_success/simple_array_param/Prover.toml similarity index 100% rename from test_programs/compile_success_empty/simple_array_param/Prover.toml rename to test_programs/execution_success/simple_array_param/Prover.toml diff --git a/test_programs/compile_success_empty/simple_array_param/src/main.nr b/test_programs/execution_success/simple_array_param/src/main.nr similarity index 100% rename from test_programs/compile_success_empty/simple_array_param/src/main.nr rename to test_programs/execution_success/simple_array_param/src/main.nr diff --git a/tooling/nargo_fmt/src/visitor/item.rs b/tooling/nargo_fmt/src/visitor/item.rs index 82cfefba632..a5d042dc71e 100644 --- a/tooling/nargo_fmt/src/visitor/item.rs +++ b/tooling/nargo_fmt/src/visitor/item.rs @@ -6,7 +6,7 @@ use crate::{ }, visitor::expr::{format_seq, NewlineMode}, }; -use noirc_frontend::ast::{Distinctness, NoirFunction, Visibility}; +use noirc_frontend::ast::{NoirFunction, Visibility}; use noirc_frontend::{ hir::resolution::errors::Span, parser::{Item, ItemKind}, @@ -118,10 +118,6 @@ impl super::FmtVisitor<'_> { if let Some(span) = return_type_span { result.push_str(" -> "); - if let Distinctness::Distinct = func.def.return_distinctness { - result.push_str("distinct "); - } - let visibility = match func.def.return_visibility { Visibility::Public => "pub", Visibility::DataBus => "return_data", diff --git a/tooling/nargo_fmt/tests/expected/fn.nr b/tooling/nargo_fmt/tests/expected/fn.nr index 0088dba6a8f..3d231cd3f7f 100644 --- a/tooling/nargo_fmt/tests/expected/fn.nr +++ b/tooling/nargo_fmt/tests/expected/fn.nr @@ -36,7 +36,7 @@ fn apply_binary_field_op( registers: &mut Registers ) -> bool {} -fn main() -> distinct pub [Field; 2] {} +fn main() -> pub [Field; 2] {} fn ret_normal_lambda1() -> ((fn() -> Field)) {} diff --git a/tooling/nargo_fmt/tests/input/fn.nr b/tooling/nargo_fmt/tests/input/fn.nr index 26ff5933802..1c6d201fa39 100644 --- a/tooling/nargo_fmt/tests/input/fn.nr +++ b/tooling/nargo_fmt/tests/input/fn.nr @@ -23,7 +23,7 @@ fn main(tape: [Field; TAPE_LEN], initial_registers: [Field; REGISTER_COUNT], ini fn apply_binary_field_op(lhs: RegisterIndex, rhs: RegisterIndex, result: RegisterIndex, op: u8, registers: &mut Registers) -> bool {} -fn main() -> distinct pub [Field;2] {} +fn main() -> pub [Field;2] {} fn ret_normal_lambda1() -> ((fn() -> Field)) {}