Error: `Definition of base has to precede definition of derived contract` when specific file in standard-json-input `outputSelection` but works when `outputSelection` file is specific

Description

When compiling with the specific file in the standard-json-input's outputSelection field, the compiler throws the error Definition of base has to precede definition of derived contract but when the files in the outputSelection is given as "*" the compiler compiles successfully.

Noticed this as the Sourcify recompiler targets the specific file and output in the generated json-input and couldn't compile but the user was able to compile and deploy the contract via Hardhat (which has "*" in outputSelection.)

I'd say the error is correct since according to the order of importing the definition of the base contract is below the inheriting contract, as suggested by the error. Nevertheless a change in the outputSelection should not be effecting how the compiler behaves.

this outputSelection throws:

  "settings": {
    "evmVersion": "london",
    "libraries": { "": {} },
    "metadata": { "bytecodeHash": "ipfs" },
    "optimizer": { "enabled": true, "runs": 200 },
    "remappings": [],
    "outputSelection": {
      "contracts/teleporteth/contracts/TeleportTokenFactory.sol": {
        "TeleportTokenFactory": [
          "evm.bytecode.object",
          "evm.deployedBytecode.object",
          "metadata"
        ]
      }
    }
  }

while this compiles:

  "settings": {
    "evmVersion": "london",
    "libraries": { "": {} },
    "metadata": { "bytecodeHash": "ipfs" },
    "optimizer": { "enabled": true, "runs": 200 },
    "remappings": [],
    "outputSelection": {
      "*": { // <-- diff
        "TeleportTokenFactory": [
          "evm.bytecode.object",
          "evm.deployedBytecode.object",
          "metadata"
        ]
      }
    }
  }

Environment

Compiler version: 0.8.7 (originally), also tried 0.8.13
Target EVM version (as per compiler settings): london
Framework/IDE (e.g. Truffle or Remix): solc
EVM execution environment / backend / blockchain client:
Operating system: Ubuntu 20.04

Steps to Reproduce

I am adding the full standard-json-input files with just the diff in the outputSelection. Also adding the .sol files for reference. compiles-with-asterisk.json.txt error-specific-file.json.txt TeleportToken.sol.txt TeleportTokenFactory.sol.txt

I have extracted minimal code that lets to reproduce the issue:

{
    "language": "Solidity",
    "settings": {
        "outputSelection": {
            "fileB.sol": {
                "B": [
                    "abi"
                ]
            }
        }
    },
    "sources": {
        "fileA.sol": {
            "content": "
                pragma solidity ^0.8.6;
                // SPDX-License-Identifier: MIT
                import \"./fileB.sol\";
                contract A is BaseA {}
            "
        },
        "fileB.sol": {
            "content": "
                pragma solidity ^0.8.6;
                // SPDX-License-Identifier: MIT
                import \"./fileA.sol\";
                contract BaseA {}
                contract B {}
            "
        }
    }
}

The interesting thing about the above code is it can be compiled seamlessly after renaming source file names (fileA.sol->fileB.sol and fileB.sol->fileA.sol). The thing is, an order of type and name resolving (NameAndTypeResolver::resolveNamesAndTypesInternal) depends on the source file analysis order (CompilerStack::analyze), which in turn depends on CompilerStack::resolveImports. The latter takes std::map container with source files and sort them based on file relations and outputSelection. In case of circular imports, the determining factors are file names, std::map container implementation and outputSelection. Whenever fileA.sol from above example is ordered before fileB.sol compilation ends with errors. This is why file renaming and outputSelection both have an impact on compilation result.

The proper way to deal with this issue would be to make NameAndTypeResolver robust against bases being defined only after inheriting contracts and then only later validate that all inheritance is sane. Presently, we work on a similar mechanism for the sake of compile-time-constant expression evaluation, which will need the TypeChecker to be robust against out-of-order execution. Issues: #13365, #13318. We decided to postpone this ticket until TypeChecker changes are delivered so we could try to use the same mechanism here.

Closing the issue, since it'll be fixed as part of https://github.com/ethereum/solidity/issues/13365

I'm going to reopen this because it does not look like we'll have the full solution promised in #13365 before experimental type system is released (the issue is in fact closed) and such cycles are probably common, which is a significant obstacle for parallelizing compilation.

