bugc: reject == and != on arrays and structs (#397)

* bugc: reject == and != on arrays and structs

As in Solidity, `==` and `!=` on an array (fixed or dynamic), a struct
or a mapping are now type errors. Before, an array or struct in memory,
or a struct copied from storage, compared by its address in memory, so
equal contents in two places were unequal, and an array in storage
failed in IR generation. Bytes and strings still compare by contents.

* bugc: changelog for #397

* bugc: say what main did with a mapping's ==

Author: gnidan

packages/bugc/CHANGELOG.md

@@ -43,6 +43,12 @@ support. Changes to the specification itself are tracked in the root
 - `==`, `!=` and a mapping key with bytes or a string in calldata are
   now type errors. Before, they compared or hashed the calldata
   reference's word (offset and length), not the bytes ([#379]).
+- `==` and `!=` on arrays, structs and mappings are now type errors,
+  as in Solidity. Before, an array or struct in memory, or a struct
+  copied from storage, compared by its address in memory, so equal
+  contents in two places were unequal; a mapping compared its own
+  slot, so it always equaled itself; an array in storage failed in IR
+  generation ([#397]).
 - A struct member of a struct in storage now takes all the slots its
   struct needs, as in Solidity, and the member after it starts at the
   next free slot. Before, it took one slot, so the members after a
@@ -581,3 +587,4 @@ First publication.
 [#388]: https://github.com/ethdebug/format/pull/388
 [#391]: https://github.com/ethdebug/format/pull/391
 [#395]: https://github.com/ethdebug/format/pull/395
+[#397]: https://github.com/ethdebug/format/pull/397

packages/bugc/src/typechecker/checker.test.ts

@@ -194,6 +194,61 @@ describe("checkProgram", () => {
       });
     });
 
+    it("should type check == and != on elementary types", () => {
+      const result = check(`
+        name Test;
+        storage { [0] owner: address; [1] flag: bool; [2] n: uint256; }
+        code {
+          let b1 = owner == msg.sender;
+          let b2 = flag != true;
+          let b3 = n == 1;
+          let b4 = n != n;
+        }
+      `);
+
+      expect(result.success).toBe(true);
+      expect(Result.hasMessages(result)).toBe(false);
+    });
+
+    it("should reject == and != on arrays, structs and mappings", () => {
+      const comparisons: Record<string, [string, string]> = {
+        "a dynamic array in storage": ["nums == nums", "array<uint256>"],
+        "a fixed array in storage": ["fixed != fixed", "array<uint256, 3>"],
+        "an array in memory": ["local == local", "array<uint256>"],
+        "an array and a storage array": ["local != nums", "array<uint256>"],
+        "a struct in storage": ["point == point", "Point"],
+        "a struct and a mapping value": ["point != points[0]", "Point"],
+        "a mapping": ["balances == balances", "mapping<address, uint256>"],
+      };
+
+      for (const [name, [comparison, type]] of Object.entries(comparisons)) {
+        const result = check(`
+          name Test;
+          define {
+            struct Point { x: uint256; y: uint256; };
+          }
+          storage {
+            [0] nums: array<uint256>;
+            [1] fixed: array<uint256, 3>;
+            [4] point: Point;
+            [6] points: mapping<uint256, Point>;
+            [7] balances: mapping<address, uint256>;
+          }
+          code {
+            let local: array<uint256> = [1, 2, 3];
+            let b = ${comparison};
+          }
+        `);
+
+        expect(result.success, name).toBe(false);
+        expect(Result.countErrors(result), name).toBe(1);
+        expect(result).toHaveMessage({
+          severity: Severity.Error,
+          message: `Cannot compare ${type}`,
+        });
+      }
+    });
+
     it("should type check logical operators", () => {
       const result = check(`
         name Test;

packages/bugc/src/typechecker/expressions.ts

@@ -250,15 +250,11 @@ export const expressionChecker: Pick<Visitor<Report, Context>, "expression"> = {
 
           case "==":
           case "!=":
-            // Bytes or a string in calldata is a word of offset and
-            // length, which compares with no other value's
-            if (
-              Type.Elementary.isCalldata(leftType) ||
-              Type.Elementary.isCalldata(rightType)
-            ) {
-              const type = Type.Elementary.isCalldata(leftType)
-                ? leftType
-                : rightType;
+            // Arrays, structs and mappings do not compare, as in
+            // Solidity. Bytes or a string in calldata is a word of
+            // offset and length, which compares with no other value's
+            if (isUncomparable(leftType) || isUncomparable(rightType)) {
+              const type = isUncomparable(leftType) ? leftType : rightType;
               errors.push(
                 new TypeError(
                   `Cannot compare ${Type.format(type)}`,
@@ -1210,6 +1206,20 @@ function isValidCast(fromType: Type, toType: Type): boolean {
   return false;
 }
 
+/**
+ * Whether `==` and `!=` reject an operand of this type: an array, a
+ * struct, a mapping, or bytes or a string in calldata
+ */
+function isUncomparable(type: Type | undefined): type is Type {
+  return (
+    !!type &&
+    (Type.isArray(type) ||
+      Type.isStruct(type) ||
+      Type.isMapping(type) ||
+      Type.Elementary.isCalldata(type))
+  );
+}
+
 const integerOperators = new Set([
   "+",
   "-",

packages/web/docs/implementation-guides/compiler/case-study-bug.mdx

@@ -114,7 +114,8 @@ data of each operand. As for a mapping key, this assumes that two
 different contents do not have the same hash. Both operands must have
 the same type, in memory: bytes or a string in calldata do not
 compare, so cast or copy them to memory first. A `bytesN` is a value,
-and compares as a word.
+and compares as a word. Arrays, structs and mappings do not compare,
+as in Solidity: `==` and `!=` on them are type errors.
 
 ```bug
 let a: bytes = msg.data[0:4];