Modifies the rendering of constant values in rustdoc. #112167

sladyn98 · 2023-06-01T06:52:08Z

Previously, non-literal expressions (like 50 + 50) would be printed alongside their value as a comment (like = _; // 100i32). This approach often resulted in confusing documentation, especially for complex expressions.

To improve clarity, this commit changes the rendering to display the evaluated value of the expression directly (like = 100i32;) unless the constant is a literal expression. The original expression is still included as a comment if it
doesn't match the evaluated value.

These changes aim to improve the readability and usefulness of constant value documentation generated by rustdoc.

rustbot · 2023-06-01T06:52:15Z

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

sladyn98 · 2023-06-01T06:52:33Z

CC @fmease

fmease · 2023-06-01T07:08:12Z

src/librustdoc/html/render/print_item.rs

-            if let Some(value) = &value {
-                let value_lowercase = value.to_lowercase();
-                let expr_lowercase = expr.to_lowercase();
+        let value_lowercase = value.as_ref().map(|s| s.to_lowercase());


In your earlier PR I meant to suggest removing the entire branch including the body (the // business) not just the if-condition, sorry if that wasn't clear. This here still prints 3i32 // 3i32 for 1 + 2 if I'm not mistaken.

Correct that makes sense, sorry if I might not have understood it earlier.

13: @!hasraw check failed `PATTERN` did not match // @!hasraw show_const_contents/constant.CONST_I32_HEX.html ';' 50: @hasraw check failed `PATTERN` did not match // @hasraw show_const_contents/constant.PI.html '= 3.14159265358979323846264338327950288f32;' 51: @hasraw check failed `PATTERN` did not match // @hasraw show_const_contents/constant.PI.html '; // 3.14159274f32'

These tests still seem to be failing, should the code be suggesting something else, I am a bit confused on what the correct comment should be here

fmease · 2023-06-05T13:40:02Z

Wondering if you need help with the failing test or is everything all good?

sladyn98 · 2023-06-05T17:53:15Z

@fmease Yeah I am not sure how to actually make it pass : )

fmease · 2023-06-06T19:05:25Z

Yeah I am not sure how to actually make it pass : )

Here's a diff:

diff --git a/tests/rustdoc/show-const-contents.rs b/tests/rustdoc/show-const-contents.rs
index 96a47218e0a..28a63de4b75 100644
--- a/tests/rustdoc/show-const-contents.rs
+++ b/tests/rustdoc/show-const-contents.rs
@@ -2,53 +2,43 @@
 // documentation.
 
 // @hasraw show_const_contents/constant.CONST_S.html 'show this'
-// @!hasraw show_const_contents/constant.CONST_S.html '; //'
 pub const CONST_S: &'static str = "show this";
 
 // @hasraw show_const_contents/constant.CONST_I32.html '= 42;'
-// @!hasraw show_const_contents/constant.CONST_I32.html '; //'
 pub const CONST_I32: i32 = 42;
 
 // @hasraw show_const_contents/constant.CONST_I32_HEX.html '= 0x42;'
-// @!hasraw show_const_contents/constant.CONST_I32_HEX.html ';'
 pub const CONST_I32_HEX: i32 = 0x42;
 
 // @hasraw show_const_contents/constant.CONST_NEG_I32.html '= -42;'
-// @!hasraw show_const_contents/constant.CONST_NEG_I32.html '; //'
 pub const CONST_NEG_I32: i32 = -42;
 
 // @hasraw show_const_contents/constant.CONST_EQ_TO_VALUE_I32.html '= 42i32;'
-// @!hasraw show_const_contents/constant.CONST_EQ_TO_VALUE_I32.html '// 42i32'
 pub const CONST_EQ_TO_VALUE_I32: i32 = 42i32;
 
 // @hasraw show_const_contents/constant.CONST_CALC_I32.html '= 43i32'
 pub const CONST_CALC_I32: i32 = 42 + 1;
 
 // @!hasraw show_const_contents/constant.CONST_REF_I32.html '= &42;'
-// @!hasraw show_const_contents/constant.CONST_REF_I32.html '; //'
 pub const CONST_REF_I32: &'static i32 = &42;
 
 // @hasraw show_const_contents/constant.CONST_I32_MAX.html '= 2_147_483_647i32'
 pub const CONST_I32_MAX: i32 = i32::MAX;
 
 // @!hasraw show_const_contents/constant.UNIT.html '= ();'
-// @!hasraw show_const_contents/constant.UNIT.html '; //'
 pub const UNIT: () = ();
 
 pub struct MyType(i32);
 
 // @!hasraw show_const_contents/constant.MY_TYPE.html '= MyType(42);'
-// @!hasraw show_const_contents/constant.MY_TYPE.html '; //'
 pub const MY_TYPE: MyType = MyType(42);
 
 pub struct MyTypeWithStr(&'static str);
 
 // @!hasraw show_const_contents/constant.MY_TYPE_WITH_STR.html '= MyTypeWithStr("show this");'
-// @!hasraw show_const_contents/constant.MY_TYPE_WITH_STR.html '; //'
 pub const MY_TYPE_WITH_STR: MyTypeWithStr = MyTypeWithStr("show this");
 
-// @hasraw show_const_contents/constant.PI.html '= 3.14159265358979323846264338327950288f32;'
-// @hasraw show_const_contents/constant.PI.html '; // 3.14159274f32'
+// @hasraw show_const_contents/constant.PI.html '= 3.14159274f32;'
 pub use std::f32::consts::PI;
 
 // @hasraw show_const_contents/constant.MAX.html '= 2_147_483_647i32'

As seen in the diff I've removed most @!hasraw checks not necessarily because they were required to make the test pass (there were only 3 test failures) but since they've become obsolete (technically only CONST_I32_HEX & PI needed to be modified).

fmease

Looks good from my side. @GuillaumeGomez Do the broader changes still make sense to you (removal of //, showing the actual value of PI etc.)? TTBOMK, they are a natural consequence of fixing this FIXME. Edit: Maybe the commits should be squashed though?

GuillaumeGomez · 2023-06-07T11:45:04Z

I think it's fine. Just in case, does it look ok to you @jsha, @Manishearth ?

Manishearth · 2023-06-07T11:53:25Z

Honestly i think this depends on the constant: in some cases the source of the constant is more important (especially when it's platform-dependent!), in others the value is more important. I find the current display of expression AND value to be quite useful, and focusing on the expression is important because the value may change!

So I don't think this change is that great as-is. I would be open to someone RFCing an attribute that lets people control this behavior.

I'm not strongly of this behavior, I am strongly of the behavior that for non-literal constant the expression must be shown somewhere (or we show neither the expression nor the value: otherwise just a value may be misleading), so I'm willing to be convinced.

fmease · 2023-06-07T14:15:38Z

Manishearth's view is quite important and I agree with him to a not insignificant extend. We've had extensive discussion about this in the past. Intent (unevaluated expression) vs. representation (evaluated expression), e.g. on Zulip, topic new const display, and on my blocked PR #99688. Initially I was a big proponent of evaluating consts but over time I've shifted my opinion slightly and I'm no longer sure what the best way forward is. That's why I haven't written any RFC yet (as promised in #99688), though I also feel a bit guilty about it.

Looking at other parts of rustdoc, we also don't normalize assoc tys for example. There's -Znormalize-docs, maybe we could gate this PR's behavior behind it?? As a temporary solution until we intro a new #[doc] attribute or similar.

Sorry sladyn98 that you stumbled into this controversial topic. Edit: Before seeing the actual changes to the const-value-display test, I didn't fully realize we stepped into this territory again 🤦.

Manishearth · 2023-06-07T14:21:47Z

Yeah I'm extremely in favor of a new attribute here because I think it's important to give people control over this.

sladyn98 · 2023-06-07T17:47:45Z

No worries let me know if you want to close this PR in favor of another one, or need any more help adding stuff. Thanks

Manishearth · 2023-06-08T04:29:46Z

I think it's probably fine to close this PR for now and instead join the Zulip discussion led by @fmease here

sladyn98 · 2023-06-08T07:10:06Z

Closed

Implement item_constant_fixme

d6cb5f1

rustbot assigned GuillaumeGomez Jun 1, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 1, 2023

fmease reviewed Jun 1, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

sladyn98 added 2 commits June 1, 2023 00:33

Fix tests

fb95d2e

Commit suggestions

e13e5fc

This comment has been minimized.

Sign in to view

Commit suggestions and fix tests

05d3c6e

sladyn98 requested a review from fmease June 7, 2023 07:56

fmease approved these changes Jun 7, 2023

View reviewed changes

GuillaumeGomez approved these changes Jun 7, 2023

View reviewed changes

sladyn98 closed this Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifies the rendering of constant values in rustdoc. #112167

Modifies the rendering of constant values in rustdoc. #112167

sladyn98 commented Jun 1, 2023

rustbot commented Jun 1, 2023

sladyn98 commented Jun 1, 2023

fmease Jun 1, 2023

sladyn98 Jun 1, 2023

This comment has been minimized.

This comment has been minimized.

fmease commented Jun 5, 2023 •

edited

Loading

sladyn98 commented Jun 5, 2023

fmease commented Jun 6, 2023 •

edited

Loading

fmease left a comment •

edited

Loading

GuillaumeGomez commented Jun 7, 2023

Manishearth commented Jun 7, 2023

fmease commented Jun 7, 2023 •

edited

Loading

Manishearth commented Jun 7, 2023

sladyn98 commented Jun 7, 2023

Manishearth commented Jun 8, 2023

sladyn98 commented Jun 8, 2023

Modifies the rendering of constant values in rustdoc. #112167

Modifies the rendering of constant values in rustdoc. #112167

Conversation

sladyn98 commented Jun 1, 2023

rustbot commented Jun 1, 2023

sladyn98 commented Jun 1, 2023

fmease Jun 1, 2023

Choose a reason for hiding this comment

sladyn98 Jun 1, 2023

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

fmease commented Jun 5, 2023 • edited Loading

sladyn98 commented Jun 5, 2023

fmease commented Jun 6, 2023 • edited Loading

fmease left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 7, 2023

Manishearth commented Jun 7, 2023

fmease commented Jun 7, 2023 • edited Loading

Manishearth commented Jun 7, 2023

sladyn98 commented Jun 7, 2023

Manishearth commented Jun 8, 2023

sladyn98 commented Jun 8, 2023

fmease commented Jun 5, 2023 •

edited

Loading

fmease commented Jun 6, 2023 •

edited

Loading

fmease left a comment •

edited

Loading

fmease commented Jun 7, 2023 •

edited

Loading