Solidity: Allow comments to ignore compiler warnings.

[MicahZoltu]

I would like the ability to disable compiler warnings on a per-line basis using something like comments or pragma. C++ has a mechanism to also disable comments across a block using #pragma push (disable: 1234) and #pragma pop. I'll leave it up to the language designers to decide the exact mechanism based on which one is most reasonable to implement. The key is that I need to be able to disable any single specific warning in the source code that is generating that warning.

With solc becoming more and more opinionated with its compiler warnings, there needs to be a way to disable them selectively. At the moment, we use solc compiler warnings/errors as a CI gate block but we are finding that some of the warnings _should_ be ignored, yet we don't have a reasonable way to do this right now.

For example, we have a SafeMath library that leverages using SafeMath for <type>. This library does function overloading/shadowing so that it can have both add(uint256,uint256) and add(int256,int256) in the same library. I appreciate the warning, but I need the ability to disable it on a case-by-case basis. The same goes for a number of other warnings that have been added recently. In general I think are all very valuable, but may not be able to be applied exhaustively to all code.

[axic]

This is a duplicate of #2675. Please follow up on that thread.

Note: it falsely reports overloading as shadowing and that is being fixed (also addressed in a different issue).

[MicahZoltu]

@axic I saw that, but it appears that discussion is around file-wide ignores, rather than per-line ignores. Are you suggesting that you think both tasks would likely be solved at the same time? Normally, when I'm running a GitHub project I prefer issues to be very specific as to the feature being requested and this feels like two separate features to me. (per line ignore and per file ignore)

[axic]

Sorry I didn't pick up you are suggesting a way to disable warnings on specific lines with adding a specific marker at that (like a comment). That is of course a different issue than #2675. If so, please reopen this and explain your proposal.

However, if you read it carefully, there is a strong sentiment at the moment not to support any option to disable warnings. (We need to remove false negatives though.)

[MicahZoltu]

I'll propose my arguments against no-disable option in the other issue.

I have updated the description of this issue to add clarity that I'm looking for a targeted in-source mechanism for disabling, but I don't have the access rights to re-open the issue. Would you mind re-opening it for me if you believe the issue is clear enough now @axic?

[axic]

Thanks, it is clear now!

[ryankc33]

+1. The compiler warnings break backwards compatibility with many external libraries. This will be a great feature to have.

[lisuyong]

Hi, there

What's this issue's status now? I'm so hopeful for this nice feature. 😺

[yarrumretep]

+1 for this - it's hard when the compiler is printing warnings that you acknowledge and want to ignore. It distracts from reading the warnings that may crop up but you don't want to ignore. A comment mechanism as proposed here would allow one to express that they understand the risks.

[chriseth]

@yarrumretep we take special care such that any warning given by the compiler can be removed by a small change to the source code. Could you please give an example where it would be easier to "remove" the warning by adding a comment instead of changing the source code to remove the risk altogether?

[axic]

Actually a short term solution will be the 0.5.0 release turning most, if not all, these warnings into an error :wink:

[yarrumretep]

@chriseth we are seeing Warning: The "returndatacopy" instruction is only available after the Metropolis hard fork. Before that it acts as an invalid instruction. I am happy to have been notified of this, but would like to suppress this message as we are happy to proceed understanding that limitation. Same for returndatasize, methinks.

We are also seeing Warning: Jump instructions are low-level EVM features that can lead to incorrect stack access. Because of that they are discouraged. Please consider using "switch" or "for" statements instead. We are getting these from compiling Aracnid's string library - they are in functions we aren't using so we can prune them from our copy of the library. But if jump is not going to be allowed shouldn't it be deprecated and eventually removed from the language? BTW I don't have an opinion on this - just want to be able to get to a clean build when using the available features of the language.

[chriseth]

@yarrumretep we are still debating how to handle different versions of the EVM, but I hope that we can resolve the returndatacopy issue soon - sorry that it takes so long.

Concerning jumps: Yes, please either remove the code or submit a pull request to the library turning it into code that does not use manual jumps.

We actually did deprecate them, but yes, that should probably be mentioned literally as part of the warning.

[yarrumretep]

Thank you for the clarification @chriseth. Maybe you could handle the hardfork thing with a compiler flag indicating what version of the EVM you want to target. Then the returndatasize etc would error appropriately.

[chriseth]

@yarrumretep yes, the question is basically whether we want to use a pragma or a compiler switch, but we will probably go with a compiler switch. Since this would be the first switch that affects code generation itself, it is a little tricky :)

