diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..5180b1e --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,23 @@ +name: Node.js CI + +on: push + +jobs: + build: + runs-on: ubuntu-latest + + strategy: + matrix: + node-version: [14.x, 16.x, 18.x] + + steps: + - uses: actions/checkout@v3 + + - name: Use Node.js ${{ matrix.node-version }} + uses: actions/setup-node@v3 + with: + node-version: ${{ matrix.node-version }} + + - run: yarn install + + - run: yarn test diff --git a/sample-data/sample.sol b/sample-data/BasicSample.sol similarity index 98% rename from sample-data/sample.sol rename to sample-data/BasicSample.sol index 5b9e932..c29b6d2 100644 --- a/sample-data/sample.sol +++ b/sample-data/BasicSample.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity =0.8.19; -contract Sample { +contract BasicSample { /** * @notice Empty string for revert checks * @dev result of doing keccak256(bytes('')) diff --git a/sample-data/InterfacedSample.sol b/sample-data/InterfacedSample.sol new file mode 100644 index 0000000..ead2a2e --- /dev/null +++ b/sample-data/InterfacedSample.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT +pragma solidity =0.8.19; + +interface IInterfacedSample { + /** + * @notice Greets the caller + * + * @return _greeting The greeting + * @return _balance Current token balance of the caller + */ + function greet() external view returns (string memory _greeting, uint256 _balance); +} + +contract InterfacedSample is IInterfacedSample { + /// @inheritdoc IInterfacedSample + /// @dev some dev thingy + function greet() external view returns (string memory _greeting, uint256 _balance) {} +} diff --git a/src/main.ts b/src/main.ts index e0c647e..7e82c86 100644 --- a/src/main.ts +++ b/src/main.ts @@ -23,21 +23,26 @@ import { getRemappings, getSolidityFiles } from './utils'; }); for (const [fileName, source] of Object.entries(compiledFiles.data.sources)) { - const nodes = (source as any).ast.nodes.slice(-1)[0].nodes as SolcContractNode[]; if (fileName.startsWith('node_modules') || fileName.startsWith('lib')) continue; - nodes.forEach(node => { - const nodeNatspec = parseNodeNatspec(node); - const alerts = validate(node, nodeNatspec); - - // print alerts - if (alerts.length) { - console.warn(`Natspec alerts in ${fileName}:${node.name}`); - alerts.forEach(alert => { - console.warn(' ' + alert.message); - }); - } + const fileContracts = (source as any).ast.nodes.filter((node: any) => node.nodeType === 'ContractDefinition'); + fileContracts.forEach((contract: any) => { + const nodes = contract.nodes as SolcContractNode[]; + + nodes.forEach(node => { + const nodeNatspec = parseNodeNatspec(node); + const warnings = validate(node, nodeNatspec); + + // print warnings + if (warnings.length) { + console.warn(`Natspec warnings in ${fileName}:${contract.name}:${node.name}`); + warnings.forEach(warning => { + console.warn(' ' + warning); + }); + } + }); }); + } })().catch(console.error); diff --git a/src/parser.ts b/src/parser.ts index c316a11..a316237 100644 --- a/src/parser.ts +++ b/src/parser.ts @@ -1,4 +1,4 @@ -import { Natspec, NatspecParam, NatspecReturn, NatspecTag } from "./types/natspec.t"; +import { Natspec, NatspecDefinition } from "./types/natspec.t"; import { SolcContractNode } from "./types/solc-typed-ast.t"; export function parseNodeNatspec(node: SolcContractNode): Natspec { @@ -8,7 +8,7 @@ export function parseNodeNatspec(node: SolcContractNode): Natspec { const docLines = node.documentation.text.split('\n'); - let currentTag: null | NatspecTag | NatspecParam | NatspecReturn = null; + let currentTag: NatspecDefinition | null = null; const result: Natspec = { tags: [], params: [], @@ -20,25 +20,27 @@ export function parseNodeNatspec(node: SolcContractNode): Natspec { if (tagTypeMatch) { const tagName = tagTypeMatch[1]; - if (tagName === 'param' || tagName === 'return') { + if (tagName === 'inheritdoc') { + const tagMatch = line.match(/^\s*@(\w+) (.*)$/); + if (tagMatch) { + currentTag = null; + result.inheritdoc = { content: tagMatch[2] }; + } + } else if (tagName === 'param' || tagName === 'return') { const tagMatch = line.match(/^\s*@(\w+) (\w+) (.*)$/); if (tagMatch) { - currentTag = { name: tagMatch[2], description: tagMatch[3].trim() }; - if (tagName === 'param') { - result.params.push(currentTag as NatspecParam); - } else { - result.returns.push(currentTag as NatspecReturn); - } + currentTag = { name: tagMatch[2], content: tagMatch[3].trim() }; + result[tagName === 'param' ? 'params' : 'returns'].push(currentTag); } } else { const tagMatch = line.match(/^\s*@(\w+) (.*)$/); if (tagMatch) { - currentTag = { name: tagName, description: tagMatch[2] }; - result.tags.push(currentTag as NatspecTag); + currentTag = { name: tagName, content: tagMatch[2] }; + result.tags.push(currentTag); } } } else if (currentTag) { - currentTag.description += '\n' + line; + currentTag.content += '\n' + line; } }); diff --git a/src/types/natspec.t.ts b/src/types/natspec.t.ts index 2669d96..07c1e8f 100644 --- a/src/types/natspec.t.ts +++ b/src/types/natspec.t.ts @@ -1,21 +1,12 @@ -export interface NatspecTag { - name: string; - description: string; -} - -export interface NatspecParam { - name: string; - description: string; -} - -export interface NatspecReturn { - name: string | undefined; - description: string; +export interface NatspecDefinition { + name?: string; + content: string; } export interface Natspec { - tags: NatspecTag[]; - params: NatspecParam[]; - returns: NatspecReturn[]; + inheritdoc?: NatspecDefinition; + tags: NatspecDefinition[]; + params: NatspecDefinition[]; + returns: NatspecDefinition[]; } diff --git a/src/validator.ts b/src/validator.ts index d7cda2c..ed8ff8e 100644 --- a/src/validator.ts +++ b/src/validator.ts @@ -1,64 +1,43 @@ import { Natspec } from '../src/types/natspec.t'; import { SolcContractNode } from "./types/solc-typed-ast.t"; -interface IAlert { - severity: string; - message: string; -} +export function validate(contractNode: SolcContractNode, natspec: Natspec): string[] { + let alerts: string[] = []; -export function validate(contractNode: SolcContractNode, natspec: Natspec): IAlert[] { - let alerts: IAlert[] = []; + if (natspec.inheritdoc) { + return alerts; + } - if(natspec.tags.length === 0) { - alerts.push({ - severity: 'error', - message: `Natspec is missing`, - }); + if(!natspec.tags.length) { + alerts.push(`Natspec is missing`); + return alerts; }; - // Make sure all defined function parameters have natspec let functionParameters = contractNode.parameters?.parameters.map(p => p.name) ?? []; let natspecParameters = natspec.params.map(p => p.name); - - for(let param of functionParameters) { - if(!natspecParameters.includes(param)) { - alerts.push({ - severity: 'error', - message: `Natspec for ${param} is missing`, - }); - } - } - - // Make sure there is no natspec defined for non-existing parameters - for(let param of natspecParameters) { - if(!functionParameters.includes(param)) { - alerts.push({ - severity: 'error', - message: `Found natspec for undefined function parameter ${param}`, - }); + + // Make sure all defined function parameters have natspec + for(let paramName of functionParameters) { + if(!natspecParameters.includes(paramName)) { + alerts.push(`@param ${paramName} is missing`); } } let functionReturns = contractNode.returnParameters?.parameters.map(p => p.name) ?? []; let natspecReturns = natspec.returns.map(p => p.name); - for(let param of functionReturns) { - if(!natspecReturns.includes(param)) { - let message = param === '' ? 'Natspec for a return parameter is missing' : `Natspec for ${param} is missing`; - alerts.push({ - severity: 'error', - message: message, - }); + // 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`; + alerts.push(message); } } // Make sure there is no natspec defined for non-existing returns - for(let param of natspecReturns) { - if(param && !functionReturns.includes(param)) { - alerts.push({ - severity: 'error', - message: `Found natspec for undefined returned value ${param}`, - }); + 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 04307b1..2dc53b8 100644 --- a/test/parser.test.ts +++ b/test/parser.test.ts @@ -4,11 +4,11 @@ import { parseSolidityFile } from './test-utils'; describe('parseNodeNatspec', () => { - describe('sample.sol', () => { + describe('BasicSample.sol', () => { let nodes: SolcContractNode[]; beforeAll(async () => { - const file = 'sample-data/sample.sol'; + const file = 'sample-data/BasicSample.sol'; const compileResult = await parseSolidityFile(file); nodes = compileResult.data.sources[file].ast.nodes[1].nodes as SolcContractNode[]; }); @@ -20,10 +20,10 @@ describe('parseNodeNatspec', () => { expect(result).toEqual({ tags: [{ name: 'notice', - description: 'Empty string for revert checks', + content: 'Empty string for revert checks', }, { name: 'dev', - description: `result of doing keccak256(bytes(''))`, + content: `result of doing keccak256(bytes(''))`, }], params: [], returns: [], @@ -39,21 +39,21 @@ describe('parseNodeNatspec', () => { expect(result).toEqual({ tags: [{ name: 'notice', - description: 'External function that returns a bool', + content: 'External function that returns a bool', }, { name: 'dev', - description: 'A dev comment', + content: 'A dev comment', }], params: [{ name: '_magicNumber', - description: 'A parameter description', + content: 'A parameter description', }, { name: '_name', - description: 'Another parameter description', + content: 'Another parameter description', }], returns: [{ name: '_isMagic', - description: 'Some return data', + content: 'Some return data', }], }); }); @@ -65,11 +65,11 @@ describe('parseNodeNatspec', () => { expect(result).toEqual({ tags: [{ name: 'notice', - description: 'Private test function', + content: 'Private test function', }], params: [{ name: '_magicNumber', - description: 'A parameter description', + content: 'A parameter description', }], returns: [], }); @@ -83,7 +83,7 @@ describe('parseNodeNatspec', () => { expect(result).toEqual({ tags: [{ name: 'notice', - description: 'Private test function\n with multiple\n lines', + content: 'Private test function\n with multiple\n lines', }], params: [], returns: [], @@ -97,10 +97,37 @@ describe('parseNodeNatspec', () => { expect(result).toEqual({ tags: [{ name: 'notice', - description: 'Private test function', + content: 'Private test function', }, { name: 'notice', - description: 'Another notice', + content: 'Another notice', + }], + params: [], + returns: [], + }); + }); + }); + + describe('InterfacedSample.sol', () => { + let nodes: SolcContractNode[]; + + beforeAll(async () => { + const file = 'sample-data/InterfacedSample.sol'; + const compileResult = await parseSolidityFile(file); + nodes = compileResult.data.sources[file].ast.nodes[2].nodes as SolcContractNode[]; + }); + + it('should parse the inheritdoc tag', async () => { + const functionNode = nodes[0]; + const result = parseNodeNatspec(functionNode); + + expect(result).toEqual({ + inheritdoc: { + content: 'IInterfacedSample', + }, + tags: [{ + name: 'dev', + content: 'some dev thingy', }], params: [], returns: [], diff --git a/test/validator.test.ts b/test/validator.test.ts index 84c0235..cbfd7cc 100644 --- a/test/validator.test.ts +++ b/test/validator.test.ts @@ -1,17 +1,13 @@ import { validate } from '../src/validator'; import { parseSolidityFile } from '../test/test-utils'; -import { resolve} from 'path'; -import { Natspec } from '../src/types/natspec.t'; import { SolcContractNode } from "../src/types/solc-typed-ast.t"; -const SOLIDITY_FILE_PATH = resolve(__dirname, '..', 'sample-data', 'sample.sol'); - describe.only('validator function', () => { let nodes: SolcContractNode[]; let functionParsedData: SolcContractNode; beforeAll(async () => { - const file = 'sample-data/sample.sol'; + const file = 'sample-data/BasicSample.sol'; const compileResult = await parseSolidityFile(file); nodes = compileResult.data.sources[file].ast.nodes[1].nodes as SolcContractNode[]; functionParsedData = nodes[1]; @@ -20,32 +16,32 @@ describe.only('validator function', () => { let natspec = { tags: [ { - 'name': 'notice', - 'description': 'External function that returns a bool' + name: 'notice', + content: 'External function that returns a bool' }, { - 'name': 'dev', - 'description': 'A dev comment' + name: 'dev', + content: 'A dev comment' } ], params: [ { - 'name': '_magicNumber', - 'description': 'A parameter description' + name: '_magicNumber', + content: 'A parameter description' }, { - 'name': '_name', - 'description': 'Another parameter description' + name: '_name', + content: 'Another parameter description' } ], returns: [ { - 'name': '_isMagic', - 'description': 'Some return data' + name: '_isMagic', + content: 'Some return data' }, { - 'name': undefined, - 'description': 'Test test' + name: undefined, + content: 'Test test' } ] }; @@ -56,139 +52,107 @@ describe.only('validator function', () => { }); it('should reveal missing natspec for parameters', () => { - const parameter = '_magicNumber'; + const paramName = '_magicNumber'; let natspec = { tags: [ { - 'name': 'notice', - 'description': 'External function that returns a bool' + name: 'notice', + content: 'External function that returns a bool' } ], params: [ { - 'name': '_name', - 'description': 'Another parameter description' + name: '_name', + content: 'Another parameter description' } ], returns: [ { - 'name': '_isMagic', - 'description': 'Some return data' + name: '_isMagic', + content: 'Some return data' } ] }; const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - message: `Natspec for ${parameter} is missing`, - severity: 'error', - }); - }); - - it('should reveal extra natspec for parameters', () => { - const parameter = 'someParameter'; - const natspec = { - tags: [], - params: [ - { - 'name': parameter, - 'description': 'Some text' - } - ], - returns: [] - }; - - const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - severity: 'error', - message: `Found natspec for undefined function parameter ${parameter}`, - }); + expect(result).toContainEqual(`@param ${paramName} is missing`); }); it('should reveal missing natspec for returned values', () => { - const returnedValue = '_isMagic'; + const paramName = '_isMagic'; let natspec = { tags: [ { - 'name': 'notice', - 'description': 'External function that returns a bool' + name: 'notice', + content: 'External function that returns a bool' }, { - 'name': 'dev', - 'description': 'A dev comment' + name: 'dev', + content: 'A dev comment' } ], params: [ { - 'name': '_magicNumber', - 'description': 'A parameter description' + name: '_magicNumber', + content: 'A parameter description' }, { - 'name': '_name', - 'description': 'Another parameter description' + name: '_name', + content: 'Another parameter description' } ], returns: [] }; const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - severity: 'error', - message: `Natspec for ${returnedValue} is missing`, - }); + expect(result).toContainEqual(`@return ${paramName} is missing`); }); - it('should reveal extra natspec for returned values', () => { - const returnedValue = 'someValue'; - natspec.returns.push({ - 'name': returnedValue, - 'description': 'Some text' - }); + // it('should reveal extra natspec for returned values', () => { + // const paramName = 'someValue'; + // natspec.returns.push({ + // name: paramName, + // content: 'Some text' + // }); - const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - severity: 'error', - message: `Found natspec for undefined returned value ${returnedValue}`, - }); - }); + // const result = validate(functionParsedData, natspec); + // expect(result).toContainEqual(`Found natspec for undefined returned value ${paramName}`); + // }); it('should reveal missing natspec for unnamed returned values', () => { functionParsedData = nodes[5]; - const returnedValue = ''; + const paramName = ''; let natspec = { tags: [ { - 'name': 'notice', - 'description': 'External function that returns a bool' + name: 'notice', + content: 'External function that returns a bool' }, { - 'name': 'dev', - 'description': 'A dev comment' + name: 'dev', + content: 'A dev comment' } ], params: [ { - 'name': '_magicNumber', - 'description': 'A parameter description' + name: '_magicNumber', + content: 'A parameter description' }, { - 'name': '_name', - 'description': 'Another parameter description' + name: '_name', + content: 'Another parameter description' } ], returns: [ { - 'name': '_isMagic', - 'description': 'Some return data' + name: '_isMagic', + content: 'Some return data' } ] }; const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - severity: 'error', - message: `Natspec for a return parameter is missing`, - }); + expect(result).toContainEqual(`@return missing for unnamed return`); }); // TODO: Check overridden functions, virtual, etc? @@ -205,9 +169,6 @@ describe.only('validator function', () => { returns: [] }; const result = validate(functionParsedData, natspec); - expect(result).toContainEqual({ - severity: 'error', - message: `Natspec is missing`, - }); + expect(result).toContainEqual(`Natspec is missing`); }); });