Fix storage deref bug (#1123)

Signed-off-by: Sean Young <sean@mess.org>

src/codegen/expression.rs

@@ -28,7 +28,7 @@ use num_bigint::BigInt;
 use num_traits::{FromPrimitive, One, ToPrimitive, Zero};
 use solang_parser::pt;
 use solang_parser::pt::{CodeLocation, Loc};
-use std::ops::Mul;
+use std::{cmp::Ordering, ops::Mul};
 
 pub fn expression(
     expr: &ast::Expression,
@@ -2636,6 +2636,52 @@ fn array_subscript(
 
     cfg.set_basic_block(in_bounds);
 
+    if let Type::Bytes(array_length) = array_ty.deref_any() {
+        let res_ty = Type::Bytes(1);
+        let from_ty = Type::Bytes(*array_length);
+        let index_ty = Type::Uint(*array_length as u16 * 8);
+
+        let to_width = array_ty.bits(ns);
+        let shift_arg_raw = Expression::Variable(index_loc, coerced_ty.clone(), pos);
+
+        let shift_arg = match index_width.cmp(&to_width) {
+            Ordering::Equal => shift_arg_raw,
+            Ordering::Less => Expression::ZeroExt(*loc, index_ty.clone(), shift_arg_raw.into()),
+            Ordering::Greater => Expression::Trunc(*loc, index_ty.clone(), shift_arg_raw.into()),
+        };
+
+        return Expression::Trunc(
+            *loc,
+            res_ty,
+            Expression::ShiftRight(
+                *loc,
+                from_ty,
+                array.into(),
+                // shift by (array_length - 1 - index) * 8
+                Expression::ShiftLeft(
+                    *loc,
+                    index_ty.clone(),
+                    Box::new(Expression::Subtract(
+                        *loc,
+                        index_ty.clone(),
+                        true,
+                        Expression::NumberLiteral(
+                            *loc,
+                            index_ty.clone(),
+                            BigInt::from_u8(array_length - 1).unwrap(),
+                        )
+                        .into(),
+                        shift_arg.into(),
+                    )),
+                    Expression::NumberLiteral(*loc, index_ty, BigInt::from_u8(3).unwrap()).into(),
+                )
+                .into(),
+                false,
+            )
+            .into(),
+        );
+    }
+
     if let Type::StorageRef(_, ty) = &array_ty {
         let elem_ty = ty.storage_array_elem();
         let slot_ty = ns.storage_type();
@@ -2726,56 +2772,6 @@ fn array_subscript(
         }
     } else {
         match array_ty.deref_memory() {
-            Type::Bytes(array_length) => {
-                let res_ty = Type::Bytes(1);
-                let from_ty = Type::Bytes(*array_length);
-                let index_ty = Type::Uint(*array_length as u16 * 8);
-
-                let to_width = array_ty.bits(ns);
-                let shift_arg_raw = Expression::Variable(index_loc, coerced_ty.clone(), pos);
-
-                let shift_arg = if index_width == to_width {
-                    shift_arg_raw
-                } else if index_width < to_width && array_ty.is_signed_int() {
-                    Expression::SignExt(*loc, array_ty.clone(), Box::new(shift_arg_raw))
-                } else if index_width < to_width && !array_ty.is_signed_int() {
-                    Expression::ZeroExt(*loc, array_ty.clone(), Box::new(shift_arg_raw))
-                } else {
-                    Expression::Trunc(*loc, array_ty.clone(), Box::new(shift_arg_raw))
-                };
-
-                Expression::Trunc(
-                    *loc,
-                    res_ty,
-                    Box::new(Expression::ShiftRight(
-                        *loc,
-                        from_ty,
-                        Box::new(array),
-                        // shift by (array_length - 1 - index) * 8
-                        Box::new(Expression::ShiftLeft(
-                            *loc,
-                            index_ty.clone(),
-                            Box::new(Expression::Subtract(
-                                *loc,
-                                index_ty.clone(),
-                                true,
-                                Box::new(Expression::NumberLiteral(
-                                    *loc,
-                                    index_ty.clone(),
-                                    BigInt::from_u8(array_length - 1).unwrap(),
-                                )),
-                                Box::new(shift_arg),
-                            )),
-                            Box::new(Expression::NumberLiteral(
-                                *loc,
-                                index_ty,
-                                BigInt::from_u8(3).unwrap(),
-                            )),
-                        )),
-                        false,
-                    )),
-                )
-            }
             Type::DynamicBytes | Type::Array(..) => Expression::Subscript(
                 *loc,
                 elem_ty.clone(),

src/lib.rs

@@ -1,7 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
 
-extern crate core;
-
 pub mod abi;
 pub mod codegen;
 #[cfg(feature = "llvm")]
@@ -14,7 +12,6 @@ pub mod standard_json;
 // In Sema, we use result unit for returning early
 // when code-misparses. The error will be added to the namespace diagnostics, no need to have anything but unit
 // as error.
-#[allow(clippy::result_unit_err)]
 pub mod sema;
 
 use file_resolver::FileResolver;

src/sema/builtin.rs

@@ -898,7 +898,7 @@ pub fn is_reserved(fname: &str) -> bool {
 }
 
 /// Resolve a builtin call
-pub fn resolve_call(
+pub(super) fn resolve_call(
     loc: &pt::Loc,
     namespace: Option<&str>,
     id: &str,
@@ -1016,7 +1016,7 @@ pub fn resolve_call(
 /// to handle the special case "abi.decode(foo, (int32, bool, address))" where the
 /// second argument is a type list. The generic expression resolver cannot deal with
 /// this. It is only used in for this specific call.
-pub fn resolve_namespace_call(
+pub(super) fn resolve_namespace_call(
     loc: &pt::Loc,
     namespace: &str,
     name: &str,
@@ -1323,7 +1323,7 @@ pub fn resolve_namespace_call(
 }
 
 /// Resolve a builtin call
-pub fn resolve_method_call(
+pub(super) fn resolve_method_call(
     expr: &Expression,
     id: &pt::Identifier,
     args: &[pt::Expression],

src/sema/diagnostics.rs

@@ -280,14 +280,14 @@ impl Namespace {
     }
 }
 
+#[derive(Default)]
 pub struct RawBuffer {
     buf: Vec<u8>,
 }
 
 impl RawBuffer {
-    #[allow(clippy::new_without_default)]
     pub fn new() -> RawBuffer {
-        RawBuffer { buf: Vec::new() }
+        RawBuffer::default()
     }
 
     pub fn into_string(self) -> String {

src/sema/expression.rs

@@ -135,7 +135,7 @@ impl Expression {
 
     /// Cast from one type to another, which also automatically derefs any Type::Ref() type.
     /// if the cast is explicit (e.g. bytes32(bar) then implicit should be set to false.
-    pub fn cast(
+    pub(super) fn cast(
         &self,
         loc: &pt::Loc,
         to: &Type,
@@ -5285,7 +5285,7 @@ fn array_subscript(
     symtable: &mut Symtable,
     diagnostics: &mut Diagnostics,
 ) -> Result<Expression, ()> {
-    let array = expression(
+    let mut array = expression(
         array,
         context,
         ns,
@@ -5345,11 +5345,18 @@ fn array_subscript(
     // make sure we load the index value if needed
     index = index.cast(&index.loc(), index_ty.deref_any(), true, ns, diagnostics)?;
 
-    match array_ty.deref_any() {
+    let deref_ty = array_ty.deref_any();
+    match deref_ty {
         Type::Bytes(_) | Type::Array(..) | Type::DynamicBytes => {
             if array_ty.is_contract_storage() {
                 let elem_ty = array_ty.storage_array_elem();
 
+                // When subscripting a bytes32 type array, we need to load it. It is not
+                // assignable and the value is calculated by shifting in codegen
+                if let Type::Bytes(_) = deref_ty {
+                    array = array.cast(&array.loc(), deref_ty, true, ns, diagnostics)?;
+                }
+
                 Ok(Expression::Subscript(
                     *loc,
                     elem_ty,
@@ -5360,7 +5367,7 @@ fn array_subscript(
             } else {
                 let elem_ty = array_ty.array_deref();
 
-                let array = array.cast(
+                array = array.cast(
                     &array.loc(),
                     if array_ty.deref_memory().is_fixed_reference_type() {
                         &array_ty

src/sema/namespace.rs

@@ -312,7 +312,7 @@ impl Namespace {
     }
 
     /// Resolve a contract name with namespace
-    pub fn resolve_contract_with_namespace(
+    pub(super) fn resolve_contract_with_namespace(
         &mut self,
         file_no: usize,
         name: &pt::IdentifierPath,
@@ -338,7 +338,7 @@ impl Namespace {
     }
 
     /// Resolve a free function name with namespace
-    pub fn resolve_free_function_with_namespace(
+    pub(super) fn resolve_free_function_with_namespace(
         &mut self,
         file_no: usize,
         name: &pt::IdentifierPath,
@@ -364,7 +364,7 @@ impl Namespace {
     }
 
     /// Resolve an event. We should only be resolving events for emit statements
-    pub fn resolve_event(
+    pub(super) fn resolve_event(
         &mut self,
         file_no: usize,
         contract_no: Option<usize>,
@@ -752,7 +752,7 @@ impl Namespace {
     /// Resolve the parsed data type. The type can be a primitive, enum and also an arrays.
     /// The type for address payable is "address payable" used as a type, and "payable" when
     /// casting. So, we need to know what we are resolving for.
-    pub fn resolve_type(
+    pub(super) fn resolve_type(
         &mut self,
         file_no: usize,
         contract_no: Option<usize>,
@@ -1233,7 +1233,7 @@ impl Namespace {
     // this as an expression, so we need to convert it to Type and check there are
     // no unexpected expressions types.
     #[allow(clippy::vec_init_then_push)]
-    pub fn expr_to_type<'a>(
+    pub(super) fn expr_to_type<'a>(
         &mut self,
         file_no: usize,
         contract_no: Option<usize>,

src/sema/types.rs

@@ -1083,7 +1083,7 @@ impl Type {
     pub fn storage_array_elem(&self) -> Self {
         match self {
             Type::Mapping(_, v) => Type::StorageRef(false, v.clone()),
-            Type::DynamicBytes | Type::String => Type::Bytes(1),
+            Type::DynamicBytes | Type::String | Type::Bytes(_) => Type::Bytes(1),
             Type::Array(ty, dim) if dim.len() > 1 => Type::StorageRef(
                 false,
                 Box::new(Type::Array(ty.clone(), dim[..dim.len() - 1].to_vec())),

tests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.dot

@@ -0,0 +1,18 @@
+strict digraph "tests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol" {
+	contract [label="contract fixed_bytes_subscript_not_assign\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:1:1-9:2"]
+	var [label="variable x\nvisibility internal\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:2:5-14"]
+	storage_test [label="function storage_test\ncontract: fixed_bytes_subscript_not_assign\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:3:5-35\nsignature storage_test()\nvisibility public\nmutability nonpayable"]
+	memory_test [label="function memory_test\ncontract: fixed_bytes_subscript_not_assign\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:6:5-43\nsignature memory_test(bytes32)\nvisibility public\nmutability nonpayable"]
+	parameters [label="parameters\nbytes32 y"]
+	diagnostic [label="found contract 'fixed_bytes_subscript_not_assign'\nlevel Debug\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:1:1-9:2"]
+	diagnostic_8 [label="expression is not assignable\nlevel Error\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:4:9-13"]
+	diagnostic_9 [label="expression is not assignable\nlevel Error\ntests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol:7:9-13"]
+	contracts -> contract
+	contract -> var [label="variable"]
+	contract -> storage_test [label="function"]
+	contract -> memory_test [label="function"]
+	memory_test -> parameters [label="parameters"]
+	diagnostics -> diagnostic [label="Debug"]
+	diagnostics -> diagnostic_8 [label="Error"]
+	diagnostics -> diagnostic_9 [label="Error"]
+}

tests/contract_testcases/substrate/arrays/fixed_bytes_subscript_not_assign.sol

@@ -0,0 +1,9 @@
+contract fixed_bytes_subscript_not_assign {
+    bytes32 x;
+    function storage_test() public {
+        x[1] = 2;
+    }
+    function memory_test(bytes32 y) public {
+        y[1] = 2;
+    }
+}

tests/substrate_tests/arrays.rs

@@ -1459,3 +1459,56 @@ fn alloc_size_from_storage() {
     runtime.function("contfunc", Vec::new());
     assert_eq!(runtime.vm.output, vec![0u64].encode());
 }
+
+#[test]
+fn fixed_bytes() {
+    let mut runtime = build_solidity(
+        r##"
+        contract Storage {
+            bytes32[] data;
+            constructor() {
+                data.push(hex"0000000000000000000000000000000000000000000000000000000000000000");
+                data.push(hex"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f");
+            }
+            function uploadData(uint8 i, uint8 j) public view returns(bytes1) {
+                return(data[j][i]);
+            }
+        }
+        "##,
+    );
+
+    runtime.constructor(0, vec![]);
+
+    for i in 0..32u8 {
+        runtime.function("uploadData", vec![i, 0]);
+        assert_eq!(runtime.vm.output[..], [0]);
+
+        runtime.function("uploadData", vec![i, 1]);
+        assert_eq!(runtime.vm.output[..], [i]);
+    }
+
+    let mut runtime = build_solidity(
+        r##"
+        contract Memory {
+            constructor() {
+            }
+            function uploadData(uint8 i, uint8 j) public view returns(bytes1) {
+                bytes32[] data = new bytes32[](2);
+                data[0] = hex"0000000000000000000000000000000000000000000000000000000000000000";
+                data[1] = hex"000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f";
+                return(data[j][i]);
+            }
+        }
+        "##,
+    );
+
+    runtime.constructor(0, vec![]);
+
+    for i in 0..32u8 {
+        runtime.function("uploadData", vec![i, 0]);
+        assert_eq!(runtime.vm.output[..], [0]);
+
+        runtime.function("uploadData", vec![i, 1]);
+        assert_eq!(runtime.vm.output[..], [i]);
+    }
+}