rustc: Panic by default in `DefIdTree::parent` #96431

petrochenkov · 2022-04-26T12:30:05Z

Only crate root def-ids don't have a parent, and in majority of cases the argument of DefIdTree::parent cannot be a crate root.
So we now panic by default in parent and introduce a new non-panicing function opt_parent for cases where the argument can be a crate root.

Same applies to local_parent/opt_local_parent.

rust-highfive · 2022-04-26T12:30:08Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-04-26T12:30:10Z

r? @fee1-dead

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

fee1-dead · 2022-04-26T12:34:46Z

compiler/rustc_resolve/src/lib.rs

@@ -1105,7 +1105,8 @@ impl<'a> AsMut<Resolver<'a>> for Resolver<'a> {
 }

 impl<'a, 'b> DefIdTree for &'a Resolver<'b> {
-    fn parent(self, id: DefId) -> Option<DefId> {
+    #[inline]


Does the use of #[inline] here actually improve perf?

I guess we could start with a perf run on this PR first. I don't know if I would be available when CI passes. Could you initiate a perf run? Thanks.

Looks like it does not - #96431 (comment).

I added these mostly for completeness, together with the default methods on DefIdTree.
All of these functions may convert DefId to LocalDefId and back multiple times, so it would be good if all of these conversions disappeared in the end.

petrochenkov · 2022-04-26T13:05:59Z

@bors try @rust-timer queue

rust-timer · 2022-04-26T13:06:01Z

Awaiting bors try build completion.

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

bors · 2022-04-26T13:06:07Z

⌛ Trying commit 74cd66d8a52cd4368ae5bb7b5ab49f589094cfdf with merge 014631e3348b61e1086e3923458a7225d9663b97...

bors · 2022-04-26T14:28:04Z

☀️ Try build successful - checks-actions
Build commit: 014631e3348b61e1086e3923458a7225d9663b97 (014631e3348b61e1086e3923458a7225d9663b97)

rust-timer · 2022-04-26T14:28:06Z

Queued 014631e3348b61e1086e3923458a7225d9663b97 with parent d6a57d3, future comparison URL.

rust-timer · 2022-04-26T17:23:59Z

Finished benchmarking commit (014631e3348b61e1086e3923458a7225d9663b97): comparison url.

Summary: This benchmark run did not return any relevant results.

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.

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

fee1-dead · 2022-04-28T07:17:51Z

compiler/rustc_lint/src/types.rs

                    // needed in case this is a ctor, not a variant
-                    || parent.map_or(false, |parent| tcx.parent(parent) == atomic_ordering)
+                    || tcx.opt_parent(parent) == atomic_ordering


This is a bit weird since atomic_ordering could be None, which could return true accidentally I think? Anyways this is an existing logic bug. I hope no_core has an actual purpose otherwise fixing this feels useless..

fee1-dead · 2022-04-28T10:30:07Z

r? rust-lang/compiler

rust-lang/highfive#399

fee1-dead · 2022-04-28T10:30:45Z

r? rust-lang/compiler

cjgillot · 2022-05-01T16:50:48Z

r? @cjgillot
r=me after rebase

nagisa · 2022-05-01T16:53:17Z

Heh, I was just about to take a look. Thanks for taking over @cjgillot!

Only crate root def-ids don't have a parent, and in majority of cases the argument of `DefIdTree::parent` cannot be a crate root. So we now panic by default in `parent` and introduce a new non-panicing function `opt_parent` for cases where the argument can be a crate root. Same applies to `local_parent`/`opt_local_parent`.

petrochenkov · 2022-05-01T23:12:58Z

@bors r=cjgillot

bors · 2022-05-01T23:12:59Z

📌 Commit 5b5964f has been approved by cjgillot

bors · 2022-05-02T01:37:15Z

⌛ Testing commit 5b5964f with merge f1e60edc527525412e3fcb9f58916f8e22bc8cfc...

rust-log-analyzer · 2022-05-02T03:27:38Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2022-05-02T03:28:09Z

💔 Test failed - checks-actions

petrochenkov · 2022-05-02T10:23:37Z

Timeout on dist-aarch64-apple.
@bors retry

petrochenkov · 2022-05-02T10:24:15Z

@bors rollup=maybe

bors · 2022-05-02T14:03:27Z

⌛ Testing commit 5b5964f with merge 879fb42...

bors · 2022-05-02T16:28:41Z

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

rust-timer · 2022-05-02T17:46:56Z

Finished benchmarking commit (879fb42): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	0	1	0	1
mean²	N/A	N/A	-0.8%	N/A	-0.8%
max	N/A	N/A	-0.8%	N/A	-0.8%

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 ↩

rust-highfive assigned fee1-dead Apr 26, 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 26, 2022

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

fee1-dead reviewed Apr 26, 2022

View reviewed changes

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

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

fee1-dead reviewed Apr 28, 2022

View reviewed changes

rust-highfive assigned nagisa and unassigned fee1-dead Apr 28, 2022

rust-highfive assigned cjgillot and unassigned nagisa May 1, 2022

petrochenkov force-pushed the parent branch from 74cd66d to 5b5964f Compare May 1, 2022 23:11

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 1, 2022

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

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 2, 2022

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

bors merged commit 879fb42 into rust-lang:master May 2, 2022

rustbot added this to the 1.62.0 milestone May 2, 2022

This was referenced May 2, 2022

rustdoc: Reduce clean::Type size #93963

Merged

rustdoc: Cleanup doc link resolution #96345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Panic by default in `DefIdTree::parent` #96431

rustc: Panic by default in `DefIdTree::parent` #96431

petrochenkov commented Apr 26, 2022

rust-highfive commented Apr 26, 2022

rust-highfive commented Apr 26, 2022

fee1-dead Apr 26, 2022

fee1-dead Apr 26, 2022

petrochenkov Apr 26, 2022

petrochenkov commented Apr 26, 2022

rust-timer commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

rust-timer commented Apr 26, 2022

fee1-dead Apr 28, 2022

fee1-dead commented Apr 28, 2022

fee1-dead commented Apr 28, 2022

cjgillot commented May 1, 2022

nagisa commented May 1, 2022

petrochenkov commented May 1, 2022

bors commented May 1, 2022

bors commented May 2, 2022

rust-log-analyzer commented May 2, 2022

bors commented May 2, 2022

petrochenkov commented May 2, 2022

petrochenkov commented May 2, 2022

bors commented May 2, 2022

bors commented May 2, 2022

rust-timer commented May 2, 2022

rustc: Panic by default in DefIdTree::parent #96431

rustc: Panic by default in DefIdTree::parent #96431

Conversation

petrochenkov commented Apr 26, 2022

rust-highfive commented Apr 26, 2022

rust-highfive commented Apr 26, 2022

fee1-dead Apr 26, 2022

Choose a reason for hiding this comment

fee1-dead Apr 26, 2022

Choose a reason for hiding this comment

petrochenkov Apr 26, 2022

Choose a reason for hiding this comment

petrochenkov commented Apr 26, 2022

rust-timer commented Apr 26, 2022

bors commented Apr 26, 2022

bors commented Apr 26, 2022

rust-timer commented Apr 26, 2022

rust-timer commented Apr 26, 2022

fee1-dead Apr 28, 2022

Choose a reason for hiding this comment

fee1-dead commented Apr 28, 2022

fee1-dead commented Apr 28, 2022

cjgillot commented May 1, 2022

nagisa commented May 1, 2022

petrochenkov commented May 1, 2022

bors commented May 1, 2022

bors commented May 2, 2022

rust-log-analyzer commented May 2, 2022

bors commented May 2, 2022

petrochenkov commented May 2, 2022

petrochenkov commented May 2, 2022

bors commented May 2, 2022

bors commented May 2, 2022

rust-timer commented May 2, 2022

Footnotes

rustc: Panic by default in `DefIdTree::parent` #96431

rustc: Panic by default in `DefIdTree::parent` #96431