Report implement Error, and a few more suggestions

[OriDevTeam]

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]

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]

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]

@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]

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]

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.