RFC: Proposal for "modern" API #13
Comments
|
cc list of potentially interested parties:
|
|
Hi @CAD97 ! Thanks for taking a look at this and I'm excited to work together, if we end up deciding that we're aiming for the same shape of the API. I did a first read of your proposal and only have very rough, unsorted initial thoughts to share so far. Most of them are critical, but please, do not read it as a general criticizm - it's just easier to focus on what I see as potentially incompatible. The code generally looks good! Foundational CrateYou seem to use dependencies liberally. I'm of strong opinion that this functionality should be treated as a "foundational crate" per raphlinus terminology and as such should aim to maintain minimal, or even zero if possible, dependency tree. Your proposal has 22 dependencies, APIYour API seems to resemble more imperative approach much closer than what I'm aiming for with In principle, I see what I call So, instead of making people write If I'm not mistaken, the only shortcoming of that approach is lack of inter-field validation allowing one to define a snippet with 5 lines, but an annotation on a sixth. Initially, that led me to try to squeeze one or more constructors, because I dislike ability to produce internally inconsistent data, but eventually I decided that it's not necessary to fix it. In I really like this model for a foundational functionality of a foundational crate. I'm sure it's possible to build different nicer high-level APIs to aid people in constructing Your model seems much closer to what Flexibility
For me, the difference between: let diagnostic = Diagnostic::build()
.primary(
Annotation::build()
.span(50..777)
.message("mismatched types")
.build(),
)
.code("E0308")
.level(Level::Err)
.secondary(
Annotation::build()
.span(55..69)
.message("expected `Option<String>` because of return type")
.build(),
)
.secondary(
Annotation::build()
.span(76..775)
.message("expected enum `std::option::Option`, found ()")
.build(),
)
.build();and let snippet = Snippet {
title: Some(Annotation {
id: Some("E0308"),
label: Some("mismatched types"),
annotation_type: AnnotationType::Error,
}),
slices: &[Slice {
source,
line_start: Some(51),
annotations: vec![
SourceAnnotation {
label: "expected `Option<String>` because of return type",
annotation_type: AnnotationType::Warning,
range: (5, 19),
},
SourceAnnotation {
label: "expected enum `std::option::Option`",
annotation_type: AnnotationType::Error,
range: (23, 725),
},
],
}],
};is that in the latter, the only meaning is assigned to annotations via In the former, the title is decided by a
Your API seems much more constrain and intended for getting just one style of annotation snippets. CowYou use a lot of PerformanceI focus a lot on performance in my rewrite of API discrepancyFinally, and I struggle to ask this since it borderlines NIH-bias which I'd like to avoid, I'm wondering why do you feel the need to design a new API. I asked the authors of === I've been on vacation over this week, but I plan to get back and finish #12 now. If you believe that there's a value in diverging from its API, I'm happy to discuss the above differences (or any other that you see!) and compare the results! Let me know what you think! |
|
Big apologies: I forgot that I had an out-of-date example in the repository when I posted this; I didn't mean to mislead. I've dropped the builder API (if you look at the API overview in the OP, it's not present) and that's why the example won't compile. There were a few other issues as well because I pushed a WIP checkpoint commit. The implementation is still not quite finished for performance testing as I still need to port one final bit first. So let me address the points some: Dependencies
For some reason I'm having trouble installing APIThe builder API used in the example was old and discarded; I'm just allowing record creation much closer to let diagnostic = Diagnostic {
primary: Annotation {
span: (50, 777),
level: Level::Err,
message: "mismatched types".into(),
},
code: Some("E0308".into()),
secondary: vec![
Annotation {
span: (55, 69),
level: Level::Info,
message: "expected `Option<String>` because of return type".into(),
},
Annotation {
span: (76, 775),
level: Level::Err,
message: "expected enum `std::option::Option`, found ()".into(),
},
] // can also be borrowed, `vec` for simplicity
.into(),
};FlexibilityYeah, the API I've proposed as-is is quite targeted at diagnostics and being compatible with the LSP diagnostic API. I'm perfectly happy to generalize it some more, though. The intent as currently designed is that the CowThe main reason I've used PerformanceShould be on par with NIHI'm happy to find a middle ground, this is mainly to share the results of my experimentation so that we can try to find the best end result. The big parts of this I'd really like to see adopted in some form: 1) A LSP target is practical, 2) delayed |
|
I didn't read this past the first paragraph (I hope to do so, once I have more time), but I'd advise against using lsp-types as a dependency, even an optional one, It changes way to often, and I don't think it's worth it make it non-breaking. Rather, I think it's better to vendor just diagnostics related bits of the LSP, which should be stable. See, for example how I've hard-coded setup/tear down messages in lsp-server. It seems like the case where depending on a 3rd party lib doesn't actually by that much |
|
Thanks for the response! I'm just responding to your points, I'll review more tomorrow: DependenciesYes, I can understand why you aim for As for Others - I'd like to all I saw things like APIOh, I'm sorry for not reviewing the example vs. code. As I said, I just got back from PTO. As for your example, it looks much better to me! I still would like to separate title from in-source annotations, because it's easier to reference/copy one from another than to separate if you need them to be different. FlexibilityAs with above - it's easier to specialize later, if the crate allows for generic behavior. I'd like to not dictate what behavior happens on the level of our crate, but rather on the level of some higher level API. Then you can have an API that uses the foundational crate and is specific to generating errors and it picks the "primary" and sets it as the only title, and as the primary annotation and so on. CowI can be talked up to incorporate Cow. My main concern about it is that I'm torn on whether the API should be constructable. There's one way to think about it that it should. There's another that there should be some higher level that constructs it (maybe over time) and then spawns the One idea against is that I'm trying to minimize allocations and one way to do that was to cut out all PerformanceCool! Let's both finish our PRs and compare! :) NIHOh, awesome! I'm so happy to see actual points listed out this way. This is very helpful, thank you!
The last step is the last piece of my cleanup, so I'd like to finish my proposal. I like your updated example much more (d'uh! it's closer to annotate-snippets ;)) and I'm really excited to see you bringing your experience and perspective! Let's finish our PRs and compare them and figure out how to approach it. Onwards! :) |
|
Just a few more notes:
The rest is a question of what layout decisions should be at what layer. |
|
Sounds good! I'll look into |
|
Is the intention that you could The requirement that a span's start and end are What if the interface exposed column numbers for the library to do its layout computations, but kept trait SpanResolver<Sp> {
type Origin: Eq + Display;
type Line: Copy + Eq + Ord + Into<usize>;
fn resolve(&mut self, span: Sp) -> ResolvedSpan<Origin, Line>;
fn write_source(
&mut self,
w: &mut dyn WriteColor,
file: &Origin,
line: Line,
start_col: usize,
end_col: usize,
) -> io::Result<()>;
fn next_line(&mut self, line: Line) -> Option<Line>;
}
struct ResolvedSpan<Origin, Line> {
file: Origin,
start_line: Line,
start_col: usize,
end_line: Line,
end_col: usize,
}
|
|
@kevinmehall yes, the intent was that it would be possible to implement |
|
|
In |
|
Let me drop this here while it's fresh on my mind, though I should admit it's unbaked while I need to run off to bed again. Here's the input structure I'm experimenting with for a much reduced API/// A span of a snippet to be annotated.
pub trait Span {
/// A position within the span. The only requirement is that positions
/// sort correctly for every `Span` from the same origin.
///
/// For most spans, this will be a `usize` index
/// or a `(usize, usize)` line/column pair.
type Pos: Ord;
/// The start position of the span.
///
/// This is expected to be equivalent in cost to an access.
fn start(&self) -> Self::Pos;
/// The end position of the span.
///
/// This is expected to be equivalent in cost to an access.
fn end(&self) -> Self::Pos;
}
/// A type to resolve spans from opaque spans to information required for annotation.
pub trait SpanResolver<Sp> {
/// Write the span to a [`WriteColor`] sink.
///
/// When calling `write_span`, the writer is styled with the base style.
/// Style can be customized manually or by proxying through the stylesheet.
fn write_span(
&mut self,
w: &mut dyn WriteColor,
stylesheet: &mut dyn Stylesheet,
span: Sp,
) -> io::Result<()>;
/// Count the number of characters wide this span is in a terminal font.
fn count_chars(&mut self, span: Sp) -> usize;
/// Get the first line in a span. The line includes the whole line,
/// even if that extends out of the source span being iterated.
///
/// If the input span is empty, the line it is on is produced.
fn first_line_of(&mut self, span: Sp) -> Line<Sp>;
/// Get the next line in a span. The line includes the whole line,
/// even if that extends out of the source span being iterated.
///
/// If the next line does not overlap the span at all, `None` is produced.
fn next_line_of(&mut self, span: Sp, previous: Line<Sp>) -> Option<Line<Sp>>;
}
/// A reference to a line within a snippet.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub struct Line<Sp> {
/// The span of the line, _not_ including the terminating newline (if present).
pub span: Sp,
/// The line number.
pub num: usize,
}
/// One snippet to be annotated.
///
/// # Example
///
/// In the error message
///
// Please keep in sync with the `moved_value` example!
/// ```text
/// error[E0382]: use of moved value: `x`
/// --> examples/moved_value.rs:4:5
/// |
/// 4 | let x = vec![1];
/// | - move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait
/// 7 | let y = x;
/// | - value moved here
/// 9 | x;
/// | ^ value used here after move
/// ```
///
/// there are three snippets: one for each bit of code being annotated.
/// The spans to create this error are:
///
/// ```
/// # use retort::*;
/// # let line4 = 0..0; let line7 = 0..0; let line9 = 0..0;
/// let snippets = &[
/// Snippet {
/// annotated_span: line4,
/// spacing: Spacing::TightBelow,
/// },
/// Snippet {
/// annotated_span: line7,
/// spacing: Spacing::Tight,
/// },
/// Snippet {
/// annotated_span: line9,
/// spacing: Spacing::Tight,
/// },
/// ];
/// ```
#[derive(Debug, Copy, Clone)]
pub struct Snippet<Sp> {
pub annotated_span: Sp,
pub spacing: Spacing,
}
/// Spacing of a snippet.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum Spacing {
/// Emit a spacing line above and below the snippet.
Spacious,
/// Emit a spacing line below the snippet only.
TightAbove,
/// Emit a spacing line above the snippet only.
TightBelow,
/// Emit no spacing lines.
Tight,
}
/// An annotation of some span.
///
/// # Example
///
/// In the error message
///
// Please keep in sync with the `moved_value` example!
/// ```text
/// error[E0382]: use of moved value: `x`
/// --> examples/moved_value.rs:4:5
/// |
/// 4 | let x = vec![1];
/// | - move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait
/// 7 | let y = x;
/// | - value moved here
/// 9 | x;
/// | ^ value used here after move
/// ```
///
/// there are three annotations: one on each line of code.
/// The annotations in this error are:
///
/// ```
/// # use retort::*;
/// # let line4_x = 0..0; let line7_x = 0..0; let line9_x = 0..0;
/// let annotations = &[
/// Annoatation {
/// span: line4_x,
/// message: "move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait",
/// level: Level::Information,
/// },
/// Annoatation {
/// span: line7_x,
/// message: "value moved here",
/// level: Level::Information,
/// },
/// Annoatation {
/// span: line9_x,
/// message: "value used here after move",
/// level: Level::Error,
/// },
/// ];
/// ```
#[derive(Debug, Copy, Clone)]
pub struct Annotation<'a, Sp> {
/// The span to be annotated.
pub span: Sp,
/// The message to attach to the span.
pub message: &'a str,
/// The severity of the annotation.
pub level: Level,
}
/// A level of severity for an annotation.
#[derive(Debug, Copy, Clone)]
pub enum Level {
/// An error on the hand of the user.
Error,
/// A warning of something that isn't necessarily wrong, but looks fishy.
Warning,
/// An informational annotation.
Information,
/// A hint about what actions can be taken.
Hint,
}I suspect adding a "collection of snippets" type (that if I'm not mistaken, is closer to the current
|
Did you see gluon-lang/lsp-types#117 ? Would let lsp-types go to 1.0 at the cost of having a slightly more awkward API (though in a way, a more honest one). |
|
I just want to say I really appreciate the time being put into this! Would be exciting to converge on something nice. |
|
@CAD97 I'm cool with merging |
|
@wycats I'm not on any team right now, I just chose this as a pet project for October (as I'm working on a blog post series that would benefit from it) 😛 I've a second potential API over at CAD97/retort#2 that embraces the sink-layer quality of annotate-snippets more fully. (i.e. a diagnostics library could provide a more LSP-shaped API that renders to annotate-snippets or LSP.) It's also got an amount of experimental support for I'll update the OP with an adjusted RFC proposal of the adjusted API once I have a chance to write some examples against that API, probably tomorrow sometime. |
|
@CAD97 - I pushed an update to my
My next steps are to:
At that point, I'd like to consider merging the PR into master and releasing updated As for your proposal, I'm not sure where is the best place to fit it - should we first align our codebases before any releases, or is it ok to do this on top of On the high level, except of feature set, I think we need to decide on which of the APIs is better, the one in The example I work with looks like this:
And the APIs are:
let snippet = Snippet {
title: Some(Annotation {
id: Some("E0308"),
label: Some("mismatched types"),
annotation_type: AnnotationType::Error,
}),
footer: &[],
slices: &[Slice {
source,
line_start: Some(51),
origin: Some("src/format.rs"),
annotations: &[
SourceAnnotation {
label: "expected `Option<String>` because of return type",
annotation_type: AnnotationType::Warning,
range: 5..19,
},
SourceAnnotation {
label: "expected enum `std::option::Option`",
annotation_type: AnnotationType::Error,
range: 23..725,
},
],
}],
};@CAD97's: let snippets = &[
Snippet::Title {
message: Message {
text: &"mismatched types",
level: Level::Error,
},
code: Some(&"E0308"),
},
Snippet::AnnotatedSlice {
slice: Slice {
span: 1..721,
origin: Some(Origin { file: &"src/format.rs", pos: Some((4, Some(5))) }),
spacing: Spacing::TightBelow,
fold: false,
},
annotations: &[
Annotation {
span: 5..19,
message: Message {
text: &"expected `Option<String>` because of return type",
level: Level::Warning,
},
},
Annotation {
span: 23..725,
message: Message {
text: &"expected enum `std::option::Option`",
level: Level::Error,
},
},
],
},
];Performance wise, @CAD97's PR is |
|
@zbraniecki my impl uses a lot of It's a tradeoff between monomorphization cost and performance cost. On my branch with the same benchmark, I got Branch with the adjustment: https://github.com/CAD97/retort/tree/take-two-impl Note that my version has a few features that |
|
Good analysis! I also noticed that just switching from ftm::Write to io::Write cost me 8.0->9.0 As for which of those trades we should take I'm open to discuss it! |
@zbraniecki Oooh, does that mean we'll be able to use box drawing characters for the underlines/gutters? |
|
Yeah! Christopher's POC already introduced that and my cleanup pr is designed to support pluggable "renderers". |
|
One other thing that might be interesting to figure out, but may be beyond the scope of this issue, is how to integrate pretty printers and styled output in messages. This would allow for things like type diffs and domain specific errors - something that could be handy for Rust and other languages too, like LALRPOP. This may lead to some interesting questions in regards to how it interacts with the different backends, however. |
|
I've seen Christopher toying with that. I consider it to be out of scope as long as we don't need to alter the input to produce it and I think we don't . So, worth opening a new issue I think :) |
|
OK, I decided not to update the OP but just to do the comparison here. I've attempted to normalize some naming and removed export locations for the purposes of comparison, as well as used a couple shorthands.
|
|
I've made a PR with an implementation of my new proposed API, and honestly, if we acquiesce support for non-ANSI coloring, I think it's the best yet. It even supports both the "stick &str in the slice root, use relative offsets" AND "use offsets into a late-bound source" at the same time! My benchmark results, with so basically it's all within noise of each-other, which is great! As an example of the API, see the big example I wrote on ExampleTo produce the error annotation two snippets are used: # use annotate_snippets::*;
let first_snippet = Snippet {
title: Some(Title {
code: Some(&"E0277"),
message: Message {
text: &"`std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely",
level: Level::Error,
},
}),
slices: &[Slice {
span: WithLineNumber {
data: "fn is_send<T: Send>(t: T) {\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n is_send(foo());",
line_num: 5,
},
origin: Some(&"examples/nonsend_future.rs"),
annotations: &[
Annotation {
span: 4..11,
message: None,
},
Annotation {
span: 14..18,
message: Some(Message {
text: &"required by this bound in `is_send`",
level: Level::Info,
})
},
Annotation {
span: 67..74,
message: Some(Message {
text: &"`std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely",
level: Level::Error,
})
},
],
footer: &[Message {
text: &"within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`",
level: Level::Help,
}],
}],
};
let second_snippet = Snippet {
title: Some(Title {
code: None,
message: Message {
text: &"future does not implement `std::marker::Send` as this value is used across an await",
level: Level::Note,
},
}),
slices: &[Slice {
span: WithLineNumber {
data: " let g = x.lock().unwrap();\n baz().await;\n}",
line_num: 14,
},
origin: Some(&"examples/nonsend_future.rs"),
annotations: &[
Annotation {
span: 8..9,
message: Some(Message {
text: &"has type `std::sync::MutexGuard<'_, u32>`",
level: Level::Info,
}),
},
Annotation {
span: 36..47,
message: Some(Message {
text: &"await occurs here, with `g` maybe used later",
level: Level::Error,
})
},
Annotation {
span: 50..51,
message: Some(Message {
text: &"`g` is later dropped here",
level: Level::Info,
})
},
],
footer: &[],
}],
};The API still needs fold support (cleanup does not have it yet) as well as an implementation for printing footer notes (cleanup does not have it yet) as well as anonymizing line numbers (cleanup does not have it yet), and then I think it's feature parity with PR is up as #14. |
|
@CAD97 - apologies for radio silence. I haven't had a lot of time lately to finish this work, but I feel like I now have all the pieces in place to sort it out and I'll try to take a deeper dive into your proposal and finishing cleanup branch! |
|
Is there a proposed |
|
The code for merge is super simple btw: brendanzab/codespan#149 |
zbraniecki
added
C-enhancement
…ion-3.x chore(deps): update github/codeql-action action to v3
In order to address concerns in #11, #12, wycats/language-reporting#6, and probably others.
I've been experimenting with merging the APIs of
codespan/language-reporting/annotate-snippets, and the below API surface is what that I think makes the most sense.Original Proposal
An experimental implementation of the API based on #12 is at CAD97/retort#1 (being pushed within 24 hours of posting, I've got one last bit to "port" but I've got to get to bed now but I wanted to get this posted first).
API
Notes:
dyn Trait, so the only monomorphization should be over theSpantype.Span::newis only used forimpl SpanResolver<impl Span> for &str; making that impl more specific can get rid of that trait method.SpanResolvertakes&mutfor its methods primarily because it can, in order to allow use of a single-threaded DB that requires&mutaccess for caching as a span resolver.Spanresolution is passed throughSpanResolverat the last moment such that aSpanResolvercan supply syntax highlighting for errors.SpanResolver::write_originonly getsio::Writebecause styling is done ahead of time byStylesheet. BecauseWriteColordoes not have an upcast method, this means we can't usedyn WriteColoranywhere that will end up callingSpanResolver::write_origin. This can be changed to takeWriteColorif desired.Diagnostic's layout is tuned to have similar layout to the language server protocol'sDiagnostic.Diagnosticis set up so thatDiagnostic<'_, Sp>can be borrowed but also an ownedDiagnostic<'static, Sp>can be produced by usingCows. This eases use with constructed diagnostics.Style::Codeto be an enum of general code token types (e.g. the list from pygments),SpanResolver::write_spanjust gets the ability to set the style to one of those, which goes through theStyleSheetfor styling.The text was updated successfully, but these errors were encountered: