rustdoc: run more HIR validation to mirror rustc #108576

megakorre · 2023-02-28T18:14:57Z

Explanation

While investigating these issues: #107093, #106079
I thought it maybe would be useful to test running rustdoc on all rust files under tests/ui grepping for files that causes any ICEs.
And these are the files I found would cause ICEs.

// These are handled by this fix. 
tests/ui/late-bound-lifetimes/mismatched_arg_count.rs
tests/ui/associated-consts/issue-102335-const.rs
tests/ui/const-generics/generic_const_exprs/issue-102768.rs
tests/ui/const-generics/const-arg-type-arg-misordered.rs
tests/ui/generic-associated-types/parse/trait-path-type-error-once-implemented.rs
tests/ui/typeck/issue-88643.rs
tests/ui/typeck/issue-75889.rs
tests/ui/typeck/issue-83621-placeholder-static-in-extern.rs
// These are not they will still produce a ICE after this change
tests/ui/limits/issue-56762.rs
tests/ui/union/projection-as-union-type-error-2.rs
tests/ui/union/projection-as-union-type-error.rs

I reduces the issues handled by this PR down to the tests added in the PR. That includes the linked issues.
But the 3 files that are not handled I will leave for a future PR.

This PR adds the type_collecting step from hir_analysis::check_crate to the rustdoc typechecks.
It had the following comment on it.

// this ensures that later parts of type checking can assume that items
// have valid types and not error

Adding the check report the same errors as rustc does for these input.
And not ICE when the lint checker walks the HIR or when in the rustdoc::clean pass.

This PR updates the expected errors of some existing rustdoc-ui tests (some now report less errors).
These new reported errors does mirror the errors reported by rustc.

Performance

It does more checking so it will probably regress. We should run @bors try @rust-timer queue and see.

Discussion

Maybe instead of calling a subset of the checks in hir_analysis::check_crate and having comments that say they should be kept in sync. We could instead call check_crate directly and pass in some flag. Maybe check_toplevel_signatures_only or something like that. That flag would have to skip most of the checks in that function tough.

rustbot · 2023-02-28T18:15:04Z

r? @Mark-Simulacrum

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

tests/rustdoc-ui/invalid_const_in_type_position.rs

megakorre · 2023-03-01T13:54:54Z

tests/rustdoc-ui/issue-105742.rs

 use std::ops::Index;

 pub fn next<'a, T>(s: &'a mut dyn SVec<Item = T, Output = T>) {
+    //~^ERROR


Question: Is there a nicer way to do this if you are not looking to tests the specific errors?
Or should I just write out all the errors produced here properly?

Writing the beginning of the error message would be nice indeed. No need for more I think.

Mark-Simulacrum · 2023-03-04T22:43:58Z

r? rustdoc

GuillaumeGomez · 2023-03-19T13:19:08Z

Looks good to me, thanks for doing the investigation. Let's run a perf check.

@bors try @rust-timer queue

r? @GuillaumeGomez

bors · 2023-03-19T13:19:19Z

⌛ Trying commit 21af8fee68cc627d3f6000e52f8ed100f750cf39 with merge 6ce32053507e9465bfc6a4395c317c3cfac77baf...

bors · 2023-03-19T16:24:31Z

☀️ Try build successful - checks-actions
Build commit: 6ce32053507e9465bfc6a4395c317c3cfac77baf (6ce32053507e9465bfc6a4395c317c3cfac77baf)

bors · 2023-03-19T16:24:31Z

☀️ Try build successful - checks-actions
Build commit: 6ce32053507e9465bfc6a4395c317c3cfac77baf (6ce32053507e9465bfc6a4395c317c3cfac77baf)

rust-timer · 2023-03-19T17:41:39Z

Finished benchmarking commit (6ce32053507e9465bfc6a4395c317c3cfac77baf): comparison URL.

Overall result: ❌✅ regressions and improvements - 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-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)	0.4%	[0.3%, 0.5%]	3
Regressions ❌ (secondary)	1.1%	[0.7%, 1.6%]	2
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.5%, 0.5%]	4

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)	1.6%	[1.1%, 1.8%]	3
Regressions ❌ (secondary)	3.3%	[2.6%, 4.1%]	2
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	1.1%	[-0.4%, 1.8%]	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)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.5%]	2
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-0.3%	[-0.8%, 0.5%]	3

GuillaumeGomez · 2023-03-19T18:20:08Z

It does have an impact on performance. I suppose it's fine but let's see with the other rustdoc people.

@rfcbot fcp merge

rfcbot · 2023-03-19T18:20:09Z

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

bors · 2023-03-30T08:55:16Z

⌛ Testing commit a75e290178d52d710c29eec027aa1ea4a8fd155e with merge 962989bb1245bd59506ba10377913aad3e14541d...

bors · 2023-03-30T08:59:46Z

💔 Test failed - checks-actions

GuillaumeGomez · 2023-03-30T09:32:05Z

There are tidy errors:

Error: tidy error: /checkout/tests/rustdoc-ui/issue-105742.rs:9: trailing whitespace
Error: tidy error: /checkout/tests/rustdoc-ui/issue-105742.rs:17: trailing whitespace
Error: tidy error: /checkout/tests/rustdoc-ui/issue-105742.rs:25: trailing whitespace
Error: tidy error: /checkout/tests/rustdoc-ui/issue-105742.rs:32: trailing whitespace

