Enable potential_query_instability lint in rustc_hir_typeck. #113328

michaelwoerister · 2023-07-04T14:28:27Z

Fix linting errors by using FxIndex(Map|Set) and Unord(Map|Set) as appropriate. Part of MCP 533.

I really like the potential_query_instability lint!

r? @lcnr

michaelwoerister · 2023-07-04T14:28:44Z

@bors try @rust-timer queue

bors · 2023-07-04T14:28:53Z

⌛ Trying commit 4a2a38bcb70c8623f3f71c3af1496a6dd794ca7b with merge 76199ee47fd0a53283a41c58c3eae48a6f3aa9c7...

bors · 2023-07-04T16:01:23Z

☀️ Try build successful - checks-actions
Build commit: 76199ee47fd0a53283a41c58c3eae48a6f3aa9c7 (76199ee47fd0a53283a41c58c3eae48a6f3aa9c7)

compiler/rustc_middle/src/ty/typeck_results.rs

cjgillot · 2023-07-04T17:09:36Z

compiler/rustc_middle/src/middle/region.rs

@@ -7,6 +7,7 @@
 //! [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/borrow_check.html

 use crate::ty::TyCtxt;
+use hir::HirIdMap;


Suggested change

use hir::HirIdMap;

use rustc_hir::HirIdMap;

cjgillot · 2023-07-04T17:10:05Z

compiler/rustc_middle/src/ty/rvalue_scopes.rs

@@ -1,12 +1,12 @@
 use crate::middle::region::{Scope, ScopeData, ScopeTree};
-use rustc_data_structures::fx::FxHashMap;
+use hir::ItemLocalMap;


Suggested change

use hir::ItemLocalMap;

use rustc_hir::ItemLocalMap;

compiler/rustc_middle/src/ty/closure.rs

rust-timer · 2023-07-04T17:17:27Z

Finished benchmarking commit (76199ee47fd0a53283a41c58c3eae48a6f3aa9c7): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

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)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	2.0%	[1.4%, 2.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.4%	[-8.4%, -2.5%]	2
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

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: 654.344s -> 654.024s (-0.05%)

michaelwoerister · 2023-07-06T08:41:27Z

I want to try and see if I can resolve @cjgillot's comments but it might be a few days before I get to it.

@rustbot author

lcnr

r=me after nits and @cjgillot review

compiler/rustc_hir_typeck/src/upvar.rs

compiler/rustc_hir_typeck/src/writeback.rs

lcnr · 2023-07-06T10:20:09Z

r? @cjgillot

michaelwoerister · 2023-07-06T15:56:13Z

Let's see if sorting during typeck writeback has a performance impact.
@bors try @rust-timer queue

bors · 2023-07-06T15:56:24Z

⌛ Trying commit d4f2f3bffe364fd4751dc077e3044cbe769d6d69 with merge 02ae3e8e05f123322f34b0a60bd9c88f0e712d32...

bors · 2023-07-06T17:13:35Z

☀️ Try build successful - checks-actions
Build commit: 02ae3e8e05f123322f34b0a60bd9c88f0e712d32 (02ae3e8e05f123322f34b0a60bd9c88f0e712d32)

rust-timer · 2023-07-07T00:23:43Z

Finished benchmarking commit (02ae3e8e05f123322f34b0a60bd9c88f0e712d32): comparison URL.

Overall result: no relevant changes - no 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.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

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)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-0.7%	[-2.3%, 0.8%]	2

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: 657.995s -> 656.043s (-0.30%)

bors · 2023-07-12T09:36:19Z

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

lcnr · 2023-07-13T09:50:11Z

not sure whether @cjgillot still has any open reviews? if not, r=cjgillot,lcnr after rebase

lcnr · 2023-07-13T09:51:34Z

compiler/rustc_hir_typeck/src/upvar.rs

