-
Notifications
You must be signed in to change notification settings - Fork 2
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 reporting, major refactors #69
Conversation
Sweet. I'll try to get to it, as a first impression there are some opportunities for some cleanup. For example extracting |
@oberien Also please push your changes to |
Oops, after brendanzab/codespan#55 was merged, I rebased brendanzab/codespan#56 onto the new master, but forgot to update the revision hash in
Oh absolutely, most likely. I wrote this in 3 days, and there's probably some stuff which I just hacked in without properly integrating it.
I'm not sure if that actually simplifies a lot of stuff, because especially in the frontend / cskvp we need to perform a lot of range calculations. I don't think that a newtype would help here, in fact it might even lower the user experience (with us being the user).
I'm not sure I'm following. I find the diagnostics API pretty nice actually :) (also having rewritten it thrice to better fit the needs)
Again I'm not sure I'm following. Anyways, I'm going to wait on your review, that might clear things up :) |
Forgot the major comment on the PR: I really, really like having diagnostics! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sure the tests work neatly
I think I fixed all your requests, so this PR should be ready for a second round of reviews. Sorry that this became so huge, we should definitely think about #80 in the long run to keep these kinds of PRs smaller. Also I don't really know how I would split up this PR into smaller ones. |
I'm no longer sure if emitting diagnostics immediately into its own defined writer is optimal. If you run the tests from the And sorry that this appears so late in the progress and you literally changed hundreds of lines concerning exactly this :/ |
Alternatively, we could change the owner semantics of the current |
I'll take a look into it after some sleep :) If we can improve it, I think it's worth it to investigate other possibilities. I was wondering as well why it printed out the diagnostics, considering that the rustc test framework captures it. I think that One idea I also had in the beginning was to collect all diagnostics in the Maybe we can just use a |
Something like that could also work, simply provide another constructor for tests. This is also more of a code-cleanup than a blocking concern for the PR as normal program experience is not itself affected for now. Should we ever get a case where we error but later in processing can completely 'unerror' or 'unwarn' then this becomes more important. But for now, just fixing it for tests is likely fine. |
Actually it seems like I have no idea why it shows up in the tests. |
Actually it seems like cargo test only captures stdout and not stderr. So I guess we could just use |
Apparently |
I think we can move this discussion to after this PR has landed. For the time being it's fine and it can always be refactored. I've opened #81 to track the diagnostics issue. |
74b8437
to
e8c784c
Compare
…tend diagnostics, fix frontend bugs
Canonicalization is required to ensure proper access rights and failure to do so (for example from a missing file) should not be treated as an internal implementation failure. Rather, it is most likely the input file's fault from a malformed path. An intermediate error type diagnoses different problems with path normalization when resolving a relative file.
Thanks for the reviews! |
This got larger than expected. To make this easier I have refactored the Generator into an
Iter
module and to enforce theStackElement::Context
(which previously needed to be put on the stack by the caller).Even if we print diagnostics / errors, I try to recover from then by skipping over the events resulting in diagnostics and continuing with the next ones. No matter how many diagnostics got printed, I still try to produce a pdf.
There are three types of errors:
main
and printed there.For a testrun, execute
cargo run -- examples/errors.md
.Fix #21
Example of an ICE output (this particular one should probably be fixed :) ):