Remove ItemLikeVisitor impls from rustc_typeck #96531

kckeiks · 2022-04-28T19:08:01Z

rust-highfive · 2022-04-28T19:08:04Z

(rust-highfive has picked a reviewer for you, use r? to override)

kckeiks · 2022-04-28T19:21:09Z

src/test/ui/dep-graph/dep-graph-trait-impl-two-traits.stderr

@@ -1,4 +1,4 @@
-error: no path from `x::<impl Foo for char>` to `typeck`


@cjgillot the modifications in compiler/rustc_typeck/src/coherence/inherent_impls.rs caused this change.

The current code checks each module independently. Having those checks performed on the whole crate at once create new dependency edges. This may be what causes the diagnostic changes.

This can be resolved by adding a eval_always to the hir_crate_items query declaration.

hir_crate_items already has eval_always.

/// All items in the crate. query hir_crate_items(_: ()) -> rustc_middle::hir::ModuleItems { storage(ArenaCacheSelector<'tcx>) eval_always desc { "get HIR crate items" } }

jackh726 · 2022-04-28T20:17:09Z

@bors try @rust-timer queue

rust-timer · 2022-04-28T20:17:10Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-04-28T20:17:17Z

⌛ Trying commit abad872c7640a34667505b3420af54f8631893e8 with merge 630a088a8b3689dbbaa9db4b857353a49586c7fe...

cjgillot

I did a first read through from afar. The current code performs typechecking by module, and isolates modules from each another using dedicated queries. What is the benefit of removing the queries? Can't they be implemented using hir_module_items?

compiler/rustc_typeck/src/check/check.rs

compiler/rustc_typeck/src/coherence/unsafety.rs

compiler/rustc_typeck/src/variance/test.rs

cjgillot · 2022-04-28T20:15:21Z

src/test/ui/dep-graph/dep-graph-trait-impl-two-traits.stderr

@@ -1,4 +1,4 @@
-error: no path from `x::<impl Foo for char>` to `typeck`


The current code checks each module independently. Having those checks performed on the whole crate at once create new dependency edges. This may be what causes the diagnostic changes.

kckeiks · 2022-04-28T20:52:39Z

I did a first read through from afar. The current code performs typechecking by module, and isolates modules from each another using dedicated queries. What is the benefit of removing the queries? Can't they be implemented using hir_module_items?

@cjgillot Sorry, maybe I missed something. This is the current code using those two queries like this:

tcx.sess.time("item_types_checking", || { 
  tcx.hir().for_each_module(|module| tcx.ensure().check_mod_item_types(module))
});

// and

tcx.hir().for_each_module(module| tcx.ensure().check_mod_impl_wf(module)) });

It seemed to me, that in both of these cases, the code is going over all of the Items in the crate so I think it is more straight forward and simpler to loop over tcx.hir().items. I think if we used hir_module_items, we would be doing two traversals, one from calling for_each_module and one from hir_module_items so the changes I proposed avoid that. Please let me know if I missed something or am misunderstanding.

I was not going to remove the queries but that is the only place where they are used so I removed them. I can put them back?

cjgillot · 2022-04-28T20:58:47Z

I agree that query and no-query code basically do the same thing.
The queries are used to isolate dependencies tracking. In incremental mode, this allows to skip check_mod_item_types and check_mod_impl_wf in unmodified files while recomputing them in modified files.
I'd prefer you put them back in place please.
(I eventually plan to migrate more of typechecking analyses to operate per-module instead of whole-crate.)

kckeiks · 2022-04-28T21:06:50Z

I agree that query and no-query code basically do the same thing. The queries are used to isolate dependencies tracking. In incremental mode, this allows to skip check_mod_item_types and check_mod_impl_wf in unmodified files while recomputing them in modified files. I'd prefer you put them back in place please. (I eventually plan to migrate more of typechecking analyses to operate per-module instead of whole-crate.)

I will do that. Looks like I need to research/learn more about the query system. I will message you in Zulip with questions :)

bors · 2022-04-28T21:42:22Z

