Draft: refactor cli with annotate-snippets

[suaviloquence]

Opening this PR not to merge immediately (or even ever) but to show what we could do with annotate-snippets and get feedback on the CLI design (to be more rustc/clippy-like).

Screenshots:

cargo semver-checks --baseline-root test_crates/function_const_removed/old --manifest-path test_crates/function_const_removed/new -Z unstable-options --witness-hints

old

new

when file is not available for span

on manifest_tests/workspace_overrides test crate (warnings + errors)

old

new

On compatibility with clippy/rustc:

This is mostly just a port of our current failure printing to use annotate-snippets (and adding printing the one-line span snippet).

clippy/rustc linters show generally less information per lint, and don't group instances of lints occurring by lint type. We group all lint results and print lots of info for each lint (description, ref link, info link) as well as per-result error info.

sample clippy/rustc lints

I don't know if we actually want parity with rustc in this regard, though.

[obi1kenobi]

I quite like the new format, it looks very promising — especially with the annotated snippets! Are the annotations based on line/column information only, since we don't yet have byte spans in rustdoc JSON?

Some thoughts for further experimentation:

• I'd probably skip the help: ref and help: impl links and save them for the --explain flag and/or a website that describes the lints, when we get one.
• It might be good if we printed error:/warning: only once per error instead of twice (once at the start of the error description and once with the file path).
• Would it make sense to move the annotated snippet up to right under the error: line and move the = help: and = note: lines under the code, instead of having them sort of oddly indented right under the headline?
• For the "code similar to the following", would it make sense to avoid printing that at top level and instead move it to a = note so it's clearly part of the same error report and not a new one?
• The heavily-indented Summary and Warning lines at the very end of the output look a bit out of place now. Would it make sense to change their style to mimic clippy / rustc more closely?

cc @epage, @Muscraft

[suaviloquence]

I'm currently working on the final write-up for GSoC, so I'll respond more thoroughly when I'm done with that, but just a note that this current design is imitating the current c-s-c design where we print info for each lint, then print the different occurrences of this lint. Here's an example where there are more than one instance of the same lint occurring:

old

new

If we wanted more consistency with rustc/clippy, we could not do that first part with the info and move parts of it to each instance of the lint occurring, and some to --explain like you said.

[obi1kenobi]

I think if we're going for clippy / rustc consistency, we should go all the way and tweak our error format to print once-per-instance instead of grouping like we currently do.

The only thing I'd keep is the initial info on which crate is being scanned / how many lints are being run and why etc. For all the actual lint findings, the sweet spots are probably at either end of the spectrum, not in the middle.

[epage]

I think if we're going for clippy / rustc consistency, we should go all the way and tweak our error format to print once-per-instance instead of grouping like we currently do.

If it becomes overwhelming, you can omit some of the details for a lint after the first occurrence.

[suaviloquence]

New screenshots:

warnings + errors

multiple of the same lint

We could also print the lint ID like this:

[obi1kenobi]

This looks very promising, I like it quite a bit. I think the error[lint_name] approach looks better than error: lint_name: text.

Thank you for being open to iterate on this — it's a really, really hard problem to nail down all the edge cases. I would recommend not spending 100% of time on only this: we'll have to iterate a bunch more here, and it's nice to mix in some building & merging every so often just for personal satisfaction and morale :)

A few thoughts for things to look at next:

• re: the consecutive warning lines at the end of the printout, maybe the second one should be a note: instead?
• re: the info line where the templated message goes now, that seems pretty duplicative of the annotated snippet. There are only two pieces of info there that aren't in the snippet, and it might be good to deduplicate + think about how to use that space more effectively:
  • whether the snippet is from "baseline" (previously in lib.rs:1) or "current" (in lib.rs:1)
  • the full importable path of the item that's part of the public API
• re: the snippet's length — what happens if the removed function (or module, etc.) is very long? We don't want to print the whole thing. Printing just one line isn't great either because depending on formatting, we might not get the full function signature.
• re: re-export removals that trigger function_missing, the span will point to the function's definition. But the definition wasn't removed — the re-export removal made some path that was previously public API no longer exist. We don't currently have the span for the re-export that went missing. Showing the function snippet without at least additional context would be quite misleading (and IDK, maybe showing it at all is a bad idea). I'm not sure how to tackle this.

[suaviloquence]

We may need to improve how we handle spans before we can have snippets like this. Currently, we really only have the span_filename and line number of span_begin_line, so we don't know how many lines of a function to show, and we can't annotate things like a reexport, or e.g. a #[must_use] or #[non_exhaustive] annotation.

[obi1kenobi]

100% agreed. Lints could expose span_end_line too, but that's still going to be way too coarse and imprecise in many cases (e.g. pointing to #[non_exhaustive] or to "just the function signature" etc.).

In the long run, we'll eventually probably need to use the coarse spans available in rustdoc JSON as a guide for loading & parsing the affected Rust file, and then extracting more precise spans on-demand as needed.

In the short run, I'm not sure what the best answer is. Maybe we can investigate adding span_end_line and then using some heuristic like "include up to the first 5 lines, then switch to ellipsis" for the snippet?

[suaviloquence]

I know this was blocked on not being able to have spans with perfect granularity, but are you still interested to try with the heuristic you mentioned and expose span_end_line (or do a partial refactor that starts with just not having code snippets)?

status

I (evidently) haven't had as much bandwidth as I would've liked to be able to contribute here, but I would like to do something. I've still been idly thinking about witness generation, and I think we need a fair bit of infrastructure to be able to generate correct code in every scenario, and I haven't had the time to work on that recently. I can write up my thoughts on what we'll need though.

...that is to say, if the CLI refactor is less of a priority, I can work on other things here if you'd like, but probably something smaller than witnesses for now.

[obi1kenobi]

Thank you for staying on top of this and for continuing to contribute as much as you can — I really appreciate it!

I'm a bit wary of starting complex refactors if it's unclear when they might be complete, since their intermediate states tend to be high-friction and high-maintenance. That said:

• span_end_line would be useful to have captured in lints, for purposes of building a better GitHub Action.
• I suspect a machine-readable format for lint results would be helpful to many downstream tools, including prototypes for this CLI, improved GitHub Actions, witness generation experiments etc. Instead of all those things needing to be bolted directly into cargo-semver-checks from day 1, they could initially be freely prototyped as "consume the output report, then present it how you see fit" until we're happy with their state and decide to merge them back in. I would trust you to design such a format and fit it into the CLI, if that's something you're interested in.
• I'd also love the writeup of what you think proper witnesses need, since that will probably be useful in multiple ways as well. I have an idea for enabling type-checking lints without needing to reimplement a type-checker/borrow-checker/trait solver, by (ab)using rustc via giving it code snippets to attempt to compile. I'll write it up as a blog post in the near future, and I'd love to have a document to link to about what proper witnesses will take.

[obi1kenobi]

Also, if you're looking for "short-arc contributions" that advance the project's goals without requiring weeks of effort, I'm sure we have lots of low-hanging fruit that you have sufficient context to tackle. I'd be happy to suggest some if that'd be helpful!