@@ -908,19 +909,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
    /// Combines all the reasons for 2229 migrations
    fn compute_2229_migrations_reasons(


🤔 should we just remove this function 😓 we're now simply creating a struct

cjgillot · 2023-07-13T12:14:23Z

@bors r=cjgillot,lcnr
except if you want to address #113328 (comment)

bors · 2023-07-13T12:14:25Z

📌 Commit 03169c52372d7d9c0fde285bc5cd28fb134ef35d has been approved by cjgillot,lcnr

It is now in the queue for this repository.

michaelwoerister · 2023-07-13T13:02:17Z

Thanks for reviewing, @cjgillot & @lcnr!

except if you want to address #113328 (comment)

I think that's better done separately and by someone with more context.

bors · 2023-07-13T20:14:16Z

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

bors · 2023-07-14T07:20:53Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout no_hashmap_in_typeck (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self no_hashmap_in_typeck --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging compiler/rustc_middle/src/ty/mod.rs
Auto-merging compiler/rustc_hir_typeck/src/upvar.rs
CONFLICT (content): Merge conflict in compiler/rustc_hir_typeck/src/upvar.rs
Auto-merging compiler/rustc_hir_typeck/src/method/suggest.rs
Auto-merging compiler/rustc_hir_analysis/src/check_unused.rs
Automatic merge failed; fix conflicts and then commit the result.

Fix linting errors by using FxIndex(Map|Set) and Unord(Map|Set) as appropriate.

…nordItems.

cjgillot · 2023-07-14T08:57:40Z

@bors r=cjgillot,lcnr

bors · 2023-07-14T08:57:42Z

📌 Commit 457b787 has been approved by cjgillot,lcnr

It is now in the queue for this repository.

bors · 2023-07-14T09:55:43Z

⌛ Testing commit 457b787 with merge df5c2cf...

bors · 2023-07-14T11:41:49Z

☀️ Test successful - checks-actions
Approved by: cjgillot,lcnr
Pushing df5c2cf to master...

rust-timer · 2023-07-14T14:56:14Z

Finished benchmarking commit (df5c2cf): comparison URL.

Overall result: ✅ improvements - 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	2
All ❌✅ (primary)	-	-	0

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

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: 657.803s -> 658.312s (0.08%)

rustbot assigned lcnr Jul 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 4, 2023

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

cjgillot reviewed Jul 4, 2023

View reviewed changes

compiler/rustc_middle/src/ty/typeck_results.rs Outdated Show resolved Hide resolved

cjgillot reviewed Jul 4, 2023

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 4, 2023

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

lcnr reviewed Jul 6, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/upvar.rs Show resolved Hide resolved

compiler/rustc_hir_typeck/src/writeback.rs Outdated Show resolved Hide resolved

rustbot assigned cjgillot and unassigned lcnr Jul 6, 2023

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 7, 2023

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

lcnr reviewed Jul 13, 2023

View reviewed changes

michaelwoerister force-pushed the no_hashmap_in_typeck branch from 7587806 to 03169c5 Compare July 13, 2023 11:33

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 13, 2023

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2023

michaelwoerister added 2 commits July 14, 2023 10:10

Enable potential_query_instability lint in rustc_hir_typeck.

cfb3109

Fix linting errors by using FxIndex(Map|Set) and Unord(Map|Set) as appropriate.

Introduce ExtentUnord trait for collections that can safely consume U…

457b787

…nordItems.

michaelwoerister force-pushed the no_hashmap_in_typeck branch from 03169c5 to 457b787 Compare July 14, 2023 08:12

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 14, 2023

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

bors merged commit df5c2cf into rust-lang:master Jul 14, 2023

rustbot added this to the 1.73.0 milestone Jul 14, 2023

This was referenced Jul 14, 2023

Add #[rustc_confusables] attribute to allow targeted "no method" error suggestions on standard library types #112239

Merged

Restrict recursive opaque type check #113636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable potential_query_instability lint in rustc_hir_typeck. #113328

Enable potential_query_instability lint in rustc_hir_typeck. #113328

michaelwoerister commented Jul 4, 2023

michaelwoerister commented Jul 4, 2023

This comment has been minimized.

bors commented Jul 4, 2023

bors commented Jul 4, 2023

This comment has been minimized.

cjgillot Jul 4, 2023

cjgillot Jul 4, 2023

rust-timer commented Jul 4, 2023

michaelwoerister commented Jul 6, 2023 •

edited

Loading

lcnr left a comment

lcnr commented Jul 6, 2023

michaelwoerister commented Jul 6, 2023

This comment has been minimized.

bors commented Jul 6, 2023

bors commented Jul 6, 2023

This comment has been minimized.

rust-timer commented Jul 7, 2023

bors commented Jul 12, 2023

lcnr commented Jul 13, 2023

lcnr Jul 13, 2023

cjgillot commented Jul 13, 2023

bors commented Jul 13, 2023

michaelwoerister commented Jul 13, 2023

bors commented Jul 13, 2023

bors commented Jul 14, 2023

cjgillot commented Jul 14, 2023

bors commented Jul 14, 2023

bors commented Jul 14, 2023

bors commented Jul 14, 2023

rust-timer commented Jul 14, 2023

		@@ -908,19 +909,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
		/// Combines all the reasons for 2229 migrations
		fn compute_2229_migrations_reasons(

Enable potential_query_instability lint in rustc_hir_typeck. #113328

Enable potential_query_instability lint in rustc_hir_typeck. #113328

Conversation

michaelwoerister commented Jul 4, 2023

michaelwoerister commented Jul 4, 2023

This comment has been minimized.

bors commented Jul 4, 2023

bors commented Jul 4, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-timer commented Jul 4, 2023

Overall result: no relevant changes - no action needed

michaelwoerister commented Jul 6, 2023 • edited Loading

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Jul 6, 2023

michaelwoerister commented Jul 6, 2023

This comment has been minimized.

bors commented Jul 6, 2023

bors commented Jul 6, 2023

This comment has been minimized.

rust-timer commented Jul 7, 2023

Overall result: no relevant changes - no action needed

bors commented Jul 12, 2023

lcnr commented Jul 13, 2023

Choose a reason for hiding this comment

cjgillot commented Jul 13, 2023

bors commented Jul 13, 2023

michaelwoerister commented Jul 13, 2023

bors commented Jul 13, 2023

bors commented Jul 14, 2023

cjgillot commented Jul 14, 2023

bors commented Jul 14, 2023

bors commented Jul 14, 2023

bors commented Jul 14, 2023

rust-timer commented Jul 14, 2023

Overall result: ✅ improvements - no action needed

michaelwoerister commented Jul 6, 2023 •

edited

Loading