Move to using annotate-snippets-rs for producing snippets

[Manishearth]

@zbraniecki created https://github.com/zbraniecki/annotate-snippets-rs which basically does Rust-style formatting of code snippets for errors, to be used within Fluent.

I think we should try and move our snippet code out of tree, preferably merging it with annotate-snippets-rs (which is already close to what rustc does), so that it's reusable.

@zbraniecki has agreed to make whatever changes necessary to the crate to make it usable within rustc.

cc @estebank

[oli-obk]

cc @rust-lang/wg-diagnostics

[oli-obk]

We just had our first wg-diagnostics meeting and think that a good first step would be to create a new (unstable) diagnostics renderer (we have json, short and human right now). Basically it would use the existing diagnostics backend to call into annotate-snippets.

We can then create a way to run ui tests with the new backend and see how the output changes and circle back to changing annotate-snippets until we are happy.

And once we are all happy, we can get rid of the old human emitter and replace it with the new annotate-snippets based one.

@zbraniecki in order to be able to use this in rustc, we'd need to move your crate into the rust-lang organization and give publish rights to some compiler devs so that we have quick access to it if anything needs doing. Are you ok with that?

[zbraniecki]

Yes! I'd love to hand it over to the Rust team and would be happy to see contributions or hand over to new maintainer if that's better for you! I will continue using it for fluent-rs, but I feel like the crate supports all my direct needs while it will need more features for Rust I believe (since Rust errors are more complex!)

[Manishearth]

@zbraniecki if you give me admin access to the repo I can migrate it.

[zbraniecki]

Invited!

[Manishearth]

I can push but I can't move. Hmm.

Maybe transfer the repo to me and then I can transfer it to rust-lang?

[zbraniecki]

I tried to transfer to you

[Manishearth]

Done! https://github.com/rust-lang/annotate-snippets-rs

[phansch]

If no one else has picked this up yet, I'd be interested in working on this. I'm currently working on adding the new diagnostics error-format.

[zbraniecki]

I'm happy to help you brainstorm, or review or whatever is needed. :)

[phansch]

I figured I'd post a small status update as it has been a while since the last changes.

