Refactor error handling #294
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The LLVM library we use does not (yet) provide a llvm.Zero (like it provides a llvm.Undef) so we have implemented our own. However, in theory it might return an error in some cases. No real-world errors have been seen in a while and errors would likely indicate a serious compiler bug anyway (not an external error), so make it panic instead of returning an error.
This commit replaces "unknown type" errors in getLLVMType with panics. The main reason this is done is that it simplifies the code *a lot*. Many `if err != nil` lines were there just because of type information. Additionally, simply panicking is probably a better approach as the only way this error can be produced is either with big new language features or a serious compiler bug. Panicking is probably a better way to handle this error anyway.
This commit adds getValue which gets a const, global, or result of a local SSA expression and replaces (almost) all uses of parseExpr with getValue. The only remaining use is in parseInstr, which makes sure an instruction is only evaluated once.
Most of these errors are actually "todo" or "unimplemented" errors, so the return type is known. This means that compilation can proceed (with errors) even though the output will be incorrect. This is useful because this way, all errors in a compilation unit can be shown together to the user.
aykevl
force-pushed
the
refactor-error-handling
branch
from
4c1ce3c to
ca1dd49
Compare
|
I intend to refactor diagnostics even further in the future as part of #285, but this is a good starting point. |
|
Took a little while to look over all this code. I agree with the philosophy and also agree that this is a really useful change. There is no way for "unknown types to used in the code" in Go, so that is indeed a moot point when it comes to TinyGo. Now merging. |
aykevl
added a commit
that referenced
this pull request
May 13, 2019
No error is produced, so no error needs to be returned. It was missed in #294.
Merged
aykevl
added a commit
that referenced
this pull request
May 13, 2019
No error is produced, so no error needs to be returned. It was missed in #294. Also, it fixes this smelly code: if err != nil { return <something>, nil } There could never be an error, so the code was already dead.
aykevl
added a commit
that referenced
this pull request
May 13, 2019
No error is produced, so no error needs to be returned. It was missed in #294. Also, it fixes this smelly code: if err != nil { return <something>, nil } There could never be an error, so the code was already dead.
deadprogram
pushed a commit
that referenced
this pull request
May 14, 2019
No error is produced, so no error needs to be returned. It was missed in #294. Also, it fixes this smelly code: if err != nil { return <something>, nil } There could never be an error, so the code was already dead.
torntrousers
pushed a commit
to HarringayMakerSpace/tinygo
that referenced
this pull request
May 18, 2019
No error is produced, so no error needs to be returned. It was missed in tinygo-org#294. Also, it fixes this smelly code: if err != nil { return <something>, nil } There could never be an error, so the code was already dead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors error handling. The primary motivation is to be able to show multiple compiler errors to the user at the same time. For example, if you have this code:
Before this PR, it would result in the following error because these operations have not yet been implemented on complex numbers:
After this change, it will show all the errors:
But the main part of this PR (the first 3 commits) is a huge refactor of large parts of the compiler to make error handling much easier. These changes may be controversial: they convert a few errors into panics. However, I think the simplicity in other parts of the compiler is worth it and perhaps a panic would be the best way to deal with it anyway: they happen when unknown types are used in the code and as far as I'm aware all Go types have at least a partial/stub implementation and new types get added very infrequently to Go (and would only need to be implemented by TinyGo after updating the x/tools/go/ssa dependency).