Make `traits_in_crate` and `impls_in_crate` proper queries #100601

Robert-Cunningham · 2022-08-15T22:27:22Z

This pull request addresses #95092.

shift the sorting of traits_in_crate from the query to metadata encoding. We end up not sorting traits_in_crate at all, since by default they're in source code order, and the source code is hashed into crate_hash.
use traits_in_crate for all analyses that iterate on traits;
make impls_in_crate a query, keeping the sorting outside of the query in metadata encoding; impls_in_crate returns a Map<DefId, Vec<DefId, SimplifiedTypeGen>> mapping traits to impls and their simplified type. We ultimately kept all_local_trait_impls around, but it now wraps impls_in_crate and is only responsible for stripping the SimplifiedTypeGen and converting the DefId to a LocalDefId.
use impls_in_crate for analyses that iterate on impls, like coherence. Refactored encode_impls to use impls_in_crate.

For the second and fourth ticks, I wasn't able to find many other analyses that iterate through the HIR looking for traits or impls. I'm looking for something like for item in tcx.hir().items(), but maybe the iterations referenced in the original PR have a different shape that I'm not used to? It seems like e.g. coherence makes ample use of impl_trait_ref() but never needs to iterate.

Unfortunately, we could not remove traits_in_crate as suggested in the original issue's comments, since we need that information to generate docs.

By default I'll r? @cjgillot , who has been helping me out with lots of questions during this PR :)

rust-highfive · 2022-08-15T22:27:25Z

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

Please see the contribution instructions for more information.

cjgillot

Thanks @Robert-Cunningham

For the second and fourth ticks, I wasn't able to find many other analyses that iterate through the HIR looking for traits or impls. I'm looking for something like for item in tcx.hir().items(), but maybe the iterations referenced in the original PR have a different shape that I'm not used to? It seems like e.g. coherence makes ample use of impl_trait_ref() but never needs to iterate.

rustc_typeck::coherence::inherent_impls::crate_inherent_impls uses such a loop. One in inherent_impls_overlap_check::crate_inherent_impls_overlap_check too.

I wonder if we should get rid of all_local_trait_impls replaced by impls_in_crate, and change the return type of tcx.hir().trait_impls to match that replacement.

Can we implement implementations_of_trait as a query in terms of impls_in_crate directly, and remove the special case in metadata decoding and in trait_impls_of query?

