zesterer / ariadne

A fancy diagnostics & error reporting crate
https://crates.io/crates/ariadne
MIT License
1.61k stars 72 forks source link

Public types missing derivable traits #55

Closed recmo closed 5 months ago

recmo commented 1 year ago

Line, Source, FileCache, Label, Report, etc. do not implement any of Clone, Debug, PartialEq, etc.

This makes it hard to use them in types that do need to implement those. Implementing them should be a trivial #[derive(…)] macro.

See also C-COMMON-TRAITS in the Rust API Guidelines.

zesterer commented 1 year ago

I'll try to get to this at some point. If anybody feels like opening a PR, feel free!

loewenheim commented 1 year ago

Having played around with this a bit, it's unfortunately not quite as straightforward as one might hope. The definition of the location field in Report

pub struct Report<'a, S: Span = Range<usize>> {
    kind: ReportKind<'a>,
    code: Option<String>,
    msg: Option<String>,
    note: Option<String>,
    help: Option<String>,
    location: (<S::SourceId as ToOwned>::Owned, usize),
    labels: Vec<Label<S>>,
    config: Config,
}

makes deriving Debug and Clone impossible for the struct, since <S::SourceId as ToOwned>::Owned doesn't implement either. My first idea was to change the definition of Span to

pub trait Span {
    /// The identifier used to uniquely refer to a source. In most cases, this is the fully-qualified path of the file.
    type SourceId: PartialEq + ToOwned<Owned: Debug + Clone> + ?Sized;
    […]
}

but that's not stable. The way it can actually be done is

pub trait Span
where
    <<Self as Span>::SourceId as ToOwned>::Owned: std::fmt::Debug + Clone,
{
    /// The identifier used to uniquely refer to a source. In most cases, this is the fully-qualified path of the file.
    type SourceId: PartialEq + ToOwned + ?Sized;
    […]

but you then have to repeat this bound in every place the Span trait is used (which I honestly don't understand).

To be honest I think it may be simpler to just accept that Report is not going to be Clone and implement Debug by hand, leaving the offending field out. Not having Debug at all is really inconvenient because you can't unwrap.

zesterer commented 1 year ago

Yes, implementing by hand is certainly the way to go for this case.

loewenheim commented 1 year ago

I opened a PR: https://github.com/zesterer/ariadne/pull/60

oppiliappan commented 1 year ago

Are there still any missing traits (personally haven't had the need for any)? I'd be happy to pitch in and implement a few!

zesterer commented 1 year ago

Not that I'm aware of, but do say if you find any!

nouritsu commented 5 months ago

I was looking to contribute to this repo (fishing for good first issues :p), this issue looks like it could be closed.