Skip to content

Clean up AstConv #122527

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
Merged
merged 1 commit into from
Mar 15, 2024
Merged

Clean up AstConv #122527

merged 1 commit into from
Mar 15, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Mar 15, 2024 •
edited
Loading

Split off from #120926 to make it only contain the renaming & (doc) comment updates.
Any changes other than that which have accumulated over time are now part of this PR.
Let's be disciplined ;) Inspired by #120926 (comment).

  • Remove hir_trait_to_predicates
  • Inline create_args_for_ast_trait_ref
    • Only had a single call site
    • Having it as a separate method didn't gain us anything
  • Use an if-let guard somewhere to avoid unwrapping
  • Avoid explicit trait object lifetimes
    • More legible, stylistic-only (the updated code is 100% semantically identical)
    • Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
    • Make use of object lifetime defaulting for a trait object type inside of a reference type somewhere
  • Use preexisting dedicated method ItemCtxt::to_ty over <dyn AstConv<'_>>::ast_ty_to_ty
  • Use preexisting dedicated method AstConv::astconv over explicit coercions
  • Simplify the function signature of create_args_for_ast_path and of check_generic_arg_count
    • In both cases redundant information was passed rendering the call sites verbose and confusing
    • No perf impact (tested in #120926)
  • Move diagnostic method report_ambiguous_associated_type from astconv to astconv::errors
    • The submodule errors exists specifically for that purpose
    • Use it to keep the main module clean & short

@fmease fmease added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 15, 2024
@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 15, 2024
@fmease fmease force-pushed the clean-up-hir-ty-lowering branch from 00948e4 to 5beda81 Compare March 15, 2024 04:07
@fmease fmease changed the title Clean up HIR ty lowering Clean up AstConv Mar 15, 2024
@fmease fmease removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. A-meta Area: Issues & PRs about the rust-lang/rust repository itself WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 15, 2024
@fmease fmease marked this pull request as ready for review March 15, 2024 04:08
@fmease
Copy link
Member Author

fmease commented Mar 15, 2024

r? @compiler-errors

@compiler-errors
Copy link
Member

Yeah I don't expect any of these changes to have a perf impact lol

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 15, 2024

📌 Commit 5beda81 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 Mar 15, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 15, 2024
@matthiaskrgr
Rollup merge of rust-lang#122527 - fmease:clean-up-hir-ty-lowering, r…
e1b25d9 
…=compiler-errors

Clean up AstConv

Split off from rust-lang#120926 to make it only contain the renaming & (doc) comment updates.
Any changes other than that which have accumulated over time are now part of this PR.
Let's be disciplined ;) Inspired by rust-lang#120926 (comment).

---

* Remove `hir_trait_to_predicates`
  * Unused since rust-lang#113671
* Inline `create_args_for_ast_trait_ref`
  * Only had a single call site
  * Having it as a separate method didn't gain us anything
* Use an if-let guard somewhere to avoid unwrapping
* Avoid explicit trait object lifetimes
  * More legible, stylistic-only (the updated code is 100% semantically identical)
  * Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
  * Make use of [object lifetime defaulting](https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes) for a trait object type inside of a reference type somewhere
* Use preexisting dedicated method `ItemCtxt::to_ty` over `<dyn AstConv<'_>>::ast_ty_to_ty`
* Use preexisting dedicated method `AstConv::astconv` over explicit coercions
* Simplify the function signature of `create_args_for_ast_path` and of `check_generic_arg_count`
  * In both cases redundant information was passed rendering the call sites verbose and confusing
  * No perf impact (tested in [rust-lang#120926](rust-lang#120926))
* Move diagnostic method `report_ambiguous_associated_type` from `astconv` to `astconv::errors`
  * The submodule `errors` exists specifically for that purpose
  * Use it to keep the main module clean & short
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 15, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@bors
Auto merge of rust-lang#122528 - matthiaskrgr:rollup-psx4evv, r=matth…
ee9f91e 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121885 (Move generic `NonZero` `rustc_layout_scalar_valid_range_start` attribute to inner type.)
 - rust-lang#122471 (preserve span when evaluating mir::ConstOperand)
 - rust-lang#122515 (Pass the correct DefId when suggesting writing the aliased Self type out)
 - rust-lang#122523 (Ensure RPITITs are created before def-id freezing)
 - rust-lang#122527 (Clean up AstConv)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@bors
Auto merge of rust-lang#122534 - matthiaskrgr:rollup-p95i3ph, r=matth…
d7723b2 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121207 (Add `-Z external-clangrt`)
 - rust-lang#122174 (diagnostics: suggest `Clone` bounds when noop `clone()`)
 - rust-lang#122471 (preserve span when evaluating mir::ConstOperand)
 - rust-lang#122515 (Pass the correct DefId when suggesting writing the aliased Self type out)
 - rust-lang#122523 (Ensure RPITITs are created before def-id freezing)
 - rust-lang#122526 (Docs for `thir::ExprKind::Use` and `thir::ExprKind::Let`)
 - rust-lang#122527 (Clean up AstConv)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 277df5e into rust-lang:master Mar 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@rust-timer
Unrolled build for rust-lang#122527
ad5fbdc 
Rollup merge of rust-lang#122527 - fmease:clean-up-hir-ty-lowering, r=compiler-errors

Clean up AstConv

Split off from rust-lang#120926 to make it only contain the renaming & (doc) comment updates.
Any changes other than that which have accumulated over time are now part of this PR.
Let's be disciplined ;) Inspired by rust-lang#120926 (comment).

---

* Remove `hir_trait_to_predicates`
  * Unused since rust-lang#113671
* Inline `create_args_for_ast_trait_ref`
  * Only had a single call site
  * Having it as a separate method didn't gain us anything
* Use an if-let guard somewhere to avoid unwrapping
* Avoid explicit trait object lifetimes
  * More legible, stylistic-only (the updated code is 100% semantically identical)
  * Use explicitly elided lifetimes in impl headers, they get elaborated to distinct lifetimes
  * Make use of [object lifetime defaulting](https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes) for a trait object type inside of a reference type somewhere
* Use preexisting dedicated method `ItemCtxt::to_ty` over `<dyn AstConv<'_>>::ast_ty_to_ty`
* Use preexisting dedicated method `AstConv::astconv` over explicit coercions
* Simplify the function signature of `create_args_for_ast_path` and of `check_generic_arg_count`
  * In both cases redundant information was passed rendering the call sites verbose and confusing
  * No perf impact (tested in [rust-lang#120926](rust-lang#120926))
* Move diagnostic method `report_ambiguous_associated_type` from `astconv` to `astconv::errors`
  * The submodule `errors` exists specifically for that purpose
  * Use it to keep the main module clean & short
@rustbot rustbot added this to the 1.78.0 milestone Mar 15, 2024
@fmease fmease deleted the clean-up-hir-ty-lowering branch March 15, 2024 12:29
@fmease fmease mentioned this pull request Mar 16, 2024
Merged
fmease added a commit to fmease/rust that referenced this pull request Mar 16, 2024
@fmease
Rollup merge of rust-lang#122577 - fmease:speculative-say-what, r=com…
4cbfa15 
…piler-errors

Remove obsolete parameter `speculative` from `instantiate_poly_trait_ref`

In rust-lang#122527 I totally missed that `speculative` has become obsolete with the removal of `hir_trait_to_predicates` / due to rust-lang#113671.

Fixes rust-lang#114635.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
@rust-timer
Unrolled build for rust-lang#122577
d9c600c 
Rollup merge of rust-lang#122577 - fmease:speculative-say-what, r=compiler-errors

Remove obsolete parameter `speculative` from `instantiate_poly_trait_ref`

In rust-lang#122527 I totally missed that `speculative` has become obsolete with the removal of `hir_trait_to_predicates` / due to rust-lang#113671.

Fixes rust-lang#114635.

r? `@compiler-errors`
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.78.0
Development

Successfully merging this pull request may close these issues.
4 participants
@fmease @compiler-errors @bors @rustbot