Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful #124356

fmease · 2024-04-25T01:25:54Z

[…] while I know that there's precendent to name things Issue69420, I really dislike this as it requires looking up the issue to figure out the purpose of such a variant. Actually referring to the underlying issue, e.g. AliasMayNormToUncovered or whatever and then linking to the issue in a doc comment feels a lot more desirable to me. We should ideally rename all the functions and enums which currently use issue numbers.

I've grepped through compiler/ like crazy and think that I've found all instances of this pattern.
However, I haven't renamed compute_2229_migrations_*. Should I?

The first commit introduces an abhorrent and super long name for an item because naming is hard but also scary looking / unwelcoming names are good for things related to temporary-ish backcompat hacks. I'll let you discover it by yourself.

Contains a bit of drive-by cleanup and a diag migration bc that was the simplest option.

r? lcnr or compiler

rustbot · 2024-04-25T01:26:03Z

Some changes occurred in match checking

cc @Nadrieril

lcnr

r=me after nits

thanks ❤️

lcnr · 2024-04-25T11:30:07Z

compiler/rustc_middle/src/query/mod.rs

@@ -846,8 +846,10 @@ rustc_queries! {
        separate_provide_extern
    }

-    query issue33140_self_ty(key: DefId) -> Option<ty::EarlyBinder<ty::Ty<'tcx>>> {
-        desc { |tcx| "computing Self type wrt issue #33140 `{}`", tcx.def_path_str(key) }
+    query self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(


Suggested change

query self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(

query self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(

I think exploit is not a good fithere 🤔

lcnr · 2024-04-25T11:33:32Z

compiler/rustc_ty_utils/src/ty.rs

+#[instrument(level = "debug", skip_all)]
+fn self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(
+    tcx: TyCtxt<'_>,
+    def_id: DefId,
+) -> Option<EarlyBinder<Ty<'_>>> {
+    debug!(?def_id);


Suggested change

#[instrument(level = "debug", skip_all)]

fn self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(

tcx: TyCtxt<'_>,

def_id: DefId,

) -> Option<EarlyBinder<Ty<'_>>> {

debug!(?def_id);

#[instrument(level = "debug", skip(tcx))]

fn self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(

tcx: TyCtxt<'_>,

def_id: DefId,

) -> Option<EarlyBinder<Ty<'_>>> {

was there a reason you didn't do this?

Yeah, I did that intentionally. There's a minor difference. If I just add it to the tracing span via #[instrument], the DefId doesn't get logged right away but only together with each debug! (and for context it wouldn't get logged at all if we didn't reach any debug! which won't be the case here).

I'm gonna check the actual output later since my knowledge is based on experiments I did in the past with the default tracing_subscriber setup and see how it actually fares in practice.

Output with #[instrument(skip_all)] & debug!(?def_id):

│ │ │ ├─┐rustc_ty_utils::ty::self_ty_of_trait_impl_enabling_order_dep_trait_object_hack │ │ │ │ ├─ 0ms DEBUG rustc_ty_utils::ty def_id=DefId(0:5 ~ lint_incoherent_auto_trait_objects[0a81]::{impl#1}) │ │ │ │ ├─ 0ms DEBUG rustc_ty_utils::ty trait_ref=<(dyn std::marker::Send + 'static) as Foo> │ │ │ │ ├─ 0ms DEBUG rustc_ty_utils::ty MATCHES! │ │ │ ├─┘

Output with #[instrument(skip(tcx)] & no debug call:

│ │ │ ├─┐rustc_ty_utils::ty::self_ty_of_trait_impl_enabling_order_dep_trait_object_hack def_id=DefId(0:5 ~ lint_incoherent_auto_trait_objects[0a81]::{impl#1}) │ │ │ │ ├─ 0ms DEBUG rustc_ty_utils::ty trait_ref=<(dyn std::marker::Send + 'static) as Foo> │ │ │ │ ├─ 0ms DEBUG rustc_ty_utils::ty MATCHES! │ │ │ ├─┘

Yeah, okay, in this case it doesn't matter. Taking your suggested approach.

…ngful or inline such functions if useless.

fmease · 2024-04-30T20:29:02Z

@bors r=lcnr rollup

bors · 2024-04-30T20:29:05Z

📌 Commit 2a1d748 has been approved by lcnr

It is now in the queue for this repository.

bors · 2024-05-01T00:04:39Z

⌛ Testing commit 2a1d748 with merge f5355b9...

bors · 2024-05-01T02:10:23Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing f5355b9 to master...

rust-timer · 2024-05-01T03:25:29Z

Finished benchmarking commit (f5355b9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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)	1.7%	[1.0%, 2.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[1.0%, 2.3%]	2

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

Binary size

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

Bootstrap: 673.884s -> 672.879s (-0.15%)
Artifact size: 316.00 MiB -> 315.99 MiB (-0.00%)

fmease added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Apr 25, 2024

rustbot assigned lcnr Apr 25, 2024

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 Apr 25, 2024

fmease changed the title ~~Cleanup: Replace iten names referencing GitHub issues or error code with something more meaningful~~ Cleanup: Replace item names referencing GitHub issues or error code with something more meaningful Apr 25, 2024

fmease changed the title ~~Cleanup: Replace item names referencing GitHub issues or error code with something more meaningful~~ Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful Apr 25, 2024

lcnr reviewed Apr 25, 2024

View reviewed changes

lcnr 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 Apr 27, 2024

This comment was marked as resolved.

Sign in to view

fmease added 3 commits April 30, 2024 22:27

Give items related to issue 33140 a more meaningful name

9e739b7

Give an item related to issue 27438 a more meaningful name

dec1d16

Replace item names containing an error code with something more meani…

2a1d748

…ngful or inline such functions if useless.

fmease force-pushed the fewer-magic-numbers-in-names branch from c7ebe19 to 2a1d748 Compare April 30, 2024 20:28

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 Apr 30, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2024

bors merged commit f5355b9 into rust-lang:master May 1, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 1, 2024

fmease deleted the fewer-magic-numbers-in-names branch May 1, 2024 02:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful #124356

Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful #124356

fmease commented Apr 25, 2024 •

edited

Loading

rustbot commented Apr 25, 2024

lcnr left a comment

lcnr Apr 25, 2024

lcnr Apr 25, 2024

fmease Apr 27, 2024

fmease Apr 30, 2024

This comment was marked as resolved.

fmease commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented May 1, 2024

bors commented May 1, 2024

rust-timer commented May 1, 2024

	query self_ty_of_trait_impl_enabling_order_dep_trait_object_exploit(
	query self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(

Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful #124356

Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful #124356

Conversation

fmease commented Apr 25, 2024 • edited Loading

rustbot commented Apr 25, 2024

lcnr left a comment

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

Choose a reason for hiding this comment

This comment was marked as resolved.

fmease commented Apr 30, 2024

bors commented Apr 30, 2024

bors commented May 1, 2024

bors commented May 1, 2024

rust-timer commented May 1, 2024

Overall result: no relevant changes - no action needed

fmease commented Apr 25, 2024 •

edited

Loading