cjgillot · 2022-08-16T18:31:35Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1848,9 +1830,16 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
            .map(|(trait_def_id, mut impls)| {
                // Bring everything into deterministic order for hashing
                impls.sort_by_cached_key(|&(index, _)| {


Can we get rid of this sort and the one above?

impls_in_crate returns a FxIndexMap<DefId, Vec<(DefId, Option<SimplifiedType>)>>; The Vec part should be in source code order, and the Map<TraitId, ...> part is ordered by the source code order of the Impls. Since the source code is hashed into crate_hash, I believe you're right, we can remove both of these sorts.

In fact, if I understand correctly, we can also remove a third sort in encode_incoherent_impls, since it ultimately relies on a similarly stable iteration through tcx.hir().items().

…to impls_in_crate

Robert-Cunningham · 2022-09-12T07:32:05Z

Thanks for your thoughts!

My initial take on your second point are that we can definitely drop all_local_trait_impls in favor of impls_in_crate. But I'd be in favor of leaving the return type of tcx.hir().trait_impls as it is. As far as I can tell, there's only one place (trait_impls_of_provider) where we recompute the type after calling trait_impls, and therefore would prefer trait_impls to return (DefId, Option<SimplifiedTypeGen<DefId>>) instead of just DefId). Maybe if that call type computation were expensive enough, it'd be worth including in the trait_impls return type anyway, but under the assumption that fast_reject::simplify_type is decently cheap, I'd be inclined to keep the tcx.hir().trait_impls return type as-is. I'd be open to hearing any other thoughts you have on this.

I'll continue to work through your other points over the next day or two.

bors · 2022-09-25T01:14:06Z

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

JohnCSimon · 2022-11-06T02:16:58Z

ping from triage:
@Robert-Cunningham

Can you please address the merge conflicts - and post your status on this PR?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

JohnCSimon · 2023-01-01T19:08:08Z

@Robert-Cunningham
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot · 2023-01-16T18:13:35Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Robert-Cunningham · 2023-01-17T13:08:48Z

Ok! I think the content here is ready for review, so I'll mark it @rustbot ready.

One kink to work out: I'm failing this recently added and seemingly (?) unrelated assembly test. It was added in this PR.

I'm not sure whether to treat this failure as spurious, and if possible I'd really appreciate input from @lukas-code or someone else who knows the terrain!

apiraino · 2023-01-17T16:33:22Z

Removing also S-inactive, otherwise it will be filtered out and ignored :)

@rustbot label -S-inactive

lukas-code · 2023-01-17T18:33:24Z

Looks like that assembly test has the wrong // min-llvm-version. Apparently we don't test LLVM 14 in CI at all, only 13 and 15.

…est, r=cuviper bump failing assembly & codegen tests from LLVM 14 to LLVM 15 These tests need LLVM 15. Found by `@Robert-Cunningham` in rust-lang#100601 (comment) Passed tests at 006506e93fc80318ebfd7939fe1fd4dc19ecd8cb in https://github.com/rust-lang/rust/actions/runs/3942442730/jobs/6746104740.

…est, r=cuviper bump failing assembly & codegen tests from LLVM 14 to LLVM 15 These tests need LLVM 15. Found by ``@Robert-Cunningham`` in rust-lang#100601 (comment) Passed tests at 006506e93fc80318ebfd7939fe1fd4dc19ecd8cb in https://github.com/rust-lang/rust/actions/runs/3942442730/jobs/6746104740.

…est, r=cuviper bump failing assembly & codegen tests from LLVM 14 to LLVM 15 These tests need LLVM 15. Found by ```@Robert-Cunningham``` in rust-lang#100601 (comment) Passed tests at 006506e93fc80318ebfd7939fe1fd4dc19ecd8cb in https://github.com/rust-lang/rust/actions/runs/3942442730/jobs/6746104740.

src/tools/clippy/clippy_lints/src/same_name_method.rs

bors · 2023-02-14T09:34:38Z

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

apiraino · 2023-02-15T14:11:46Z

visting this PR. Seems there some open comment(s), so tentatively flipping the review switch to
waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

cjgillot · 2023-01-19T17:11:46Z

src/tools/clippy/clippy_lints/src/same_name_method.rs

+        for id in cx.tcx.impls_in_crate(LOCAL_CRATE) {
+            let id = ItemId { owner_id: OwnerId { def_id: id.expect_local() } };


This could use a helper method in rustc_middle::hir::map.

cjgillot · 2023-02-19T11:02:56Z

Thanks @Robert-Cunningham. One nit (#100601 (comment)). Could you rebase and squash the commits?
r=me afterwards

Robert-Cunningham · 2023-03-15T16:04:43Z

Yes, will do. Thanks!

jyn514 · 2023-04-26T06:47:40Z

Hi @Robert-Cunningham, it's been a while - are you still planning to work on this PR?

Dylan-DPC · 2023-05-15T07:20:34Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

Robert-Cunningham added 10 commits August 13, 2022 15:35

hashmap compiles

8925fcc

write all_local_trait_impls in terms of new query

d84ecf2

remove fom resolver

8acd145

convert hashmap to indexmap

2d54ab7

add eval_always

ab7c2ba

stop prinnting dep graph

d3426dd

clean up

251fcbc

revert derive formatting changes

83ca511

remove comments

31222de

remove last robert-trait annotation

5a7d801

rust-highfive assigned cjgillot Aug 15, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 15, 2022

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

cjgillot reviewed Aug 17, 2022

View reviewed changes

cjgillot 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 Aug 20, 2022

Robert-Cunningham and others added 3 commits September 12, 2022 00:18

implement cjg first suggestion

058b8dc

Merge branch 'rust-lang:master' into impls_in_crate

c2a2636

Merge branch 'impls_in_crate' of github.com:Robert-Cunningham/rust in…

687280d

…to impls_in_crate

JohnCSimon closed this Jan 1, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 1, 2023

Robert-Cunningham added 4 commits January 13, 2023 02:52

temporary changed found january

6b0ab18

resolve merge comfinclits

44364ae

make compile after merge

11b2ade

fix git rename misses

3b486b2

WaffleLapkin reopened this Jan 16, 2023

Robert-Cunningham added 4 commits January 17, 2023 03:02

merge upstream changes

6e61761

finish merge

a1bee1c

remove comment churn

d313486

remove last comment

5c07123

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

rustbot removed the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 17, 2023

lukas-code mentioned this pull request Jan 17, 2023

bump failing assembly & codegen tests from LLVM 14 to LLVM 15 #106995

Merged

flip1995 reviewed Jan 19, 2023

View reviewed changes

src/tools/clippy/clippy_lints/src/same_name_method.rs Show resolved Hide resolved

add comment to address review

37aba45

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 Feb 15, 2023

cjgillot reviewed Feb 15, 2023

View reviewed changes

Dylan-DPC closed this May 15, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `traits_in_crate` and `impls_in_crate` proper queries #100601

Make `traits_in_crate` and `impls_in_crate` proper queries #100601

Robert-Cunningham commented Aug 15, 2022

rust-highfive commented Aug 15, 2022

cjgillot left a comment

cjgillot Aug 16, 2022

Robert-Cunningham Sep 12, 2022

Robert-Cunningham commented Sep 12, 2022

bors commented Sep 25, 2022

JohnCSimon commented Nov 6, 2022

JohnCSimon commented Jan 1, 2023

rustbot commented Jan 16, 2023

Robert-Cunningham commented Jan 17, 2023

apiraino commented Jan 17, 2023

lukas-code commented Jan 17, 2023

bors commented Feb 14, 2023

apiraino commented Feb 15, 2023

cjgillot Jan 19, 2023

cjgillot commented Feb 19, 2023

Robert-Cunningham commented Mar 15, 2023

jyn514 commented Apr 26, 2023

Dylan-DPC commented May 15, 2023

		for id in cx.tcx.impls_in_crate(LOCAL_CRATE) {
		let id = ItemId { owner_id: OwnerId { def_id: id.expect_local() } };

Make traits_in_crate and impls_in_crate proper queries #100601

Make traits_in_crate and impls_in_crate proper queries #100601

Conversation

Robert-Cunningham commented Aug 15, 2022

rust-highfive commented Aug 15, 2022

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Aug 16, 2022

Choose a reason for hiding this comment

Robert-Cunningham Sep 12, 2022

Choose a reason for hiding this comment

Robert-Cunningham commented Sep 12, 2022

bors commented Sep 25, 2022

JohnCSimon commented Nov 6, 2022

JohnCSimon commented Jan 1, 2023

rustbot commented Jan 16, 2023

Robert-Cunningham commented Jan 17, 2023

apiraino commented Jan 17, 2023

lukas-code commented Jan 17, 2023

bors commented Feb 14, 2023

apiraino commented Feb 15, 2023

cjgillot Jan 19, 2023

Choose a reason for hiding this comment

cjgillot commented Feb 19, 2023

Robert-Cunningham commented Mar 15, 2023

jyn514 commented Apr 26, 2023

Dylan-DPC commented May 15, 2023

Make `traits_in_crate` and `impls_in_crate` proper queries #100601

Make `traits_in_crate` and `impls_in_crate` proper queries #100601