Skip to content

Deduplication: Extract a shared helper function to remove duplicate associated const binding lowering#155196

Open
LaneAsade wants to merge 2 commits intorust-lang:mainfrom
LaneAsade:associated_const_binding_rhs_lowering
Open

Deduplication: Extract a shared helper function to remove duplicate associated const binding lowering#155196
LaneAsade wants to merge 2 commits intorust-lang:mainfrom
LaneAsade:associated_const_binding_rhs_lowering

Conversation

@LaneAsade
Copy link
Copy Markdown
Contributor

@LaneAsade LaneAsade commented Apr 12, 2026 •
edited by jackh726
Loading

This PR aims to deduplicate the code the responsible for lowering the RHS of associated const bindings (found at bounds.rs and mod.rs) by creating a helper function lower_assoc_const_binding_rhs.

Related: #150621

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 12, 2026

HIR ty lowering was modified

cc @fmease

@rustbot 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 12, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 12, 2026

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, types
  • compiler, types expanded to 69 candidates
  • Random selection from 12 candidates

@afurm
Copy link
Copy Markdown

afurm commented Apr 12, 2026

Nit: check_assoc_const_binding_type is called as bounds::check_assoc_const_binding_type at the mod.rs call site but as just check_assoc_const_binding_type (direct) at the bounds.rs call site — the helper normalises this to always call it as a method on self, which is correct, but worth verifying the two sites were always semantically equivalent (the FIXME comment in the old mod.rs code hints this was previously uncertain).

@LaneAsade
Copy link
Copy Markdown
Contributor Author

LaneAsade commented Apr 13, 2026

As far as I know, the FIXME in the old mod.rs was about code duplication in lowering the RHS type and not about the equivalence of the two call sites. Even then, the two call sites are semantically equivalent so no worries there. Thank you for the feedback though!

@rust-bors

This comment has been minimized.

Sign in to view
@LaneAsade LaneAsade force-pushed the associated_const_binding_rhs_lowering branch from 7458605 to c4c9ddc Compare April 20, 2026 20:53
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 20, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-bors

This comment has been minimized.

Sign in to view
jackh726
jackh726 requested changes Apr 23, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jackh726 jackh726 left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

FIXME no longer relevant, then r=me

View changes since this review

Comment thread compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs Outdated
@rustbot 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 Apr 23, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 23, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@jackh726
Copy link
Copy Markdown
Member

jackh726 commented Apr 23, 2026

Btw, as an aside: it's not really needed or useful to include the code of the helpful function in the PR description - I went ahead and removed it.

@LaneAsade
Copy link
Copy Markdown
Contributor Author

LaneAsade commented Apr 25, 2026

Thank you for the tip. I will keep that in mind next time.

@LaneAsade
Copy link
Copy Markdown
Contributor Author

LaneAsade commented Apr 25, 2026 •
edited
Loading

@bors r=jackh726

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 25, 2026

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@LaneAsade
Copy link
Copy Markdown
Contributor Author

LaneAsade commented Apr 25, 2026

@rustbot ready

@rustbot 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 Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jackh726 jackh726 jackh726 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@jackh726 jackh726

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@LaneAsade @rustbot @afurm @jackh726