Should private functions really clash ?

[Amxx]

Let consider the following code:

contract A {
    function __myPrivateFunction() private {
        // Does something usefull
    }
    
    function callA() public {
        __myPrivateFunction();
    }
}

contract B {
    function __myPrivateFunction() private {
        // Does something usefull
    }
    
    function callB() public {
        __myPrivateFunction();
    }
}

contract AB is A, B {}

What I am expecting

implementation of __myPrivateFunction are private, and they only make sense in the context of the contract that they are part of (A and B). They are not accessible from AB, so it is not like if a call from a function within AB wouldn't be resolvable.

IMO, this should compile, possibly with a warning, but not with an error.

What happens

I get an error:

TypeError: Derived contract must override function "__myPrivateFunction". Two or more base classes define function with same name and parameter types.

Question/Request

Is that behaviour wanted? needed? Whould it be possible to accept this kind of ghost-conflicts?

[chriseth]

The error message is of course bad because you cannot override the functions.

I'm fine with allowing it, but then without a warning.

[cameel]

I also think that such functions should be allowed and without a warning. If private functions cannot even be referenced from inherited contracts, they should not clash.

[chriseth]

We should be careful if non-private can be changed to private in inheritance.

[cameel]

Looks like it can't:

contract C {
    function f() internal virtual {}
}

contract D is C {
    function f() private override {}
}

Error: Overriding function visibility differs.
 --> test.sol:6:5:
  |
6 |     function f() private override {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: Overridden function is here:
 --> test.sol:2:5:
  |
2 |     function f() internal virtual {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[cameel]

Also:

contract C {
    function x() external virtual returns (uint) {}
    function y() public virtual returns (uint) {}
    function z() internal virtual returns (uint) {}
}

contract D is C {
    uint private override x;
    uint private override y;
    uint private override z;
}

Error: Identifier already declared.
 --> test.sol:9:5:
  |
9 |     uint private override y;
  |     ^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
 --> test.sol:3:5:
  |
3 |     function y() public virtual returns (uint) {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Error: Identifier already declared.
  --> test.sol:10:5:
   |
10 |     uint private override z;
   |     ^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
 --> test.sol:4:5:
  |
4 |     function z() internal virtual returns (uint) {}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Error: Override can only be used with public state variables.
 --> test.sol:8:5:
  |
8 |     uint private override x;
  |     ^^^^^^^^^^^^^^^^^^^^^^^

Error: Override can only be used with public state variables.
 --> test.sol:9:5:
  |
9 |     uint private override y;
  |     ^^^^^^^^^^^^^^^^^^^^^^^

Error: Override can only be used with public state variables.
  --> test.sol:10:5:
   |
10 |     uint private override z;
   |     ^^^^^^^^^^^^^^^^^^^^^^^

[axic]

Shadowing makes no sense considering inaccessible functions (i.e. private), so in essence allowing this makes sense.

However it definitely opens up the possibility for crafting/hiding confusing code, given contracts can have many internal and private functions, combining that with inheritance sometimes it is really hard to decipher what is the actual code used for the contract. The virtual/override specifiers help with that.

While I do not think it is a good idea to try "saving people from mistakes" with weird rules like this, but I would put the question on the table: should this change be made in a breaking release, just in order to trigger a more thorough review from people using the language? Users tend to review their contracts more carefully when they bump a breaking release.

[chriseth]

@Amxx can you provide more cases that would benefit from this feature? I actually think @axic 's point is very valid here.

[Amxx]

My use-case is a bit complex, and one might argue that it comes from bad design on our end.

---

OpenZeppelin provides a lot of functionalities in its different contracts and library. We also offer a framework for upgradeable contracts. This framework includes solidity code, and javascript tooling to help use deploy and update their contracts.

This tooling, in particular, checks dangerous behaviour, such a selfdestruct (see parity bug) and delegate calls (which can lead to self destruct). We want to protect users from using these in upgradeable contracts.

We have an Address library, which includes functions such as isContract, sendValue, functionStaticCall and functionDelegateCall. While most are safe, this last one can be dangerous. In our plugin we don't have the ability to filter that effectively, and whenever a user import the library, we do check the entire library for potential vulnerabilities.

To limit false positives, we decided to:

• Remove the functionDelegateCall for the Address library (in the upgradeable repo)
• In the very rare places where we internally use it (in a safe way), inline a private copy with natspec comment to locally disable our security check.

The result is that we have this private function in some of our upgradeable contracts:

• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/utils/MulticallUpgradeable.sol#L37-L43
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/proxy/ERC1967/ERC1967UpgradeUpgradeable.sol#L207-L213

Now if I want to build a UUPSUpgradeable contract (inherits from ERC1967UpgradeUpgradeable) with the multicall feature ... I get my private function defined twice.

I'm sure we can rework our transpilling workflow, and the way our checks are performed ... but you asked me for my usecase so here it is.

[frangio]

I don't think the concern that @axic shared is a significant one. Private functions are the easiest to trace back to their definition because it just requires a search in the contract where it is used.

[k06a]

Had the same issue in the mid 2020: #9610

[k06a]

Now private works as hypothetical final/nonoverride, but I wish private methods produce no side effects outside of the contracts/libraries.

[cameel]

We discussed this today on the call. Overall there were some concerns about safety of allowing this but no one was strongly against so we're fine with changing the behavior. This is however very low priority right now because we have other important tasks on the roadmap. It may take some time before we get to implementing this ourselves, but we'd accept a PR if someone badly wanted to have it right now.

[k06a]

We can use GitCoin to boost this change :) Moreover, this could attract more contributors to the repo.