diff --git a/sample-data/BasicSample.sol b/sample-data/BasicSample.sol index 5554dba..489d999 100644 --- a/sample-data/BasicSample.sol +++ b/sample-data/BasicSample.sol @@ -76,6 +76,15 @@ contract BasicSample is AbstractBasic { return (true, 111); } + /** + * @notice External function that returns a bool + * @dev A dev comment + * @return Some return data + */ + function externalSimpleMultipleUnnamedReturn() external pure returns (bool, uint256) { + return (true, 111); + } + /** * @notice This function should have an inheritdoc tag */ diff --git a/sample-data/ParserTest.sol b/sample-data/ParserTest.sol index d33084c..6b215af 100644 --- a/sample-data/ParserTest.sol +++ b/sample-data/ParserTest.sol @@ -179,5 +179,12 @@ contract ParserTestFunny is IParserTest { function _viewLinterFail() internal pure { } + + /// fun fact: there are extra spaces after the 1st return + /// @return + /// @return + function functionUnnamedEmptyReturn() external view returns (uint256, bool){ + return (1, true); + } } // forgefmt: disable-end diff --git a/src/utils.ts b/src/utils.ts index 980282d..e1de606 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -95,7 +95,7 @@ export function parseNodeNatspec(node: NodeToProcess): Natspec { result.inheritdoc = { content: tagMatch[2] }; } } else if (tagName === 'param' || tagName === 'return') { - const tagMatch = line.match(/^\s*@(\w+) *(\w+) (.*)$/); + const tagMatch = line.match(/^\s*@(\w+) *(\w*) *(.*)$/); if (tagMatch) { currentTag = { name: tagMatch[2], content: tagMatch[3].trim() }; result[tagName === 'param' ? 'params' : 'returns'].push(currentTag); diff --git a/src/validator.ts b/src/validator.ts index 2f323bb..3cc50a6 100644 --- a/src/validator.ts +++ b/src/validator.ts @@ -75,17 +75,13 @@ export class Validator { let functionReturns = node.vReturnParameters.vParameters.map((p) => p.name); // Make sure all defined returns have natspec - for (let paramName of functionReturns) { - if (!natspecReturns.includes(paramName)) { - let message = paramName === '' ? '@return missing for unnamed return' : `@return ${paramName} is missing`; + for (let [paramIndex, paramName] of functionReturns.entries()) { + if (paramIndex > natspecReturns.length - 1) { + let message = paramName === '' ? `@return missing for unnamed return №${paramIndex + 1}` : `@return ${paramName} is missing`; + alerts.push(message); + } else if (natspecReturns[paramIndex] !== paramName && paramName !== '') { + let message = `@return ${paramName} is missing`; alerts.push(message); - } - } - - // Make sure there is no natspec defined for non-existing returns - for (let paramName of natspecReturns) { - if (paramName && !functionReturns.includes(paramName)) { - alerts.push(`Missing named return for: @return ${paramName}`); } } diff --git a/test/parser.test.ts b/test/parser.test.ts index 5cdbb8d..ef3a2b0 100644 --- a/test/parser.test.ts +++ b/test/parser.test.ts @@ -420,5 +420,27 @@ describe('Parser', () => { }) ); }); + + it('should correctly parse empty return tag', async () => { + const node = contract.vFunctions.find(({ name }) => name === 'functionUnnamedEmptyReturn')!; + const result = parseNodeNatspec(node); + + expect(result).toEqual( + mockNatspec({ + tags: [], + params: [], + returns: [ + { + name: '', + content: '', + }, + { + name: '', + content: '', + }, + ], + }) + ); + }); }); }); diff --git a/test/validator.test.ts b/test/validator.test.ts index 1d19196..26746a6 100644 --- a/test/validator.test.ts +++ b/test/validator.test.ts @@ -153,7 +153,103 @@ describe('Validator', () => { }; const result = validator.validate(node, natspec); - expect(result).toContainEqual(`@return missing for unnamed return`); + expect(result).toContainEqual(`@return missing for unnamed return №2`); + }); + + it('should warn of a missing unnamed return', () => { + node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleUnnamedReturn')!; + let natspec = { + tags: [ + { + name: 'notice', + content: 'External function that returns a bool', + }, + { + name: 'dev', + content: 'A dev comment', + }, + ], + params: [], + returns: [ + { + name: 'Some', + content: 'return data', + }, + ], + }; + + const result = validator.validate(node, natspec); + expect(result).toEqual([`@return missing for unnamed return №2`]); // only 1 warning + }); + + it('should warn all returns if the first natspec tag is missing', () => { + node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleReturn')!; + let natspec = { + tags: [ + { + name: 'notice', + content: 'External function that returns a bool', + }, + { + name: 'dev', + content: 'A dev comment', + }, + ], + params: [ + { + name: '_magicNumber', + content: 'A parameter description', + }, + { + name: '_name', + content: 'Another parameter description', + }, + ], + returns: [ + { + name: 'Some', + content: 'return data', + }, + ], + }; + + const result = validator.validate(node, natspec); + expect(result).toEqual(['@return _isMagic is missing', '@return missing for unnamed return №2']); // 2 warnings + }); + + it('should warn if the last natspec tag is missing', () => { + node = contract.vFunctions.find(({ name }) => name === 'externalSimpleMultipleReturn')!; + let natspec = { + tags: [ + { + name: 'notice', + content: 'External function that returns a bool', + }, + { + name: 'dev', + content: 'A dev comment', + }, + ], + params: [ + { + name: '_magicNumber', + content: 'A parameter description', + }, + { + name: '_name', + content: 'Another parameter description', + }, + ], + returns: [ + { + name: '_isMagic', + content: 'Some return data', + }, + ], + }; + + const result = validator.validate(node, natspec); + expect(result).toEqual(['@return missing for unnamed return №2']); // 1 warnings }); // TODO: Check overridden functions, virtual, etc?