Even if a proper solution requires a rewrite of the whole type checker, I think that there is still a viable workaround that would be good enough. Since full compilation, with all outputs selected, does not seem to have this problem, we could simply run analysis on more files than were selected for output. This would be unnecessary work, but it's just analysis, which is very fast compared to full compilation and optimization, and it's better than not being able to compile at all.

I think this must be pretty common, because I stumbled into this problem already in the first project I tried to parallelize, which happened to be Uniswap v4. When comparing the output after compiling each contract independently, I noticed a small discrepancy: artifacts for IProtocolFees.sol were missing. Turns out it was failing with this error when compiled on its own.

It could also be worked around at the tooling level - by detecting the circular dependency and requesting more outputs, but this is another wart that will make parallelization more complex for tools. We should be doing it in the compiler instead.

I'm going to try to fix it, because the alternative is having to do this in the parallelization PoC instead.

Repro

Distlled example

Just for reference, here's the repro that reflects the structure I found in Uniswap:

{
    "language": "Solidity",
    "sources": {
        "IX.sol": {"content": "import {I4} from \"I4.sol\"; interface IX {}"},
        "I2.sol": {"content": "import {I1} from \"IX.sol\"; interface I2 is I1 {}"},
        "I3.sol": {"content": "import {I2} from \"I2.sol\"; interface I3 {}"},
        "I4.sol": {"content": "import {S}  from \"S.sol\";  interface I4 {}"},
        "S.sol":  {"content": "import {I3} from \"I3.sol\"; struct    S  { uint x; }"}
    },
    "settings": {
        "outputSelection": {
            "IX.sol": {"*": []}
        }
    }
}

EDIT: Updated the example to rename I1 to IX. The original one was producing the error not only with I1 selected but also with all sources selected. This one properly reproduces the discrepancy.

Full Uniswap repro

git clone https://github.com/Uniswap/v4-core
cd v4-core/
git checkout d0700ceb251afa48df8cc26d593fe04ee5e6b775 # branch main as of 2024-05-10

cat <<-EOF > input.json
{
    "language": "Solidity",
    "sources": {
        "src/interfaces/IProtocolFees.sol": {"urls": ["src/interfaces/IProtocolFees.sol"]}
    },
    "settings": {
        "outputSelection": {
            "src/interfaces/IProtocolFees.sol": {"*": ["*"]}
        }
    }
}
EOF

solc --standard-json input.json --pretty-json --json-indent 4

Output:

{
    "errors":
    [
        {
            "component": "general",
            "errorCode": "2449",
            "formattedMessage": "TypeError: Definition of base has to precede definition of derived contract\n  --> src/interfaces/IPoolManager.sol:15:27:\n   |\n15 | interface IPoolManager is IProtocolFees, IERC6909Claims, IExtsload {\n   |                           ^^^^^^^^^^^^^\n\n",
            "message": "Definition of base has to precede definition of derived contract",
            "severity": "error",
            "sourceLocation":
            {
                "end": 581,
                "file": "src/interfaces/IPoolManager.sol",
                "start": 568
            },
            "type": "TypeError"
        }
    ],
    "sources": {}
}

After spending more time on this, I don't fully agree with analysis in https://github.com/ethereum/solidity/issues/12932#issuecomment-1248032377. I mean, making it possible to avoid this error is indeed a bigger problem and better delegated to #13365 (or actually #14284 which replaced it). The thing is, this issue is not really about that error itself but about the discrepancy that appears when using outputSelection. If the error does not appear with all sources selected, it should not appear when you select one of them, because we still analyze all the sources.

I investigated the cause and it looks like a bug in the mechanism we have for excluding non-requested contracts (#7018) from compilation: https://github.com/ethereum/solidity/blob/cb1d21a9de839ce7c7a156f5364ac8877d7be41a/libsolidity/interface/CompilerStack.cpp#L1303-L1304

When a contract is present in the original input but not marked as requested, it is skipped. The problem is that later another contract may pull it in via an import, which means it will still end up in m_sourceOrder, just at a later position. And the order in which we analyze sources determines whether we'll run into Definition of base has to precede definition of derived contract in presence of circular imports.

This is entirely fixable. There's just a question if this is not breaking though. By changing the other we'll make the repro compile but I suspect that there will be some cases that compiled and now won't. Still, with this discrepancy affecting parallelization, I think we're much better off treating it as a bug and fixing regardless.

ethereum / solidity