Fix inheritedFunctions returns private functions in OverrideChecker

[enriquejr99]

Closes #12767 and #11889

[ekpyron]

Thank you for the PR!

This is mainly missing (a lot of) semantics tests. We have to make sure that, under any circumstances, the correct functions are called in the presence of multiple private functions in the inheritance graph. Since code generation was not designed with this in mind, we have to be very careful about that - it may happen to "just work", but there's no guarantee and we have to make sure with exhaustive tests.

[ghost]

Sure. Let me know what other semantic tests you'd like and I'll work on them.

[ekpyron]

Sure. Let me know what other semantic tests you'd like and I'll work on them.

I just tried to :-). But let me try to be clearer :-): The PR so far has no semantic tests whatsoever, but only syntax tests. You can see examples of semantic tests in test/libsolidity/semanticTests/*. They involve calling some functions in a test contract and checking, if the call produces the desired result. Under the hood those tests are actually compiled down to bytecode and executed in a mock EVM.

We will need test cases like that, for any kind of situation, in which a call to a private function could theoretically pick up the wrong function. As one example:

contract A {
  function f() private pure returns (uint) { return 7; }
  function g() internal pure returns (uint) { return f(); }
}
contract B is A {
  function f() private pure returns (uint) { return 42; }
  function test() external pure returns (uint, uint) {
    return (g(), f());
  }
}
// ----
// test() -> 7, 42

That would, in the simplest inheritance situation, verify that the internal call to the base contract in fact calls the correct private function, even though the derived contract defines a function of the same name with different implementation. I'd expect this test case to pass, but there may be other cases to consider. The trickiest part in the underlying issue is to identify all distinct situations in which such tests are warranted.

[ghost]

This PR includes one semantic test. Thanks for that test, I'll include it. I'll try to write some more semantic tests. I'll also join the Matrix chat tomorrow or Thursday, see if we can discuss what other scenarios need to be tested, which is what I initially did a few weeks ago. In fact, most of the tests included here were described by @cameel.

But as you say, it's tricky to identify all the different situations that need to be tested. The commit barely affects a line of code in OverrideChecker.cpp, yet it needs to test a lot of unmodified code.

[ekpyron]

Ah, right, I must have missed that one semantics test, sorry about that.

But yeah, testing is the main issue with this, especially since, as you say, the effects of the change are "non-local".

Actually, one option to alleviate the need for exhaustive testing (at least somewhat) would be to add some safety assertion about this during code generation. In particular in

https://github.com/ethereum/solidity/blob/6b60524cfe4186eb7d22d80ca67b9554902d8fb3/libsolidity/codegen/ir/IRGeneratorForStatements.cpp#L952

we could assert that if the function definition has private visibility, it's annotated contract scope (i.e. functionDef->annotation().contract) matches the current most derived contract (i.e. m_context.mostDerivedContract()). The main issue is that code generation so far is written under the assumption that functions are unique and do not have assertions like this - that's what makes this dangerous. We'd need a similar assertion for legacy code generation around https://github.com/ethereum/solidity/blob/6b60524cfe4186eb7d22d80ca67b9554902d8fb3/libsolidity/codegen/ExpressionCompiler.cpp#L627

[ghost]

Yes, I like that. I'll fix the new tests and look into adding an assertion during code generation too.

[matheusaaguiar]

If anyone is interested in taking over the PR, the changes are available at branch fixInheritedReturnsPrivateFunction