Strip impl if not re-exported and is doc(hidden) #113574

GuillaumeGomez · 2023-07-11T14:34:11Z

…name, r=notriddle Allow to have `-` in rustdoc-json test file name I extracted this commit from rust-lang#113574. When I added the test, it kept saying that the JSON file couldn't be found. After investigating for a while, I discovered that we were expecting files to always use `_`, which is quite bad. So I added support for `-` in file names. r? `@notriddle`

tests/rustdoc-json/reexport/issue-112852-dangling-trait-impl-id.rs

aDotInTheVoid · 2023-07-12T21:52:18Z

What else is needed to close #112852? Just tests?

GuillaumeGomez · 2023-07-12T22:02:57Z

Yes, more tests.

GuillaumeGomez · 2023-07-12T22:08:24Z

Applied comments.

aDotInTheVoid · 2023-07-12T22:15:21Z

Applied comments.

IDK why GitHub highlighted it differently, but I also suggested adding // @!has "$.index[*][?(@.inner.impl)]"

Yes, more tests.

Could they be added now? This code contains logic (that AFAIKT is right) for handling reexports, but if it's wrong, I'd like to know now, and not when doing a follow-up PR to add tests.

GuillaumeGomez · 2023-07-12T22:30:43Z

Sure I don't mind. I just have no idea what tests to add. If you have ideas, please don't hesitate. :)

…name, r=notriddle Allow to have `-` in rustdoc-json test file name I extracted this commit from rust-lang#113574. When I added the test, it kept saying that the JSON file couldn't be found. After investigating for a while, I discovered that we were expecting files to always use `_`, which is quite bad. So I added support for `-` in file names. r? ``@notriddle``

aDotInTheVoid · 2023-07-13T19:51:06Z

Sure I don't mind. I just have no idea what tests to add. If you have ideas, please don't hesitate. :)

See here

When this is fixed, we should add tests for these 2 cases, them with a pub use of the struct, them with --document-hidden-items, and them with --document-hidden-items and a pub use.

GuillaumeGomez · 2023-07-14T14:47:36Z

I added two more tests. To fix the -3 one, I had to actually fix the --document-hidden-items option in rustdoc which was handled very poorly.

GuillaumeGomez · 2023-07-14T15:56:11Z

Fixed the CI errors.

GuillaumeGomez · 2023-07-17T12:02:35Z

Considering it got completely away from JSON (because of --document-hidden-items), I'll set @notriddle as reviewer.

r? @notriddle

notriddle · 2023-07-17T20:24:13Z

The #[doc(hidden)] changes seem reasonable to me.

notriddle · 2023-07-17T20:26:55Z

tests/rustdoc-json/impls/issue-112852-dangling-trait-impl-id.rs

+#![no_core]
+
+// @!has "$.index[*][?(@.name == 'HiddenPubStruct')]"
+// @!has "$.index[*][?(@.inner.impl)]"


!has tests are fragile when the schema changes. The ideal way to write it would be a version that has two structs, and asserts that exactly one shows up.

// @count "$.index[*][?(@.inner.impl)]" 1 // @!has "$.index[*][?(@.name == 'HiddenPubStruct')]" // @has "$.index[*][?(@.name == 'NotHiddenPubStruct')]" // @has "$.index[*][?(@.name=='PubTrait')]" pub trait PubTrait {} #[doc(hidden)] pub mod hidden { pub struct HiddenPubStruct; impl crate::PubTrait for HiddenPubStruct {} } pub mod not_hidden { pub struct NotHiddenPubStruct; impl crate::PubTrait for HiddenPubStruct {} }

That's still not ideal (I really don't like @!has) but in JSON what you suggested is already much better. Going to update.

GuillaumeGomez · 2023-07-17T21:37:31Z

Updated!

aDotInTheVoid · 2023-07-17T22:22:01Z

@bors r=aDotInTheVoid,notriddle

bors · 2023-07-17T22:22:03Z

📌 Commit c132cdb has been approved by aDotInTheVoid,notriddle

It is now in the queue for this repository.

bors · 2023-07-18T02:47:06Z

⌛ Testing commit c132cdb with merge ec362f0...

bors · 2023-07-18T04:30:43Z

☀️ Test successful - checks-actions
Approved by: aDotInTheVoid,notriddle
Pushing ec362f0 to master...

rust-timer · 2023-07-18T07:45:25Z

