Parser crashes when parsing exactly's codebase via solhint

juanpcapurro commented 1 year ago

What I'm trying to do

I've forked solhint and been trying it out against exactly's codebase

The problem

however, when updating the dependency & running yarn lint:sol at the tip of the aforementioned PR's tip, I get the following error:

[I] capu ~/s/exactly-protocol (basedhint)> yarn lint:sol
yarn run v1.22.19
warning package.json: License should be a valid SPDX license expression
$ solhint '{contracts,test}/**/*.sol'
/home/capu/src/exactly-protocol/node_modules/solhint-community/lib/index.js:40
    throw e
    ^
Error: The specified node does not exist
    at ImportDirectiveContext.getChild (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:9354:15)
    at ImportDirectiveContext.getRuleContext (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:9417:19)
    at ImportDirectiveContext.importPath (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:32102:17)
    at ASTBuilder.visitImportDirective (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:36578:41)
    at ImportDirectiveContext.accept (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:32136:22)
    at ASTBuilder.visit (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:17669:19)
    at /home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:35544:59
    at Array.map (<anonymous>)
    at ASTBuilder.visitSourceUnit (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:35544:39)
    at SourceUnitContext.accept (/home/capu/src/exactly-protocol/node_modules/solhint-community/node_modules/@solidity-parser/parser/dist/index.cjs.js:31881:22)

Node.js v18.16.0
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

solhint-community clearly states it needs a parser of semver-range version 0.16.0

[N] capu ~/s/exactly-protocol (basedhint) [5]> jq ".dependencies.\"@solidity-parser/parser\"" < "node_modules/solhint-community/package.json"
"^0.16.0"

...and it has it under its own node_modules, and the stack trace shows that it's the one being used:

[I] capu ~/s/exactly-protocol (basedhint)> jq ".version" < "node_modules/solhint-community/node_modules/@solidity-parser/parser/package.json"
"0.16.0"

what I've tried so far

If the fixes for the no-unused-import rule are rolled back (checking out the previous commit), then the linter reports them successfully.

[I] capu ~/s/exactly-protocol ((82e3ed0e))> yarn lint:sol
yarn run v1.22.19
warning package.json: License should be a valid SPDX license expression
$ solhint '{contracts,test}/**/*.sol'

contracts/InterestRateModel.sol
4:10  error  imported name Math is not used  no-unused-import

contracts/mocks/MockPriceFeed.sol
    4:10  error  imported name MockERC20 is not used  no-unused-import
...

but that's not of much help.

Also, disabling the no-unused-import rule doesn't prevent the crash either.

juanpcapurro commented 1 year ago

update: I narrowed it down to the npx solhint test/solidity/Leverager.t.sol file causing the crash

fvictorio commented 1 year ago

I cannot reproduce this :no_mouth:

I cloned the exactly repo
yarn add solhint-community
npx solhint test/solidity/Leverager.t.sol

Am I missing something here?

juanpcapurro commented 1 year ago

sorry, I wasn't clear:

I created a PR on Exactly where this issue can be reproduced: https://github.com/exactly/protocol/pull/628 , so you should

fork my repo on branch basedhint,
run yarn to install dependencies,
run npx solhint test/solidity/Leverager.t.sol

to cause the exception

it can only be replicated when using solhint-community (first commit of my pr) and with the unused imports removed (second commit)

fvictorio commented 1 year ago

That file has a syntax error. There is a trailing comma in this import:

import {
  ERC20,
  Market,
  Leverager,
  IBalancerVault,
  NotBalancerVault,
} from "../../contracts/periphery/Leverager.sol";

As far as I can tell, solidity doesn't support that.

juanpcapurro commented 1 year ago

lol that's embarrasing :sweat_smile: thanks for taking the time

solidity-parser / parser