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

Add support for primary label in specifying line/col information #291

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Sep 19, 2023

As discussed in #91 and #208, it is sometimes desirable for the line/column numbers in a diagnostic to reference a label as opposed to the start of the context. However, there has to be a way of identifying which label to reference.

This PR implements one possible mechanism: a label(primary) attribute which acts just like label, except it marks a single label as being "primary". For example, this program:

use miette::{Diagnostic, SourceSpan};
use thiserror::Error;

#[derive(Error, Debug, Diagnostic)]
#[error("Primary span error")]
struct PrimarySpanError {
    #[source_code]
    src: String,
    #[label(primary)] // note the new attribute parameter
    span: SourceSpan,
}

fn main() -> miette::Result<()> {
    Err(PrimarySpanError {
        src: String::from("Hello\nworld"),
        span: (7..10).into(),
    }
    .into())
}

Produces this output:

Error:   × Primary span error
   ╭─[2:2]
 1 │ Hello
 2 │ world
   ·  ───
   ╰────

Note that the line/column info is 2:2 rather than 1:1 which you would get with a normal #[label].

The core technical changes are:

  • Adding a primary: bool field to LabeledSpan.
  • Adding a method LabeledSpan::new_primary_with_span that constructs a LabeledSpan with primary: true.
  • Extending the derive macro to support label(primary).
  • Modifying the GraphicalHandler to look for a primary label and use it if possible.
  • Added a test to graphical.rs to check for this behavior.

These changes were specifically made to be fully backwards compatible.

This is my first time in the miette codebase, so I may have missed a code path. Let me know if I should change anything.

@zkat
Copy link
Owner

zkat commented Sep 19, 2023

I think this kind of change is a long time coming and I really appreciate you taking the time to do this!

As far as specifics go: instead of having to add a new attribute, how would you feel about an API where, by default, the first attribute tagged with #[label] is treated as the primary one, and if you want a subsequent attribute to be the primary instead, you do #[label(primary)]? I think 99.9999% of the time, people don't really want the current "viewport" to be what shows up in that [line:col] display, and it's been an ongoing issue. I think a lot of people would be very happy if, without having to make any changes, that display just magically pointed to one of their spans, which will for the most part be the only one to begin with.

@willcrichton
Copy link
Contributor Author

I would be happy with that design as well. Two questions:

  1. When you say "an API", do you mean a change to the default API everyone uses, or a distinct new part of the miette API surface?
  2. If a user wants a format string in a primary label, should the expected syntax be #[label(primary, "{}", arg)]?

@zkat
Copy link
Owner

zkat commented Sep 19, 2023

  1. Change #[label] itself so it starts marking things as "primary". Basically in https://github.com/zkat/miette/pull/291/files#diff-a775e9b52316667cfcabc8d12a083cba8d18759534d48b7a5da6cd647bc6fe6dR394, to .or_else(|| labels.first()) (or whatever)
  2. Yes. I forgot this allowed format strings. This seems like a good solution.

@willcrichton
Copy link
Contributor Author

Ok I just updated the code and the tests. How does it look?

@willcrichton willcrichton changed the title Add support for primary_label in specifying line/col information Add support for primary label in specifying line/col information Sep 19, 2023
miette-derive/src/lib.rs Outdated Show resolved Hide resolved
@zkat zkat merged commit db0b7e4 into zkat:main Sep 20, 2023
11 checks passed
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 this pull request may close these issues.

None yet

2 participants