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

The comprehensive-interface rule results in compiler warning: Interface functions are implicitly "virtual" #519

Open
ncjones opened this issue Nov 3, 2023 · 7 comments
Assignees
Labels
awaiting user feedback awaiting user feedback

Comments

@ncjones
Copy link

ncjones commented Nov 3, 2023

I want to make sure all our public functions are carefully considered and well documented. The comprehensive-interface Solhint rule helps me to achieve this.

The problem I am facing is that the way this rule is implemented requires me to use the override keyword but the Solidity compiler doesn't like this to be used with interfaces.

When I add override without a corresponding virtual on my interface function then the compiler errors:

TypeError: Trying to override non-virtual function. Did you forget to add "virtual"?

When I address the compiler error by adding virtual to the interface function then the compiler produces a warning:

Warning: Interface functions are implicitly "virtual"

Solidity version: 0.8.21
Solhint version: 3.6.2

@ncjones
Copy link
Author

ncjones commented Nov 3, 2023

It's not possible to disable Solidity compiler warnings so I believe the Solidity compiler is out-of-line here -- warning about the potentially redundant virtual keyword should be a linting responsibility.

@ncjones
Copy link
Author

ncjones commented Nov 3, 2023

Perhaps the comprehensive-interface rule can be implemented differently to avoid this problem. Instead of requiring the override keyword, could it look for a matching signature on all implemented interfaces?

@ncjones
Copy link
Author

ncjones commented Nov 3, 2023

Probably this rule used to be helpful before override was removed as a requirement for functions implementing interfaces: ethereum/solidity#8281.

@ncjones
Copy link
Author

ncjones commented Nov 3, 2023

My plan for the workaround is to use this rule temporarily to help me find all the public functions and ensure they are all declared in interfaces. Once all functions are in interfaces, I can replace all contract interactions from hardhat tests to use the interfaces. This should ensure new public functions are not added without declaring them in the interface.

@ncjones
Copy link
Author

ncjones commented Nov 5, 2023

The @inheritdoc natspec annotation causes the solidity compiler to fail when there is no matching inherited function. So, instead of requiring the override keyword, perhaps the solhint "comprehensive-interface" rule could enforce that @inheritdoc is present.

@dbale-altoros
Copy link
Collaborator

@ncjones thanks a lot for your input and for taking the time to research possibilities on top of pointing the issue alone
We will go through this and get back to you. Probably next week...
Thanks for participating!

@dbale-altoros
Copy link
Collaborator

dbale-altoros commented Dec 5, 2023

@ncjones the way solhint works cannot check the interfaces associated to the contracts. So that idea cannot be implemented.

You can also tried to inherit the interface and define the contract with that interface:

pragma solidity 0.8.0;

import  "./IERC20.sol";

contract Foo1 is IERC20 {
	constructor {}
    
    function foo(uint bar) internal {}  
}

This will check that all public and external functions are in the interface

@dbale-altoros dbale-altoros self-assigned this Dec 5, 2023
@dbale-altoros dbale-altoros added the awaiting user feedback awaiting user feedback label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user feedback awaiting user feedback
Projects
None yet
Development

No branches or pull requests

2 participants