Keep constants for namespaced compilation (#1046)

packages/core/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## 1.34.2 (2024-07-18)
+
+- Fix Hardhat compile error when constants have references to other constants. ([#1046](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1046))
+
 ## 1.34.1 (2024-06-18)
 
 - Fix unexpected validation error when function parameter has internal function pointer. ([#1038](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1038))

packages/core/contracts/test/NamespacedToModify.sol

@@ -116,6 +116,10 @@ contract Consumer {
   }
 }
 
+contract HasConstantWithSelector {
+  bytes4 constant CONTRACT_CONSTANT_USING_SELECTOR = HasFunction.foo.selector;
+}
+
 function plusTwo(uint x) pure returns (uint) {
   return x + 2;
 }
@@ -176,17 +180,38 @@ enum FreeEnum { MyEnum }
  */
 error CustomErrorOutsideContract(Example example);
 
+int8 constant MAX_SIZE_C = 2;
+
 contract StructArrayUsesConstant {
   uint16 private constant MAX_SIZE = 10;
 
   struct NotNamespaced {
     uint16 a;
     uint256[MAX_SIZE] b;
+    uint256[MAX_SIZE_C] c;
   }
 
   /// @custom:storage-location erc7201:uses.constant
   struct MainStorage {
     uint256 x;
     uint256[MAX_SIZE] y;
+    uint256[MAX_SIZE_C] c;
   }
+}
+
+address constant MY_ADDRESS = address(0);
+uint constant CONVERTED_ADDRESS = uint160(MY_ADDRESS);
+
+interface IDummy {
+}
+
+contract UsesAddress {
+  IDummy public constant MY_CONTRACT = IDummy(MY_ADDRESS);
+}
+
+contract HasFunctionWithRequiredReturn {
+    struct S { uint x; }
+    function foo(S calldata s) internal pure returns (S calldata) {
+        return s;
+    }
 }

packages/core/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openzeppelin/upgrades-core",
-  "version": "1.34.1",
+  "version": "1.34.2",
   "description": "",
   "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/core",
   "license": "MIT",

packages/core/src/utils/make-namespaced.test.ts

@@ -38,7 +38,7 @@ async function testMakeNamespaced(
 
   // Run hardhat compile on the modified input and make sure it has no errors
   const modifiedOutput = await hardhatCompile(modifiedInput, solcVersion);
-  t.is(modifiedOutput.errors, undefined);
+  t.is(modifiedOutput.errors, undefined, JSON.stringify(modifiedInput.sources, null, 2));
 
   normalizeIdentifiers(modifiedInput);
   t.snapshot(modifiedInput);

packages/core/src/utils/make-namespaced.test.ts.md

@@ -46,22 +46,22 @@ Generated by [AVA](https://avajs.dev).
               } SecondaryStorage $SecondaryStorage_random;␊
           ␊
               /// @notice some natspec␊
-              enum $astId_id_random { dummy }␊
+              function foo() public {}␊
           ␊
               /// @param a docs␊
-              enum $astId_id_random { dummy }␊
+              function foo1(uint a) public {}␊
           ␊
               /// @param a docs␊
-              enum $astId_id_random { dummy }␊
+              function foo2(uint a) internal {}␊
               struct MyStruct { uint b; }␊
           ␊
               // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff));␊
               bytes32 private constant MAIN_STORAGE_LOCATION =␊
                   0x183a6125c38840424c4a85fa12bab2ab606c4b6d0e7cc73c0c06ba5300eab500;␊
           ␊
-              enum $astId_id_random { dummy }␊
+              function _getMainStorage() private pure returns (bool) {}␊
           ␊
-              enum $astId_id_random { dummy }␊
+              function _getXTimesY() internal view returns (bool) {}␊
           ␊
               /// @notice standlone natspec␊
           ␊
@@ -77,14 +77,14 @@ Generated by [AVA](https://avajs.dev).
               /**␊
                * doc block without natspec␊
                */␊
-              enum $astId_id_random { dummy }␊
+              function foo3() public {}␊
           ␊
               /**␊
                * doc block with natspec␊
                *␊
                * @notice some natspec␊
                */␊
-              enum $astId_id_random { dummy }␊
+              function foo4() public {}␊
           ␊
               /**␊
                * @dev a custom error inside a contract␊
@@ -99,35 +99,39 @@ Generated by [AVA](https://avajs.dev).
           }␊
           ␊
           contract HasFunction {␊
-            enum $astId_id_random { dummy }␊
-            enum $astId_id_random { dummy }␊
+            ␊
+            function foo() pure public returns (bool) {}␊
           }␊
           ␊
           contract UsingFunction is HasFunction {␊
             enum $astId_id_random { dummy }␊
           }␊
           ␊
-          enum FreeFunctionUsingSelector { dummy }␊
+          function FreeFunctionUsingSelector() pure returns (bool) {}␊
           ␊
-          enum CONSTANT_USING_SELECTOR { dummy }␊
+          bytes4 constant CONSTANT_USING_SELECTOR = HasFunction.foo.selector;␊
           ␊
           library Lib {␊
-            enum $astId_id_random { dummy }␊
+            function usingSelector() pure public returns (bool) {}␊
           ␊
-            enum $astId_id_random { dummy }␊
+            function plusOne(uint x) pure public returns (bool) {}␊
           }␊
           ␊
           contract Consumer {␊
             enum $astId_id_random { dummy }␊
           ␊
-            enum $astId_id_random { dummy }␊
+            function usingFreeFunction() pure public returns (bool) {}␊
           ␊
-            enum $astId_id_random { dummy }␊
+            function usingConstant() pure public returns (bool) {}␊
           ␊
-            enum $astId_id_random { dummy }␊
+            function usingLibraryFunction() pure public returns (bool) {}␊
           }␊
           ␊
-          enum plusTwo { dummy }␊
+          contract HasConstantWithSelector {␊
+            bytes4 constant CONTRACT_CONSTANT_USING_SELECTOR = HasFunction.foo.selector;␊
+          }␊
+          ␊
+          function plusTwo(uint x) pure returns (bool) {}␊
           ␊
           /**␊
            * @notice originally orphaned natspec␊
@@ -137,7 +141,7 @@ Generated by [AVA](https://avajs.dev).
            * @dev plusThree␊
            * @param x x␊
            */␊
-          enum plusThree { dummy }␊
+          function plusThree(uint x) pure returns (bool) {}␊
           ␊
           /// @notice originally orphaned natspec 2␊
           ␊
@@ -146,9 +150,9 @@ Generated by [AVA](https://avajs.dev).
            * @param x x␊
            * @param y y␊
            */␊
-          enum $astId_id_random { dummy }␊
+          function plusThree(uint x, uint y) pure returns (bool) {}␊
           ␊
-          enum originallyNoDocumentation { dummy }␊
+          function originallyNoDocumentation() pure {}␊
           ␊
           /**␊
            * @param foo foo␊
@@ -158,7 +162,7 @@ Generated by [AVA](https://avajs.dev).
           contract UsingForDirectives {␊
             enum $astId_id_random { dummy }␊
           ␊
-            enum $astId_id_random { dummy }␊
+            function usingFor(uint x) pure public returns (bool) {}␊
           }␊
           ␊
           /**␊
@@ -179,19 +183,38 @@ Generated by [AVA](https://avajs.dev).
            */␊
           enum CustomErrorOutsideContract { dummy }␊
           ␊
+          int8 constant MAX_SIZE_C = 2;␊
+          ␊
           contract StructArrayUsesConstant {␊
             uint16 private constant MAX_SIZE = 10;␊
           ␊
             struct NotNamespaced {␊
               uint16 a;␊
               uint256[MAX_SIZE] b;␊
+              uint256[MAX_SIZE_C] c;␊
             }␊
           ␊
             /// @custom:storage-location erc7201:uses.constant␊
             struct MainStorage {␊
               uint256 x;␊
               uint256[MAX_SIZE] y;␊
+              uint256[MAX_SIZE_C] c;␊
             } MainStorage $MainStorage_random;␊
+          }␊
+          ␊
+          address constant MY_ADDRESS = address(0);␊
+          uint constant CONVERTED_ADDRESS = uint160(MY_ADDRESS);␊
+          ␊
+          interface IDummy {␊
+          }␊
+          ␊
+          contract UsesAddress {␊
+            IDummy public constant MY_CONTRACT = IDummy(MY_ADDRESS);␊
+          }␊
+          ␊
+          contract HasFunctionWithRequiredReturn {␊
+              struct S { uint x; }␊
+              function foo(S calldata s) internal pure returns (bool) {}␊
           }`,
         },
         'contracts/test/NamespacedToModifyImported.sol': {
@@ -234,8 +257,8 @@ Generated by [AVA](https://avajs.dev).
           pragma solidity 0.7.6;␊
            ␊
           contract HasFunction {␊
-            enum $astId_id_random { dummy }␊
-            enum $astId_id_random { dummy }␊
+            ␊
+            function foo() pure public returns (bool) {}␊
           }␊
           ␊
           contract UsingFunction is HasFunction {␊

packages/core/src/utils/make-namespaced.ts

@@ -3,6 +3,7 @@ import { Node } from 'solidity-ast/node';
 import { SolcInput, SolcOutput } from '../solc-api';
 import { getStorageLocationAnnotation } from '../storage/namespace';
 import { assert } from './assert';
+import { FunctionDefinition } from 'solidity-ast';
 
 const OUTPUT_SELECTION = {
   '*': {
@@ -40,7 +41,6 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI
 
     const orig = Buffer.from(source.content, 'utf8');
 
-    const replacedIdentifiers = new Set<string>();
     const modifications: Modification[] = [];
 
     for (const node of output.sources[sourcePath].ast.nodes) {
@@ -60,6 +60,10 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI
           const contractNodes = contractDef.nodes;
           for (const contractNode of contractNodes) {
             switch (contractNode.nodeType) {
+              case 'FunctionDefinition': {
+                replaceFunction(contractNode, orig, modifications);
+                break;
+              }
               case 'VariableDeclaration': {
                 // If variable is a constant, keep it since it may be referenced in a struct
                 if (contractNode.constant) {
@@ -69,7 +73,6 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI
               }
               case 'ErrorDefinition':
               case 'EventDefinition':
-              case 'FunctionDefinition':
               case 'ModifierDefinition':
               case 'UsingForDirective': {
                 // Replace with an enum based on astId (the original name is not needed, since nothing should reference it)
@@ -100,27 +103,30 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI
           break;
         }
 
-        // - UsingForDirective isn't needed, but it might have NatSpec documentation which is not included in the AST.
-        //   We convert it to a dummy enum to avoid orphaning any possible documentation.
-        // - ErrorDefinition, FunctionDefinition, and VariableDeclaration might be imported by other files, so they cannot be deleted.
+        case 'FunctionDefinition': {
+          replaceFunction(node, orig, modifications);
+          break;
+        }
+
+        // - VariableDeclaration and ErrorDefinition might be imported by other files, so they cannot be deleted.
         //   However, we need to remove their values to avoid referencing other deleted nodes.
-        //   We do this by converting them to dummy enums, but avoiding duplicate names.
-        case 'UsingForDirective':
-        case 'ErrorDefinition':
-        case 'FunctionDefinition':
+        //   We do this by converting them to dummy enums with the same name.
         case 'VariableDeclaration': {
-          // If an identifier with the same name was not previously written, replace with a dummy enum using its name.
-          // Otherwise replace with an enum based on astId to avoid duplicate names, which can happen if there was overloading.
-          // This does not need to check all identifiers from the original contract, since the original compilation
-          // should have failed if there were conflicts in the first place.
-          if ('name' in node && !replacedIdentifiers.has(node.name)) {
-            modifications.push(makeReplace(node, orig, toDummyEnumWithName(node.name)));
-            replacedIdentifiers.add(node.name);
-          } else {
-            modifications.push(makeReplace(node, orig, toDummyEnumWithAstId(node.id)));
+          // If variable is a constant, keep it since it may be referenced in a struct
+          if (node.constant) {
+            break;
           }
+          // Otherwise, fall through to convert to dummy enum
+        }
+        case 'ErrorDefinition': {
+          modifications.push(makeReplace(node, orig, toDummyEnumWithName(node.name)));
           break;
         }
+        // - UsingForDirective isn't needed, but it might have NatSpec documentation which is not included in the AST.
+        //   We convert it to a dummy enum to avoid orphaning any possible documentation.
+        case 'UsingForDirective':
+          modifications.push(makeReplace(node, orig, toDummyEnumWithAstId(node.id)));
+          break;
         case 'EnumDefinition':
         case 'ImportDirective':
         case 'PragmaDirective':
@@ -158,6 +164,26 @@ function toDummyEnumWithAstId(astId: number) {
   return `enum $astId_${astId}_${(Math.random() * 1e6).toFixed(0)} { dummy }`;
 }
 
+function replaceFunction(node: FunctionDefinition, orig: Buffer, modifications: Modification[]) {
+  if (node.kind === 'constructor') {
+    modifications.push(makeDelete(node, orig));
+  } else {
+    if (node.modifiers.length > 0) {
+      for (const modifier of node.modifiers) {
+        modifications.push(makeDelete(modifier, orig));
+      }
+    }
+
+    if (node.returnParameters.parameters.length > 0) {
+      modifications.push(makeReplace(node.returnParameters, orig, '(bool)'));
+    }
+
+    if (node.body) {
+      modifications.push(makeReplace(node.body, orig, '{}'));
+    }
+  }
+}
+
 function getPositions(node: Node) {
   const [start, length] = node.src.split(':').map(Number);
   const end = start + length;