-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[rustdoc] Don't try to print the value of a non-primitive const #150629
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
base: main
Are you sure you want to change the base?
Conversation
|
|
|
@GuillaumeGomez are you free to take a look, maybe? |
|
NB: We might want to cease calling More concretely, for provided assoc consts we should just print |
|
Would you be interested in spinning up such an alternative PR? |
Yes! Not sure when I'll get to it, but I'll be happy to take a look. Thanks! |
| @@ -0,0 +1,18 @@ | |||
| #![crate_name = "foo"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add a comment explaining what this is testing (we should maybe enforce that in all tests).
|
Fix looks good to me but my knowledge in this part of the code is quite limited so letting @fmease handle it. ;) |
Hey @fmease, I'm messing around with this, and want to make sure you've taken the consequences into account: There are probably more examples even in the STD docs, but this is one that I found quite quickly: https://doc.rust-lang.org/stable/std/primitive.f32.html#associatedconstant.INFINITY |
|
Thanks for reaching out! Yes, indeed, those are the consequences unfortunately. However note that prior to PR #95316 (2022) we didn't show anything; now we would at least still show simple literals. If you think about it, starting to eagerly evaluate associated constants (even in generic contexts) was technically speaking a correctness regression. It's reasonable that users expect assoc consts to be evaluated post-mono & only when referenced just like during normal compilation. Currently we're rejecting valid patterns (like setting an assoc const to I knew about this issue for quite a while but I never got around to fixing it the way I'm proposing now because I've always hoped that we'd introduce new "control" |
b127b9f to
d03698f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@fmease took initiative and decided to try and also print marking as draft since it's not ready for proper review, but I would like to see if you like the direction this is going, specifically - if you could review the changes to the tests to see that this is what you meant. |
|
BTW, |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as resolved.
This comment was marked as resolved.
bfe62e7 to
d6018cf
Compare
| let tcx = cx.tcx(); | ||
| render_attributes_in_code(w, it, "", cx)?; | ||
|
|
||
| let val = fmt::from_fn(|f| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we could continue using c.value() for free const items that don't have non-lifetime (generic) parameters since rustc evaluates them already anyway. 🤷 As I've said in a bunch of comments, we definitely want to stop evaluating associated const items by default.
However, if you consider it not worth the complexity we can leave like this for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry about that, managed to get a bit lost on the way :)
Let me try to implement it and we'll see just how complex it ends up.
r? @fmease maybe? (feel free to re-assign, ofc 😁)
Pretty new to these parts of the code, so I have no idea if this is an acceptable fix.
Fixes #150312