Support for Solidity 0.6.x

[christianparpart]

Hi,

first, many thanks for this great too. We're using solhint in our own CI @ solidity to check the Solidity source code within the Solidity documentation.

Now, the 0.6.0 release is around the corner, and our CI is failing due to some of the new features being implemented.

See: https://solidity.readthedocs.io/en/develop/060-breaking-changes.html#new-features

From what I can tell, at least the new try/catch for external calls is missing the implementation part on solhint-side.

Many thanks,
Christian Parpart.

[fvictorio]

Hi @christianparpart, I'm very glad Solhint has been useful to you!

We are in the middle of some work towards Solhint 3.0. One of the things that will come out of it is that we are going to leverage solidity-parser-antlr for the parsing and that will let us keep in sync with new Solidity versions more easily.

That being said, we are considering deprecating the style rules and delegating this to prettier-plugin-solidity (through the solhint's plugin). Do you think that would work for you?

[christianparpart]

Hm, the solidity-parser-antlr project isn't maintained by us, so you make yourself depending on an external project where you are not in control how long it will take them to adapt with syntax changes. I am not sure that this will speed up adaptability.

With regard to the other: using a solhint plugin in our CI shouldn't be of any issues.

[fvictorio]

Oh, sure, but Federico has done an excellent job so far, and AFAIK he's just adding a thin abstraction over the antlr grammar we are using directly right now. If at some point he doesn't maintain it anymore, it should be forkable since it's MIT licensed.

(ccing @federicobond since he might be interested on this discussion).

[sohkai]

That being said, we are considering deprecating the style rules and delegating this to prettier-plugin-solidity (through the solhint's plugin)

I don't know if this makes complete sense, as some people may want a linting and not code formatting tool, but for users of prettier (including me), forcing more people to use the prettier plugin seems like a good idea to mature that plugin :).

I guess it's a question of what we'd consider style, as almost everything outside of the security package could be considered style.

[fvictorio]

some people may want a linting and not code formatting tool

I think this should be doable with a plugin. I personally would love to see someone working on that. The problem is that style rules are hard and we prefer to use that effort in other stuff.

I guess it's a question of what we'd consider style, as almost everything outside of the security package could be considered style.

I think pretty much anything that doesn't change the AST is a style rule. If that means that solhint's core will only have security rules, that seems like a sensible choice to me.

[nventuro]

Is there an estimate for when 3.0 will be released? Sadly 0.6's new keywords (abstract, virtual, receive, etc.) completely break solhint, making it unusable :(

[fvictorio]

Hi @nventuro!

We want to release 3.0 as soon as possible. We have some pending things, but the main blocker is a situation with solidity-parser-antlr (issue: federicobond/solidity-parser-antlr#98) so we'll probably have to wait for that to be resolved.

In the meantime, I think we can publish a release candidate to npm, and you can use that to start the migration. Plus, we would love the feedback you could give us. Do you think that would help?

[nventuro]

Sure! We don't use try/catch yet, so if that's the only missing feature on an rc I'd love to give it a go.

[fvictorio]

Cool, I'll try to publish it tomorrow. Will let you know as soon as it's available.

[fvictorio]

@nventuro you should be able to try the new version with npm i solhint@next

[nventuro]

Thanks @fvictorio! The new version is able to correctly parse my 0.6 code, but I get a new warning that didn't show up when using 2.3: it looks like a linter bug. Other than this, everything seems to be working fine!

Given an override like the following, where not all function arguments are used:

function withdrawalAllowed(address) public view override returns (bool) {
    return _state == State.Refunding;
}

I get the following warning:

warning  Variable "null" is unused  no-unused-vars

[fvictorio]

Wow, that's weird. I'll take a look into it, thanks!

And by the way, a heads up: we are removing all the styling rules in 3.0. We might move them to a plugin, or let the community do it, but we've found that they take a lot of work to maintain and we want to focus more in the best practices and security rules. Just letting you know because I saw that you are using a couple of them (maybe we should emit a warning when the config has a rule that doesn't exist?)

[nventuro]

Sounds good!

(maybe we should emit a warning when the config has a rule that doesn't exist?)

This is a great idea, yes!

[fvictorio]

@nventuro Fixed that in 3.0.0-rc.2. It will also warn you about the rules that don't exist anymore.

By the way, with respect to the styling, have you guys considered using prettier-solidity? It's still kind of beta but I think it should work well enough, and being adopted by OZ would surely give it the push it needs to get to a stable version 😅

Plus, you can use it along with solhint (well, not in 3.0, but that's an easy fix).

[nventuro]

Thanks for the suggestion! What needs to be done to have it working on 3.0?

[fvictorio]

Mainly upgrading the parser in prettier-solidity, and then checking what new formatting rules need to be added (if any) for solidity 0.6. I already have it working for 0.6 in the sense that it can parse it, but without the formatting. I'll use that in your 0.6 branch to see what new nodes need to be handled.

[nventuro]

Great, thanks! Let me know when you want for me to give it a try with 0.6.

[nventuro]

Solidity v0.6.4 deprecated the .value(x) syntax in favor of {value: x}. This fails to parse in solhint v3.0.0-rc.5 with Error: Unrecognized expression.

As an example, the following statement:

(bool success, ) = recipient.call.value(amount)("");

Should now be written as:

(bool success, ) = recipient.call{value: amount}("");

[fvictorio]

Thanks @nventuro. I opened an issue in the parser's repo for this.

[juanfranblanco]

Hi, how safe is to upgrade vscode to use the RC version of solium?

[fvictorio]

I think it's pretty safe, and in fact it would help us a lot to validate the RC. We haven't released as a stable version mainly because docs are not completely up to date, but functionality-wise it should be pretty much ready.

[nventuro]

Keep in mind Consensys/solidity-parser-antlr#14 is still open, so if you're using that syntax the linter will crash :/ We're keeping OpenZeppelin/openzeppelin-contracts#2115 unmerged because of that.

[fvictorio]

Aghh, that's really frustrating. I'll try to make a PR for that this weekend.

[nventuro]

@fvictorio the latest rc (3.0.0-rc.7) parses the new { value: x } syntax correctly, but yields false-positives.

For example, the following code

function _asyncTransfer(address dest, uint256 amount) internal virtual {
    _escrow.deposit{ value: amount }(dest);
}

errors out with

warning  Variable "amount" is unused  no-unused-vars

[fvictorio]

@nventuro This should be fixed in v3.0.0-rc.8

[fvictorio]

Thanks everyone for the feedback! I'm going to close this issue to keep things organized. Please open a new issue if something else shows up, or if something in this thread wasn't fixed.