From 38c732c9dfc0c52cf4f020df6dea3bf628c376bb Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 8 Jan 2024 19:47:14 +0000 Subject: [PATCH] chore: remove some empty `unreachable!`s (#3982) # Description ## Problem\* Resolves ## Summary\* This PR adds some strings to unreachable panics so that if they manage to get run then we get a usable error message. This should be a noop. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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. --- acvm-repo/acir/src/circuit/mod.rs | 2 +- acvm-repo/brillig_vm/src/black_box.rs | 23 ++++++++++++++----- .../src/ssa/function_builder/data_bus.rs | 8 ++++--- .../src/ssa/opt/constant_folding.rs | 4 ++-- .../src/ssa/opt/flatten_cfg.rs | 4 ++-- compiler/noirc_frontend/src/ast/mod.rs | 15 +++++++++++- .../noirc_frontend/src/hir/type_check/stmt.rs | 2 +- compiler/noirc_frontend/src/parser/parser.rs | 4 ++-- tooling/noirc_abi/src/lib.rs | 21 +++++++++-------- 9 files changed, 55 insertions(+), 28 deletions(-) diff --git a/acvm-repo/acir/src/circuit/mod.rs b/acvm-repo/acir/src/circuit/mod.rs index df58b949b85..a1e63b4fe54 100644 --- a/acvm-repo/acir/src/circuit/mod.rs +++ b/acvm-repo/acir/src/circuit/mod.rs @@ -99,7 +99,7 @@ impl FromStr for OpcodeLocation { let brillig_index = parts[1].parse()?; Ok(OpcodeLocation::Brillig { acir_index, brillig_index }) } - _ => unreachable!(), + _ => unreachable!("`OpcodeLocation` has too many components"), } } diff --git a/acvm-repo/brillig_vm/src/black_box.rs b/acvm-repo/brillig_vm/src/black_box.rs index 66d40c48aec..32a93298d97 100644 --- a/acvm-repo/brillig_vm/src/black_box.rs +++ b/acvm-repo/brillig_vm/src/black_box.rs @@ -85,11 +85,7 @@ pub(crate) fn evaluate_black_box( signature, result: result_register, } => { - let bb_func = match op { - BlackBoxOp::EcdsaSecp256k1 { .. } => BlackBoxFunc::EcdsaSecp256k1, - BlackBoxOp::EcdsaSecp256r1 { .. } => BlackBoxFunc::EcdsaSecp256r1, - _ => unreachable!(), - }; + let bb_func = black_box_function_from_op(op); let public_key_x: [u8; 32] = to_u8_vec(read_heap_array( memory, @@ -124,7 +120,7 @@ pub(crate) fn evaluate_black_box( BlackBoxOp::EcdsaSecp256r1 { .. } => { ecdsa_secp256r1_verify(&hashed_msg, &public_key_x, &public_key_y, &signature)? } - _ => unreachable!(), + _ => unreachable!("`BlackBoxOp` is guarded against being a non-ecdsa operation"), }; registers.set(*result_register, result.into()); @@ -178,6 +174,21 @@ pub(crate) fn evaluate_black_box( } } +fn black_box_function_from_op(op: &BlackBoxOp) -> BlackBoxFunc { + match op { + BlackBoxOp::Sha256 { .. } => BlackBoxFunc::SHA256, + BlackBoxOp::Blake2s { .. } => BlackBoxFunc::Blake2s, + BlackBoxOp::Keccak256 { .. } => BlackBoxFunc::Keccak256, + BlackBoxOp::HashToField128Security { .. } => BlackBoxFunc::HashToField128Security, + BlackBoxOp::EcdsaSecp256k1 { .. } => BlackBoxFunc::EcdsaSecp256k1, + BlackBoxOp::EcdsaSecp256r1 { .. } => BlackBoxFunc::EcdsaSecp256r1, + BlackBoxOp::SchnorrVerify { .. } => BlackBoxFunc::SchnorrVerify, + BlackBoxOp::PedersenCommitment { .. } => BlackBoxFunc::PedersenCommitment, + BlackBoxOp::PedersenHash { .. } => BlackBoxFunc::PedersenHash, + BlackBoxOp::FixedBaseScalarMul { .. } => BlackBoxFunc::FixedBaseScalarMul, + } +} + #[cfg(test)] mod test { use acir::brillig::BlackBoxOp; diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs index 7f337089321..58d9e0f6d15 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/data_bus.rs @@ -84,7 +84,6 @@ impl FunctionBuilder { databus.values.push_back(value); databus.index += 1; } - Type::Reference(_) => unreachable!(), Type::Array(typ, len) => { assert!(typ.len() == 1, "unsupported composite type"); databus.map.insert(value, databus.index); @@ -98,8 +97,11 @@ impl FunctionBuilder { self.add_to_data_bus(element, databus); } } - Type::Slice(_) => unreachable!(), - Type::Function => unreachable!(), + Type::Reference(_) => { + unreachable!("Attempted to add invalid type (reference) to databus") + } + Type::Slice(_) => unreachable!("Attempted to add invalid type (slice) to databus"), + Type::Function => unreachable!("Attempted to add invalid type (function) to databus"), } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 57c93c17fc4..7d345a9a4ab 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -280,11 +280,11 @@ mod test { let return_value_id = match entry_block.unwrap_terminator() { TerminatorInstruction::Return { return_values, .. } => return_values[0], - _ => unreachable!(), + _ => unreachable!("Should have terminator instruction"), }; let return_element = match &main.dfg[return_value_id] { Value::Array { array, .. } => array[0], - _ => unreachable!(), + _ => unreachable!("Return type should be array"), }; // The return element is expected to refer to the new add instruction result. assert_eq!(main.dfg.resolve(new_add_instr_result), main.dfg.resolve(return_element)); diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index fdd7c66684c..6bdf2ab1c0a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1102,7 +1102,7 @@ mod test { let main = ssa.main(); let ret = match main.dfg[main.entry_block()].terminator() { Some(TerminatorInstruction::Return { return_values, .. }) => return_values[0], - _ => unreachable!(), + _ => unreachable!("Should have terminator instruction"), }; let merged_values = get_all_constants_reachable_from_instruction(&main.dfg, ret); @@ -1473,7 +1473,7 @@ mod test { None => unreachable!("Expected constant 200 for return value"), } } - _ => unreachable!(), + _ => unreachable!("Should have terminator instruction"), } } } diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 5c10d3fe8f0..d9af5893024 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -257,7 +257,20 @@ impl UnresolvedTypeExpression { BinaryOpKind::Multiply => BinaryTypeOperator::Multiplication, BinaryOpKind::Divide => BinaryTypeOperator::Division, BinaryOpKind::Modulo => BinaryTypeOperator::Modulo, - _ => unreachable!(), // impossible via operator_allowed check + + BinaryOpKind::Equal + | BinaryOpKind::NotEqual + | BinaryOpKind::Less + | BinaryOpKind::LessEqual + | BinaryOpKind::Greater + | BinaryOpKind::GreaterEqual + | BinaryOpKind::And + | BinaryOpKind::Or + | BinaryOpKind::Xor + | BinaryOpKind::ShiftRight + | BinaryOpKind::ShiftLeft => { + unreachable!("impossible via `operator_allowed` check") + } }; Ok(UnresolvedTypeExpression::BinaryOperation(lhs, op, rhs, expr.span)) } diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs index 0cbfafb4c79..1c4b08af350 100644 --- a/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/compiler/noirc_frontend/src/hir/type_check/stmt.rs @@ -147,7 +147,7 @@ impl<'interner> TypeChecker<'interner> { // Must push new lvalue to the interner, we've resolved any field indices self.interner.update_statement(stmt_id, |stmt| match stmt { HirStatement::Assign(assign) => assign.lvalue = new_lvalue, - _ => unreachable!(), + _ => unreachable!("statement is known to be assignment"), }); let span = self.interner.expr_span(&assign_stmt.expression); diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 660c85759b9..b149eb24f07 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -312,7 +312,7 @@ fn function_return_type() -> impl NoirParser<((Distinctness, Visibility), Functi fn attribute() -> impl NoirParser { token_kind(TokenKind::Attribute).map(|token| match token { Token::Attribute(attribute) => attribute, - _ => unreachable!(), + _ => unreachable!("Parser should have already errored due to token not being an attribute"), }) } @@ -369,7 +369,7 @@ fn function_parameters<'a>(allow_self: bool) -> impl NoirParser> + 'a /// This parser always parses no input and fails fn nothing() -> impl NoirParser { - one_of([]).map(|_| unreachable!()) + one_of([]).map(|_| unreachable!("parser should always error")) } fn self_parameter() -> impl NoirParser { diff --git a/tooling/noirc_abi/src/lib.rs b/tooling/noirc_abi/src/lib.rs index 884c49c0106..066b1635ced 100644 --- a/tooling/noirc_abi/src/lib.rs +++ b/tooling/noirc_abi/src/lib.rs @@ -157,11 +157,7 @@ impl AbiType { .expect("Cannot have variable sized strings as a parameter to main"); Self::String { length: size } } - Type::FmtString(_, _) => unreachable!("format strings cannot be used in the abi"), - Type::Error => unreachable!(), - Type::Unit => unreachable!(), - Type::Constant(_) => unreachable!(), - Type::TraitAsType(..) => unreachable!(), + Type::Struct(def, ref args) => { let struct_type = def.borrow(); let fields = struct_type.get_fields(args); @@ -175,12 +171,17 @@ impl AbiType { let fields = vecmap(fields, |typ| Self::from_type(context, typ)); Self::Tuple { fields } } - Type::TypeVariable(_, _) => unreachable!(), - Type::NamedGeneric(..) => unreachable!(), - Type::Forall(..) => unreachable!(), - Type::Function(_, _, _) => unreachable!(), + Type::Error + | Type::Unit + | Type::Constant(_) + | Type::TraitAsType(..) + | Type::TypeVariable(_, _) + | Type::NamedGeneric(..) + | Type::Forall(..) + | Type::NotConstant + | Type::Function(_, _, _) => unreachable!("Type cannot be used in the abi"), + Type::FmtString(_, _) => unreachable!("format strings cannot be used in the abi"), Type::MutableReference(_) => unreachable!("&mut cannot be used in the abi"), - Type::NotConstant => unreachable!(), } }