Improve compilation job error messages #989
Conversation
| @@ -981,13 +972,11 @@ subtask(TASK_COMPILE_SOLIDITY_HANDLE_COMPILATION_JOBS_FAILURES) | |||
| { | |||
| compilationJobsCreationErrors, | |||
| }: { | |||
| compilationJobsCreationErrors: CompilationJobsCreationErrors; | |||
| compilationJobsCreationErrors: CompilationJobCreationError[]; | |||
| }, | |||
| { run } | |||
| ) => { | |||
There was a problem hiding this comment.
This function is huge. I think we should move it to another module, but I didn't do it yet so that the PR it's easier to review.
Also, there's a lot of duplication here, especially between the "invalid imports" and the "invalid indirect imports" parts. Maybe we can abstract that, but I'm inclined to leave this as straightforward as possible.
| reason: CompilationJobCreationErrorReason; | ||
| file: ResolvedFile; | ||
| extra?: any; | ||
| } |
There was a problem hiding this comment.
This is the main type change. The CompilationJobsCreationError type, that was a map from an error tipe to a list of source names with that error, is gone. Instead I used a list of errors. Each error has a reason (this is the extendable enum previously called CompilationJobCreationError), a file, and an optional "extra" type, that can have more information about the error.
I don't love having an any for the extra field, but I'm not sure it's worth it to do something trickier here.
| import { | ||
| CompilationJobCreationError, | ||
| CompilationJobCreationErrorReason, | ||
| } from "../../../src/types/builtin-tasks"; |
There was a problem hiding this comment.
This file needs more tests, but I will add them after the new types are definitive.
| if (incompatibleImports.length === 1) { | ||
| importsText = ` imports ${incompatibleImports[0]}`; | ||
| } else if (incompatibleImports.length === 2) { | ||
| importsText = ` imports ${incompatibleImports[0]} and ${incompatibleImports[1]}`; |
There was a problem hiding this comment.
ah, yes. They are. Maybe add a "direct" to the var names?
| ); | ||
|
|
||
| log( | ||
| `File ${sourceName} depends on files ${incompatibleImportsFiles |
There was a problem hiding this comment.
We could add the entire import path here, can't we?
Something like a second line with "Import path: A.sol > B.sol > Incompatible.sol"
|
|
||
| `; | ||
| } | ||
|
|
||
| reasons += `Learn more about compiler configuration at https://hardhat.org/config | ||
| errorMessage += `Learn more about compiler configuration at https://hardhat.org/config |
There was a problem hiding this comment.
| errorMessage += `Learn more about compiler configuration at https://hardhat.org/config | |
| errorMessage += `This can happen because they have incompatible Solidity pragmas, or don't match any of your configured Solidity compilers. Learn more about compiler configuration at https://hardhat.org/config |
There was a problem hiding this comment.
Couldn't that message be misleading? If there are no matching compilers or if there are incompatible imports, we should have detected it and reported it as a different error reason.
There was a problem hiding this comment.
I believe this error has two reasons:
- The range described by the concatenation of all the pragmas is empty.
- The range described by the concatenation of all the pragmas doesn't include any of the configured compilers, even though each individual pragma does match at least one of the compilers.
Is that right?
If so, we should aim to explain that without too many details.
fvictorio
force-pushed
the
better-compilation-errors
branch
from
3fcb3e8
to
a6d4c9b
Compare
This PR improves the errors shown when the files can't be compiled.