Fix rustdoc const computed value #94091

GuillaumeGomez · 2022-02-17T17:00:30Z

Fixes #85088.

It looks like this now (instead of hexadecimal):

r? @oli-obk

rust-highfive · 2022-02-17T17:00:33Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_middle/src/mir/interpret/pointer.rs

RalfJung · 2022-02-17T18:06:02Z

compiler/rustc_middle/src/mir/interpret/pointer.rs

+// all the Miri types.
+impl<Tag: Provenance> fmt::Display for Pointer<Tag> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        Provenance::fmt(self, f)


Instead of duplicating the code, please make one of Display and Debug dispatch to the other (not sure which way makes more sense, but with the existing comment it probably makes most sense to have Debug call Display)

I think it's better to keep both even if duplicated. It allows to have two different formatting depending on what you need like that.

Well, we won't need two different ones, so forwarding makes it easier for future readers to see that they are the same

Ok. Now I have a question: do you to have instead for Debug:

write!(f, "{}", self)

?

I would have assumed fmt::Display::fmt(self, f)

That works too!

It allows to have two different formatting depending on what you need like that.

The issue is that it is entirely unclear which style is used where -- both debug and display end up being user-visible in various situations, and when changing the code it becomes extremely hard to predict which one will change which output. IOW we can't actually meaningfully make them different, currently, since the consequences are so unclear. That is why I entirely removed many of the Display implementations for Miri types -- then at least there is no ambiguity over which kind of formatting is used.

This PR adds back some of those instances which I removed; if that cannot be avoided I won't block it but then we can at least consistently use the pattern of forwarding one to the other.

oli-obk · 2022-02-17T19:53:41Z

compiler/rustc_middle/src/mir/interpret/value.rs