Finished benchmarking commit (ec362f0): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	0.6%	[0.5%, 0.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.5%	[1.1%, 5.3%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[1.1%, 5.3%]	7

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: 659.727s -> 657.921s (-0.27%)

…ms-test, r=notriddle Add tests for `--document-hidden-items` option Since `--document-hidden-items` was greatly fixed/improved in rust-lang#113574, thought it might be worth adding some more tests for it to prevent new regressions. As for the first commit, it allows to go from: ``` Traceback (most recent call last): File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 706, in <module> check(sys.argv[1], get_commands(rust_test_path)) File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 689, in check for c in commands: File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 274, in get_commands args = shlex.split(args) File "/usr/lib/python3.10/shlex.py", line 315, in split return list(lex) File "/usr/lib/python3.10/shlex.py", line 300, in __next__ token = self.get_token() File "/usr/lib/python3.10/shlex.py", line 109, in get_token raw = self.read_token() File "/usr/lib/python3.10/shlex.py", line 191, in read_token raise ValueError("No closing quotation") ValueError: No closing quotation ``` to: ``` Traceback (most recent call last): File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 708, in <module> check(sys.argv[1], get_commands(rust_test_path)) File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 691, in check for c in commands: File "/home/imperio/rust/rust/src/etc/htmldocck.py", line 278, in get_commands raise Exception("line {}: {}".format(lineno + 1, exc)) from None Exception: line 57: No closing quotation ``` Having the line where the error occurred is quite useful. r? `@notriddle`

rustbot assigned aDotInTheVoid Jul 11, 2023

rustbot added A-rustdoc-json Area: Rustdoc JSON backend 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 Jul 11, 2023

GuillaumeGomez mentioned this pull request Jul 12, 2023

Allow to have - in rustdoc-json test file name #113613

Merged

aDotInTheVoid reviewed Jul 12, 2023

View reviewed changes

tests/rustdoc-json/reexport/issue-112852-dangling-trait-impl-id.rs Outdated Show resolved Hide resolved

aDotInTheVoid mentioned this pull request Jul 12, 2023

Rustdoc JSON: Invalid output for core #106435

Open

10 tasks

aDotInTheVoid reviewed Jul 12, 2023

View reviewed changes

tests/rustdoc-json/reexport/issue-112852-dangling-trait-impl-id.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the rustdoc-json-strip-hidden-impl branch from a776cf2 to 7e50c9e Compare July 12, 2023 22:08

GuillaumeGomez force-pushed the rustdoc-json-strip-hidden-impl branch from 7e50c9e to 1380d21 Compare July 12, 2023 22:31

GuillaumeGomez added 2 commits July 14, 2023 16:45

Strip impl if not re-exported and is doc(hidden)

a5e0ca9

Add regression test for rust-lang#112852

449cc65

GuillaumeGomez force-pushed the rustdoc-json-strip-hidden-impl branch from 1380d21 to 1fd57ff Compare July 14, 2023 14:46

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustdoc-json-strip-hidden-impl branch from 1fd57ff to 5a7ddc9 Compare July 14, 2023 14:50

This comment has been minimized.

Sign in to view

GuillaumeGomez added 2 commits July 14, 2023 17:25

Correctly handle --document-hidden-items

ab80b36

Add more tests for not-reexported impl

2861a56

GuillaumeGomez force-pushed the rustdoc-json-strip-hidden-impl branch from 5a7ddc9 to 2861a56 Compare July 14, 2023 15:25

rustbot assigned notriddle and unassigned aDotInTheVoid Jul 17, 2023

notriddle approved these changes Jul 17, 2023

View reviewed changes

notriddle requested changes Jul 17, 2023

View reviewed changes

Improve issue-112852 tests

c132cdb

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 Jul 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 18, 2023

bors merged commit ec362f0 into rust-lang:master Jul 18, 2023

rustbot added this to the 1.73.0 milestone Jul 18, 2023

bors mentioned this pull request Jul 18, 2023

Fix invalid display of inlined re-export when both local and foreign items are inlined #113785

Merged

GuillaumeGomez deleted the rustdoc-json-strip-hidden-impl branch July 18, 2023 08:37

GuillaumeGomez mentioned this pull request Jul 19, 2023

Add tests for --document-hidden-items option #113857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip impl if not re-exported and is doc(hidden) #113574

Strip impl if not re-exported and is doc(hidden) #113574

GuillaumeGomez commented Jul 11, 2023

aDotInTheVoid commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

aDotInTheVoid commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

aDotInTheVoid commented Jul 13, 2023

GuillaumeGomez commented Jul 14, 2023

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jul 14, 2023

GuillaumeGomez commented Jul 17, 2023

notriddle commented Jul 17, 2023

notriddle Jul 17, 2023

GuillaumeGomez Jul 17, 2023

GuillaumeGomez commented Jul 17, 2023

aDotInTheVoid commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 18, 2023

bors commented Jul 18, 2023

rust-timer commented Jul 18, 2023

Strip impl if not re-exported and is doc(hidden) #113574

Strip impl if not re-exported and is doc(hidden) #113574

Conversation

GuillaumeGomez commented Jul 11, 2023

aDotInTheVoid commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

aDotInTheVoid commented Jul 12, 2023

GuillaumeGomez commented Jul 12, 2023

aDotInTheVoid commented Jul 13, 2023

GuillaumeGomez commented Jul 14, 2023

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jul 14, 2023

GuillaumeGomez commented Jul 17, 2023

notriddle commented Jul 17, 2023

notriddle Jul 17, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jul 17, 2023

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 17, 2023

aDotInTheVoid commented Jul 17, 2023

bors commented Jul 17, 2023

bors commented Jul 18, 2023

bors commented Jul 18, 2023

rust-timer commented Jul 18, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size