Fix impl block in const expr

[GuillaumeGomez]

Fixes #83026.

The problem was that we didn't visit block expressions. Considering how big the walk_expr function is, I decided to instead implement the hir visitor on the struct. It also answers the question which was in a comment for RustdocVisitor: we should have used a visitor instead of our ad-hoc implementation.

Adding this visitor also added some extra checks that weren't present before (check changes in rustdoc-ui tests).

r? @notriddle

[GuillaumeGomez]

I have some doubts about the performance impact of this PR. I have a few ideas on how to improve it if needed (making empty implementations of some of the visitor's methods that we don't need). Anyway, let's check the perf results.

@bors try @rust-timer queue

[bors]

⌛ Trying commit cee8bc90d2d76f83018849fff0064aa1e2b45717 with merge e0e0ee43eb66d9f483bfdd80edd1491942c20545...

[GuillaumeGomez]

@bors try-

[GuillaumeGomez]

In the meantime:

@bors try @rust-timer queue

[bors]

⌛ Trying commit 7ead30705bd3eec016b093b6d81f6c83b4180f7d with merge a981051c73f8ac7e9d89b4d03c9de3c5019c4b6d...

[notriddle]

This patch should fix the invalid doc comment, but I suspect that rustdoc is doing more work now, and as a result, it's going to be a lot slower on code that makes heavy use of nested functions.

diff --git a/src/librustdoc/html/render/write_shared.rs b/src/librustdoc/html/render/write_shared.rs
index 94d8a9feca6..38a9af99568 100644
--- a/src/librustdoc/html/render/write_shared.rs
+++ b/src/librustdoc/html/render/write_shared.rs
@@ -137,7 +137,7 @@ fn collect(path: &Path, krate: &str) -> io::Result<(Vec<String>, Vec<String>)> {
         Ok((ret, krates))
     }
 
-    /// Read a file and return all lines that match the <code>"{crate}":{data},\</code> format,
+    /// Read a file and return all lines that match the <code> "{crate}":{data},\ </code> format,
     /// and return a tuple `(Vec<DataString>, Vec<CrateNameString>)`.
     ///
     /// This forms the payload of files that look like this:

[bors]

💔 Test failed - checks-actions

[GuillaumeGomez]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 26e6345eda0508cb53a0cdbe1907f144a230e054 with merge 3c7dddbc0604180131c7b80915fdff01a092c675...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3c7dddbc0604180131c7b80915fdff01a092c675 (3c7dddbc0604180131c7b80915fdff01a092c675)

[rust-timer]

Finished benchmarking commit (3c7dddbc0604180131c7b80915fdff01a092c675): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +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)	4.9%	[0.6%, 18.6%]	7
Regressions ❌ (secondary)	1.5%	[1.3%, 1.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.9%	[0.6%, 18.6%]	7

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.1%	[1.1%, 3.3%]	4
Regressions ❌ (secondary)	3.7%	[2.1%, 5.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	2.1%	[1.1%, 3.3%]	4

Cycles

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)	8.0%	[4.1%, 16.3%]	4
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	8.0%	[4.1%, 16.3%]	4

[GuillaumeGomez]

That's even worth than I expected... It's a shame but I suppose that I will need to manually go through bodies... It sounds like a terrible idea though. A preference maybe @notriddle ?

[notriddle]

Am I correct that rustdoc should only care about impl blocks nested in const blocks, but not other items?

If so, this problem might be worked around by using separate visitors for const and fn blocks and for modules.

[craterbot]

🎉 Experiment pr-104889 is completed!
📊 6 regressed and 1 fixed (59 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[GuillaumeGomez]

Seems like the remaining ones are all spurious, so this change seems to not badly impact the existing ecosystem. So what do you want to do now? Accept the perf impact and merge or reject it and reimplement the hir visitor?

[notriddle]

@bors r+

[bors]

📌 Commit a954d63 has been approved by notriddle

It is now in the queue for this repository.

[bors]

⌛ Testing commit a954d63 with merge cce9e72...

[bors]

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing cce9e72 to master...

[rust-timer]

Finished benchmarking commit (cce9e72): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	4.9%	[0.5%, 18.4%]	7
Regressions ❌ (secondary)	1.6%	[1.5%, 1.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.9%	[0.5%, 18.4%]	7

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.7%	[1.4%, 3.9%]	2
Regressions ❌ (secondary)	2.6%	[1.6%, 3.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[1.4%, 3.9%]	2

Cycles

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)	7.9%	[4.0%, 16.6%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.9%	[4.0%, 16.6%]	4

[notriddle]

This bug isn’t going to be fixed without some kind of perf regression, because it just has to walk through more of the AST.

[pnkfelix]

Agree with @notriddle

@rustbot label: +perf-regression-triaged