Skip to content

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

Merged
bors merged 3 commits into
rust-lang:masterfrom
fmease:fewer-magic-numbers-in-names
May 1, 2024
Merged

Cleanup: Replace item names referencing GitHub issues or error codes with something more meaningful#124356
bors merged 3 commits into
rust-lang:masterfrom
fmease:fewer-magic-numbers-in-names

Conversation

@fmease

@fmeasefmease commented Apr 25, 2024

Copy link
Copy Markdown
Member

lcnr in #117164 (review):

[…] 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

@fmeasefmease added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Apr 25, 2024
@rustbotrustbot 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
@rustbot

Copy link
Copy Markdown
Collaborator

Some changes occurred in match checking

cc @Nadrieril

@fmeasefmease changed the title Cleanup: Replace iten names referencing GitHub issues or error code with something more meaningfulCleanup: Replace item names referencing GitHub issues or error code with something more meaningfulApr 25, 2024
@fmeasefmease changed the title Cleanup: Replace item names referencing GitHub issues or error code with something more meaningfulCleanup: Replace item names referencing GitHub issues or error codes with something more meaningfulApr 25, 2024

@lcnrlcnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after nits

thanks ❤️


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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Comment threadcompiler/rustc_ty_utils/src/ty.rs Outdated
Comment on lines +250 to +255
#[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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lcnrlcnr 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
@bors

This comment was marked as resolved.

@fmease
fmeaseforce-pushed the fewer-magic-numbers-in-names branch from c7ebe19 to 2a1d748CompareApril 30, 2024 20:28
@fmease

Copy link
Copy Markdown
MemberAuthor

@bors r=lcnr rollup

@bors

bors commented Apr 30, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 2a1d748 has been approved by lcnr

It is now in the queue for this repository.

@borsbors 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

bors commented May 1, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 2a1d748 with merge f5355b9...

@bors

bors commented May 1, 2024

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2024
@bors
bors merged commit f5355b9 into rust-lang:masterMay 1, 2024
@rustbotrustbot added this to the 1.80.0 milestone May 1, 2024
@fmease
fmease deleted the fewer-magic-numbers-in-names branch May 1, 2024 02:40
@rust-timer

Copy link
Copy Markdown
Collaborator

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.

meanrangecount
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.

meanrangecount
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%)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@fmease@rustbot@bors@rust-timer@lcnr