Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HIR analysis: Remove unnecessary abstraction over list of clauses #137100

Merged
merged 1 commit into from
Feb 17, 2025
Merged

HIR analysis: Remove unnecessary abstraction over list of clauses #137100

merged 1 commit into from
Feb 17, 2025

Conversation

fmease
Copy link
Member

@fmease fmease commented Feb 15, 2025 •
edited
Loading

rustc_hir_analysis::bounds::Bounds with its methods is nowadays a paper-thin wrapper around Vec<(Clause, Span)>s and Vec::push essentially.

Its existence slightly annoyed me (and I keep opening its corresp. file instead of the identically named bounds.rs in hir_ty_lowering/ that I actually want most of the time :P).

Opening to check if you agree with inlining it.
r? compiler-errors or reassign

@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 Feb 15, 2025
@fmease fmease added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Feb 15, 2025
compiler-errors
compiler-errors approved these changes Feb 15, 2025
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

that closure is dissatisfying but that's basically the only qualm i have here; i think it's fine anyways. you can further simplify it if you want, but if not then r=me

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fmease fmease 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 Feb 15, 2025
@compiler-errors
Copy link
Member

compiler-errors commented Feb 15, 2025 •
edited
Loading

Looks like we should add those const trait checks back, or possibly alternatively just suppress host effect predicates for traits that aren't const.

@fmease fmease force-pushed the hirtylow-rm-clauses-wrapper branch from b719f28 to 84bdc5d Compare February 15, 2025 22:55
@fmease
Copy link
Member Author

fmease commented Feb 15, 2025 •
edited
Loading

So in lower_poly_trait_ref I missed that we don't return early if {,~}const is applied to a non-#[const_trait] trait bound which rendered the "We're checked above that the trait is conditionally const in this case." null and void.

I've now split the predicate filtering logic to handle predicates and const conditions in two separate steps. This allowed me to (1) inline the closure and to (2) gate adding const condition behind the necessary validation step.

@compiler-errors
Copy link
Member

Oh yeah that's much nicer

@fmease fmease 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 Feb 15, 2025
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 84bdc5d has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Feb 16, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 16, 2025
@workingjubilee
Rollup merge of rust-lang#137100 - fmease:hirtylow-rm-clauses-wrapper…
692729b 
…, r=compiler-errors

HIR analysis: Remove unnecessary abstraction over list of clauses

`rustc_hir_analysis::bounds::Bounds` with its methods is nowadays a paper-thin wrapper around `Vec<(Clause, Span)>`s and `Vec::push` essentially.

Its existence slightly annoyed me (and I keep opening its corresp. file instead of the identically named `bounds.rs` in `hir_ty_lowering/` that I actually want most of the time :P).

Opening to check if you agree with inlining it.
r? compiler-errors or reassign
@workingjubilee workingjubilee mentioned this pull request Feb 16, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137153 - workingjubilee:rollup-1cjmnuh, r=wor…
57fb317 
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#136953 (rustc_target: import TargetMetadata)
 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137164 - matthiaskrgr:rollup-dj5826k, r=matth…
2162e9d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2a25a16 into rust-lang:master Feb 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@rust-timer
Unrolled build for rust-lang#137100
2e3a59a 
Rollup merge of rust-lang#137100 - fmease:hirtylow-rm-clauses-wrapper, r=compiler-errors

HIR analysis: Remove unnecessary abstraction over list of clauses

`rustc_hir_analysis::bounds::Bounds` with its methods is nowadays a paper-thin wrapper around `Vec<(Clause, Span)>`s and `Vec::push` essentially.

Its existence slightly annoyed me (and I keep opening its corresp. file instead of the identically named `bounds.rs` in `hir_ty_lowering/` that I actually want most of the time :P).

Opening to check if you agree with inlining it.
r? compiler-errors or reassign
@fmease fmease deleted the hirtylow-rm-clauses-wrapper branch February 17, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
5 participants
@fmease @rust-log-analyzer @compiler-errors @bors @rustbot