[POC] Using native solc for parsing #1720
Conversation
|
Hey @maxsam4 thanks for getting this started! I'll try to get eyes on this early this upcoming week. |
There was a problem hiding this comment.
So upon first review, this seems to make sense. I'd love to learn more about what you tried / what led you to this solution.
I'm also wondering if you have any ideas for anything we can do to clean up that whole import error key mechanism, since it was a hack when you started :)
Thanks for this!
| @@ -5,7 +5,7 @@ const VersionRange = require("./VersionRange"); | |||
| class Native extends LoadingStrategy { | |||
| load() { | |||
| const versionString = this.validateAndGetSolcVersion(); | |||
| const command = "solc --standard-json"; | |||
| const command = "solc --standard-json --allow-paths '.'"; | |||
There was a problem hiding this comment.
Curious why this was necessary. I think it's fine but I'm a bit concerned about this causing problems if it's too restrictive. My guess is that Truffle currently does not support paths outside the project root, and I don't believe it's normal use case, so it's probably fine, but, yeah, just wondering what led you to include this.
There was a problem hiding this comment.
"just wondering what led you to include this."
Failing tests.
Without this, solc was not allowing the 'incorrect paths' to be accessed in the test solidity code which included incorrect paths by choice. Before merging, we can fix the incorrect paths in the test to be incorrect allowed paths and remove this change.
FWIW, this change is not adding any restriction, it is only removing some restrictions. by default, solc native does not allow any paths outside the contracts. This change allows the base directory of the project to be included.
| @@ -18,10 +18,9 @@ module.exports = { | |||
| // the imports speedily without doing extra work. | |||
|
|
|||
| // If we're using docker/native, we'll still want to use solcjs to do this part. | |||
| if (solc.importsParser) solc = solc.importsParser; | |||
|
|
|||
| // if (solc.importsParser) solc = solc.importsParser; | |||
There was a problem hiding this comment.
(We'll have to remember to delete this commented out line of code)
There was a problem hiding this comment.
Also, need to update comments before merging. Right now, it's just a POC to make sure I am heading in the right direction.
| // Helper to detect import errors with an easy regex. | ||
| var importErrorKey = "TRUFFLE_IMPORT"; | ||
| var importErrorKey = "not found: File"; |
There was a problem hiding this comment.
Gosh. This is what we have to do now? (I mean, if it's the only option, it's the only option)
| // Without this, we'll get an error message we *can* detect, but the key will make it easier. | ||
| // Note: This is not a normal callback. See docs here: https://github.com/ethereum/solc-js#from-version-021 | ||
| return { error: importErrorKey }; | ||
| }); |
There was a problem hiding this comment.
Oh I see. So if I get this right, the crux of the problem seems to be that our profiling relied on this weird error object, which was only available via solc-js?
There was a problem hiding this comment.
To be honest, I don't totally understand how this was even working. First of all, the object is named "errors" and not "error". Secondly, an almost same object is being returned by native solc as well but it did not work for the native solc object (the changes seemed to be in values and not keys so they should not matter).
|
Hey @gnidan , thanks for the review! Currently, the profiling took the errors array generated by solcjs and added "TRUFFLE_IMPORT" to all the "file not found" errors. Later, the code checks if the error contains "TRUFFLE_IMPORT" to decide if the error is a "file not found" error or not. The errors object passed by native and docker solc is a bit different so the code was failing to add "TRUFFLE_IMPORT" to the file not found errors. This meant that when it was searching for "file not found" errors later by searching for "TRUFFLE_IMPORT" in the string, it was finding none. What I did was stop adding "TRUFFLE_IMPORT" to the errors altogether and use "not found: File" as the substring to search for when checking "file not found" errors. The advantage is that this works with all solc versions I tested. the disadvantage is that if solc changes the error message in the future, this will break. It will be an easy fix but yeah. We can use a less restrictive/different search query as well. Or even make a regex like "Source.*not found". |
|
Thanks for continuing this poc @CruzMolina . |
Currently, truffle requires solcjs for parsing. This is a poc to show that it can do it with native solc as well.
Relevant issue #1567
PS. This is just a POC. I don't really know why it was decided to use solcjs for parsing nor do I know if my changes are in the right place.