@@ -152,8 +152,8 @@ impl<Tag: Provenance> fmt::Debug for Scalar<Tag> {
 impl<Tag: Provenance> fmt::Display for Scalar<Tag> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
-            Scalar::Ptr(ptr, _size) => write!(f, "pointer to {:?}", ptr),


This causes the ui test fallout. Please make sure the code that prints ui tests uses the debug formatting then

GuillaumeGomez · 2022-02-17T20:25:34Z

@oli-obk: I'm actually wondering: is there any reason why we display in hex instead of decimal? The error messages look simpler to me this way.

RalfJung · 2022-02-17T21:32:01Z

is there any reason why we display in hex instead of decimal? The error messages look simpler to me this way.

For large values, hex becomes more readable - in particular things close to INT_MAX.

Also we use leading zeroes to indicate the size of the value we talk about -- 0x03 and 0x0003 are not the same thing. These are data representations we talk about, which means they are usually very byte-driven, which makes hex often the better representation.

GuillaumeGomez · 2022-02-17T22:19:50Z

Ok. Using _ like rustdoc does for const wouldn't be better in this case if it's for readability? (doesn't fix the prepending 0 thing though)

oli-obk · 2022-02-17T22:44:22Z

These scalars are not necessarily integers. We're printing them as hex, because that's essentially the memory representation. Anything the layout algorithm decides is a Scalar will use this representation.

Also: We can certainly open a discussion on the diagnostics of CTFE failures, but not as a drive by change of something unrelated.

RalfJung · 2022-02-17T22:52:02Z

Using _ like rustdoc does for const wouldn't be better in this case if it's for readability?

Not really -- it's still pretty hard to recognize 0xffff_ffff when written in decimal, even if the thousands are separated via _.

GuillaumeGomez · 2022-02-18T11:41:49Z

It was a nightmare to get through all the changes and find out what was broken so I took the easy way out and simply made a smaller change in rustdoc and in the Debug implementation for Scalar. :)

GuillaumeGomez · 2022-02-18T13:14:06Z

Which broke things in a completely different way...

GuillaumeGomez · 2022-02-18T14:21:44Z

Seems like my fix worked! \o/

RalfJung · 2022-02-18T18:06:49Z

Hm, but that leaves us with 3 ways to print a Scalar, and it also means rustdoc has to break the Scalar abstraction.

I think impl Display for ScalarInt is definitely better than adding a display method. But the interesting question is what to do with Scalar itself -- that one already supports Debug and Display, and they differ in how pointers are rendered. Maybe we should do something similar to ScalarInt, and impl LowerHex for Scalar, to give whoever prints a scalar the choice between hex and decimal output? That means we would have to change the existing users to explicitly opt into hex, but those users should be easy to find if one first replaces the current impl Display for Scalar by impl LowerHex, fixes the fallout, and then adds the new impl Display that uses decimal rendering.

GuillaumeGomez · 2022-02-18T19:15:23Z

Considering the complexity of it, I'd much rather do it in another PR.

RalfJung · 2022-02-18T20:59:54Z

Well, if Rust had private enums then this PR wouldn't even work... quoting from the Scalar docs

These variants would be private if there was a convenient way to achieve that in Rust. Do not match on a Scalar! Use the various to_* methods instead.

GuillaumeGomez · 2022-02-18T22:55:11Z

True but I don't really care about the value inside, just want to display it. I can use to_u128 though to cover every possible case if it's better?

RalfJung · 2022-02-18T23:28:57Z

Then you won't be able to print pointer values. Pointers and integers are very different beasts and neither subsumes the other. That's why we have the display impls in that module...

I don't think changing the current impl Display for Scalar to an impl LowerHex will be a lot of work (./x.py check will literally point you to the places where you have to add a :x), but if you think that is easier then alternatively you could add a fn rustdoc_display to Scalar that uses the impl Display for ScalarInt, with a FIXME describing the plan I sketched above.

GuillaumeGomez · 2022-02-18T23:42:08Z

Sounds like a plan!

GuillaumeGomez · 2022-02-19T13:42:43Z

Updated! I added a FIXME comment too as you asked. Once this is merged, I think I'll give it a try.

oli-obk · 2022-02-19T13:43:23Z

@bors r+

bors · 2022-02-19T13:43:25Z

📌 Commit 296adba has been approved by oli-obk

oli-obk · 2022-02-19T13:43:33Z

@bors rollup

…ed-value, r=oli-obk Fix rustdoc const computed value Fixes rust-lang#85088. It looks like this now (instead of hexadecimal): ![Screenshot from 2022-02-17 17-55-39](https://user-images.githubusercontent.com/3050060/154532115-0f9861a0-406f-4c9c-957f-32bedd8aca7d.png) r? `@oli-obk`

…ed-value, r=oli-obk Fix rustdoc const computed value Fixes rust-lang#85088. It looks like this now (instead of hexadecimal): ![Screenshot from 2022-02-17 17-55-39](https://user-images.githubusercontent.com/3050060/154532115-0f9861a0-406f-4c9c-957f-32bedd8aca7d.png) r? ``@oli-obk``

…ed-value, r=oli-obk Fix rustdoc const computed value Fixes rust-lang#85088. It looks like this now (instead of hexadecimal): ![Screenshot from 2022-02-17 17-55-39](https://user-images.githubusercontent.com/3050060/154532115-0f9861a0-406f-4c9c-957f-32bedd8aca7d.png) r? ```@oli-obk```

…askrgr Rollup of 14 pull requests Successful merges: - rust-lang#93580 (Stabilize pin_static_ref.) - rust-lang#93639 (Release notes for 1.59) - rust-lang#93686 (core: Implement ASCII trim functions on byte slices) - rust-lang#94002 (rustdoc: Avoid duplicating macros in sidebar) - rust-lang#94019 (removing architecture requirements for RustyHermit) - rust-lang#94023 (adapt static-nobundle test to use llvm-nm) - rust-lang#94091 (Fix rustdoc const computed value) - rust-lang#94093 (Fix pretty printing of enums without variants) - rust-lang#94097 (Add module-level docs for `rustc_middle::query`) - rust-lang#94112 (Optimize char_try_from_u32) - rust-lang#94113 (document rustc_middle::mir::Field) - rust-lang#94122 (Fix miniz_oxide types showing up in std docs) - rust-lang#94142 (rustc_typeck: adopt let else in more places) - rust-lang#94146 (Adopt let else in more places) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…RalfJung Implement LowerHex on Scalar to clean up their display in rustdoc Follow-up of rust-lang#94091. r? `@RalfJung`

…RalfJung Implement LowerHex on Scalar to clean up their display in rustdoc Follow-up of rust-lang#94091. r? ``@RalfJung``

…RalfJung Implement LowerHex on Scalar to clean up their display in rustdoc Follow-up of rust-lang#94091. r? ```@RalfJung```

…RalfJung Implement LowerHex on Scalar to clean up their display in rustdoc Follow-up of rust-lang#94091. r? ````@RalfJung````

GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 17, 2022

rust-highfive assigned oli-obk Feb 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2022

RalfJung reviewed Feb 17, 2022

View reviewed changes

compiler/rustc_middle/src/mir/interpret/pointer.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 17, 2022

View reviewed changes

GuillaumeGomez force-pushed the rustdoc-const-computed-value branch from 9702022 to edab15b Compare February 17, 2022 18:54

This comment has been minimized.

Sign in to view

oli-obk reviewed Feb 17, 2022

View reviewed changes

GuillaumeGomez force-pushed the rustdoc-const-computed-value branch from edab15b to 277a678 Compare February 18, 2022 11:36

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustdoc-const-computed-value branch from 277a678 to 04c319a Compare February 18, 2022 13:33

GuillaumeGomez added 2 commits February 19, 2022 14:00

Don't render Const computed values in hexadecimal for Display

c5ce3e1

Add rustdoc test for const computed value

296adba

GuillaumeGomez force-pushed the rustdoc-const-computed-value branch from 04c319a to 296adba Compare February 19, 2022 13:00

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2022

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 12 pull requests #94161

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 12 pull requests #94162

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 11 pull requests #94164

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 14 pull requests #94174

Merged

bors merged commit 9e9cc66 into rust-lang:master Feb 20, 2022

rustbot added this to the 1.61.0 milestone Feb 20, 2022

GuillaumeGomez deleted the rustdoc-const-computed-value branch February 20, 2022 14:08

GuillaumeGomez mentioned this pull request Feb 20, 2022

Implement LowerHex on Scalar to clean up their display in rustdoc #94189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rustdoc const computed value #94091

Fix rustdoc const computed value #94091

GuillaumeGomez commented Feb 17, 2022

rust-highfive commented Feb 17, 2022

RalfJung Feb 17, 2022

GuillaumeGomez Feb 17, 2022

oli-obk Feb 17, 2022

GuillaumeGomez Feb 17, 2022

oli-obk Feb 17, 2022 •

edited

Loading

GuillaumeGomez Feb 17, 2022

RalfJung Feb 17, 2022 •

edited

Loading

This comment has been minimized.

oli-obk Feb 17, 2022

GuillaumeGomez Feb 17, 2022

GuillaumeGomez commented Feb 17, 2022

RalfJung commented Feb 17, 2022 •

edited

Loading

GuillaumeGomez commented Feb 17, 2022

oli-obk commented Feb 17, 2022

RalfJung commented Feb 17, 2022

GuillaumeGomez commented Feb 18, 2022

This comment has been minimized.

GuillaumeGomez commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

GuillaumeGomez commented Feb 19, 2022

oli-obk commented Feb 19, 2022

bors commented Feb 19, 2022

oli-obk commented Feb 19, 2022

Fix rustdoc const computed value #94091

Fix rustdoc const computed value #94091

Conversation

GuillaumeGomez commented Feb 17, 2022

rust-highfive commented Feb 17, 2022

RalfJung Feb 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez Feb 17, 2022

Choose a reason for hiding this comment

oli-obk Feb 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez Feb 17, 2022

Choose a reason for hiding this comment

oli-obk Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez Feb 17, 2022

Choose a reason for hiding this comment

RalfJung Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk Feb 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez Feb 17, 2022

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 17, 2022

RalfJung commented Feb 17, 2022 • edited Loading

GuillaumeGomez commented Feb 17, 2022

oli-obk commented Feb 17, 2022

RalfJung commented Feb 17, 2022

GuillaumeGomez commented Feb 18, 2022

This comment has been minimized.

GuillaumeGomez commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

RalfJung commented Feb 18, 2022

GuillaumeGomez commented Feb 18, 2022

GuillaumeGomez commented Feb 19, 2022

oli-obk commented Feb 19, 2022

bors commented Feb 19, 2022

oli-obk commented Feb 19, 2022

oli-obk Feb 17, 2022 •

edited

Loading

RalfJung Feb 17, 2022 •

edited

Loading

RalfJung commented Feb 17, 2022 •

edited

Loading