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

Zero length span #356

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

Zero length span #356

wants to merge 6 commits into from

Conversation

Nahor
Copy link
Contributor

@Nahor Nahor commented Mar 19, 2024

Ideally, this PR would just be commit d63b2bb and c7cbb071 stacked on top of PR #354, but Github doesn't support PR stacking, so this also contains the commits from #354.

The first is to add a "error line only" mode, where one does not want extra context line but still get the line with the error, which is necessary when using zero-length span without context.
The second adds support for the zero-length span at the end of the source or the context.

This is a API breaking change. It changes the usize to Option<usize> in SourceCode::read_span().

GraphicalReportHandler and NarratableReportHandler supports the new capability but in a non-API breaking way, by adding with_opt_context_lines instead of changing the signature of with_context_lines. The latter's behavior is also unchanged (where 0 means None in the new function rather than Some(0))

Footnotes

  1. and commit 8e4849e to fix new miri errors.

@Nahor
Copy link
Contributor Author

Nahor commented Mar 19, 2024

I fixed the Miri error by removing the unused structs since it doesn't look like they have ever been used.

@Nahor
Copy link
Contributor Author

Nahor commented Mar 19, 2024

... and mark the unused structs in the tests as dead_code


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

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.

- Fix a panic when a 0 length span covers the end of a document
- Fix incorrect `line_count`
- Add new unit tests and complete existing ones
- Improve readability
…ent line" and "just the span"

Fixes zkat#355

Change the number of context lines from usize to Option<usize> to allow
choosing between "just the span" (as implemented previously when using
0 context line) using `None`, and displaying the error line without
extra context (not possible before) using `Some(0)`.
- Mark as dead_code the structs uses in testing
- Remove unused code inherited from Eyre for converting `Option` into
  `Result`
A zero-length span at a line boundary can be associated either with the
previous or the next line. Current code prefer to use the next line,
however there isn't always a "next line" (end of source, or "no context"
configuration).
There is also the extra-special case of an empty document which has no
"next line" but also no "previous line".

This commit adds an empty newline at the end of a document if appropriate
so that there is a "next line", or use the "previous line"
@Nahor Nahor changed the title Zero length span without context lines Zero length span Mar 22, 2024
@Nahor
Copy link
Contributor Author

Nahor commented Mar 22, 2024

I don't know why the CI got aborted, I'm guessing because it failed on my fork. There it failed because of an update of the backtrace crate (rust-lang/backtrace-rs#597).

@workingjubilee
Copy link

Sorry about that, backtrace 0.3.71 is out now!
Though uhh Windows backtraces may be... interesting, still, on the current stable. Sorry about that!

@Nahor
Copy link
Contributor Author

Nahor commented Mar 23, 2024

FYI, my branch has passed the checks with backtrace 0.3.71

@zkat zkat added the breaking A semver-major breaking change label Mar 27, 2024
@zkat
Copy link
Owner

zkat commented Mar 27, 2024

I'll have to think about this one for a bit: it's a pretty significant API breakage, and I'm trying to limit things that change the API this significantly. The only thing that I know I want to change about miette that would be notably semver-breaking is that I have some plans for #242 that might involve replacing/deprecating Report.

@Nahor
Copy link
Contributor Author

Nahor commented Mar 28, 2024

Not the most significant API change I was thinking about 😜. A bigger one was to change the SpanContents::data to an array of lines, but I haven't dared yet 😓

@zkat
Copy link
Owner

zkat commented Mar 28, 2024

Yeah there's a certain extent to which, considering how widely used Miette is, it needs to be "mostly done", and we don't really have the ability to make major changes to it. It's ok to make "breaking" changes that only affect the small minority of folks who use certain niche APIs, I think, but the upgrade path for 99% of people should be clean, even across semver-major boundaries. It can otherwise be a lot of work to upgrade.

@Nahor
Copy link
Contributor Author

Nahor commented Mar 28, 2024

Agreed, which is why I hesitated doing the "vector of lines", and why I chose to add with_opt_context_line rather than fix with_context_lines for the Option<usize> change.

And in that regard, I don't think this Option<usize> is a big change (I'm biased of course). For most people, nothing changes. Pure "users" (those who don't impl anything) won't be affected beside a recompile.

The main group of affected people would be the one having their own SourceCode implementation (it's hard to know how big that group is though, so I don't know if it's a minority or not). I think most implementations would be trivial to fix since it's not really a new feature is "just" a matter of renaming 0 to None and extending non-zero case to also cover 0. As an example, see my own fix for source_impl.rs, which is just a couple of is_some and unwrap_or (although it relies on some big prior cleanup I did for other unrelated reasons, see PR #354).

Another group of affected people would be the one implementing their own Report, but that group should be very small, and quite likely, the change should be very trivial too (see my changes to GraphicalReportHandler and NarratableReportHandler).

Maybe to you the change looks bigger to you than it to you is to you because, as mentioned earlier, this PR also includes some unrelated commits (PR #354) due to the lack of stacked PR support in GitHub? See commit d63b2bb for what actually changed to support Option<usize>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A semver-major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants