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

Reduce redundancy in our error messages #2200

Open
RalfJung opened this issue Jun 6, 2022 · 7 comments
Open

Reduce redundancy in our error messages #2200

RalfJung opened this issue Jun 6, 2022 · 7 comments
Labels
A-diagnostics errors and warnings emitted by miri C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2022

Our errors currently print the same message twice:

error: Undefined Behavior: pointer arithmetic failed: alloc1655 has size 4, so pointer to 1 byte starting at offset -1 is out-of-bounds
   --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:455:18
    |
455 |         unsafe { intrinsics::offset(self, count) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pointer arithmetic failed: alloc1655 has size 4, so pointer to 1 byte starting at offset -1 is out-of-bounds
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

That is unnecessarily redundant redundant. So maybe we could avoid duplication, e.g. by changing the above output to

error: Undefined Behavior: pointer arithmetic failed
   --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/const_ptr.rs:455:18
    |
455 |         unsafe { intrinsics::offset(self, count) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ alloc1655 has size 4, so pointer to 1 byte starting at offset -1 is out-of-bounds
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

This will require

  • changing rustc: the InterpError type needs separate methods to compute the "title" and "text" of a message
  • adjusting our test suite: many ERROR annotations will probably fail if the error details no longer appear in the "error:" message. On a case-by-case basis, we should either change this to just match on what the error message still says (we do have ui tests after all, so if the details change in unexpected ways we will notice), or add a second pattern to also match some key part of the details.
@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement E-good-first-issue A good way to start contributing, mentoring is available A-diagnostics errors and warnings emitted by miri labels Jun 6, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jun 6, 2022

What about the case where no source is available for the error origin?

@RalfJung
Copy link
Member Author

RalfJung commented Jun 6, 2022

You are concerned about something like rust-lang/rust#97699? IMO it is a bug in the rustc diagnostic printing that it entirely omits information attached to spans where the source is missing. We might have to work around that somehow on our side.

But Miri requires rust-src to be installed, so I am not sure if this can ever happen?

@RalfJung
Copy link
Member Author

If we have a "title" and "text" for errors, that could also help const-eval errors in rustc itself which currently don't even state things like "you caused UB".

@RalfJung
Copy link
Member Author

RalfJung commented Aug 7, 2022

While at it we should probably also let errors have a "help" output for extra text to print after the main diagnostic (e.g. for things like #2074).

@RalfJung
Copy link
Member Author

RalfJung commented Nov 2, 2022

Also this can be improved:

throw_ub_format!(
"atomic operations cannot be performed on read-only memory\n\
many platforms require atomic read-modify-write instructions to be performed on writeable memory, even if the operation fails \
(and is hence nominally read-only)\n\
some platforms implement (some) atomic loads via compare-exchange, which means they do not work on read-only memory; \
it is possible that we could have an exception permitting this for specific kinds of loads\n\
please report an issue at <https://github.com/rust-lang/miri/issues> if this is a problem for you"
);

@RalfJung RalfJung removed the E-good-first-issue A good way to start contributing, mentoring is available label Nov 20, 2023
@RalfJung
Copy link
Member Author

Now that we have translatable diagnostics, this is probably a lot more work... we can't just have a "title" function on InterpError that returns the "caption" of the error, we have to also add fluent IDs for all of them.

Or for now we just have these not be translatable and we wait until the translatable diagnostics are less painful to use.

@RalfJung
Copy link
Member Author

Hm, maybe we don't actually have to do this. In rustc we already avoid this redundancy by simply just putting "evaluation of constant value failed" or "it is undefined behavior to use this value" in the error title, so Miri could do the same -- just say whether it is UB / Unsupported in the title, but the rest in the span.

The main effort then is updating all our tests to add new annotations that check that the span shows what it should. (Not sure if ui_test supports that yet, #2483 is still open so I think the answer is no.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics errors and warnings emitted by miri C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

No branches or pull requests

2 participants