Report implement Error, and a few more suggestions #87

OriDevTeam commented 2023年08月31日 09:04:14 +02:00 (Migrated from github.com)

Hello hope y'all doing well 👋

I have seen that ariadne::Report did not implement Error and Display and i thought it was a problem for some reasons:

• It doesn't allow it to act more as a Result, harder to maneuver
• It makes situations like error chaining harder due having to manually put into a string first then wrap it somehow
• Sometimes using Display and Debug on it can be useful

Also a detail about ariadne::Cache, i tried to make it Clone but i failed to address all the needs there with the dupe function, and maybe it could implement Clone directly instead?

Another thing is that for Display to work well it needs pre-acess to C: Cache, so i took the liberty to move it into ReportBuilder directly, which sequentially then goes to Report (I also included a assert to be sure it tells the developer that it is needed), but i guess y'all might want this different, just an idea

I have tried to make some changes to address this, take this PR as more of a request since i'm unsure if i did it too well, but i think there is very few things to address to make this PR ok

I also took the liberty to move write_to_string impl from the tests into Report directly, as it its useful there

Thank you for your time 🎈 🎈

Hello hope y'all doing well :wave: I have seen that `ariadne::Report` did not implement `Error` and `Display` and i thought it was a problem for some reasons: - It doesn't allow it to act more as a Result, harder to maneuver - It makes situations like error chaining harder due having to manually put into a string first then wrap it somehow - Sometimes using Display and Debug on it can be useful Also a detail about `ariadne::Cache`, i tried to make it `Clone` but i failed to address all the needs there with the `dupe` function, and maybe it could implement `Clone` directly instead? Another thing is that for `Display` to work well it needs pre-acess to `C: Cache`, so i took the liberty to move it into `ReportBuilder` directly, which sequentially then goes to `Report` (I also included a assert to be sure it tells the developer that it is needed), but i guess y'all might want this different, just an idea I have tried to make some changes to address this, take this PR as more of a request since i'm unsure if i did it too well, but i think there is very few things to address to make this PR ok I also took the liberty to move `write_to_string` impl from the tests into `Report` directly, as it its useful there Thank you for your time :balloon: :balloon:

zesterer commented 2023年09月01日 09:25:16 +02:00

Thanks for the PR!

I think most of these changes are really beneficial, but I'm a bit concerned about moving the cache into the report builder (and hence adding the C: Cache parameter to it).

Reports are supposed to be abstract representations of an error and aren't supposed to carry file caches around with them (instead, it's up to the consumer of the error to provide the necessary source files to render them). I think I'd prefer it if the Display impl didn't go through the usual report rendering path and instead just produced some simple textual representation of the error without needing to look up spans in caches and the like.

What do you think?

