Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/solc 0.7.4 warnings #2391 #2396

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

zemse
Copy link
Contributor

@zemse zemse commented Oct 25, 2020

Fixes #2391

This PR aims to remove plenty of warnings that show up when compiling contracts with solc 0.7.4.

There were three kinds of warnings:

  1. declaration shadowing
WARNING
  contracts/proxy/TransparentUpgradeableProxy.sol:33:33: Warning: This declaration shadows an existing declaration.
      constructor(address _logic, address _admin, bytes memory _data) payable UpgradeableProxy(_logic, _data) {
                                  ^------------^
  contracts/proxy/TransparentUpgradeableProxy.sol:126:5: The shadowed declaration is here:
      function _admin() internal view returns (address adm) {
      ^ (Relevant source part starts here and spans across multiple lines).
  1. return variable
WARNING
  contracts/proxy/TransparentUpgradeableProxy.sol:70:48: Warning: Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable.
      function admin() external ifAdmin returns (address) {
                                                 ^-----^
  1. state mutability
WARNING
  contracts/mocks/GSNRecipientMock.sol:14:5: Warning: Function state mutability can be restricted to pure
      function acceptRelayedCall(address, address, bytes calldata, uint256, uint256, uint256, uint256, bytes calldata, uint256)
      ^ (Relevant source part starts here and spans across multiple lines).
  1. To prevent declaration shadowing, contract method arguments are suffixed with an underscore. Given that name naming is generally used for public vars or functions and _name naming for private vars or functions. Just to mention, this convention is already present in OpenZeppelin contracts and many other projects use this convention. Also, many devs use underscore prepending to function args to prevent shadowing but since solc 0.7.4, the compiler highlights if that shadows functions too (which were missed before). So having an alternate convention for arguments easily solves the problem. Inspired from here, the convention of having name_ naming for function arguments to prevent shadowing is taken in this PR in the first commit.

  2. The second commit tries to silence this warning.

  3. The third commit simply changes view to pure.

Please let me know if this is fine or any changes that need to be done.

@frangio
Copy link
Contributor

frangio commented Oct 27, 2020

Thank you @zemse!

I'm not very fond of the underscore suffix. I know we use it in a few places, but so far it has only been when the underscore prefix was not available due to shadowing. Rather than adopting this convention for all arguments, I would prefer to continue with our current convention:

  1. arguments by default are not underscored,
  2. if the name isn't available due to shadowing (and a different name is not reasonable), use prefix underscore,
  3. if prefix underscore isn't available due to shadowing, use suffix underscore.

There may be some issues with this approach but I would rather we solved the warnings as simply as possible first, then discuss the issues with this convention and if necessary come up with a new one.

Would you like to make the changes I suggested?

@zemse
Copy link
Contributor Author

zemse commented Oct 27, 2020

I get the point. This is something which I could discuss before actually making the change.

Would you like to make the changes I suggested?

Yeah, likely by EOD.

This commit fixes warnings thrown by the solc 0.7.4 compiler:
"Warning: Unnamed return variable can remain unassigned. Add an explicit
return with value to all non-reverting code paths or name the variable."
This commit fixes warnings thrown by the solc 0.7.4 compiler:
"Warning: Function state mutability can be restricted to pure"
@zemse zemse force-pushed the fix/solc-0.7.4-warnings-#2391 branch from a28d37e to 909216a Compare October 28, 2020 04:00
This commit fixes warnings thrown by the solc 0.7.4 compiler:
"Warning: This declaration shadows an existing declaration."

1. Arguments by default are not underscored.
2. If the name isn't available due to shadowing, use prefix underscore.
3. If prefix underscore isn't available due to shadowing, use suffix underscore.
@zemse zemse force-pushed the fix/solc-0.7.4-warnings-#2391 branch from 909216a to 106be12 Compare October 28, 2020 04:07
@zemse
Copy link
Contributor Author

zemse commented Oct 28, 2020

@frangio I've removed previous commit and added new commit based on the followed conventions. Can you please have a look?

@frangio frangio changed the base branch from release-v3.2.0-solc-0.7 to solc-0.7 October 28, 2020 16:41
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, thank you!

@frangio frangio merged commit 0f55c18 into OpenZeppelin:solc-0.7 Oct 28, 2020
frangio added a commit that referenced this pull request Oct 28, 2020
@frangio frangio mentioned this pull request Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants