Allow inner source type of a NamedSource to be borrowed directly

[erratic-pattern]

Fixes: #236

This is a simple direct implementation that just makes NamedSource generic over a SourceCode type parameter. It would be nice if we could give the type parameter a sensible default so that existing diagnostics don't break. I'll test various approaches to make this more backwards compatible over the next few days.

Maybe keeping the source in a Box, allowing S to be ?Sized, and then defaulting S to dyn SourceCode + 'static but I'm not sure how to change the type of NamedSource::new to allow for that, since function arguments must be Sized.

[erratic-pattern]

Looks like you can make this backwards compatibile, but there are still a few issues to address with the new constructor.

You can change NamedSource to take a ?Sized parameter and then Box it, with the default type parameter being dyn SourceCode + 'static

pub struct NamedSource<S: SourceCode + ?Sized + 'static = dyn SourceCode + 'static> {
    source: Box<S>,
    name: String,
}

This allows NamedSource to refer to exactly the same type as before, so that this is no longer a breaking change.

However, in NamedSource::new you need to adjust the input parameter so that it converts properly. Here's what I came up with so far:

impl<S: SourceCode + ?Sized + 'static> NamedSource<S> {
    /// Create a new `NamedSource` using a regular [`SourceCode`] and giving
    /// its returned [`SpanContents`] a name.
    pub fn new(name: impl AsRef<str>, source: impl Into<Box<S>>) -> Self
    where
        S: Send + Sync,
    {
        Self {
            source: source.into(),
            name: name.as_ref().to_string(),
        }
    }
  }

This works fine for NamedSource<String> or NamedSource<str>, but for NamedSource<dyn SourceCode + 'static> it becomes too ambiguous and the compiler can't figure out which trait implementation to use.

error[E0283]: type annotations needed
   --> tests/graphical.rs:268:23
    |
268 |     .with_source_code(NamedSource::new("bad_file.rs", src));
    |                       ^^^^^^^^^^^^^^^^ cannot infer type for struct `Box<_>`
    |
    = note: multiple `impl`s satisfying `Box<_>: From<String>` found in the following crates: `alloc`, `miette`:
            - impl From<String> for Box<(dyn Diagnostic + 'static)>;
            - impl From<String> for Box<(dyn Diagnostic + Send + Sync + 'static)>;
            - impl From<String> for Box<(dyn std::error::Error + 'static)>;
            - impl From<String> for Box<(dyn std::error::Error + Send + Sync + 'static)>;
            - impl From<String> for Box<str>;
            - impl<T> From<T> for Box<T>;
    = note: required for `String` to implement `Into<Box<_>>`
note: required by a bound in `NamedSource::<S>::new`
   --> /Users/adam/Desktop/miette/src/named_source.rs:23:52
    |
23  |     pub fn new(name: impl AsRef<str>, source: impl Into<Box<S>>) -> Self
    |                                                    ^^^^^^^^^^^^ required by this bound in `NamedSource::<S>::new`

Is there a constraint that we could use instead of Into<Box<S>> that would work better here? And do we prefer this over the "simple but breaking` change to generics that's currently in the PR?

[zkat]

I don't really see how we can work around that constraint, yeah, so this would necessarily need to be a breaking change :\

My recommendation continues to be that people write their own NamedSource implementation if they want something like this, considering that NamedSource is just so trivial

[erratic-pattern]

Yeah, I don't have a particular opinion either way. I was just looking for simple issues to pick up and this one seemed like a quick fix. I think it actually improves the docs a bit because it makes it a bit more clear in the examples what's going on when you see NamedSource<String> instead of NamedSource.

but I don't like that this is a breaking change for anything currently using NamedSource, and agree that writing a custom NamedSource would be very trivial.

[MariaSolOs]

Ohhh I like this PR because I think it can help with #258.

nvm, it wouldn't. But doing something like this with LabeledSpan would. That is, changing it to:

pub struct LabeledSpan<S: Into<SourceSpan>> {
    label: Option<String>,
    span: S,
}

[MariaSolOs]

To avoid the breaking changes, what if new generic types are added instead? The existing types can then just be specific variants of the new types.

[zkat]

To avoid the breaking changes, what if new generic types are added instead? The existing types can then just be specific variants of the new types.

Sorry, I have mom brain right now. What do you mean by this? Use a newtype or an alias? (I think using either of these might be breaking too??)

[MariaSolOs]

Sorry, I have mom brain right now. What do you mean by this? Use a newtype or an alias? (I think using either of these might be breaking too??)

No worries 😉

I was thinking of adding a new generic InnerNamedSource<T> and then changing NamedSource to use it. But why do you think that a new type would be a breaking change? Wouldn't that just change the implementation details?