☀️ Try build successful - checks-actions
Build commit: 630a088a8b3689dbbaa9db4b857353a49586c7fe (630a088a8b3689dbbaa9db4b857353a49586c7fe)

rust-timer · 2022-04-28T21:42:23Z

Queued 630a088a8b3689dbbaa9db4b857353a49586c7fe with parent 1388b38, future comparison URL.

rust-timer · 2022-04-28T23:00:04Z

Finished benchmarking commit (630a088a8b3689dbbaa9db4b857353a49586c7fe): comparison url.

Summary:

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	83	48	0	0	83
mean²	4.3%	5.5%	N/A	N/A	4.3%
max	24.3%	36.9%	N/A	N/A	24.3%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

davidtwco · 2022-04-29T01:06:01Z

r? @cjgillot

kckeiks · 2022-04-29T17:26:13Z

I added back the two queries and addressed the feedback.

cjgillot · 2022-04-29T17:41:04Z

compiler/rustc_typeck/src/check/check.rs

-            tcx.ensure().typeck(it.def_id);
-            maybe_check_static_with_link_section(tcx, it.def_id, it.span);
-            check_static_inhabited(tcx, it.def_id, it.span);
+    match tcx.hir().def_kind(id.def_id) {


Suggested change

match tcx.hir().def_kind(id.def_id) {

match tcx.def_kind(id.def_id) {

tcx.def_kind is a query, and is cached. tcx.hir().def_kind recomputes the value every time, and should be made inaccessible in favor of the query.

Sorry, I see there are some I left over. I'll make an update.

cjgillot · 2022-04-29T17:49:00Z

compiler/rustc_typeck/src/outlives/test.rs

+            let inferred_outlives_of = tcx.inferred_outlives_of(id.def_id);
+            struct_span_err!(
+                tcx.sess,
+                tcx.hir().span(id.hir_id()),


Suggested change

tcx.hir().span(id.hir_id()),

tcx.def_span(id.def_id),

cjgillot · 2022-04-29T22:09:45Z

@bors try @rust-timer queue

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

…l_wf query Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

kckeiks · 2022-05-06T17:04:10Z

umbrella The latest upstream changes (presumably #96735) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot I fixed the conflict. Let me know if there is anything else that I should address.

cjgillot · 2022-05-06T21:06:09Z

@bors r+

bors · 2022-05-06T21:06:11Z

📌 Commit 91ef3ba has been approved by cjgillot

bors · 2022-05-07T01:59:13Z

⌛ Testing commit 91ef3ba with merge f6e5570...

bors · 2022-05-07T04:15:47Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing f6e5570 to master...

rust-timer · 2022-05-07T05:51:21Z

Finished benchmarking commit (f6e5570): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	0	9	9	9
mean²	N/A	N/A	-0.3%	-0.4%	-0.3%
max	N/A	N/A	-0.4%	-0.6%	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

…-rustc-typeck, r=cjgillot Remove ItemLikeVisitor impls from rustc_typeck Issue rust-lang#95004 cc `@cjgillot`

rust-highfive assigned davidtwco Apr 28, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2022

kckeiks force-pushed the remove-item-like-visitor-from-rustc-typeck branch from 80650cd to abad872 Compare April 28, 2022 19:17

kckeiks commented Apr 28, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 28, 2022

cjgillot requested changes Apr 28, 2022

View reviewed changes

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 28, 2022

rust-highfive assigned cjgillot and unassigned davidtwco Apr 29, 2022

kckeiks force-pushed the remove-item-like-visitor-from-rustc-typeck branch from abad872 to c966b98 Compare April 29, 2022 17:24

kckeiks force-pushed the remove-item-like-visitor-from-rustc-typeck branch from c966b98 to b9d15d3 Compare April 29, 2022 17:29

cjgillot reviewed Apr 29, 2022

View reviewed changes

kckeiks added 14 commits May 6, 2022 11:55

remove ItemLikeVisitor impl for TermsContext

a722527

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

use DefKind::Fn instead of DefKind::AsscFn for foreign items

03e955d

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove VarianceTest

9c111dd

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove OutlivesTest

c27f308

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove ItemLikeVisitor impl for InherentCollect

63849b6

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove ItemLikeVisitor impl for InherentOverlapChecker

7c3c588

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove UnsafetyChecker

f47f39b

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

use hir_module_items instead of visit_all_item_likes in check_mod_imp…

422dd3b

…l_wf query Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

use hir_module_items in check_mod_item_types query

d88dfd2

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove CheckItemTypesVisitor

dfe0746

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove ImplWfCheck

1b07c4a

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

pass ItemId to check_item_type instead of Item

ef1d112

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

use def_span and def_kind queries instead of calling tcx.hir() methods

3afc5ea

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

remove all usages of hir().def_kind

91ef3ba

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>

kckeiks force-pushed the remove-item-like-visitor-from-rustc-typeck branch from f0bb18a to 91ef3ba Compare May 6, 2022 16:11

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 May 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2022

bors merged commit f6e5570 into rust-lang:master May 7, 2022

rustbot added this to the 1.62.0 milestone May 7, 2022

bors mentioned this pull request May 7, 2022

Move various checks to typeck so them failing causes the typeck result to get tainted #96046

Merged

kckeiks mentioned this pull request May 8, 2022

Retire ItemLikeVisitor trait #96825

Merged

xFrednet pushed a commit to xFrednet/rust that referenced this pull request May 21, 2022

Auto merge of rust-lang#96531 - kckeiks:remove-item-like-visitor-from…

dd6ee7f

…-rustc-typeck, r=cjgillot Remove ItemLikeVisitor impls from rustc_typeck Issue rust-lang#95004 cc `@cjgillot`

kckeiks mentioned this pull request Jun 11, 2022

Refactor HIR item-like traversal #95004

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ItemLikeVisitor impls from rustc_typeck #96531

Remove ItemLikeVisitor impls from rustc_typeck #96531

kckeiks commented Apr 28, 2022

rust-highfive commented Apr 28, 2022

kckeiks Apr 28, 2022

cjgillot Apr 28, 2022

cjgillot Apr 30, 2022

kckeiks May 2, 2022

jackh726 commented Apr 28, 2022

rust-timer commented Apr 28, 2022

bors commented Apr 28, 2022

cjgillot left a comment

cjgillot Apr 28, 2022

kckeiks commented Apr 28, 2022

cjgillot commented Apr 28, 2022

kckeiks commented Apr 28, 2022

bors commented Apr 28, 2022

rust-timer commented Apr 28, 2022

rust-timer commented Apr 28, 2022

davidtwco commented Apr 29, 2022

kckeiks commented Apr 29, 2022

cjgillot Apr 29, 2022

kckeiks Apr 29, 2022

cjgillot Apr 29, 2022

cjgillot commented Apr 29, 2022

kckeiks commented May 6, 2022

cjgillot commented May 6, 2022

bors commented May 6, 2022

bors commented May 7, 2022

bors commented May 7, 2022

rust-timer commented May 7, 2022

		@@ -1,4 +1,4 @@
		error: no path from `x::<impl Foo for char>` to `typeck`

	match tcx.hir().def_kind(id.def_id) {
	match tcx.def_kind(id.def_id) {

Remove ItemLikeVisitor impls from rustc_typeck #96531

Remove ItemLikeVisitor impls from rustc_typeck #96531

Conversation

kckeiks commented Apr 28, 2022

rust-highfive commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Apr 28, 2022

rust-timer commented Apr 28, 2022

bors commented Apr 28, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kckeiks commented Apr 28, 2022

cjgillot commented Apr 28, 2022

kckeiks commented Apr 28, 2022

bors commented Apr 28, 2022

rust-timer commented Apr 28, 2022

rust-timer commented Apr 28, 2022

Footnotes

davidtwco commented Apr 29, 2022

kckeiks commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Apr 29, 2022

kckeiks commented May 6, 2022

cjgillot commented May 6, 2022

bors commented May 6, 2022

bors commented May 7, 2022

bors commented May 7, 2022

rust-timer commented May 7, 2022

Footnotes