[yarrumretep]

@chriseth - interesting. BTW, we are using those opcodes to grab a string return value b/c solidity currently doesn't return strings from calls. Is this going to burn us or simply be unnecessary with future solidity versions?

[chriseth]

We can hopefully merge the new abi decoder soon, then it will be automatic.

[chriseth]

but you can of course also do it using inline asssembly

[celeduc]

This has been particularly annoying since var was deprecated, since there is presently no way to implement var (,,,,,what,,evs,,,dude,) = lib.extFunc("arg"). I'd like to be able to silence these warnings until that is resolved, as it spits out a separate error for each variable (what, evs, dude) and makes it very hard to see real warnings.

[xuguojie]

Agree with getting var warning suppressed.

[chriseth]

@celeduc can you explain why var (,,,,,what,,evs,,,dude,) = lib.extFunc("arg") cannot be done without warnings anymore? W what; E evs; D dude; (,,,,,what,,evs,,,dude,) = lib.extFunc("arg") should work.

[celeduc]

@chriseth Thank you for your note.

1) Solium already throws a style error for such a line: " 88:24 warning Only use indent of 4 spaces. indentation"
2) because it contains multiple statements in one line, it will _probably_ be considered unreadable by linting tools as they become more sophisticated
3) because it is an ugly hack, perhaps even uglier than var (,,,,,what,,evs,,,dude,) = lib.extFunc("arg")

I agree with previous commenters in #3301 that the syntax should be (,,,,,W what,,E evs,,,D dude,) = lib.extFunc("arg") if var is to be deprecated without removing functionality.

