Various `HirTyLowerer` cleanups by oli-obk · Pull Request #125819 · rust-lang/rust

oli-obk · 2024-05-31T14:04:27Z

Previously there was some ad-hoc specialization going on, because you could call allows_infer, which basically was deciding between whether the trait object was backed by FnCtxt or by ItemCtxt. I moved all the different logic into dedicated methods on HirTyLowerer and removed allows_infer

best reviewed commit-by-commit

rustbot · 2024-05-31T14:04:35Z

r? @fmease

rustbot has assigned @fmease.
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

rustbot · 2024-05-31T14:04:36Z

HIR ty lowering was modified

cc @fmease

fmease · 2024-05-31T14:37:11Z

Wasn't it a very intentional decision back in the day to have ItemCtxt not contain InferCtxt to prevent ppl from ever "relying" on it (i.e., accidentally performing inference in items). Ofc, we still have the assert check_type (but calling it could easily be forgotten) and we still don't populate TypeckResults for non-bodies.

compiler-errors

Yeah, I concur w/ @fmease here. I think adding an InferCtxt gives rustc devs a much easier chance to misuse it.

compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs

oli-obk · 2024-05-31T14:59:13Z

Yea... I was wondering about that. I'd think we'd want to use a full InferCtxt in the future and collect all the results.

fmease · 2024-05-31T15:11:33Z

Do you mean in preparation for potential changes to language like rust-lang/rfcs#3546 (which hasn't been accepted of course)? Or do you have something else in mind? To be honest, I haven't looked at the linked issue yet or at the issues in tests/crashes/ this PR fixes ^^'?

oli-obk · 2024-06-03T09:28:03Z

I removed the InferCtxt changes and left just the refactorings that do not have user-visible effects

bors · 2024-06-04T11:52:30Z

☔ The latest upstream changes (presumably #125960) made this pull request unmergeable. Please resolve the merge conflicts.

fmease

sorry for the delay! that's very nice cleanup, thanks a lot! r=me with nits addressed

fmease · 2024-06-04T12:42:38Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs

@@ -90,7 +90,7 @@ pub trait HirTyLowerer<'tcx> {
    fn tcx(&self) -> TyCtxt<'tcx>;

    /// Returns the [`DefId`] of the overarching item whose constituents get lowered.


Suggested change

/// Returns the [`DefId`] of the overarching item whose constituents get lowered.

/// Returns the [`LocalDefId`] of the overarching item whose constituents get lowered.

fmease · 2024-06-04T12:49:38Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs

-    fn re_infer(&self, param: Option<&ty::GenericParamDef>, span: Span)
-    -> Option<ty::Region<'tcx>>;
+    ///
+    /// The `borrowed` argument states whether this lifetime is from a reference.


it's object_lifetime_default not borrowed (copy/paste error from lower_ty_common?)

fmease · 2024-06-04T12:50:45Z

compiler/rustc_hir_analysis/src/collect.rs

+        &self,
+        _: Option<&ty::GenericParamDef>,
+        span: Span,
+        object_lifetime_default: bool,


would love this bool to be a custom enum instead but that's not blocking

fmease · 2024-06-04T12:58:10Z

compiler/rustc_hir_analysis/src/collect.rs

+            // We will have already emitted an error E0106 complaining about a
+            // missing named lifetime in `&dyn Trait`, so we elide this one.


this comment doesn't make sense anymore. it was about delay_as_bug vs emit depending on borrowed : bool which you no longer do

fmease · 2024-06-04T14:57:59Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs

 }

+#[derive(Debug)]
+pub enum RegionInferReason<'a> {


Beautiful 😭 ❤️

I guess we don't actually use most of the reasons but anyway...

Yea, but that's true for obligation causes, too :D maybe we should. I dislike those delay span bugs

fmease · 2024-06-04T14:59:31Z

@bors r+ rollup

bors · 2024-06-04T14:59:34Z

📌 Commit ac117f0 has been approved by fmease

It is now in the queue for this repository.

bors · 2024-06-04T17:07:58Z

