Support multiple errors from any stage in driver #586
Conversation
rabuu
changed the title
lang/driver/src/result.rs
Outdated
| pub enum Error { | ||
| #[error("Failed parsing")] | ||
| Parser(#[related] Vec<polarity_lang_parser::ParseError>), | ||
| #[error("One or more errors occured.")] |
There was a problem hiding this comment.
This information message is a bit redundant. Can you try to make this transparent instead?
There was a problem hiding this comment.
Ideally, this should also reduce the diff that affects the *.expected files.
There was a problem hiding this comment.
This information message is a bit redundant. Can you try to make this transparent instead?
This is not so easy, at least using miette. There is no good way of handling multiple errors in the library, the #[related] is more of a hack but works fine.
If we want to implement Diagnostic/Error for the AppErrors type, we must provide such an error message, it can't be "transparent".
We could also not implement Diagnostic and Error but then we can't transform AppErrors into a single miette::Report anymore and would have to handle Vec<Report>s instead of single Reports in the result of the app/src/cli/... commands. This is not as elegant -- for example, we'd have to manually emit all errors in main and can't just return it as miette::Result -- but probably the "intended" way for multiple errors in miette.
Let me know what you think is better.
There was a problem hiding this comment.
I see, thanks for the detailed explanation! I think in that case it may be better to replace miette::Result and handle printing the list of errors ourselves. This is slightly more code on our side, but should be manageable.
test/test-runner/src/phases.rs
Outdated
|
|
||
| // There was no panic and `run` returned with an error. | ||
| self.report_phases | ||
| .push(PhaseReport { name: phase.name(), output: format!("{reports:?}") }); |
There was a problem hiding this comment.
format!("{reports:?}") produces a debug representation of the reports. I think we might want to use render_reports here, as well?
test/test-runner/src/phases.rs
Outdated
| } | ||
|
|
||
| fn render_report(report: &miette::Report, colorize: bool) -> String { | ||
| fn render_reports(reports: &[miette::Report], colorize: bool) -> String { |
There was a problem hiding this comment.
Does it make sense to move this function to driver and reuse it in app as well?
There was a problem hiding this comment.
Since it is a miette-only thing, wouldn't it make sense to move to miette_util?
There was a problem hiding this comment.
I am a bit unhappy about our current miette_util crate: If we publish the project on crates.io then we also have to publish a polarity-miette-util crate, but it barely contains 100 lines of code, and we might not even need that code in the future if we do a rewrite. I think it might be more futureproof to rename the crate to polarity-util and use it as a more general purpose util library with super generic code that we use all over the codebase. An alternative would be to merge it with the printer crate. Wdyt @timsueberkrueb ?
There was a problem hiding this comment.
I would move render_reports to driver for now, we can deal with miette_util separately.
| use miette::Report; | ||
|
|
||
| /// Terminal width for pretty-printing error messages. | ||
| const TERMINAL_WIDTH: usize = 200; |
There was a problem hiding this comment.
In the future we might want to consider getting the terminal width dynamically.
This introduces the
AppErrorswrapper type that contains a positive number ofAppErrorand uses that as main error and result type in the driver.That makes the error handling for multiple and single errors uniform between the different stages (for now only parsing can return multiple errors).