diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index afbf859a3..b32510465 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Fix Hardhat compile errors when contracts have overloaded functions or standalone NatSpec documentation. ([#918](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/918)) + ## 1.31.2 (2023-11-28) - Fix `upgradeProxy` in Hardhat from an implementation that has a fallback function and is not using OpenZeppelin Contracts 5.0. ([#926](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/926)) diff --git a/packages/core/contracts/test/NamespacedToModify.sol b/packages/core/contracts/test/NamespacedToModify.sol index c8c1d0e75..d30458f8b 100644 --- a/packages/core/contracts/test/NamespacedToModify.sol +++ b/packages/core/contracts/test/NamespacedToModify.sol @@ -61,6 +61,11 @@ contract Example { * @notice some natspec */ function foo4() public {} + + /** + * @dev a custom error inside a contract + */ + error CustomErrorInsideContract(address a); } contract HasFunction { @@ -89,6 +94,8 @@ library Lib { } contract Consumer { + bytes4 public variableFromConstant = CONSTANT_USING_SELECTOR; + function usingFreeFunction() pure public returns (bytes4) { return FreeFunctionUsingSelector(); } @@ -106,6 +113,34 @@ function plusTwo(uint x) pure returns (uint) { return x + 2; } +/** + * @notice originally orphaned natspec + */ + +/** + * @dev plusThree + * @param x x + */ +function plusThree(uint x) pure returns (uint) { + return x + 3; +} + +/// @notice originally orphaned natspec 2 + +/** + * @dev plusThree overloaded + * @param x x + * @param y y + */ +function plusThree(uint x, uint y) pure returns (uint) { + return x + y + 3; +} + +function originallyNoDocumentation() pure {} + +/** + * @param foo foo + */ using {plusTwo} for uint; contract UsingForDirectives { @@ -114,4 +149,22 @@ contract UsingForDirectives { function usingFor(uint x) pure public returns (uint) { return x.plusOne() + x.plusTwo(); } -} \ No newline at end of file +} + +/** + * @title a + * @author a + * @inheritdoc Example + * @dev a + * @custom:a a + * @notice a + * @param a a + * @return a a + */ +enum FreeEnum { MyEnum } + +/** + * @dev a custom error outside a contract + * @param example example parameter + */ +error CustomErrorOutsideContract(Example example); diff --git a/packages/core/contracts/test/NamespacedToModifyImported.sol b/packages/core/contracts/test/NamespacedToModifyImported.sol new file mode 100644 index 000000000..23828588d --- /dev/null +++ b/packages/core/contracts/test/NamespacedToModifyImported.sol @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {CONSTANT_USING_SELECTOR, plusTwo, plusThree, CustomErrorOutsideContract} from "./NamespacedToModify.sol"; + +contract Example {} diff --git a/packages/core/src/utils/make-namespaced.test.ts b/packages/core/src/utils/make-namespaced.test.ts index 6dbbe56ae..ed29bafe7 100644 --- a/packages/core/src/utils/make-namespaced.test.ts +++ b/packages/core/src/utils/make-namespaced.test.ts @@ -12,7 +12,7 @@ import { SolcInput, SolcOutput } from '../solc-api'; import { BuildInfo } from 'hardhat/types'; test('make namespaced input', async t => { - const origBuildInfo = await artifacts.getBuildInfo('contracts/test/NamespacedToModify.sol:Example'); + const origBuildInfo = await artifacts.getBuildInfo('contracts/test/NamespacedToModifyImported.sol:Example'); await testMakeNamespaced(origBuildInfo, t, '0.8.20'); }); @@ -35,23 +35,25 @@ async function testMakeNamespaced( const origInput = JSON.parse(JSON.stringify(origBuildInfo.input)); const modifiedInput = makeNamespacedInput(origBuildInfo.input, origBuildInfo.output); - normalizeStateVariableNames(modifiedInput); - t.snapshot(modifiedInput); - - t.deepEqual(origBuildInfo.input, origInput); - t.notDeepEqual(modifiedInput, origInput); // 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); + + normalizeIdentifiers(modifiedInput); + t.snapshot(modifiedInput); + + t.deepEqual(origBuildInfo.input, origInput); + t.notDeepEqual(modifiedInput, origInput); } -function normalizeStateVariableNames(input: SolcInput): void { +function normalizeIdentifiers(input: SolcInput): void { for (const source of Object.values(input.sources)) { if (source.content !== undefined) { source.content = source.content - .replace(/\$MainStorage_\d{1,6};/g, '$MainStorage_random;') - .replace(/\$SecondaryStorage_\d{1,6}/g, '$SecondaryStorage_random'); + .replace(/\$MainStorage_\d{1,6}/g, '$MainStorage_random') + .replace(/\$SecondaryStorage_\d{1,6}/g, '$SecondaryStorage_random') + .replace(/\$astId_\d+_\d{1,6}/g, '$astId_id_random'); } } } diff --git a/packages/core/src/utils/make-namespaced.test.ts.md b/packages/core/src/utils/make-namespaced.test.ts.md index 0256c911a..fc7020329 100644 --- a/packages/core/src/utils/make-namespaced.test.ts.md +++ b/packages/core/src/utils/make-namespaced.test.ts.md @@ -45,27 +45,27 @@ Generated by [AVA](https://avajs.dev). uint256 b;␊ } SecondaryStorage $SecondaryStorage_random;␊ ␊ - ␊ - ␊ + /// @notice some natspec␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ - ␊ + /// @param a docs␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ - ␊ + /// @param a docs␊ + enum $astId_id_random { dummy }␊ struct MyStruct { uint b; }␊ ␊ // keccak256(abi.encode(uint256(keccak256("example.main")) - 1)) & ~bytes32(uint256(0xff));␊ - ␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ + enum $astId_id_random { dummy }␊ ␊ /// @notice standlone natspec␊ ␊ - ␊ - ␊ + /// @notice natspec for var␊ + enum $astId_id_random { dummy }␊ ␊ // standalone doc␊ ␊ @@ -73,49 +73,114 @@ Generated by [AVA](https://avajs.dev). * standlone doc block␊ */␊ ␊ - ␊ - ␊ + /**␊ + * doc block without natspec␊ + */␊ + enum $astId_id_random { dummy }␊ + ␊ + /**␊ + * doc block with natspec␊ + *␊ + * @notice some natspec␊ + */␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ - ␊ + /**␊ + * @dev a custom error inside a contract␊ + */␊ + enum $astId_id_random { dummy }␊ }␊ ␊ contract HasFunction {␊ - ␊ - ␊ + enum $astId_id_random { dummy }␊ + enum $astId_id_random { dummy }␊ }␊ ␊ contract UsingFunction is HasFunction {␊ - ␊ + enum $astId_id_random { dummy }␊ }␊ ␊ - uint256 constant FreeFunctionUsingSelector = 0;␊ + enum FreeFunctionUsingSelector { dummy }␊ ␊ - uint256 constant CONSTANT_USING_SELECTOR = 0;␊ + enum CONSTANT_USING_SELECTOR { dummy }␊ ␊ library Lib {␊ - ␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ + enum $astId_id_random { dummy }␊ }␊ ␊ contract Consumer {␊ - ␊ + enum $astId_id_random { dummy }␊ + ␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ + enum $astId_id_random { dummy }␊ ␊ - ␊ + enum $astId_id_random { dummy }␊ }␊ ␊ - uint256 constant plusTwo = 0;␊ + enum plusTwo { dummy }␊ + ␊ + /**␊ + * @notice originally orphaned natspec␊ + */␊ + ␊ + /**␊ + * @dev plusThree␊ + * @param x x␊ + */␊ + enum plusThree { dummy }␊ + ␊ + /// @notice originally orphaned natspec 2␊ ␊ + /**␊ + * @dev plusThree overloaded␊ + * @param x x␊ + * @param y y␊ + */␊ + enum $astId_id_random { dummy }␊ ␊ + enum originallyNoDocumentation { dummy }␊ + ␊ + /**␊ + * @param foo foo␊ + */␊ + enum $astId_id_random { dummy }␊ ␊ contract UsingForDirectives {␊ - ␊ + enum $astId_id_random { dummy }␊ + ␊ + enum $astId_id_random { dummy }␊ + }␊ + ␊ + /**␊ + * @title a␊ + * @author a␊ + * @inheritdoc Example␊ + * @dev a␊ + * @custom:a a␊ + * @notice a␊ + * @param a a␊ + * @return a a␊ + */␊ + enum FreeEnum { MyEnum }␊ + ␊ + /**␊ + * @dev a custom error outside a contract␊ + * @param example example parameter␊ + */␊ + enum CustomErrorOutsideContract { dummy }␊ + `, + }, + 'contracts/test/NamespacedToModifyImported.sol': { + content: `// SPDX-License-Identifier: MIT␊ + pragma solidity ^0.8.20;␊ ␊ - ␊ - }`, + import {CONSTANT_USING_SELECTOR, plusTwo, plusThree, CustomErrorOutsideContract} from "./NamespacedToModify.sol";␊ + ␊ + contract Example {}␊ + `, }, }, } @@ -148,12 +213,12 @@ Generated by [AVA](https://avajs.dev). pragma solidity 0.7.6;␊ ␊ contract HasFunction {␊ - ␊ - ␊ + enum $astId_id_random { dummy }␊ + enum $astId_id_random { dummy }␊ }␊ ␊ contract UsingFunction is HasFunction {␊ - ␊ + enum $astId_id_random { dummy }␊ }␊ `, }, diff --git a/packages/core/src/utils/make-namespaced.test.ts.snap b/packages/core/src/utils/make-namespaced.test.ts.snap index b89e40319..f9bc10b39 100644 Binary files a/packages/core/src/utils/make-namespaced.test.ts.snap and b/packages/core/src/utils/make-namespaced.test.ts.snap differ diff --git a/packages/core/src/utils/make-namespaced.ts b/packages/core/src/utils/make-namespaced.ts index 282c2dde5..d6481f4dd 100644 --- a/packages/core/src/utils/make-namespaced.ts +++ b/packages/core/src/utils/make-namespaced.ts @@ -17,10 +17,9 @@ const OUTPUT_SELECTION = { * * This makes the following modifications to the input: * - Adds a state variable for each namespaced struct definition - * - Deletes all contracts' functions since they are not needed for storage layout - * - Deletes all contracts' modifiers, variables, and parent constructor invocations to avoid compilation errors due to deleted functions and constructors - * - Deletes all using for directives (at file level and in contracts) since they may reference deleted functions - * - Converts all free functions and constants (at file level) to dummy variables (do not delete them since they might be imported by other files) + * - For each contract, for all node types that are not needed for storage layout or may reference deleted functions and constructors, converts them to dummy enums with random id + * - Converts all using for directives (at file level and in contracts) to dummy enums with random id (do not delete them to avoid orphaning possible NatSpec documentation) + * - Converts all custom errors, free functions and constants (at file level) to dummy enums with the same name (do not delete them since they might be imported by other files) * * Also sets the outputSelection to only include storageLayout and ast, since the other outputs are not needed. * @@ -41,55 +40,95 @@ export function makeNamespacedInput(input: SolcInput, output: SolcOutput): SolcI const orig = Buffer.from(source.content, 'utf8'); + const replacedIdentifiers = new Set(); const modifications: Modification[] = []; for (const node of output.sources[sourcePath].ast.nodes) { - if (isNodeType('ContractDefinition', node)) { - const contractDef = node; - - // Remove any calls to parent constructors from the inheritance list - const inherits = contractDef.baseContracts; - for (const inherit of inherits) { - if (Array.isArray(inherit.arguments)) { - assert(inherit.baseName.name !== undefined); - modifications.push(makeReplace(inherit, orig, inherit.baseName.name)); + switch (node.nodeType) { + case 'ContractDefinition': { + const contractDef = node; + + // Remove any calls to parent constructors from the inheritance list + const inherits = contractDef.baseContracts; + for (const inherit of inherits) { + if (Array.isArray(inherit.arguments)) { + assert(inherit.baseName.name !== undefined); + modifications.push(makeReplace(inherit, orig, inherit.baseName.name)); + } } - } - const contractNodes = contractDef.nodes; - for (const contractNode of contractNodes) { - if ( - isNodeType('FunctionDefinition', contractNode) || - isNodeType('ModifierDefinition', contractNode) || - isNodeType('VariableDeclaration', contractNode) - ) { - if (contractNode.documentation) { - modifications.push(makeDelete(contractNode.documentation, orig)); - } - modifications.push(makeDelete(contractNode, orig)); - } else if (isNodeType('UsingForDirective', contractNode)) { - modifications.push(makeDelete(contractNode, orig)); - } else if (isNodeType('StructDefinition', contractNode)) { - const storageLocation = getStorageLocationAnnotation(contractNode); - if (storageLocation !== undefined) { - const structName = contractNode.name; - const variableName = `$${structName}_${(Math.random() * 1e6).toFixed(0)}`; - const insertText = ` ${structName} ${variableName};`; - - modifications.push(makeInsertAfter(contractNode, insertText)); + const contractNodes = contractDef.nodes; + for (const contractNode of contractNodes) { + switch (contractNode.nodeType) { + case 'ErrorDefinition': + case 'EventDefinition': + case 'FunctionDefinition': + case 'ModifierDefinition': + case 'UsingForDirective': + case 'VariableDeclaration': { + // Replace with an enum based on astId (the original name is not needed, since nothing should reference it) + modifications.push(makeReplace(contractNode, orig, toDummyEnumWithAstId(contractNode.id))); + break; + } + case 'StructDefinition': { + const storageLocation = getStorageLocationAnnotation(contractNode); + if (storageLocation !== undefined) { + const structName = contractNode.name; + const variableName = `$${structName}_${(Math.random() * 1e6).toFixed(0)}`; + const insertText = ` ${structName} ${variableName};`; + + modifications.push(makeInsertAfter(contractNode, insertText)); + } + break; + } + case 'EnumDefinition': + case 'UserDefinedValueTypeDefinition': + default: { + // - EnumDefinition may be used in structures with storage locations + // - UserDefinedValueTypeDefinition may be used in structures with storage locations + // - default: in case unexpected ast nodes show up + break; + } } } + 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. + // 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': + 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))); + } + break; } - } else if (isNodeType('FunctionDefinition', node) || isNodeType('VariableDeclaration', node)) { - if (node.documentation) { - modifications.push(makeDelete(node.documentation, orig)); + case 'EnumDefinition': + case 'ImportDirective': + case 'PragmaDirective': + case 'StructDefinition': + case 'UserDefinedValueTypeDefinition': + default: { + // - EnumDefinition may be used in structures with storage locations + // - ImportDirective may import types used in structures with storage locations + // - PragmaDirective is necessary for compilation + // - StructDefinition may be used in structures with storage locations + // - UserDefinedValueTypeDefinition may be used in structures with storage locations + // - default: in case unexpected ast nodes show up (file-level events since 0.8.22) + break; } - // Replace with a dummy variable of arbitrary type - const name = node.name; - const insertText = `uint256 constant ${name} = 0;`; - modifications.push(makeReplace(node, orig, insertText)); - } else if (isNodeType('UsingForDirective', node)) { - modifications.push(makeDelete(node, orig)); } } @@ -105,6 +144,14 @@ interface Modification { text?: string; } +function toDummyEnumWithName(name: string) { + return `enum ${name} { dummy }`; +} + +function toDummyEnumWithAstId(astId: number) { + return `enum $astId_${astId}_${(Math.random() * 1e6).toFixed(0)} { dummy }`; +} + function getPositions(node: Node) { const [start, length] = node.src.split(':').map(Number); const end = start + length;