ICE when calling an unimplemented virtual function via super

[prestige-club]

Description

Trying to call super in a contract with multiple inheritance throws an InternalCompilerError with no message.
In my case, it appeared when overriding a function of a parent class and implemented interface and then calling super on that same function.

Environment

• Compiler version: 0.8.1 (also appears at least on 0.8.4 and 0.6.1)
• Target EVM version (as per compiler settings): istanbul
• Framework/IDE (e.g. Truffle or Remix): Both Truffle and Remix show this behaviour
• EVM execution environment / backend / blockchain client: n.a.
• Operating system: Win10

Steps to Reproduce

pragma solidity 0.8.1;
// SPDX-License-Identifier: MIT

contract A{
    function x() public virtual {
    }
}

interface I{
    function x() external;
}

contract B is A, I{
    
    function x() override(A, I) public {
        super.x();
    }
    
}

Compiler error occurs when compiling above Source code.
Although using super in this case is definitely not best-practice, I think there should be error message which helps identify the source (searched for 4 hours myself).

[cameel]

Thanks for the report! I can confirm that this still happens on 0.8.4 and affects the new codegen too:

Internal compiler error during compilation:
/solidity/libsolidity/codegen/ir/IRGeneratorForStatements.cpp(1948): Throw in function virtual void solidity::frontend::IRGeneratorForStatements::endVisit(const solidity::frontend::MemberAccess&)
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what:
[solidity::util::tag_comment*] =

The problem appears to be not the multiple inheritance by itself but the fact that I.x() (which has no implementation) comes before A.x() in the C3 linearization order. The code generator tries to replace super.x() with a call to it and fails because of the missing definition. I think it should be skipped and A.x() should be called in its place.

By the way, if you need a workaround for this bug, it's enough to change the order in which B inherits from other contracts. If you change it to B is I, A, the code will compile.

[cameel]

I have a fix that modifies resolveVirtual() to skip unimplemented functions but after some experimenting with this I think that this case should produce an error after all. super.x() is just a kind of magic so as a user maybe I'd expect it to skip the unimplemented method but this would be forbidden I just called I.x() directly:

contract A {
    function x() public virtual {}
}

interface I {
    function x() external;
}

contract B is A, I {
    function x() override(A, I) public {
        I.x();
    }
}

Error: Cannot call unimplemented base function.
  --> test.sol:11:9:
   |
11 |         I.x();
   |

[cameel]

Also, I'm not sure if it even makes sense to allow this kind of inheritance at all. It would not be possible if we "straightened" the hierarchy:

contract A {
    function x() public virtual {}
}

abstract contract I is A {
    function x() public virtual override;
}

contract B is I {
    function x() override public {
        super.x();
    }
}

Error: Overriding an implemented function with an unimplemented function is not allowed.
 --> test.sol:6:5:
  |
6 |     function x() public virtual override;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: Overridden function is here:
 --> test.sol:2:5:
  |
2 |     function x() public virtual {}
  | 

Maybe it would be better to require anything that explicitly provides an unimplemented virtual function to be listed before anything that overrides it?

This would be a breaking change though.

[cameel]

Discussed with @chriseth on gitter:

of course the super in the end can resolve to a different function that overrides the unimplemented function
but the condition should be that it can always resolve to something
i.e. there must be at least one implemented function above the current one
so in short: I think it should just skip unimplemented functions (unless they are themselves overridden by another function)

In that case I will submit the fix for resolveVirtual() that I already have.