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.

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

[MCP 723] Rename astconv::AstConv and related items #120926

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Feb 11, 2024

See rust-lang/compiler-team#723.
Corresponding rustc-dev-guide PR: rust-lang/rustc-dev-guide#1916.

Please consult the following normative list of changes here:
https://fmease.dev/rustc-dev/astconv-no-mo.html (2024-03-22 archive link).

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. labels Feb 11, 2024
@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 11, 2024
@fmease fmease added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. 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 Feb 11, 2024
@rust-log-analyzer

This comment has been minimized.

fmease

This comment was marked as resolved.

@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Feb 11, 2024
@bors

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@fmease fmease removed the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Feb 21, 2024
@fmease fmease changed the title [WIP] [waiting on MCP] Rename astconv::AstConv and related items [WIP] [MCP 723] Rename astconv::AstConv and related items Feb 21, 2024
This makes it easier to read the trait definition for newcomers:
Sorted from least “complex” to most “complex” followed by trivial “plumbing”
and grouped by area.

* Move `allow_infer` above all `*_infer` methods
  * It's the least complex method of those
  * Allows the `*_infer` to be placed right next to each other
* Move `probe_ty_param_bounds` further down right next to `lower_assoc_ty` and `probe_adt`
  * It's more complex than the `infer` methods, it should come “later”
  * Now all required lowering functions are grouped together
* Move the “plumbing” function `set_tainted_by_errors` further down
  below any actual lowering methods.
* Provided method should come last
@fmease fmease marked this pull request as ready for review March 22, 2024 05:53
@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

@fmease
Copy link
Member Author

fmease commented Mar 22, 2024

Alrighty, the renaming of items and the updating of all relevant (doc) comments has been completed at last!

Best reviewed commit by commit. Some commits contain a short explainer outlining the motivation for the respective changes if not entirely obvious.

Please consult the following exhaustive & normative list of changes here:
https://fmease.dev/rustc-dev/astconv-no-mo.html (2024-03-22 archive link).
The MCP doesn't contain every single change but it does contain the majority of it.

Feel free to suggest better names for individual items but keep in mind that most of the name changes are well motivated and are part of a cohesive naming scheme. If you do please double-check first if there's an explainer for the name change in question over at the linked web page.

Ping @oli-obk @compiler-errors

@fmease fmease changed the title [WIP] [MCP 723] Rename astconv::AstConv and related items [MCP 723] Rename astconv::AstConv and related items Mar 22, 2024
@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 Mar 22, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Mar 22, 2024

<3 Thanks for doing this. The html doc is amazing. Still reading the doc, then will give this PR a review

@bors p=10 bitrotty

/// **A note on binders:** there is an implied binder around
/// `param_ty` and `ast_bounds`. See `instantiate_poly_trait_ref`
/// for more details.
/// There is an implied binder around `param_ty` and `ast_bounds`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "implied binder" mean here? If no binder is specified, you get an empty binder?

Copy link
Member Author

@fmease fmease Mar 22, 2024

Choose a reason for hiding this comment

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

It just means that param_ty: Ty<'tcx> may contain late-bound vars even though it's not wrapped in a Binder<'tcx, ·> (rendering them escaping I guess). Neither is ast_bounds/hir_bounds but well that param is of a HIR data type, so obviously it can't be wrapped in a Binder.

Wording is a bit poor, I guess I could've adjusted it.

@@ -37,6 +37,7 @@ pub use crate::traits::{MethodViolationCode, ObjectSafetyViolation};
/// Currently that is `Self` in supertraits. This is needed
/// because `object_safety_violations` can't be used during
/// type collection.
#[instrument(level = "debug", skip(tcx))]
Copy link
Contributor

Choose a reason for hiding this comment

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

could also use ret here

@oli-obk
Copy link
Contributor

oli-obk commented Mar 22, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2024

📌 Commit 5e73a8b has been approved by oli-obk

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 22, 2024
@bors
Copy link
Contributor

bors commented Mar 22, 2024

⌛ Testing commit 5e73a8b with merge eff958c...

@bors
Copy link
Contributor

bors commented Mar 22, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing eff958c to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (eff958c): 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)

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

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

Binary size

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

Bootstrap: 669.305s -> 669.088s (-0.03%)
Artifact size: 315.01 MiB -> 314.99 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself merged-by-bors This PR was explicitly merged by bors. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants