Merge pull request #11456 from ethereum/ice-abstract-contract-mapping-constructor

chriseth · web-flow · commit 3dfa68a5745a · 2021-05-31T15:35:26.000+02:00
Fix ICE related to mapping types in abstract contract constructor
diff --git a/Changelog.md b/Changelog.md
@@ -18,6 +18,7 @@ Bugfixes:
  * AST: Do not output value of Yul literal if it is not a valid UTF-8 string.
  * SMTChecker: Fix internal error on struct constructor with fixed bytes member initialized with string literal.
  * Standard JSON: Properly allow the ``inliner`` setting under ``settings.optimizer.details``.
+ * Type Checker: Fix internal compiler error related to having mapping types in constructor parameter for abstract contracts.
 
 
 AST Changes:
diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp
@@ -588,7 +588,12 @@ bool TypeChecker::visit(VariableDeclaration const& _variable)
 		if (result)
 		{
 			bool isLibraryStorageParameter = (_variable.isLibraryFunctionParameter() && referenceType->location() == DataLocation::Storage);
-			bool callDataCheckRequired = ((_variable.isConstructorParameter() || _variable.isPublicCallableParameter()) && !isLibraryStorageParameter);
+			// We skip the calldata check for abstract contract constructors.
+			bool isAbstractConstructorParam = _variable.isConstructorParameter() && m_currentContract && m_currentContract->abstract();
+			bool callDataCheckRequired =
+				!isAbstractConstructorParam &&
+				(_variable.isConstructorParameter() || _variable.isPublicCallableParameter()) &&
+				!isLibraryStorageParameter;
 			if (callDataCheckRequired)
 			{
 				if (!referenceType->interfaceType(false))
diff --git a/test/libsolidity/semanticTests/types/array_mapping_abstract_constructor_param.sol b/test/libsolidity/semanticTests/types/array_mapping_abstract_constructor_param.sol
@@ -0,0 +1,19 @@
+abstract contract A {
+	constructor (mapping (uint => uint) [] storage m) {
+		m.push();
+		m[0][1] = 2;
+	}
+}
+
+contract C is A {
+	mapping(uint => mapping (uint => uint) []) public m;
+
+	constructor() A(m[1]) {
+	}
+}
+// ====
+// compileViaYul: also
+// ----
+// m(uint256,uint256,uint256): 0, 0, 0 -> FAILURE
+// m(uint256,uint256,uint256): 1, 0, 1 -> 2
+// m(uint256,uint256,uint256): 1, 0, 5 -> 0
diff --git a/test/libsolidity/semanticTests/types/mapping_abstract_constructor_param.sol b/test/libsolidity/semanticTests/types/mapping_abstract_constructor_param.sol
@@ -0,0 +1,17 @@
+abstract contract A {
+    constructor (mapping (uint => uint) storage m) {
+        m[5] = 20;
+    }
+}
+
+contract C is A {
+    mapping (uint => uint) public m;
+
+    constructor() A(m) {
+    }
+}
+// ====
+// compileViaYul: also
+// ----
+// m(uint256): 1 -> 0
+// m(uint256): 5 -> 20
diff --git a/test/libsolidity/semanticTests/types/struct_mapping_abstract_constructor_param.sol b/test/libsolidity/semanticTests/types/struct_mapping_abstract_constructor_param.sol
@@ -0,0 +1,26 @@
+struct S {
+	mapping (uint => uint) m;
+}
+
+abstract contract A {
+	constructor (S storage s) {
+		s.m[5] = 16;
+	}
+}
+
+contract C is A {
+	mapping(uint => S) m;
+
+	constructor() A(m[1]) {
+	}
+
+	function getM(uint a, uint b) external returns (uint) {
+		return m[a].m[b];
+	}
+}
+// ====
+// compileViaYul: also
+// ----
+// getM(uint256,uint256): 0, 0 -> 0
+// getM(uint256,uint256): 1, 5 -> 0x10
+// getM(uint256,uint256): 1, 0 -> 0
diff --git a/test/libsolidity/syntaxTests/types/mapping/abstract_contructor_param.sol b/test/libsolidity/syntaxTests/types/mapping/abstract_contructor_param.sol
@@ -0,0 +1,4 @@
+abstract contract A {
+    constructor (mapping (uint => uint) [] storage) { }
+}
+// ----
diff --git a/test/libsolidity/syntaxTests/types/mapping/constructor_parameter.sol b/test/libsolidity/syntaxTests/types/mapping/constructor_parameter.sol
@@ -0,0 +1,5 @@
+contract A {
+    constructor (mapping (uint => uint) [] storage) { }
+}
+// ----
+// TypeError 3644: (30-63): This parameter has a type that can only be used internally. You can make the contract abstract to avoid this problem.

Original file line number	Diff line number	Diff line change
`@@ -588,7 +588,12 @@ bool TypeChecker::visit(VariableDeclaration const& _variable)`
`588`	`588`	`if (result)`
`589`	`589`	`{`
`590`	`590`	`bool isLibraryStorageParameter = (_variable.isLibraryFunctionParameter() && referenceType->location() == DataLocation::Storage);`
`591`		`- bool callDataCheckRequired = ((_variable.isConstructorParameter() \|\| _variable.isPublicCallableParameter()) && !isLibraryStorageParameter);`
	`591`	`+ // We skip the calldata check for abstract contract constructors.`
	`592`	`+ bool isAbstractConstructorParam = _variable.isConstructorParameter() && m_currentContract && m_currentContract->abstract();`
	`593`	`+ bool callDataCheckRequired =`
	`594`	`+ !isAbstractConstructorParam &&`
	`595`	`+ (_variable.isConstructorParameter() \|\| _variable.isPublicCallableParameter()) &&`
	`596`	`+ !isLibraryStorageParameter;`
`592`	`597`	`if (callDataCheckRequired)`
`593`	`598`	`{`
`594`	`599`	`if (!referenceType->interfaceType(false))`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +abstract contract A {
 +    constructor (mapping (uint => uint) [] storage) { }
 +}
 +// ----