Binding rules to support contract inheritance

[ggiraldez]

This PR adds binding rules to resolve contract inheritance and virtual method lookups (both direct and via super). It does that by introducing a generalized strategy to deal with references that resolve to multiple definitions. It ranks the definitions according to selector tags found in the definitions. Currently implements two ranking algorithms:

• A simple one, for aliasing definitions such as import aliases for symbols. In this case, the aliased definition is marked down, prioritizing the actual definitions the aliases point to.
• One based on C3 linearisation, that can resolve calls to virtual methods in a hierarchy of contracts (either directly or via super).

For the second algorithm, a substantial amount of extra infrastructure is added to the stack graph, in the form of extra attributes that the bindings system tracks. The newly introduced graph attributes are:

• tag to mark definitions and references with a tag to change the behaviour of the ranking algorithm.
• parents which is applicable to both definitions and references, contain an ordered list of definitions and references to construct a hierarchy. This is relevant in contract definitions, to enumerate the base contracts through their references in the inheritance list, and in function call expressions, to indicate from which contract they are being done.
• export_node applies to contract definitions and provides a connection point for parent contracts to access virtual members defined in derived contracts.
• imports_node also applies to contract definitions and provide the analogous connection point used for base contracts.

These last two attributes are used in conjunction with a new property of the bindings module context which can be used to indicate the resolver what is the current binding context (ie. what contract is being compiled). With virtual methods, the actual definition of a reference can only be correctly resolved knowing the full inheritance hierarchy of the contract being compiled. When setting the context, the bindings module with patch its internal stack graph and create new links between the context contract and all the parents in its hierarchy. These are "reverse" links, in the sense that allow a reference in a base contract to resolve to a definition in a sub-contract down the hierarchy.

All this machinery (parent links, context and import/export nodes) provide the necessary information for a reference to resolve to all possible definitions in the compilation contract hierarchy. Then, the C3 linearisation ranking algorithm helps decide which one to choose.

In particular, for Solidity:

• Contract definitions are linked to their bases via the parents attribute.
• All contract functions definitions are linked to the contract via the parents attribute, and are also tagged with the value c3.
• Function call references that occur in contract functions are also linked to the enclosing contract via the parents attribute.
• Super call references are tagged with the value super.
• All contracts have export_node set to its @contract.members scope node.
• All contracts have imports_node set to both its @contract.lexical_scope and a super_import (which is similar to @contract.super_scope) scope nodes.
• Import alias definitions are tagged with the value alias.

Issues remaining:

• Generalize the detection of super calls. Currently that is done crudely by inspecting the stack graph path from the reference to the definition and searching for a push node with the symbol super.
• Decide what to return when there are multiple definitions and the ranking algorithm cannot disambiguate them. Currently we panic and crash the program, which should be fine for well-formed Solidity programs. Probably makes sense to return a Result<Definition, ResolutionError> instead of an Option<Definition>.

[changeset-bot]

⚠️ No Changeset found

Latest commit: caf4bc0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[AntonyBlakey]

I think this is totally correct. The issue is, with multiple superclasses with multiple impls of foo(), does it pick the right one according to C3, which is merely the first one found in C3 traversal order.

[ggiraldez]

I think this is totally correct. The issue is, with multiple superclasses with multiple impls of foo(), does it pick the right one according to C3, which is merely the first one found in C3 traversal order.

I don't think it works with C3 linearisation in the general case, because of the way the graph is constructed. It will prioritize right-most declared base and then go depth-first.

[ggiraldez]

I think this is totally correct. The issue is, with multiple superclasses with multiple impls of foo(), does it pick the right one according to C3, which is merely the first one found in C3 traversal order.

I don't think it works with C3 linearisation in the general case, because of the way the graph is constructed. It will prioritize right-most declared base and then go depth-first.

Here's an example that illustrates this problem:

contract A {
    function foo() public virtual {
        //   ^def:1
    }
}

contract B is A {
    function foo() public virtual override {
        //   ^def:2
    }
}

contract C is A {}

contract D is B, C {
    function foo() public virtual override {
        super.foo();
        //    ^ref:2  !!! This FAILS because it incorrectly resolves to ref:1
    }
}

The way the graph is constructed, the super resolution goes through C because it has higher precedence than B, and it keeps going deeper until it reaches A's implementation.

[AntonyBlakey]

LGTM, except for the panic on invalid input. A specific requirement is that we do best-effort binding on input that has errors, which will likely form the majority of the inputs we see in e.g. an LSP that tracks real-time code edits.

[OmarTawfik]

nit suggestion: using resolve_definition() instead of jump_to_definition(). The latter is an IDE feature name, and only one use case of resolving definitions. Outside IDEs, there isn't an active editor/viewing panel to "jump" in.

[ggiraldez]

Makes sense. I'll rename it. Although, shouldn't it be named resolve_reference() (or just resolve() since it's a Reference method)?

[OmarTawfik]

IIUC, the reference is already "resolved". We created/returned a handle to it.
What we are resolving here is the definition it belongs to, which may or may not exist.

[ggiraldez]

Hmm... we may be overloading the word resolve. With resolve I mean finding the Definition (or Definitions in case there is ambiguity) that a Reference should bind to. I think you also refer to resolve as retrieving a Reference/Definition from a Cursor?

[OmarTawfik]

We are talking about Reference::jump_to_definition() here, right?
IIUC, user already has a Reference handle to invoke this method, so IMO it is a bit confusing to resolve_reference(), considering that it is already resolved (from the identifier). The thing that needs resolving (and the user doesn't have yet) is the definition...
Similarly, I think find_definition() would work here as well..

[ggiraldez]

Ok, I like find_definition(). It seems to be the most unambiguous.