Don't print "private fields" on empty tuple structs #118192

Kyuuhachi · 2023-11-23T01:03:38Z

While working on this I also noticed that empty struct variants are also rendered rather awkwardly. I'll make another issue for that, since I don't know what the correct rendering would be.

rustbot · 2023-11-23T01:03:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jsha (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

compiler-errors

As for the changes, this looks correct.

fmease

Thanks for fixing this!

This PR is almost good to go, I only have some suggestions for the added test: I'd turn the “absence checks” (@!has) into “presence checks” (@has) since they're more robust and future-proof. Furthermore, I wonder if the tests Hidden and Private are useful since we already have tests/rustdoc/private-fields-tuple-struct.rs for example.

fmease · 2023-11-23T08:16:13Z

tests/rustdoc/issue-118180-empty-tuple-struct.rs

@@ -0,0 +1,19 @@
+// @has issue_118180_empty_tuple_struct/enum.Enum.html
+pub enum Enum {
+    // @!has - '//*[@id="variant.Empty"]//h3' '/* private fields */'


Suggested change

// @!has - '//*[@id="variant.Empty"]//h3' '/* private fields */'

// @has - '//*[@id="variant.Empty"]//h3' 'Empty()'

fmease · 2023-11-23T08:16:28Z

tests/rustdoc/issue-118180-empty-tuple-struct.rs

+}
+
+// @has issue_118180_empty_tuple_struct/struct.Empty.html
+// @!has - '//pre/code' '/* private fields */'


Suggested change

// @!has - '//pre/code' '/* private fields */'

// @has - '//pre/code' 'Empty()'

Kyuuhachi · 2023-11-23T12:18:14Z

Thanks for the feedback!

Presence rather than absence: sure. I think both are more future-proof in their own way, but if you think this way is better, fine with me.

As for the tests, I figured these would be edge cases that might plausibly be affected by a potential fix. But you are right — any such issues should be caught by the rest of the suite.

fmease

Thanks! One last thing, could you maybe squash the commits? Since there aren't that many changes?

fmease · 2023-11-23T14:57:12Z

@!has is alright but imagine if we were to change /* private fields */ to /* hidden fields */ or something in this vein, then this test would become absolutely useless instead of continuing to test the right thing.

Test for presence rather than absence Remove redundant tests Issues in those parts will likely be caught by other parts of the test suite.

Kyuuhachi · 2023-11-23T15:06:25Z

Sure. I think this is how I do that, right? (git rebase -i HEAD~3; git push --force-with-lease)

fmease · 2023-11-23T17:10:05Z

@bors r+ rollup

bors · 2023-11-23T17:10:07Z

📌 Commit a21d771 has been approved by fmease

It is now in the queue for this repository.

bors · 2023-11-23T17:41:49Z

⌛ Testing commit a21d771 with merge a4a5c97...

bors · 2023-11-23T19:44:04Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing a4a5c97 to master...

rust-timer · 2023-11-23T21:12:37Z

Finished benchmarking commit (a4a5c97): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.283s -> 675.669s (-0.09%)
Artifact size: 313.72 MiB -> 313.71 MiB (-0.00%)

rustbot assigned jsha Nov 23, 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 Nov 23, 2023

compiler-errors approved these changes Nov 23, 2023

View reviewed changes

Kyuuhachi mentioned this pull request Nov 23, 2023

rustdoc: How to render struct variants with no visible fields? #118195

Closed

fmease assigned fmease and unassigned jsha Nov 23, 2023

fmease requested changes Nov 23, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2023

fmease approved these changes Nov 23, 2023

View reviewed changes

Don't print "private fields" on empty tuple structs

a21d771

Test for presence rather than absence Remove redundant tests Issues in those parts will likely be caught by other parts of the test suite.

Kyuuhachi force-pushed the issue-118180 branch from b0f2721 to a21d771 Compare November 23, 2023 15:04

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2023

bors merged commit a4a5c97 into rust-lang:master Nov 23, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 23, 2023

This was referenced Nov 23, 2023

rustdoc: migrate item_struct to an Askama template #111430

Open

[rustdoc] Add highlighting for comments in items declaration #117869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't print "private fields" on empty tuple structs #118192

Don't print "private fields" on empty tuple structs #118192

Kyuuhachi commented Nov 23, 2023

rustbot commented Nov 23, 2023

compiler-errors left a comment

fmease left a comment

fmease Nov 23, 2023

fmease Nov 23, 2023

Kyuuhachi commented Nov 23, 2023

fmease left a comment

fmease commented Nov 23, 2023 •

edited

Loading

Kyuuhachi commented Nov 23, 2023

fmease commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

	// @!has - '//[@id="variant.Empty"]//h3' '/ private fields */'
	// @has - '//*[@id="variant.Empty"]//h3' 'Empty()'

	// @!has - '//pre/code' '/* private fields */'
	// @has - '//pre/code' 'Empty()'

Don't print "private fields" on empty tuple structs #118192

Don't print "private fields" on empty tuple structs #118192

Conversation

Kyuuhachi commented Nov 23, 2023

rustbot commented Nov 23, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

fmease left a comment

Choose a reason for hiding this comment

fmease Nov 23, 2023

Choose a reason for hiding this comment

fmease Nov 23, 2023

Choose a reason for hiding this comment

Kyuuhachi commented Nov 23, 2023

fmease left a comment

Choose a reason for hiding this comment

fmease commented Nov 23, 2023 • edited Loading

Kyuuhachi commented Nov 23, 2023

fmease commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

fmease commented Nov 23, 2023 •

edited

Loading