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

core: Make Debug impl of raw pointers print metadata if present #135080

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Enselic
Copy link
Member

@Enselic Enselic commented Jan 3, 2025

Make Rust pointers appear less magic by including metadata information in their Debug output.

This does not break Rust stability guarantees because Debug impl are explicitly exempted from stability:
https://doc.rust-lang.org/std/fmt/trait.Debug.html#stability

Stability

Derived Debug formats are not stable, and so may change with future Rust versions. Additionally, Debug implementations of types provided by the standard library (std, core, alloc, etc.) are not stable, and may also change with future Rust versions.

Note that a regression test is added as a separate commit to make it clear what impact the last commit has on the output.

Closes #128684 because the output of that code now becomes:

thread 'main' panicked at src/main.rs:5:5:
assertion `left == right` failed
  left: Pointer { addr: 0x7ffd45c6fc6b, metadata: 5 }
 right: Pointer { addr: 0x7ffd45c6fc6b, metadata: 3 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 3, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 3, 2025

r? libs
lol

@rustbot rustbot assigned Amanieu and unassigned BoxyUwU Jan 3, 2025

use std::fmt::Display;

fn main() {
Copy link
Member

Choose a reason for hiding this comment

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

can you make this a core unit test instead?

Copy link
Member Author

@Enselic Enselic Jan 3, 2025

Choose a reason for hiding this comment

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

Sure no problem, I'll fix whatever my reviewer(s) ask me to do.

Would you mind elaborating a bit on why though? Do you prefer it as a core unit test because then the test can be run with a stage0 compiler, maybe? What I like about having it as a UI test is that --bless works when the output change.

Edit: Ok I can see we already have a bunch of related tests at library/core/tests/fmt/mod.rs, and one of them fails now. So I'll move my test there too.

Copy link
Member Author

@Enselic Enselic Jan 4, 2025

Choose a reason for hiding this comment

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

Done. I had to (with the current test code) add "regex" as an allowed library (dev-)dependency, but it is already present as an allowed rustc dependency, so it's fine I think.

@rust-log-analyzer

This comment has been minimized.

@Enselic Enselic force-pushed the debug-ptr-metadata branch from b628909 to ee8187a Compare January 3, 2025 19:56
@rust-log-analyzer

This comment has been minimized.

Enselic and others added 3 commits January 4, 2025 10:04
Because `.as_ptr()` changes the type of the pointer (e.g. `&[u8]`
becomes `*const u8` instead of `*const [u8]`), and it can't be expected
that different types will be formatted the same way.
Make Rust pointers less magic by including metadata information in their
`Debug` output.

This does not break Rust stability guarantees because `Debug` output is
explicitly exempted from stability:
https://doc.rust-lang.org/std/fmt/trait.Debug.html#stability

Co-authored-by: Lukas <26522220+lukas-code@users.noreply.github.com>
@Enselic Enselic force-pushed the debug-ptr-metadata branch from ee8187a to 4c641b1 Compare January 4, 2025 09:20
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2025

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug implementation of fat pointers doesn't show metadata
7 participants