(This whole issue wouldn't be as important if external functions could return structs.)

[chriseth]

@celeduc I was talking about a solution that has newlines and proper indentation. You mentioned that there is "presently no way" to do this. Can you please explain in more detail why it is not possible, since, as you say, a multi-assignment like (,,,,,W what,,E evs,,,D dude,) = lib.extFunc("arg") should solve the problem. I agree that external functions returning structs would be much nicer, but we are unfortunately not there yet.

[celeduc]

@chriseth With var it was a single expression on one line. Without var it is four expressions on four lines. It is no longer concise. Deprecating var takes away a popular, concise solution, and the deprecation warning cannot be silenced. The proposed multi-assignment solution (,,,,,W what,,E evs,,,D dude,) = lib.extFunc("arg") does not compile ("ParserError: Expected token Comma, got 'Identifier'") with solc 0.4.21.

[chriseth]

@celeduc it should be (,,,,,what,,evs,,,dude,) = lib.extFunc("arg") is in my first example.

[celeduc]

Before var was deprecated we could use the single, concise, one-line expression:

var (,,,,,what,,evs,,,dude,) = lib.extFunc("arg");

But then var was deprecated, and the above line generates three separate warnings. To remove these warnings, we now have to use four expressions (on four lines) to do the same thing:

W what;
E evs;
D dude;
(,,,,,what,,evs,,,dude,) = lib.extFunc("arg");

It has been proposed that the following syntax be supported (IMO it should have been before var was deprecated), which would again allow a single expression in one line, just like the original code which used var:

(,,,,,W what,,E evs,,,D dude,) = lib.extFunc("arg");

Reducing the total number of lines of code helps to increase the readability and clarity of a function, especially when a large number of redundant declarations makes it difficult to fit an otherwise small amount of code on the screen. We look forward to being able to return structs from external functions, but for the time being we are stuck with this syntax as our only recourse, and it sure would be nice if the language weren't moving backward on this front.

But, given that var has already been deprecated it would at least be good to avoid polluting our compilation logs with these warnings in the meantime.

[j-h-scheufen]

@chriseth Following up on your comment from Feb. 12, I'm adding a couple of examples where it's impossible to get rid of warnings via code changes.

ERC165

The examples to implement ERC165 contracts rely on calling this in the constructor to refer to function selectors: supportedInterfaces[this.supportsInterface.selector] = true;
However, the use of this in constructors is now flagged with a warning: https://github.com/ethereum/solidity/issues/583

While ultimately, the code inspection for this warning should be able to distinguish between "allowed" and "disallowed" uses, the 583 ticket discussed the complexity of it, so it's not likely to be implemented anytime soon. So, in the meantime, it would be a great help to be able to turn these warnings off selectively (i.e. by line comments).

Abstract contracts

When using the OO concept of abstract contracts, you end up with multiple warnings for "unused var". Example (written off the cuff, no guarantee this compiles, only to make the point):

contract MyInterface {
   function processBytes32(bytes32 _param);
   function processAddress(address _param);
   function processUint(uint _param);
}

contract AbstractMyInterface is MyInterface {
   function AbstractMyInterface() internal {}
   function processBytes32(bytes32 _param) {}
   function processAddress(address _param) {}
   function processUint(uint _param) {}
}

contract MyCustomImpl is AbstractMyInterface {
   // custom impl chooses only to overwrite one function of the abstract contract
   function processBytes32(bytes32 _param) {
      // do something with bytes32
   }
}

In this example, the only way to avoid the "unused var" warnings is to not use the concept of abstract implementations at all and implement empty functions in all contracts implementing the interface. A programmer should not have to make such a design choice due to warnings being annoying. Having the ability to flag the abstract functions with a comment to ignore the warnings would therefore be very helpful.

[chriseth]

@j-h the first one is a little tricky, yes. Since this part has both a high false positives and a high false negatives rate, you can use ContractType self = this; supportedInterfaces[self.supportsInterface.selector] = self;.

The second warning can be suppressed by either removing the variable names (only the name) or commenting it out.

[j-h-scheufen]

Thanks @chriseth ! You just cleaned up my build log tremendously! I should've thought about the fact that the variable name is actually not needed for the abstract implementation. Never questioned it after the abstract contracts initially were made by copy/paste from the interface.

[nventuro]

Are there any plans/spec for implementing this? I'd guess a major blocker would be coming up warning ids, so that only specific ones are suppressed.

False-positive warnings are currently a pain point when developing/using Solidity libraries (in the software engineering sense of the word, not a Solidity library): often, base contracts will have methods to be overridden by their children, like in OpenZeppelin's Crowdsale:

function _preValidatePurchase(address beneficiary, uint256 weiAmount) internal view {
    require(beneficiary != address(0), "Crowdsale: beneficiary is the zero address");
    require(weiAmount != 0, "Crowdsale: weiAmount is 0");
}

That function is view because overrides may require e.g. reading from storage in order to perform their task, but it doesn't need this attribute in the base contract - pure would suffice.

openzeppelin-solidity/contracts/crowdsale/Crowdsale.sol:138:5: Warning: Function state mutability can be restricted to pure

There's no workaround for this (relatively common) scenario, which emits warnings on both the library developer and user's workflows. Having a way to silence warnings selectively (e.g. // solc-disable-line possible-stricter-mutability) would be extremely helpful.

[ekpyron]

@nventuro For this particular case you could use

function _preValidatePurchase(address beneficiary, uint256 weiAmount) internal view {
    require(beneficiary != address(0), "Crowdsale: beneficiary is the zero address");
    require(weiAmount != 0, "Crowdsale: weiAmount is 0");
    this; // silence state mutability warning without generating any additional byte code
}

EDIT: that is, unless we start warning about such a this; at some point :-).

[nventuro]

Thanks, that works! Still, it'd be great to now have to rely on those hacks and have a proper way to signal a warning is a false positive.

[fulldecent]

If a contract is marked as abstract then it cannot be compiled. And therefore any notes warnings (not errors) about state mutability don't matter. This is because they are going to be overridden and the mutability will be considered on the compilable version.

Would suppressing can-have-stricter-mutability-guarantees for abstracts solve @nventuro's issue?

[mudgen]

What is the status of being able to disable a warning from a line?

[mudgen]

I am getting the following warning that I need to suppress: "Return value of low-level calls not used." How can I suppress this warning? I don't want to use the return value of the low level call.

[chriseth]

There is no such plan, sorry. The way to disable the warning is to not ignore the return value.

[mudgen]

Okay. So I don't ignore the return value. I assign it to a local variable. Now solidity gives me this warning: "Unused local variable." So now what I am I supposed to do? I have no need of this variable at all. Do I do some useless thing with it just to get rid of the warning? Any suggestions?

[mudgen]

Here is my code to get rid of the warning:

// success variable not need, only here to remove warning from compiler
(bool success,) = myaddress.call(abi.encodeWithSelector(0x3cfc70f7, _Id));
success;

[chriseth]

bool success;
(success, ) = myaddress.call("");

would be another way.

[chriseth]

You can also use

try myaddress.realFunction(_id) { } catch {}

[mudgen]

ah, these are better. Much appreciated!

I like using try, catch to avoid low level calls. Is there a gas difference?