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

Decouple from codemap #7

Open
kevinmehall opened this issue Jul 14, 2019 · 4 comments
Open

Decouple from codemap #7

kevinmehall opened this issue Jul 14, 2019 · 4 comments

Comments

@kevinmehall
Copy link
Owner

The data structure defined in the codemap crate isn't for everyone. This library should use traits to make it agnostic to the codemap / span implementation, and the dependency on codemap should be made an optional cargo feature. Looking through the source, its interaction with the CodeMap API surface is actually pretty minimal.

Prior art: The language-reporting crate defines traits for a similar purpose. I think we can go further than this by removing the code_map field from the Emitter completely, and making the API deal with spans that are already decomposed into file, line, column.

Something like:

#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct LineCol {
    /// The line number within the file (0-indexed).
    pub line: usize,

    /// The column within the line (0-indexed).
    pub column: usize,
}

trait SourceSpan {
    type File: SourceFile;

    fn file() -> &File;
    fn start() -> LineCol;
    fn end() -> LineCol;
}

trait SourceFile: PartialEq {
    type Name: Display;

    /// Gets the name of the file as it should be displayed in the error message.
    fn name(&self) -> &Name;

    /// Gets the text of a line by zero-indexed line number. This
    /// should not include the trailing newline.
    fn line_source(&self, line: usize) -> &str;
}
@Lymia
Copy link

Lymia commented Jul 14, 2019

Note that this would be a breaking change versus the current version, while a version that abstracts CodeMap with a trait could remain backwards compatible via a defaulted type parameter on Emitter.

@kevinmehall
Copy link
Owner Author

It's currently version 0.1 with 5000 total downloads. I wouldn't mind releasing 0.2 with a more flexible API rather than getting stuck with the current design for backwards compatibility.

On the other thread, @stepancheg pointed out a drawback of the current API that would be solved by making each emit call independent of shared state:

I was thinking about having own struct like
struct SpanWithCodeMap { span, Rc<CodeMap> }
But unfortunately codemap-diagnostic crate heavily depends on Span without context (e. g. struct Diagnostic or emitter which requires all spans to belong to single code map), so it's hard to use that struct without rewriting copy-pasting a lot codemap-diagnostic.

@kevinmehall
Copy link
Owner Author

Also worth considering annotate-snippets-rs (#9) before we spend too much time redesigning this.

@kevinmehall
Copy link
Owner Author

annotate-snippets is discussing similar API considerations at rust-lang/annotate-snippets-rs#13. I posted an alternate trait proposal there.

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

No branches or pull requests

2 participants