zesterer / ariadne

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

Difficulties with implementing Cache #10

Open bestouff opened 2 years ago

bestouff commented 2 years ago

I've got some troubles trying to implement Cache; trying to compile this:

struct CdlFiles<'p> {
    project: &'p spircer::Project,
    cache: Vec<ariadne::Source>,
}

impl<'p> ariadne::Cache<spircer::FileId> for CdlFiles<'p> {
    fn fetch(&mut self, id: &spircer::FileId) -> Result<&ariadne::Source, Box<dyn Debug>> {
        for i in *id..self.project.files.len() {
            self.cache.push(ariadne::Source::from(unsafe {
                std::str::from_utf8_unchecked(self.project.files[i].bytes())
            }))
        }
        Ok(&self.cache[*id])
    }
    fn display<'a>(&self, id: &'a spircer::FileId) -> Option<Box<dyn Display + 'a>> {
        Some(Box::new(self.project.files[*id].path().display()))
    }
}

Errors like this:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter in function call due to conflicting requirements
  --> src/cli.rs:36:23
   |
36 |         Some(Box::new(self.project.files[*id].path().display()))
   |                       ^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'p` as defined on the impl at 26:6...
  --> src/cli.rs:26:6
   |
26 | impl<'p> ariadne::Cache<spircer::FileId> for CdlFiles<'p> {
   |      ^^
note: ...so that reference does not outlive borrowed content
  --> src/cli.rs:36:23
   |
36 |         Some(Box::new(self.project.files[*id].path().display()))
   |                       ^^^^^^^^^^^^^^^^^^
note: but, the lifetime must be valid for the lifetime `'a` as defined on the method body at 35:16...
  --> src/cli.rs:35:16
   |
35 |     fn display<'a>(&self, id: &'a spircer::FileId) -> Option<Box<dyn Display + 'a>> {
   |                ^^
note: ...so that the expression is assignable
  --> src/cli.rs:36:9
   |
36 |         Some(Box::new(self.project.files[*id].path().display()))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected `Option<Box<(dyn std::fmt::Display + 'a)>>`
              found `Option<Box<dyn std::fmt::Display>>`

For more information about this error, try `rustc --explain E0495`.

Shouldn't the Cache trait have an explicit lifetime ? Otherwise I don't know how to reconcile 'p and 'a in my example.FileId (what stands for an SourceId) is just an usize indexing into the right File structure, in my Project structure.

bestouff commented 2 years ago

In fact my problem seems to be that SourceId needs Display, but I can't impl that on a usize alone. I've tried using (&spircer::Project, spircer::FileId) as a SourceId but then that makes temporaries which can't be returned by reference from Span::source() ...

bestouff commented 2 years ago

I got out of that by using a std::path::Path as SourceId and building a separate HashMap for storing the mapping, but it's a bit of a pity to have to duplicate an existing structure instead of just implementing a trait ...

zesterer commented 2 years ago

I'm planning to do a big overhaul to the API of Ariadne within the next week or two, and this is one of the big things on my list to change. I'll let you know when I have something worth testing!

bestouff commented 2 years ago

Ok, thanks for that.
If you also could avoid passing the Cache by value (I have to clone() it for each message), that's be great.

zesterer commented 2 years ago

There is a blanket impl of Cache for &mut impl Cache: https://docs.rs/ariadne/0.1.3/ariadne/trait.Cache.html#impl-Cache%3CId%3E-for-%26%27b%20mut%20C

You can pass it via mutable reference.

bestouff commented 2 years ago

Yes but as I can access it from multiple threads I only have a shared reference ...

zesterer commented 2 years ago

In that case, you should be able to implement it for &MyType if MyType is local. This will still work because accessing &MyType mutably only gives you a mutable reference to the shared reference, not the underlying value.

bestouff commented 2 years ago

Yes that works, thanks.

azdavis commented 1 year ago

I just ran into something similar. It's unfortunate that the current API design basically forces Id be a std::path::Path or something similar, as opposed to e.g. a cheap integer ID that can look up a Path for display. The lifetimes on fn display essentially require this.