We currently have the AnnotateSnippetEmitterWriter that emits
basic diagnostics using the annotate-snippet library. This writer is
essentially taking rustc's Diagnostic and turns that into an
annotate-snippet::Snippet. Currently this doesn't include suggestions
(#61809) and that's the thing I've been working on and off, with some
help from @zbraniecki.

The main problem with turning rustc annotations into annotate-snippet
annotations is that rustc annotation spans are line-based (using
FileWithAnnotatedLines), whereas annotate-snippet
expects the annotation spans to be relative to the start of the provided
snippet.

There's currently two open WIP PRs for annotate-snippet that would make it
possible to provide line-based annotations. It would be great to make some
progress there as that means we won't have to convert from line-relative to
snippet-relative annotation spans in our emitter.

Once annotation support has landed for the new emitter it would be a good time
to see where the output differs the most and what the next steps are.

[8573]

If @rust-lang has ownership of annotate-snippets on GitHub, should it have ownership on Crates.io too? It currently doesn't.

[oli-obk]

Yes, those permissions should be in sync

[lastmjs]

Is rustc now officially using https://github.com/rust-lang/annotate-snippets-rs? We're trying to find the best way to produce rustc-like errors for our own compiler, and we're wondering if https://github.com/rust-lang/annotate-snippets-rs would be the best choice.

[bjorn3]

No, we aren't using it right now afaik.

Edit: We are actually using it. Must have missed the switch.

rust/compiler/rustc_errors/Cargo.toml

Line 23 in ee1c3b3

annotate-snippets = "0.9"

[kevinmehall]

It's on nightly behind -Z unstable-options --error-format=human-annotate-rs, but not on by default

[bjorn3]

Looks like it still has some bugs. For example compiling an empty bin crate should give

error[E0601]: `main` function not found in crate `rust_out`
  |
  = note: consider adding a `main` function at the crate level

error: aborting due to previous error

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

but it doesn't show anything with --error-format=human-annotate-rs. Also colors and error: aborting due to previous error are missing.

[nnethercote]

I've been doing a lot of cleanups in diagnostic handling lately and I just stumbled across this annotate-snippets-rs stuff. The entire project appears to be moribund; it looks like no work has been done in more than four years. This has me wondering if it should be abandoned and the relevant code removed from the compiler.

If anyone says "is there any harm in keeping the code around" I will pre-emptively say that all code maintenance has a cost. Here is one recent example where this caused some friction.

[Manishearth]

I think the goal was to move to this entirely, so that the Rust compiler's annotation machinery is shareable.

In general annotate-snippets-rs is not moribund, it's mostly "done" but the Cargo people are looking at using it and have been contributing things they need recently.

I think the only thing left here is the decision to switch over by default which should be made by @estebank / @davidtwco / @oli-obk or someone else who works on diagnostics.

So I'd actually say we should tend towards removing the existing rustc diagnostic code and switching over to annotate-snippets, rather than removing the annotate-snippets code.

[ehuss]

As of about a month ago, @Muscraft has taken over maintenance of the annotate-snippets library, and Cargo is incorporating it.

I can recognize that the current implementation of this option in the compiler is pretty buggy, but I imagine is also a good candidate to have new people try to fix some of the issues. I think it would be great if we could follow through with the plan @Manishearth mentioned. I don't know if @Muscraft is interested in that?

[nnethercote]

I think the only thing left here is the decision to switch over by default

It sounds like that's not the only thing, that there's also a non-trivial amount of work to ensure the annotate-snippets-rs diagnostics work as well as the existing diagnostics.

In general, this pattern gets my spidey-senses tingling:

• "We're going to switch X to the new way of doing things!"
• A bunch of code lands to support the new way of doing things.
• Years pass without further progress, and the old way of doing things endures.

[oli-obk]

Yea, the way to remove cruft here is to remove the old diagnostics emitter and switch to annotate-snippets.

[Manishearth]

It sounds like that's not the only thing, that there's also a non-trivial amount of work to ensure the annotate-snippets-rs diagnostics work as well as the existing diagnostics.

Let's see if the Cargo team is willing to help with that, or if the diagnostics team is willing to take this on.

Or we could even "shoot first ask questions later" by switching the default now and seeing what breaks (seems like this is what @oli-obk is suggesting). It doesn't seem like the work needed is in critical functionality.

I would rather have rustc and Cargo be able to share code for this. There's another bad pattern at play here, rustc tends to often be reluctant to share code; citing maintenance reasons, and the other Rust tools have to reinvent or fork things. We went through this with compiletest. This code in rustc is not touched that often and I'd rather consolidate on something shareable.

[nnethercote]

I'm in hard agreement on the idea "we should only have one way to produce human-readable diagnostics". The lack of progress for four years made it seem like annotate-snippets was not a serious contender to be that one way. If I'm wrong about that and someone can pick it up and make the switch in a relatively short period of time, great. I just don't want to be back here restarting this discussion in 18 months.

[Muscraft]

Or we could even "shoot first ask questions later" by switching the default now and seeing what breaks (seems like this is what @oli-obk is suggesting). It doesn't seem like the work needed is in critical functionality.

annotate-snippets can't be used as the default yet, it is not at feature parity and won't be until after I am done working on cargo's diagnostic system unless others are helping with the effort. I will happily accept contributors to the effort and would be glad to discuss what changes need to be made to accommodate rustc!

I'm in hard agreement on the idea "we should only have one way to produce human-readable diagnostics".

This is my goal with my work on annotate-snippets. Having a bunch of different renderers across various rust-lang projects leads to reimplementations of the same idea, wasting time and energy. Having one renderer also makes it so there is a consistent style. There are already problems with cargo and rustc having different sets of colors.

The lack of progress for four years made it seem like annotate-snippets was not a serious contender to be that one way. If I'm wrong about that and someone can pick it up and make the switch in a relatively short period of time, great. I just don't want to be back here restarting this discussion in 18 months.

As @ehuss said, I picked up work on annotate-snippets a month ago, so we can use it within cargo. I cannot promise work on having it be compatible with rustc will be done in "a relatively short period of time", but it is high on my list of things to do after work on cargo's diagnostic system.

---

All of that being said it seems like there is interest in having annotate-snippets be the default renderer for rustc in the future. The best path forward for that to happen is probably to work out a plan of what needs to be done to annotate-snippets to accommodate rustc. I am happy to discuss this in whatever way is best, and with whoever I need to.

[Manishearth]

How's this for a plan:

• Cargo continues bringing annotate-snippets up to scratch for themselves. It sounds like that work will bring us most of the way.
• Once done, we check again to see what is needed for feature parity with rustc's need and file issues. wg-diagnostics or Cargo make that happen
• Fix them
• Throw the switch.

I cannot promise work on having it be compatible with rustc will be done in "a relatively short period of time"

I think if there is activity then we should not be worried about deadlines, as long as stuff is actually happening.

[jieyouxu]

annotate-snippets can't be used as the default yet, it is not at feature parity and won't be until after I am done working on cargo's diagnostic system unless others are helping with the effort. I will happily accept contributors to the effort and would be glad to discuss what changes need to be made to accommodate rustc!

@Muscraft I'm interested in contributing to annotate-snippets, but I have no clue where to get started. In particular:

• What are the communciation channels for tracking efforts for maintaining and improving annotate-snippets? Does a zulip stream for it exist? Is there a working group or something?
• What are the areas / missing features that need to be worked on so rustc can use annotate-snippets in the future?
• Is there a high-level overview for describing the concepts of annotate-snippets?

I'm specifically interested in bringing annotate-snippets closer to what rustc needs/wants (I'm not as interested in cargo's diagnostics).

[nnethercote]

I don't know anything about the internals of annotate-snippets or the answer to those questions, but a couple of details that might be useful.

• To enable annotate-snippets output, use rustc +nightly -Zunstable-options --error-format=human-annotate-rs. At first glance, the output is quite usable: similar to the default output, but with minor content differences, and no colour.
• The compiler has a trait called Emitter. Its most important method is called emit_diagnostic, which prints a diagnostic.
• The default Emitter impl is called HumanEmitter.
• The annotate-snippet equivalent is called AnnotateSnippetEmitter. There are several FIXME comments in that file.
• The overall goal of this project is to get the AnnotateSnippetEmitter output the same as the HumanEmitter output, and then remove the latter.