Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 #12932

Open
kuzdogan opened this issue Apr 13, 2022 · 5 comments · May be fixed by #15173
Assignees
Labels
bug 🐛 high impact Changes are very prominent and affect users or the project in a major way. low effort There is not much implementation work to be done. The task is very easy or tiny. must have Something we consider an essential part of Solidity 1.0. should compile without error Error is reported even though it shouldn't. Source is fine.

Comments

@kuzdogan
Copy link
Member

kuzdogan commented Apr 13, 2022

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

@kuzdogan kuzdogan changed the title Error: Definition of base has to precede definition of derived contract when specific file in outputSelection but works when outputSelection file is specific Error: Definition of base has to precede definition of derived contract when specific file in standard-json outputSelection but works when outputSelection file is specific Apr 13, 2022
@kuzdogan kuzdogan changed the title Error: Definition of base has to precede definition of derived contract when specific file in standard-json outputSelection but works when outputSelection file is specific 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 Apr 13, 2022
@cameel cameel added bug 🐛 should compile without error Error is reported even though it shouldn't. Source is fine. labels Apr 27, 2022
@cameel cameel added this to New issues in Solidity via automation Apr 27, 2022
@wechman wechman self-assigned this Sep 12, 2022
@wechman
Copy link
Collaborator

wechman commented Sep 14, 2022

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.

@wechman
Copy link
Collaborator

wechman commented Sep 15, 2022

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.

@ekpyron
Copy link
Member

ekpyron commented Nov 17, 2022

Closing the issue, since it'll be fixed as part of #13365

@cameel
Copy link
Member

cameel commented May 15, 2024

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": {}
}

@cameel cameel reopened this May 15, 2024
@cameel cameel added high impact Changes are very prominent and affect users or the project in a major way. must have Something we consider an essential part of Solidity 1.0. medium effort Default level of effort low effort There is not much implementation work to be done. The task is very easy or tiny. and removed medium effort Default level of effort labels May 15, 2024
@cameel
Copy link
Member

cameel commented Jun 3, 2024

After spending more time on this, I don't fully agree with analysis in #12932 (comment). 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:

if (isRequestedSource(sourcePair.first))
toposort(&sourcePair.second);

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.

@cameel cameel linked a pull request Jun 3, 2024 that will close this issue
@cameel cameel assigned cameel and unassigned wechman Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 high impact Changes are very prominent and affect users or the project in a major way. low effort There is not much implementation work to be done. The task is very easy or tiny. must have Something we consider an essential part of Solidity 1.0. should compile without error Error is reported even though it shouldn't. Source is fine.
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants