Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aweary opened this issue Apr 13, 2020 · 7 comments · May be fixed by #323
Open

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

aweary opened this issue Apr 13, 2020 · 7 comments · May be fixed by #323

Comments

@aweary
Copy link

aweary commented Apr 13, 2020

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
Copy link
Collaborator

ratmice commented May 14, 2020

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 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
Copy link
Author

aweary commented May 14, 2020

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
Copy link
Owner

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
Copy link
Collaborator

jyn514 commented May 14, 2020

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

@brendanzab
Copy link
Owner

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

@brendanzab
Copy link
Owner

brendanzab commented May 15, 2020

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
Copy link
Collaborator

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>{
        // ...
    }
}

@Johann150 Johann150 linked a pull request Mar 1, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants