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.

Sign up for GitHub

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

Zero length span #356

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Zero length span #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f8fbf8
refactor(get_lines): Simplify `get_lines`
Nahor Mar 4, 2024
a37328a
refactor(span): Simplify checks how a span applies on a line
Nahor Mar 4, 2024
f7fa50d
fix(SourceCode): Fix and improve default `SourceCode` implementation
Nahor Mar 3, 2024
d63b2bb
fix(zero length, no context): Add Option to distinguish between "curr…
Nahor Mar 11, 2024
d0c1143
fix(miri): remove or mark as dead_code unused internal structs
Nahor Mar 19, 2024
c7cbb07
fix(zero length): Improve rendering for zero-length error spans
Nahor Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/eyreish/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,5 +213,4 @@ pub(crate) mod private {
pub trait Sealed {}

impl<T, E> Sealed for Result<T, E> where E: ext::Diag {}
impl<T> Sealed for Option<T> {}
}
43 changes: 1 addition & 42 deletions src/eyreish/wrapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,9 @@ use crate::{Diagnostic, LabeledSpan, Report, SourceCode};

use crate as miette;

#[repr(transparent)]
pub(crate) struct DisplayError<M>(pub(crate) M);

#[repr(transparent)]
pub(crate) struct MessageError<M>(pub(crate) M);

pub(crate) struct NoneError;

impl<M> Debug for DisplayError<M>
where
M: Display,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(&self.0, f)
}
}

impl<M> Display for DisplayError<M>
where
M: Display,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(&self.0, f)
}
}

impl<M> StdError for DisplayError<M> where M: Display + 'static {}
impl<M> Diagnostic for DisplayError<M> where M: Display + 'static {}

Copy link
Owner

Choose a reason for hiding this comment

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

wasn't all this here for a reason? I'm nervous about this change cause it touches the code that was taken from anyhow, which I honestly don't fully understand, and does some Weird Things with pointers and types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know either but I couldn't find any mention of DisplayError in the whole Miette history except for adding the struct and its impl. It is not public outside of the crate either (and thus the Miri error), unlike the one in anyhow. So I don't know how/where it could be useful and figured deleting it was the best option.

If you don't agree, I can easily change that to dead_code instead.
(and if so, does your comment apply to NoneError as well? I couldn't find it in anyhow's code, so I'm not sure if it does)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional info: if I understand correctly, DisplayError in Anyhow (and Eyre, which also has a DisplayError but private to the crate, like Miette and unlike Anyhow, where its public) is used to convert an Option<T> to a Result<T>. Miette does not have such a feature.

Copy link
Contributor Author

@Nahor Nahor Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

3 more points, in Eyre:

  • in Eyre, DisplayError seems to only be used for compatibility with Anyhow (via ContextCompat). For the pure Eyre (ok_or_eyre), the Option->Result uses MessageError instead (which is also not available in Miette).
  • to answer my own question about NoneError, it is in Eyre and used along side DisplayError, for the same Option-to-Result in that ContextCompat.
  • Miri on Eyre will also start complaining about DisplayError/NoneError if I remove ContextCompat there (and does not complain otherwise of course)

So at this point, I'm 99% sure that it's safe to remove. And I believe I can remove an extra line of code related to that Option conversion.

impl<M> Debug for MessageError<M>
where
M: Display + Debug,
Expand All @@ -56,21 +30,6 @@ where
impl<M> StdError for MessageError<M> where M: Display + Debug + 'static {}
impl<M> Diagnostic for MessageError<M> where M: Display + Debug + 'static {}

impl Debug for NoneError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Debug::fmt("Option was None", f)
}
}

impl Display for NoneError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt("Option was None", f)
}
}

impl StdError for NoneError {}
impl Diagnostic for NoneError {}

#[repr(transparent)]
pub(crate) struct BoxedError(pub(crate) Box<dyn Diagnostic + Send + Sync>);

Expand Down Expand Up @@ -283,7 +242,7 @@ mod tests {
report
.source_code()
.unwrap()
.read_span(&(0..5).into(), 0, 0)
.read_span(&(0..5).into(), None, None)
.unwrap()
.data()
.to_vec(),
Expand Down
Loading
Loading