[codespan-reporting] Don't require FileId when instantiating Label

[aweary]

Problem

This is just a small pain point I've been having with Label: you are required to pass in the FileId when creating a Label, which means that any error reporting logic must always be aware of what file it's operating on. This has led to some unfortunate coupling where FileId has to be threaded throughout any logic that might report a diagnostic.

For example, I have a Parser struct that can be constructed with a string reference:

let source : &str = "...";
let mut parser = Parser::new(source);
let ast = parser.parse_module();

But actually the Parser needs the FileId to create Label instances, so it looks more like:

let file = filesystem.load("...");
let source = filesystem.resolve(file).unwrap();
let mut parser = Parser::new(source, file);
let ast = parser.parse_module();

Not a huge deal, but it's more annoying. Parser will only ever report diagnostics for a single file. It also adds friction for writing tests with modules like Parser . I have to instantiate that filesystem for the tests instead of just passing in that &str. I want Parser and other modules like it to be independent of my filesystem abstraction, but this FileId requirement makes that difficult.

Proposal

It would be nice if FileId was not required when instantiating Label and instead you could add it with another chained method like with_file

Label::primary(span).with_message("Invalid character").with_file(file_id);

This could be paired with an API for Diagnostic that applies a FileId to all Label instances that don't have one, which would be useful for Parser.

[ratmice]

This is also (somewhat) related to #200, Looking at the existing/current Label structure:

pub struct Label<FileId> {
    pub style: LabelStyle,
    pub file_id: FileId,
    pub range: Range<usize>,
    pub message: String,
}

It seems like there are 2 options for achieving the proposed behavior:

• Turn pub file_id:FileId into pub file_id: Option<FileId>
• Remove file_id from Label, and have with_file(file_id) return some other type such as FileLabel.

It is worth noting that your parser could perform the second case already with a struct like:

pub struct AnonLabel {
    pub style: LabelStyle,
    pub range: Range<usize>,
    pub message: String,
}

impl AnonLabel {
    pub fn with_file<FileId>(self,file:FileId) -> Label<FileId> {
        // or maybe this should just construct directly rather than new().with_message()
        Label::new(self.style, file, self.range).with_message(self.message)
    }
}

So, even if codespan-reporting doesn't change the FileId requirement or include something like the above itself, you should still be able to achieve something like what you want.

Edit: It seems like you can also get the first option without modifying codespan-reporting as well, since FileId is generic you could pass an Option<Something> or a reference to an option initialized to None, and then set it later or something, haven't tried to see if it works nicely however.

[aweary]

So, even if codespan-reporting doesn't change the FileId requirement or include something like the above itself, you should still be able to achieve something like what you want.

Yeah I've already gotten around this by using my own Diagnostic struct, it's just somewhat annoying to have to re-implement the all APIs for this.

[brendanzab]

Sorry for the lack of updates on this, it's been a while! This is definitely a bit annoying. It would be helpful to know what any proposal would look like in the multi-file use-case. Like, when you have labels from different files in the same diagnostic.

[jyn514]

Possibly FileId could implement Default, where the default means an unknown file? Then it would be displayed without the filename.

[brendanzab]

I mean, we have SimpleFiles which uses () for the file id?

[brendanzab]

I'm now wondering if we could investigate a richer 'diagnostic tree'. Perhaps something like:

Diagnostic::error()
    FileLabel::primary(file_id, range)
    FileLabel::secondary(file_id, range)
    FileLabel::secondary(file_id, range)
FileDiagnostic::error(file_id)
    Label::primary(range)
    Label::secondary(range)
    FileLabel::secondary(file_id, range)

I'm unsure how this would actually work in practice though.

[Johann150]

I think the simplest solution is to use () if the file id is to be inserted later. And then implement with_file only for Label<()>/Diagnostic<()>. I would only add it to those specific variants to prevent people from shooting themselves in the foot by changing the file id, not just inserting one.

impl Label<()>{
    fn with_file<NewFileId>(self, file_id: NewFileId)->Label<NewFileId>{
        Label {
            file_id,
            ..self
        }
    }
}

impl Diagnostic<()>{
    fn with_file<NewFileId>(self, file_id: NewFileId)->Diagnostic<NewFileId>{
        // ...
    }
}