I'm surprised they weren't caught before...

jyn514 · 2023-03-30T14:11:12Z

@bors r=GuillaumeGomez

bors · 2023-03-30T14:11:14Z

📌 Commit 1f9e2d0 has been approved by GuillaumeGomez

It is now in the queue for this repository.

bors · 2023-03-30T15:21:25Z

⌛ Testing commit 1f9e2d0 with merge 789ee5e...

bors · 2023-03-30T17:54:49Z

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 789ee5e to master...

rust-timer · 2023-03-30T21:57:14Z

Finished benchmarking commit (789ee5e): 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.4%, 0.5%]	2
Regressions ❌ (secondary)	1.0%	[0.6%, 1.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.4%, 0.5%]	2

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

Cycles

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

rustbot assigned Mark-Simulacrum Feb 28, 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 Feb 28, 2023

megakorre force-pushed the rustdock_additional_typecheck_before_clean branch 2 times, most recently from ca2336a to d9fec98 Compare February 28, 2023 18:26

This comment has been minimized.

Sign in to view

megakorre commented Mar 1, 2023

View reviewed changes

tests/rustdoc-ui/invalid_const_in_type_position.rs Outdated Show resolved Hide resolved

megakorre commented Mar 1, 2023

View reviewed changes

megakorre marked this pull request as ready for review March 1, 2023 13:55

megakorre changed the title ~~WIP: rustdoc: run more HIR validation to mirror rustc~~ rustdoc: run more HIR validation to mirror rustc Mar 1, 2023

rustbot assigned jsha and unassigned Mark-Simulacrum Mar 4, 2023

This comment has been minimized.

Sign in to view

rustbot assigned GuillaumeGomez and unassigned jsha Mar 19, 2023

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 19, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 19, 2023

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 19, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 30, 2023

This comment has been minimized.

Sign in to view

megakorre added 7 commits March 30, 2023 14:55

rustdoc: run more HIR validation to mirror rustc

9b5115f

rustdoc: update with --bless and change expected errors

466fc4a

rustdoc: remove redundant test

2ee19c9

rustdoc: remove excess from rustdoc test

d1b6aa6

rustdoc: remove other redundant item

4d571a9

rustdoc: add error messages to the test

df556a3

rustdoc: tidy excess whitespace

1f9e2d0

megakorre force-pushed the rustdock_additional_typecheck_before_clean branch from a75e290 to 1f9e2d0 Compare March 30, 2023 13:47

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 Mar 30, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Mar 30, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 30, 2023

bors merged commit 789ee5e into rust-lang:master Mar 30, 2023

rustbot added this to the 1.70.0 milestone Mar 30, 2023

rustbot removed the perf-regression Performance regression. label Mar 30, 2023

Alexendoo mentioned this pull request Mar 31, 2023

ICE: bad_placeholder_type: rustdoc on glacier fixed/69204.rs #79461

Closed

This was referenced Apr 30, 2023

[ICE]: rustdoc: unexpected const parent in type_of(): TraitRef .. #107093

Closed

ICE: rustdoc: Could not find associated const on trait #106079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: run more HIR validation to mirror rustc #108576

rustdoc: run more HIR validation to mirror rustc #108576

megakorre commented Feb 28, 2023 •

edited

Loading

rustbot commented Feb 28, 2023

This comment has been minimized.

megakorre Mar 1, 2023

GuillaumeGomez Mar 19, 2023

megakorre Mar 19, 2023 •

edited

Loading

Mark-Simulacrum commented Mar 4, 2023

GuillaumeGomez commented Mar 19, 2023

This comment has been minimized.

bors commented Mar 19, 2023

bors commented Mar 19, 2023

bors commented Mar 19, 2023

This comment has been minimized.

rust-timer commented Mar 19, 2023

GuillaumeGomez commented Mar 19, 2023

rfcbot commented Mar 19, 2023 •

edited by aDotInTheVoid

Loading

bors commented Mar 30, 2023

bors commented Mar 30, 2023

GuillaumeGomez commented Mar 30, 2023

This comment has been minimized.

jyn514 commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

rust-timer commented Mar 30, 2023

rustdoc: run more HIR validation to mirror rustc #108576

rustdoc: run more HIR validation to mirror rustc #108576

Conversation

megakorre commented Feb 28, 2023 • edited Loading

Explanation

Performance

Discussion

rustbot commented Feb 28, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megakorre Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

Mark-Simulacrum commented Mar 4, 2023

GuillaumeGomez commented Mar 19, 2023

This comment has been minimized.

bors commented Mar 19, 2023

bors commented Mar 19, 2023

bors commented Mar 19, 2023

This comment has been minimized.

rust-timer commented Mar 19, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

GuillaumeGomez commented Mar 19, 2023

rfcbot commented Mar 19, 2023 • edited by aDotInTheVoid Loading

bors commented Mar 30, 2023

bors commented Mar 30, 2023

GuillaumeGomez commented Mar 30, 2023

This comment has been minimized.

jyn514 commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

rust-timer commented Mar 30, 2023

Overall result: ❌ regressions - no action needed

megakorre commented Feb 28, 2023 •

edited

Loading

megakorre Mar 19, 2023 •

edited

Loading

rfcbot commented Mar 19, 2023 •

edited by aDotInTheVoid

Loading