Improve function names and other minor changes (nfc) (#1470)

This PR has general fixes in Solang, namely:

1. Added an assertion at `fn must_fail` to make sure the called function
returns an error.
2. Rename `edges` to `successors`, as the function returns the
successors of a block.
3. Rename `clone_for_parent_block` to `deep_clone`.
4. Rename `set_maxi_signed` to `get_max_signed`
5. Rename `set_max_unsigned` to `set_max_unsigned`
6. Fix typos in comments and in function names.

Edit: I slipped in another change.
7. Use `.expect("reason")` instead of `if {} else { unreachable!()}` at
`dead_storage.rs`.

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

src/codegen/cfg.rs

@@ -428,7 +428,7 @@ pub enum ASTFunction {
 
 impl BasicBlock {
     /// Fetch the blocks that can be executed after the block passed as argument
-    pub fn edges(&self) -> Vec<usize> {
+    pub fn successors(&self) -> Vec<usize> {
         let mut out = Vec::new();
 
         // out cfg has edge as the last instruction in a block

src/codegen/dead_storage.rs

@@ -140,34 +140,33 @@ fn reaching_definitions(cfg: &mut ControlFlowGraph) -> (Vec<Vec<Vec<Transfer>>>,
             &mut block_vars,
         );
 
-        for edge in cfg.blocks[block_no].edges() {
+        for edge in cfg.blocks[block_no].successors() {
             if !block_vars.contains_key(&edge) {
                 blocks_todo.insert(edge);
                 block_vars.insert(edge, vec![vars.clone()]);
             } else if block_vars[&edge][0] != vars {
                 blocks_todo.insert(edge);
-                if let Some(block_vars) = block_vars.get_mut(&edge) {
-                    // merge incoming vars
-                    for (var_no, defs) in &vars.vars {
-                        if let Some(entry) = block_vars[0].vars.get_mut(var_no) {
-                            for (incoming_def, storage) in defs {
-                                if !entry.contains_key(incoming_def) {
-                                    entry.insert(*incoming_def, storage.clone());
-                                }
+                let block_vars = block_vars
+                    .get_mut(&edge)
+                    .expect("block vars must contain edge");
+                // merge incoming vars
+                for (var_no, defs) in &vars.vars {
+                    if let Some(entry) = block_vars[0].vars.get_mut(var_no) {
+                        for (incoming_def, storage) in defs {
+                            if !entry.contains_key(incoming_def) {
+                                entry.insert(*incoming_def, storage.clone());
                             }
-                        } else {
-                            block_vars[0].vars.insert(*var_no, defs.clone());
                         }
+                    } else {
+                        block_vars[0].vars.insert(*var_no, defs.clone());
                     }
+                }
 
-                    // merge storage stores
-                    for store in &vars.stores {
-                        if !block_vars[0].stores.iter().any(|(def, _)| *def == store.0) {
-                            block_vars[0].stores.push(store.clone());
-                        }
+                // merge storage stores
+                for store in &vars.stores {
+                    if !block_vars[0].stores.iter().any(|(def, _)| *def == store.0) {
+                        block_vars[0].stores.push(store.clone());
                     }
-                } else {
-                    unreachable!();
                 }
             }
         }

src/codegen/dispatch/solana.rs

@@ -207,12 +207,7 @@ pub(crate) fn function_dispatch(
         .enumerate()
         .find(|(_, cfg)| cfg.public && cfg.ty == pt::FunctionTy::Fallback);
 
-    let receive = all_cfg
-        .iter()
-        .enumerate()
-        .find(|(_, cfg)| cfg.public && cfg.ty == pt::FunctionTy::Receive);
-
-    if fallback.is_none() && receive.is_none() {
+    if fallback.is_none() {
         cfg.add(
             &mut vartab,
             Instr::ReturnCode {

src/codegen/reaching_definitions.rs

@@ -63,7 +63,7 @@ pub fn find(cfg: &mut ControlFlowGraph) {
             apply_transfers(transfers, &mut vars);
         }
 
-        for edge in cfg.blocks[block_no].edges() {
+        for edge in cfg.blocks[block_no].successors() {
             if cfg.blocks[edge].defs != vars {
                 blocks_todo.insert(edge);
                 // merge incoming set

src/codegen/solana_accounts/account_collection.rs

@@ -201,7 +201,7 @@ fn check_function(cfg: &ControlFlowGraph, data: &mut RecurseData) {
         // TODO: Block edges is an expensive function, we use it six times throughout the code,
         // perhaps we can just use the dag I calculate during cse.
         // Changes in constant folding would be necessary
-        for edge in cfg.blocks[cur_block].edges() {
+        for edge in cfg.blocks[cur_block].successors() {
             if !visited.contains(&edge) {
                 queue.push_back(edge);
                 visited.insert(edge);

src/codegen/solana_accounts/account_management.rs

@@ -58,7 +58,7 @@ fn traverse_cfg(cfg: &mut ControlFlowGraph, functions: &[Function], ast_no: usiz
             process_instruction(instr, functions, ast_no);
         }
 
-        for edge in cfg.blocks[cur_block].edges() {
+        for edge in cfg.blocks[cur_block].successors() {
             if !visited.contains(&edge) {
                 queue.push_back(edge);
                 visited.insert(edge);

src/codegen/strength_reduce/mod.rs

@@ -15,7 +15,7 @@ use num_traits::{One, ToPrimitive};
 use reaching_values::{reaching_values, transfer};
 use std::collections::{HashMap, HashSet};
 use std::convert::TryInto;
-use value::{is_single_constant, set_max_signed, set_max_unsigned, Value};
+use value::{get_max_signed, get_max_unsigned, is_single_constant, Value};
 
 /**
   Strength Reduce optimization pass - replace expensive arithmetic operations with cheaper ones
@@ -269,7 +269,7 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
 
                     if ty.is_signed_int(ns) {
                         if let (Some(left_max), Some(right_max)) =
-                            (set_max_signed(&left_values), set_max_signed(&right_values))
+                            (get_max_signed(&left_values), get_max_signed(&right_values))
                         {
                             // We can safely replace this with a 64 bit multiply which can be encoded in a single wasm/bpf instruction
                             if (left_max * right_max).to_i64().is_some() {
@@ -299,8 +299,8 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
                             }
                         }
                     } else {
-                        let left_max = set_max_unsigned(&left_values);
-                        let right_max = set_max_unsigned(&right_values);
+                        let left_max = get_max_unsigned(&left_values);
+                        let right_max = get_max_unsigned(&right_values);
 
                         // We can safely replace this with a 64 bit multiply which can be encoded in a single wasm/bpf instruction
                         if left_max * right_max <= BigInt::from(u64::MAX) {
@@ -386,7 +386,7 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
 
                     if ty.is_signed_int(ns) {
                         if let (Some(left_max), Some(right_max)) =
-                            (set_max_signed(&left_values), set_max_signed(&right_values))
+                            (get_max_signed(&left_values), get_max_signed(&right_values))
                         {
                             if left_max.to_i64().is_some() && right_max.to_i64().is_some() {
                                 ns.hover_overrides.insert(
@@ -414,8 +414,8 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
                             }
                         }
                     } else {
-                        let left_max = set_max_unsigned(&left_values);
-                        let right_max = set_max_unsigned(&right_values);
+                        let left_max = get_max_unsigned(&left_values);
+                        let right_max = get_max_unsigned(&right_values);
 
                         // If both values fit into u64, then the result must too
                         if left_max.to_u64().is_some() && right_max.to_u64().is_some() {
@@ -495,7 +495,7 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
 
                     if ty.is_signed_int(ns) {
                         if let (Some(left_max), Some(right_max)) =
-                            (set_max_signed(&left_values), set_max_signed(&right_values))
+                            (get_max_signed(&left_values), get_max_signed(&right_values))
                         {
                             if left_max.to_i64().is_some() && right_max.to_i64().is_some() {
                                 ns.hover_overrides.insert(
@@ -523,8 +523,8 @@ fn expression_reduce(expr: &Expression, vars: &Variables, ns: &mut Namespace) ->
                             }
                         }
                     } else {
-                        let left_max = set_max_unsigned(&left_values);
-                        let right_max = set_max_unsigned(&right_values);
+                        let left_max = get_max_unsigned(&left_values);
+                        let right_max = get_max_unsigned(&right_values);
 
                         // If both values fit into u64, then the result must too
                         if left_max.to_u64().is_some() && right_max.to_u64().is_some() {

src/codegen/strength_reduce/tests.rs

@@ -27,7 +27,7 @@ fn test_highest_bit() {
 }
 
 #[test]
-fn expresson_known_bits() {
+fn expression_known_bits() {
     use crate::Target;
     use solang_parser::pt::Loc;
 

src/codegen/strength_reduce/value.rs

@@ -66,8 +66,8 @@ pub(super) fn is_single_constant(set: &HashSet<Value>) -> Option<BigInt> {
     None
 }
 
-/// Get the maximum unsigned value in a set
-pub(super) fn set_max_signed(set: &HashSet<Value>) -> Option<BigInt> {
+/// Get the maximum signed value in a set
+pub(super) fn get_max_signed(set: &HashSet<Value>) -> Option<BigInt> {
     let mut m = BigInt::zero();
 
     for v in set {
@@ -91,8 +91,8 @@ pub(super) fn set_max_signed(set: &HashSet<Value>) -> Option<BigInt> {
     Some(m)
 }
 
-/// Get the maximum signed value in a set
-pub(super) fn set_max_unsigned(set: &HashSet<Value>) -> BigInt {
+/// Get the maximum unsigned value in a set
+pub(super) fn get_max_unsigned(set: &HashSet<Value>) -> BigInt {
     let mut m = BigInt::zero();
 
     for v in set {

src/codegen/subexpression_elimination/anticipated_expressions.rs

@@ -96,8 +96,7 @@ impl<'a> AnticipatedExpressions<'a> {
                     // of all a block's descendants can be anticipated there.
                     set.union_sets(&cur_set);
                 } else {
-                    self.reverse_sets
-                        .insert(*edge, cur_set.clone_for_parent_block());
+                    self.reverse_sets.insert(*edge, cur_set.deep_clone());
                 }
             }
         }

src/codegen/subexpression_elimination/available_expression_set.rs

@@ -13,8 +13,8 @@ use std::collections::{HashMap, HashSet};
 use std::rc::Rc;
 
 impl<'a, 'b: 'a> AvailableExpressionSet<'a> {
-    /// Clone a set for a given parent block
-    pub fn clone_for_parent_block(&self) -> AvailableExpressionSet<'a> {
+    /// Deep clone a set
+    pub fn deep_clone(&self) -> AvailableExpressionSet<'a> {
         let mut new_set = AvailableExpressionSet {
             expression_memory: HashMap::default(),
             expr_map: self.expr_map.clone(),
@@ -355,7 +355,7 @@ impl<'a, 'b: 'a> AvailableExpressionSet<'a> {
         }
     }
 
-    /// When a reaching definition change, we remove the variable node and all its descendants from
+    /// When a reaching definition changes, we remove the variable node and all its descendants from
     /// the graph
     pub fn kill(&mut self, var_no: usize) {
         let key = ExpressionType::Variable(var_no);

src/codegen/subexpression_elimination/instruction.rs

@@ -201,7 +201,7 @@ impl<'a, 'b: 'a> AvailableExpressionSet<'a> {
         }
     }
 
-    /// Regenerate instructions after that we exchanged common subexpressions for temporaries
+    /// Regenerate instructions after that we exchange common subexpressions for temporaries
     pub fn regenerate_instruction(
         &mut self,
         instr: &'b Instr,

src/codegen/subexpression_elimination/mod.rs

@@ -191,7 +191,7 @@ fn add_neighbor_blocks<'b>(
         if let Some(set) = sets.get_mut(edge) {
             set.intersect_sets(&cur_set, cst);
         } else {
-            sets.insert(*edge, cur_set.clone_for_parent_block());
+            sets.insert(*edge, cur_set.deep_clone());
         }
     }
 }
@@ -238,7 +238,7 @@ fn find_visiting_order(cfg: &ControlFlowGraph) -> CfgAsDag {
 
     while let Some(block_no) = queue.pop_front() {
         order.push((block_no, has_cycle[block_no]));
-        for edge in cfg.blocks[block_no].edges() {
+        for edge in cfg.blocks[block_no].successors() {
             degrees[edge] -= 1;
             if degrees[edge] == 0 {
                 queue.push_back(edge);
@@ -279,7 +279,7 @@ fn cfg_dfs(
 
     stack.insert(block_no);
 
-    for edge in cfg.blocks[block_no].edges() {
+    for edge in cfg.blocks[block_no].successors() {
         degrees[edge] += 1;
         if cfg_dfs(
             edge,

src/codegen/subexpression_elimination/tests.rs

@@ -648,7 +648,7 @@ fn clone() {
     let mut cst = CommonSubExpressionTracker::default();
 
     set.process_instruction(&instr, &mut ave, &mut Some(&mut cst));
-    let set_2 = set.clone_for_parent_block();
+    let set_2 = set.deep_clone();
 
     // Available expressions
     assert!(set_2.find_expression(&unary).is_some());
@@ -810,7 +810,7 @@ fn intersect() {
     cst.set_cur_block(0);
     set.process_instruction(&instr, &mut ave, &mut Some(&mut cst));
     set.process_instruction(&instr2, &mut ave, &mut Some(&mut cst));
-    let mut set_2 = set.clone().clone_for_parent_block();
+    let mut set_2 = set.clone().deep_clone();
     cst.set_cur_block(2);
     ave.set_cur_block(2);
     set.kill(1);

tests/solana.rs

@@ -1743,6 +1743,10 @@ impl<'a, 'b> VmFunction<'a, 'b> {
                 .validate_account_data_heap(&self.accounts[idx].pubkey);
         }
 
+        if let ProgramResult::Ok(num) = result {
+            assert_ne!(num, 0);
+        }
+
         result
     }
 }

tests/solana_tests/math.rs

@@ -116,8 +116,7 @@ fn safe_math() {
         },
     );
 
-    let res = vm
-        .function("mul_test")
+    vm.function("mul_test")
         .arguments(&[
             BorshToken::Uint {
                 width: 256,
@@ -131,9 +130,7 @@ fn safe_math() {
         .accounts(vec![("dataAccount", data_account)])
         .must_fail();
 
-    assert_ne!(res.unwrap(), 0);
-
-    let res = vm.function(
+    vm.function(
         "add_test")
         .arguments(
         &[
@@ -150,10 +147,7 @@ fn safe_math() {
         .accounts(vec![("dataAccount", data_account)])
         .must_fail();
 
-    assert_ne!(res.unwrap(), 0);
-
-    let res = vm
-        .function("sub_test")
+    vm.function("sub_test")
         .arguments(&[
             BorshToken::Uint {
                 width: 256,
@@ -166,6 +160,4 @@ fn safe_math() {
         ])
         .accounts(vec![("dataAccount", data_account)])
         .must_fail();
-
-    assert_ne!(res.unwrap(), 0);
 }

tests/solana_tests/primitives.rs

@@ -967,7 +967,7 @@ fn test_overflow_boundaries() {
 
         let lower_second_op = lower_boundary_minus_two.div(2);
 
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -982,9 +982,7 @@ fn test_overflow_boundaries() {
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
 
-        assert_ne!(res.unwrap(), 0);
-
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -999,9 +997,7 @@ fn test_overflow_boundaries() {
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
 
-        assert_ne!(res.unwrap(), 0);
-
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -1016,9 +1012,7 @@ fn test_overflow_boundaries() {
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
 
-        assert_ne!(res.unwrap(), 0);
-
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -1033,9 +1027,7 @@ fn test_overflow_boundaries() {
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
 
-        assert_ne!(res.unwrap(), 0);
-
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -1049,8 +1041,6 @@ fn test_overflow_boundaries() {
             ])
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
-
-        assert_ne!(res.unwrap(), 0);
     }
 }
 
@@ -1199,7 +1189,7 @@ fn test_overflow_detect_signed() {
         // Calculate a number that when multiplied by first_operand_rand, the result will overflow N bits
         let second_operand_rand = rng.gen_bigint_range(&BigInt::from(2usize), &limit);
 
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -1214,8 +1204,6 @@ fn test_overflow_detect_signed() {
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
 
-        assert_ne!(res.unwrap(), 0);
-
         // The range of values that can be held in signed N bits is [-2^(N-1), 2^(N-1)-1] .
         let mut lower_limit: BigInt = BigInt::from(2_u32).pow((width - 1) as u32);
         lower_limit.sub_assign(1usize);
@@ -1225,7 +1213,7 @@ fn test_overflow_detect_signed() {
         let first_operand_rand =
             rng.gen_bigint_range(&lower_limit, &(lower_limit.clone().div(2usize)).add(1usize));
 
-        let res = contract
+        contract
             .function("mul")
             .arguments(&[
                 BorshToken::Int {
@@ -1239,8 +1227,6 @@ fn test_overflow_detect_signed() {
             ])
             .accounts(vec![("dataAccount", data_account)])
             .must_fail();
-
-        assert_ne!(res.unwrap(), 0);
     }
 }
 
@@ -1275,7 +1261,7 @@ fn test_overflow_detect_unsigned() {
             // Calculate a number that when multiplied by first_operand_rand, the result will overflow N bits
             let second_operand_rand = rng.gen_biguint_range(&BigUint::from(2usize), &limit);
 
-            let res = contract
+            contract
                 .function("mul")
                 .arguments(&[
                     BorshToken::Uint {
@@ -1289,7 +1275,6 @@ fn test_overflow_detect_unsigned() {
                 ])
                 .accounts(vec![("dataAccount", data_account)])
                 .must_fail();
-            assert_ne!(res.unwrap(), 0);
         }
     }
 }