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

Various HirTyLowerer cleanups #125819

Merged
merged 7 commits into from
Jun 5, 2024
Merged

Various HirTyLowerer cleanups #125819

merged 7 commits into from
Jun 5, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 31, 2024

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
Copy link
Collaborator

rustbot commented May 31, 2024

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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 31, 2024

HIR ty lowering was modified

cc @fmease

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

This comment has been minimized.

@fmease
Copy link
Member

fmease commented May 31, 2024

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.

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.

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 Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented May 31, 2024

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
Copy link
Member

fmease commented May 31, 2024

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
Copy link
Contributor Author

oli-obk commented Jun 3, 2024

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

@bors
Copy link
Contributor

bors commented Jun 4, 2024

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

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

&self,
_: Option<&ty::GenericParamDef>,
span: Span,
object_lifetime_default: bool,
Copy link
Member

@fmease fmease Jun 4, 2024

Choose a reason for hiding this comment

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

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

Comment on lines 388 to 389
// We will have already emitted an error E0106 complaining about a
// missing named lifetime in `&dyn Trait`, so we elide this one.
Copy link
Member

@fmease fmease Jun 4, 2024

Choose a reason for hiding this comment

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

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

@@ -82,22 +80,32 @@ pub enum PredicateFilter {
SelfAndAssociatedTypeBounds,
}

#[derive(Debug)]
pub enum RegionInferReason<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful 😭 ❤️

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@fmease
Copy link
Member

fmease commented Jun 4, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit ac117f0 has been approved by fmease

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

bors commented Jun 4, 2024

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

@bors 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
`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
…n enum that exhaustively supports all call sites
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 5, 2024

@bors r=fmease

@bors
Copy link
Contributor

bors commented Jun 5, 2024

📌 Commit c8a331a has been approved by fmease

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 5, 2024

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

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
…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
@bors bors merged commit 94c19c6 into rust-lang:master Jun 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

6 participants