Thanks for the PR! I think most of these changes are really beneficial, but I'm a bit concerned about moving the cache into the report builder (and hence adding the `C: Cache` parameter to it). Reports are supposed to be abstract representations of an error and aren't supposed to carry file caches around with them (instead, it's up to the *consumer* of the error to provide the necessary source files to render them). I think I'd prefer it if the `Display` impl didn't go through the usual report rendering path and instead just produced some simple textual representation of the error without needing to look up spans in caches and the like. What do you think?

OriDevTeam commented 2023年09月01日 23:18:06 +02:00 (Migrated from github.com)

Now that you mention about carrying caches i see that yeah that would easily build up memory & cloning once one starts stacking errors, so building the textual representation by just giving a reference to a cache would likely be the way to go 👍

I'm thinking maybe all its needed is to go to the report rendering path at ReportBuilder by passing a reference to a cache(but not holding it) to finish and store it in Report, which then its Display would show it with no need to hold C: Cache at all or even know about it

I'm going to give it a shot later and see what i can do, thanks 🦖

Now that you mention about carrying caches i see that yeah that would easily build up memory & cloning once one starts stacking errors, so building the textual representation by just giving a reference to a cache would likely be the way to go :+1: I'm thinking maybe all its needed is to go to the report rendering path at `ReportBuilder` by passing a reference to a cache(but not holding it) to `finish` and store it in `Report`, which then its `Display` would show it with no need to hold `C: Cache` at all or even know about it I'm going to give it a shot later and see what i can do, thanks :t-rex:

OriDevTeam commented 2023年09月02日 01:09:55 +02:00 (Migrated from github.com)

@zesterer Okay so i made roughly what i described above and would like for a idea review on it, i think its roughly on that direction.

Mind that i didn't check somethings like the sanity of doing report.to_string() since it might clone (not sure, but should since it uses Display i also think), if it does i rather just manually implement it to return self.rendered instead.

Also that error test can be a bit improved (and prints removed), but overall i consider these details for now.

Let me know thoughts on the change

@zesterer Okay so i made roughly what i described above and would like for a idea review on it, i think its roughly on that direction. Mind that i didn't check somethings like the sanity of doing `report.to_string()` since it might clone (not sure, but should since it uses Display i also think), if it does i rather just manually implement it to return `self.rendered` instead. Also that error test can be a bit improved (and prints removed), but overall i consider these details for now. Let me know thoughts on the change

zesterer commented 2023年09月13日 16:35:36 +02:00

I'm sorry, but I'm not sure I can accept this PR. The API changes still require the cache to be present in the call to finish, and the formatted string held locally runs counter to the design of Rust's formatting system in general. There is not simply one way to Debug/Display data: Rust's Formatter type includes an array of configuration options that are only available at invocation time, and the approach in this PR completely ignores them.

Further, Report is not supposed to be a Display-able type: it is a data structure that represents a report in the abstract, not the output of a report. I just don't agree that it should be capable of fully being displayed. A Debug implementation is reasonable, I think, but there's no reason for it to do much more than that which an automatically derived implementation would do.

I am fine, in general, with Report implementing std::error::Error (with some stubby implementation of Display, perhaps even as simple as displaying the string "<error report>"). However, it is not intended to behave like a regular error type, because it is not a regular error type. In fact, from the perspective of a program in which ariadne would be typically used such as a compiler, compilation errors are not errors: they are the happy path of a compiler and a good compiler will not implement the generation of compiler errors using the 'usual' error machinery like Result, panic!, std::error::Error, etc. because these approaches are heavily geared toward early aborting, which contradicts the priorities of a fault-tolerant compiler.

I appreciate the time that's been put into this PR, but the changes as they are right now are fundamentally incompatible with the intended use of the crate's API features.

I'm sorry, but I'm not sure I can accept this PR. The API changes still require the cache to be present in the call to `finish`, and the formatted string held locally runs counter to the design of Rust's formatting system in general. There is not simply one way to `Debug`/`Display` data: Rust's `Formatter` type includes an array of configuration options that are only available at invocation time, and the approach in this PR completely ignores them. Further, `Report` is not *supposed* to be a `Display`-able type: it is a data structure that represents a report in the abstract, not the output of a report. I just don't agree that it should be capable of fully being displayed. A `Debug` implementation is reasonable, I think, but there's no reason for it to do much more than that which an automatically derived implementation would do. I am fine, in general, with `Report` implementing `std::error::Error` (with some stubby implementation of `Display`, perhaps even as simple as displaying the string `"<error report>"`). However, it is not intended to behave like a regular error type, because it is not a regular error type. In fact, from the perspective of a program in which `ariadne` would be typically used such as a compiler, compilation errors are not errors: they are the happy path of a compiler and a good compiler will not implement the generation of compiler errors using the 'usual' error machinery like `Result`, `panic!`, `std::error::Error`, etc. because these approaches are heavily geared toward early aborting, which contradicts the priorities of a fault-tolerant compiler. I appreciate the time that's been put into this PR, but the changes as they are right now are fundamentally incompatible with the intended use of the crate's API features.

ratmice commented 2023年12月11日 16:10:35 +01:00 (Migrated from github.com)

In light of the above rejection, It seems worth mentioning what people who want this should do instead in the current API. I believe that you can just call Report::write their report to something that implements Display and Error, and that already accepts a Cache.

In light of the above rejection, It seems worth mentioning what people who want this should do instead in the current API. I believe that you can just call `Report::write` their report to something that implements `Display` and `Error`, and that already accepts a `Cache`.

This pull request has changes conflicting with the target branch.