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

Conditionally implement miette::Diagnostic for pest::error:Error #663

Closed
wants to merge 3 commits into from

Conversation

bobbbay
Copy link
Contributor

@bobbbay bobbbay commented Jul 24, 2022

Hides an implementation of miette's Diagnostic type under the miette feature flag. Also adds example usage to the parens example.

The only issue I have with this is that the Display of error::Error changes based on this flag. Since miette displays the error and then the diagnostic, the error would be printed twice: once in pest's form, and once in miette's form. Hnce, error.rs:32-33 changes the display value of Error if the miette feature is enabled. In my opinion, it's not a clean solution, so I'd love to hear feedback.

Note: public API change: pest::error::Error::formt(&self) -> String is publicized, so users on the miette feature flag can still read Pest's errors if needed.

Closes #582

@CAD97
Copy link
Contributor

CAD97 commented Jul 24, 2022

The only issue I have with this is that the Display of error::Error changes based on this flag.

The standard workaround for this would be a .into_miette(self) -> impl Diagnostic adapter. (.as_miette(&self) -> impl '_ + Diagnostic would be nicer, but having 'static is necessary for normal use of miette::Report. You can format non-'static diagnostics, but it's more difficult.)

pest/examples/parens.rs Outdated Show resolved Hide resolved
@bobbbay
Copy link
Contributor Author

bobbbay commented Jul 24, 2022

The standard workaround for this would be a .into_miette(self) -> impl Diagnostic adapter.

Sure, but what would the impl Diagnostic be? It can't be Self, because it will have the other, proper formatting as Display. Unless there's another type - MietteError that implements Diagnostic and From<Error> - but that overcomplicates the design.

@CAD97
Copy link
Contributor

CAD97 commented Jul 24, 2022

Personally, I don't think a non-exposed MietteAdapter(Error) (outside only sees impl Diagnostic) is a significant complication. The only difference is implementing Display/Diagnostic on MietteAdapter instead of Error directly.

For comparison, this is also the pattern used by Path::display.

#[error("Failure to parse at {}", self.0.line_col)]
pub(crate) struct MietteAdapter<R: RuleType>(pub(crate) Error<R>);

impl<R: RuleType> Diagnostic for MietteAdapter<R> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors could also contain path and continued_line -- should those also be included in a Diagnostic (e.g. as additional labels) or would including those make the Miette default formatted print outs look strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would make the miette-fornatted output strange. Adding it to the Display is fine, though.

Copy link
Contributor

@CAD97 CAD97 Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, there's a way around it, but it's a bit hacky:

Provide a custom impl SourceCode which reattaches the line to the span offset information, and then the LabeledSpan can carry the correct span. If the label span is outside SourceCode, weird things can happen (mostly just dropping the source pointer, hopefully).

Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want to see a snapshot-style test that shows one or more instances of the miette-formatted error before merging this.

(This could be as simple as assert_eq!(format!("{it:?}"), "expected") or as complex as (waiting for) #661 and using insta or expect_test.)

That said, it's about time to pass the buck on pressing the green button, so...

pest/examples/parens.rs Show resolved Hide resolved
@CAD97 CAD97 requested review from a team and NoahTheDuke July 25, 2022 16:18
@CAD97
Copy link
Contributor

CAD97 commented Jul 25, 2022

(apparently I forgot to push the button to unassign the team when it chooses a load-balanced member)

@pest-parser/maintainers should have write/merge permissions 🎉

@CAD97 CAD97 removed the request for review from a team July 25, 2022 16:20
@bobbbay
Copy link
Contributor Author

bobbbay commented Jul 26, 2022

Great!

(blocked by #666)

@bobbbay bobbbay closed this Sep 18, 2022
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.

Support miette::IntoDiagnostic for pest::error::Error<R>
3 participants