☔ The latest upstream changes (presumably #125976) made this pull request unmergeable. Please resolve the merge conflicts.

…r (can) use it for local items

`ct_infer` and `lower_ty` will correctly result in an error constant or type respectively, as they go through a `HirTyLowerer` method (just like `HirTyLowerer::allow_infer` is a method implemented by both implementors

…y forbidden

…Lowerer`

…n enum that exhaustively supports all call sites

oli-obk · 2024-06-05T09:17:56Z

@bors r=fmease

bors · 2024-06-05T09:17:58Z

📌 Commit c8a331a has been approved by fmease

It is now in the queue for this repository.

bors · 2024-06-05T09:17:59Z

🌲 The tree is currently closed for pull requests below priority 101. This pull request will be tested once the tree is reopened.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#124746 (`rustc --explain E0582` additional example) - rust-lang#125407 (Detect when user is trying to create a lending `Iterator` and give a custom explanation) - rust-lang#125505 (Add intra-doc-links to rustc_middle crate-level docs.) - rust-lang#125792 (Don't drop `Unsize` candidate in intercrate mode) - rust-lang#125819 (Various `HirTyLowerer` cleanups) - rust-lang#125861 (rustc_codegen_ssa: fix `get_rpath_relative_to_output` panic when lib only contains file name) - rust-lang#125911 (delete bootstrap build before switching to bumped rustc) - rust-lang#125921 (coverage: Carve out hole spans in a separate early pass) - rust-lang#125940 (std::unix::fs::get_path: using fcntl codepath for netbsd instead.) - rust-lang#126022 (set `has_unconstrained_ty_var` when generalizing aliases in bivariant contexts) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#125819 - oli-obk:localize, r=fmease Various `HirTyLowerer` cleanups Previously there was some ad-hoc specialization going on, because you could call `allows_infer`, which basically was deciding between whether the trait object was backed by `FnCtxt` or by `ItemCtxt`. I moved all the different logic into dedicated methods on `HirTyLowerer` and removed `allows_infer` best reviewed commit-by-commit

rustbot assigned fmease May 31, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2024

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 31, 2024

This comment has been minimized.

Sign in to view

compiler-errors suggested changes May 31, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the localize branch from 3694975 to 5f67780 Compare June 3, 2024 09:26

fmease approved these changes Jun 4, 2024

View reviewed changes

oli-obk force-pushed the localize branch from 5f67780 to ac117f0 Compare June 4, 2024 13:36

fmease mentioned this pull request Jun 4, 2024

Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking #125978

Merged

fmease approved these changes Jun 4, 2024

View reviewed changes

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 Jun 4, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2024

oli-obk added 6 commits June 5, 2024 08:43

Use a LocalDefId for HirTyLowerer::item_def_id, since we only eve…

dc020ae

…r (can) use it for local items

Simplify some code paths and remove an unused field

9d387d1

`ct_infer` and `lower_ty` will correctly result in an error constant or type respectively, as they go through a `HirTyLowerer` method (just like `HirTyLowerer::allow_infer` is a method implemented by both implementors

Remove an Option and instead eagerly create error lifetimes

abd308b

Only collect infer vars to error about in case infer vars are actuall…

6a2e15a

…y forbidden

Remove allows_infer now that every use of it is delegated to `HirTy…

7870674

…Lowerer`

Remove some unnecessary explicit lifetimes

4146b82

Unify optional param info with object lifetime default boolean into a…

c8a331a

…n enum that exhaustively supports all call sites

oli-obk force-pushed the localize branch from ac117f0 to c8a331a Compare June 5, 2024 09:17

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2024

matthiaskrgr mentioned this pull request Jun 5, 2024

Rollup of 10 pull requests #126037

Merged

bors merged commit 94c19c6 into rust-lang:master Jun 5, 2024

rustbot added this to the 1.80.0 milestone Jun 5, 2024

fmease mentioned this pull request Nov 4, 2024

Document HIR ty lowering rust-lang/rustc-dev-guide#178

Open

4 tasks

		@@ -90,7 +90,7 @@ pub trait HirTyLowerer<'tcx> {
		fn tcx(&self) -> TyCtxt<'tcx>;

		/// Returns the [`DefId`] of the overarching item whose constituents get lowered.

	/// Returns the [`DefId`] of the overarching item whose constituents get lowered.
	/// Returns the [`LocalDefId`] of the overarching item whose constituents get lowered.

		// We will have already emitted an error E0106 complaining about a
		// missing named lifetime in `&dyn Trait`, so we elide this one.

Uh oh!

Conversation

oli-obk commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 31, 2024

Uh oh!

rustbot commented May 31, 2024

Uh oh!

This comment has been minimized.

fmease commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented May 31, 2024

Uh oh!

fmease commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jun 3, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

fmease left a comment

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

fmease Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

oli-obk Jun 4, 2024

Choose a reason for hiding this comment

Uh oh!

fmease commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

oli-obk commented Jun 5, 2024

Uh oh!

bors commented Jun 5, 2024

Uh oh!

bors commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

oli-obk commented May 31, 2024 •

edited

Loading

fmease commented May 31, 2024 •

edited

Loading

fmease commented May 31, 2024 •

edited

Loading

fmease Jun 4, 2024 •

edited

Loading

fmease Jun 4, 2024 •

edited

Loading