Closed madvas closed 1 year ago
I understand that this is quite annoying. On the other hand, I guess we owe it to our users to constantly improve all code, including library code.
Can you think of some kind of "middle ground" between these two extremes?
Without having reviewed everything, the warnings about using jumps may be actually cover bugs:
strings.sol:474:21: 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.
jumpi(exit, eq(and(mload(ptr), mask), needledata))
^---^
strings.sol:476:21: 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.
jumpi(loop, lt(sub(ptr, 1), end))
^---^
strings.sol:511:21: 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.
jumpi(ret, eq(and(mload(ptr), mask), needledata))
^---^
strings.sol:513:21: 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.
jumpi(loop, gt(add(ptr, 1), selfptr))
^---^
strings.sol:515:21: 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.
jump(exit)
^--^
This shouldn't be a warning though:
strings.sol:636:5: Warning: This declaration shadows an existing declaration.
function rsplit(slice self, slice needle) internal returns (slice token) {
^
Spanning multiple lines.
The shadowed declaration is here: strings.sol:614:5:
function rsplit(slice self, slice needle, slice token) internal returns (slice) {
^
Spanning multiple lines.
I think compiler and linter shouldn't be one thing without option to run each one separately. Turning off linter for 3rd party libraries is not extreme option. We don't run it on node_modules
files, when we develop JS project, do we?
On the other hand, I guess we owe it to our users to constantly improve all code, including library code.
It's pretty naive to think people will happily start to make changes to 3rd party code when compiler makes them feel annoyed. They'll just use grep
to filter out useless warnings. I wouldn't touch 3rd party code that's been audited and deployed on mainnet even if compiler warnings want me to.
@Madvas doesn't that defeat the purpose of a Dapp and ethereum in general? Or am I confused about the point of a decentralized application framework? I know RNGs can be hard to make securely in the ethereum environment and someone has done it -- but do you really want to deploy a roll-your-own version or a even copy of that RNG contract to the mainnet along side whatever contracts you've built that need it and maintain it? Aside from inundating the blockchain with repeated or similarly purposed contracts and code, you'll end up making your applications' code base fatter than it needs to be. I thought programmers were supposed to be reducing and reusing repeated code. Or am I thoroughly misinformed? My apologies if I'm out of bounds or off topic.
Function overloads wrongly considered as shadowing is tracked in #2676.
I am a strong advocate of providing a mechanism for ignoring warnings. I personally prefer per line warnings like in #2691 but warnings do need a way of being ignored. The reasoning for this is because if you don't give people a way to ignore specific warnings, they will end up ignoring all warnings just to silence a few. We want to encourage people to treat warnings as errors for the purpose of CI and release, but we can only do that if we give them a way to ignore specific warnings when it is appropriate. In a large project with thousands of lines of code, there is likely one or two places where the warning should be suppressed. If the only option to the user is to ignore all warnings in CI in order to get a build pushed through, then we have just created a vector for introducing bugs in a project that otherwise would have followed a good procedure for minimizing risk.
One option could be to take .soliumignore
into account
The reasoning for this is because if you don't give people a way to ignore specific warnings, they will end up ignoring all warnings just to silence a few.
Our team has stopped reading the several screens of warnings from OpenZeppelin, Chainlink, etc. that are being outputted and consequently we are missing the warnings from our own code now. Wish there were an option to turn off the several screens of useless warnings so we can see the few from ours at the end only. (you may debate if they are useless, but the vast majority are about license identifiers...). I may just implement the recommendation above from someone to grep out the useless ones.
In the interest of pushing this forward, I added this to the design backlog and we'll be discussing this on Wednesday's call. The meetings are open so if anyone's interested, feel free to join.
Do you have any proposal? This issue suggest adding some option into standard json and the CLI.
If we consider ignoring warnings, I would prefer #2691.
Good point. I was actually thinking about both and did not realize that there's a separate issue for ignoring warnings via comments. I think we could discuss both at the same time.
Actually to clarify, I think these two issues try to solve very different problems: 1) Disabling warnings for certain files aims to ignore imported projects.
2) Disabling warnings-per-statement is aimed at making warnings go away on constructs the programmer feels safe, but the compiler disagrees. However, do we still have such cases? We did not had such warnings for a very long time.
So 1) is a more pressing issue and perhaps we should just consider that situation. I wonder whether for that case a compilation setting or an in-source annotation better shows the intent? Perhaps if a file is ignored, we should raise a warning stating the file is ignored? 🙊
@chanhosuh what kind of warnings are you seeing?
Right they're not the same but I think they're still closely related. I think it might be good to start with discussing all of these in general and at least decide which ones we're going to address.
Also, I'd add two more to the list:
I really would like to see what warnings are there which cause annoyance. Read through all the examples brought up here and in #2691, but to me it looks like the large majority of them are not relevant anymore, or a good way to silence them exists.
These were the issues brought up:
try myaddress.realFunction(_id) { } catch {}
)Can we perhaps open a section in the documentation for common warnings, what they mean, how to work them around?
Regarding the case when imported libraries must be silenced: I wonder why people want to trust libraries emitting large number of warnings?
@axic You didn't mention the warning "constructor visibility is ignored".
I wonder why people want to trust libraries emitting large number of warnings?
Take OpenZeppelin Contracts 3.x. It was free of warnings for Solidity 0.6, but the warning I mentioned above was introduced in Solidity 0.7. We could either release a new major version of the library (due to the breaking change that is bumping the pragma), or extend pragma solidity
to cover both 0.6 and 0.7. We did the latter because we wanted to spare our users the major update, but this resulted in lots of warnings, so at the same time we had to release a separate version of the package specifically for Solidity 0.7. There is nothing to distrust from the package with warnings.
I don't think warnings have anything to do with trust. The developer can know when a warning can be safely ignored. There is more to distrust from inline assembly, and there are no warnings about its use.
Is there a warning about unused function arguments? I've seen it but I can't seem to reproduce it now.
State mutability can be restricted -- IIRC narrowing mutability during overriding is allowed now
@axic Isn't the problem the inverse? We sometimes want to define a virtual view
function as view because we want to allow child contracts to be able to read state, yet the default implementation is pure. Narrowing mutability is allowed but widening it is (rightly) not allowed.
Edit: Apparently this warning is not generated for virtual functions anymore.
We discussed this on the last weekly call and the point of contention was whether it's really an issue to be solved in the compiler itself. So far we think that we need to gather more feedback to be able to make a decision here (@franzihei).
There were opinions that this issue is relevant only when the user is using the compiler directly on the command line and that this is actually a pretty rare case. Most people are probably using IDEs or at least invoke the compiler indirectly, under the control of a tool like Truffle, Hardhat, Brownie, etc. In these cases the tool usually uses the JSON interface and is free to present the warnings it receives in any way that makes sense for it, including suppressing ones that come from libraries. Such a tool is actually in a better position to decide this because it can actually know if the file being compiled resides in the project or is a part of an external library installed by the user. The compiler currently does not make that distinction at all.
If we did want to add more control over the non-JSON output to the compiler interface, the next question is how much control is actually needed. Do we actually need to do it for specific files? Maybe it would be enough to just display warnings and errors separately so that errors are easier to find? And then, should errors be printed first or last? Also, maybe a more complex system, with severity assigned to specific warning types would be better?
Regarding the case when imported libraries must be silenced: I wonder why people want to trust libraries emitting large number of warnings?
@axic you mean I should not trust OpenZeppelin contracts, like the ubiquitous Ownable
contract because I get this warning:
@openzeppelin/contracts-ethereum-package/contracts/access/Ownable.sol: Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
There are tons of SPDX identifier warnings from external libraries such as OpenZeppelin, Chainlink, etc.
Also, deprecation warnings like this:
Warning: Documentation tag on non-public state variables will be disallowed in 0.7.0. You will need to use the @dev tag explicitly.
We discussed this on the last weekly call and the point of contention was whether it's really an issue to be solved in the compiler itself. So far we think that we need to gather more feedback to be able to make a decision here (@franzihei).
@cameel thanks for re-opening this for discussion. I do think this is a pain point for the community, so this is really appreciated.
There were opinions that this issue is relevant only when the user is using the compiler directly on the command line and that this is actually a pretty rare case. Most people are probably using IDEs or at least invoke the compiler indirectly, under the control of a tool like Truffle, Hardhat, Brownie, etc. [snipped...] Such a tool is actually in a better position to decide this because it can actually know if the file being compiled resides in the project or is a part of an external library installed by the user. The compiler currently does not make that distinction at all.
This is a very good point. I'm going to raise this with the Hardhat devs and see what they say.
Oh, the SPDX thing I have long felt should be disablable. It is incredibly opinionated about how licencing works and not everyone agrees on it. I personally think it is amazingly dumb to have license information (metadata) inside my source files. I have a repository wide license file and I don't want my source files cluttered with that stuff. On top of that it limits licenses to SPDX licenses, which is constraining for any kind of iteration on the licensing front.
If you want to have opinionated things, put them in a litter and not a compiler. It is totally inappropriate for a compiler to be checking for that.
Oh, the SPDX thing I have long felt should be disablable. It is incredibly opinionated about how licencing works and not everyone agrees on it. I personally think it is amazingly dumb to have license information (metadata) inside my source files. I have a repository wide license file and I don't want my source files cluttered with that stuff. On top of that it limits licenses to SPDX licenses, which is constraining for any kind of iteration on the licensing front.
If you want to have opinionated things, put them in a linter and not a compiler. It is totally inappropriate for a compiler to be checking for that.
Every single compiler warning should be suppressible, by file, and by warning type, without changing source code, if at the discretion of the runner of said compiler, the warnings are acceptable or not necessary to review further.
Case in point: OpenZeppelin 3.x contract with v0.7 of compiler, like so:
@openzeppelin/contracts/access/Ownable.sol:26:5: Warning: Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.
constructor () internal {
^ (Relevant source part starts here and spans across multiple lines).
Can I grep the output before my CI tool complains? Sometimes. Depends on the toolchain, platform, etc.
Using truffle, npm, and a platform with sed installed, I do:
"scripts": {
"test": "truffle test | sed -e '/Warning: Visibility for constructor is ignored./,+2d'"
}
Gross huh?
I am not sure what the utility function of warnings looks like over time, but I would guess y=e^(-x), x>=0.
@chuckb You can install a version specific for Solidity 0.7 that does not have this warning with npm install @openzeppelin/contracts@3.4.0-solc-0.7
. Hopefully this special version will not be necessary for future releases of Solidity.
Just want to chime in and say I am also interested in disabling/hiding certain warnings. Case in point
Warning: Contract code size exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.
are irrelevant for testing contracts that will never be deployed.
But without derailing the discussion to this specific example, I just want to emphasize the general point – being able to filter warnings for signal from the noise is highly useful, and different developers value warnings differently.
Case in point
@MrChico We have a separate issue about the code limit: #10981. Maybe you could add some feedback there?
I've created a Hardhat plugin to ignore compiler warnings.
This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.
For future visitors - if you are using Foundry, there is a neat way to ignore Solidity compiler warnings using the ignored_error_codes
config option:
# ignore solc warnings for missing license and exceeded contract size
# known error codes are: ["unreachable", "unused-return", "unused-param", "unused-var", "code-size", "shadowing", "func-mutability", "license", "pragma-solidity", "virtual-interfaces", "same-varname"]
# additional warnings can be added using their numeric error code: ["license", 1337]
ignored_error_codes = ["license", "code-size"]
I successfully reproduced a test of an ancient (0.4.26) contract, but when I use forge ffi to compile the 0.4.26 contract separately, stderr always appears, because the compilation result has some warnings
forge test -C src/Capture_the_Ether/Math/Donation --ffi -vvv
[⠰] Compiling...
[⠔] Compiling 1 files with 0.4.26
[⠒] Compiling 19 files with 0.8.19
[⠒] Solc 0.4.26 finished in 43.37ms
[⠘] Solc 0.8.19 finished in 2.07s
Compiler run successful
src/Capture_the_Ether/Math/Donation/DonationChallenge.sol:28:9: Warning: Variable is declared as a storage pointer. Use an explicit "storage" keyword to silence this warning.
Donation donation;
^---------------^
src/Capture_the_Ether/Math/Donation/DonationChallenge.sol:28:9: Warning: Uninitialized storage pointer. Did you mean '<type> memory donation'?
Donation donation;
^---------------^
2023-04-15T01:22:18.630270Z ERROR foundry_evm::executor::inspector::cheatcodes::ext: stderr err="src/Capture_the_Ether/Math/Donation/DonationChallenge.sol:28:9: Warning: Variable is declared as a storage pointer. Use an explicit \"storage\" keyword to silence this warning.\n Donation donation;\n ^---------------^\nsrc/Capture_the_Ether/Math/Donation/DonationChallenge.sol:28:9: Warning: Uninitialized storage pointer. Did you mean '<type> memory donation'?\n Donation donation;\n ^---------------^\n"
Running 1 test for src/Capture_the_Ether/Math/Donation/DonationChallenge.t.sol:DonationChallengeTest
[PASS] testDonation() (gas: 173955)
Logs:
donationChallenge's owner: 0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f
donationChallenge's balance: 1000000000000000000
hacker's balance: 949036887367
after hack, donationChallenge's owner: 0xa63c492d8e9ede5476ca377797fe1dc90eeae7fe
hacker's address: 0xa63c492d8e9ede5476ca377797fe1dc90eeae7fe
after hack, donationChallenge's balance: 0
after hack, hacker's balance: 1000000949036887367
Test result: ok. 1 passed; 0 failed; finished in 55.67ms
This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.
Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.
When developing, we often include 3rd party contracts, which have been audited and deployed to mainnet, but don't play well with solc's latest warnings policy. Warnings for those files are annoying and useless.
For example if I develop dapp that facilitates ENS contracts and strings library, this is amount of warnings I get on single project compilation. All comes from AbstractENS.sol and strings.sol, which are verified and safe contracts. Makes other compiler output practically unreadable.
solc 0.4.14+commit.c2215d46.Darwin.appleclang
Thanks