Solidity: Allow cyclic imports.

[MicahZoltu]

Parent.sol

import 'ROOT/reporting/Child.sol'; // Identifier "Parent" already declared.
contract Parent {
    Child public child;
}

Child.sol

import 'ROOT/reporting/Parent.sol'; // Identifier "Child" already declared.
contract Child {
    Parent public parent;
}

In this scenario, the circular dependency is safe because neither is creating/deriving from the other. However, the imports quickly get confused trying to import each other. It would be nice if it supported graphs like this as it is a fairly common pattern.

It should be noted that this problem does _not_ occur if both contracts are placed in the same file. This suggests to me that the compiler can handle this scenario, it just gets confused by the cyclic imports.

In the meantime, I'm open to ideas for workarounds. I considered extracting out interfaces for each but in that case the interfaces would have the same problem because fundamentally I am using this cycle to deal with cross-contract access control like require(_providedChild == parent.child()).

[chriseth]

I was not able to reproduce your problem. Cyclic imports should be supported by the compiler. Could you tell me more about how you invoke it?

[MicahZoltu]

Above two files at /app/src/reporting/Parent.sol and /app/src/reporting/Child.sol (note: I just edited the paths of the two imports in the original issue to match the config below. I had simplified them originally, not realizing the issue was specific to my setup and may be impacted by the exact path).

solc --allow-paths /app/src --standard-json
{
    "language": "Solidity",
    "sources": {
        "filename": {
            "urls": [ "/app/src/reporting/Parent.sol" ]
        }
    },
    "settings": {
        "remappings": [ "ROOT=/app/src" ]
    },
    "outputSelection": {
        "*": [ "metadata", "evm.bytecode", "evm.sourceMap" ]
    }
}

Result:

{"contracts":{},"errors":[{"component":"general","formattedMessage":"filename:3:1: DeclarationError: Identifier \"Parent\" already declared.\nimport 'ROOT/reporting/Child.sol';\n^--------------------------------^
\n","message":"Identifier \"Parent\" already declared.","severity":"error","type":"DeclarationError"}],"sources":{}}

Compiler version:

root@ea55a27b47e0:/app# solc --version
solc, the solidity compiler commandline interface
Version: 0.4.13+commit.0fb4cb1a.Linux.g++

[chriseth]

Ok, I was able to reproduce it, but only with standard-json. I suspect this has something to do with trailing slashes and that the compiler does not recognize that two files are indeed identical.

@axic could you please take a look?

[MicahZoltu]

This is turning out to be a pretty significant problem for Augur's migration to Solidity. Is there anything we can do to facilitate getting this fixed sooner rather than later? We could attempt to create a PR if someone can point us in the right direction both for how to author a failing test case (e.g., exmaple of how to author such a test) and where the likely source of the problem is.

Of course, as always with open source projects the time to ramp up a new person enough to fix even a minor bug is sometimes higher than just fixing it yourself, so I'll leave it to you to decide if this is simple enough for someone who has no idea how any of the Solidity code base works to fix.

[chriseth]

Ok, this does not fix it yet, but I think you should change "filename" in your input json to "/app/src/reporting/Parent.sol".

[chriseth]

Ah no, it does fix it! So I think this is not a bug after all. Files are not identified by the source they are loaded from but by the name they are given in the input json. If you import ROOT/Parent.sol again, this is a different file because it has a different identifier.

[MicahZoltu]

If I understand you correctly @chriseth, you are saying that remapping is done _after_ checking to see if the file is already imported? So in this case, I'm bringing in /app/src/reporting/Parent.sol which is saved as "already loaded", and then that brings in ROOT/reporting/Child.sol (marking it as loaded) which then brings in ROOT/reporting/Parent.sol which is seen as a new file because so far the compiler has only seen ROOT/reporting/Child.sol and /app/src/reporting/Parent.sol.

Assuming that the sources block of the standard JSON supports remapping, I'll try replacing /app/src/ with ROOT/ to see if that resolves the issue.

Assuming that works, should I open a separate issue to apply remappings and absolute file pathing _before_ noting the file as loaded, so all loaded files use the absolute file path as the lookup key, rather than mixing relative, absolute, and remapped files?

[MicahZoltu]

@chriseth I just tried changing my JSON to:

"sources": {
        "filename": {
            "urls": [ "ROOT/reporting/Parent.sol" ]
        }
    },

Unfortunately, the compiler errors because it doesn't see that as a valid path. This means that the proposed workaround doesn't work. :/

[axic]

It is the "filename" part (the map key) which should be changed as opposed to the URL.

[MicahZoltu]

@axic Can you give an example of what you mean? When I was trying to figure out how to get the standard-json working, the only way I could get it to work with local files was in the above way. Are you suggesting that I shouldn't be including the urls element and should be doing something else? Or perhaps you are suggesting something like this:

"sources": {
    "ROOT/reporting/Parent.sol": {
        "urls": [ "ROOT/reporting/Parent.sol" ]
    }
}

[axic]

{
    "language": "Solidity",
    "sources": {
        "ROOT/reporting/Parent.sol": {
            "urls": [ "/app/src/reporting/Parent.sol" ]
        }
    },
    "settings": {
        "remappings": [ "ROOT=/app/src" ]
    },
    "outputSelection": {
        "*": [ "metadata", "evm.bytecode", "evm.sourceMap" ]
    }
}

[chriseth]

I think you have to use /app/src in the key for sources.

[MicahZoltu]

I changed filename to absoluteFilePath (in this case it is /app/src/reporting/Parent.sol) but I still get the same error.

[MicahZoltu]

Actually, let me dig a tad more before I assert that it didn't work.

[MicahZoltu]

I dug in a bit more and the proposed change of using the absolute file path for the sources key worked for my test environment. However, I am also using a Solidity extension in VS code and the same fix didn't resolve the issue. The setup of the plugin is a bit different as it uses solcjs and calls solc.compileStandardWrapper with two arguments, the first being the standard JSON and the second being a function to process import statements. In this case, the plugin is doing fs.readFileSync(importPath, 'utf-8'). I'm not terribly clear how solc-js works so this may be a problem for them instead, but the error I get back is similar to this one which suggests to me that the root cause may be the same.

What is of note is that I only have problems when viewing (compiling) files that include themselves transitively. If I open (compile) a file that transitively includes a file that then transitively includes itself there is no problem. This aligns with what you two were saying and suggests that the file being passed to the compiler for compilation isn't being keyed the same as a file that is later imported.

[MicahZoltu]

OK, I have resolved my issue, but I believe there is still a bug in solc. In the end, my issue was a path separator inconsistency on Windows, where some places were using / and other places were using \. I think solc should normalize the path to an absolute path (not relative) _and_ normalize the path separator before using the filename as a lookup key to determine whether or not a match is found.

Sorry about the thrash, let me know if you would like me to open a new issue (feature request?) for the above.

[axic]

Yes I think it would be clearer to have its own dedicated issue. If you create that can we also consider this one closed?

[axic]

Also there's this other issue you may encounter: #2147.