Skip to content

Seperate HIR owner from LocalDefId in the type system#85587

Closed
ABouttefeux wants to merge 8 commits intorust-lang:masterfrom
ABouttefeux:separate-hir-owner
Closed

Seperate HIR owner from LocalDefId in the type system#85587
ABouttefeux wants to merge 8 commits intorust-lang:masterfrom
ABouttefeux:separate-hir-owner

Conversation

@ABouttefeux
Copy link
Contributor

@ABouttefeux ABouttefeux commented May 22, 2021 •
edited
Loading

closes #83158

Separate definitions and HIR owners in the type system

@rust-highfive
Copy link
Contributor

rust-highfive commented May 22, 2021

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2021
@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented May 22, 2021

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 May 22, 2021
@ABouttefeux ABouttefeux force-pushed the separate-hir-owner branch from a931dd9 to bfca4e9 Compare May 22, 2021 18:27
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ABouttefeux ABouttefeux force-pushed the separate-hir-owner branch from d7ce673 to 79f4394 Compare May 23, 2021 10:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ABouttefeux ABouttefeux force-pushed the separate-hir-owner branch from 513d3b8 to 7622b46 Compare May 23, 2021 13:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ABouttefeux ABouttefeux force-pushed the separate-hir-owner branch from 9e11425 to 33e3db1 Compare May 23, 2021 16:06
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ABouttefeux ABouttefeux marked this pull request as ready for review May 23, 2021 16:52
bjorn3
bjorn3 reviewed May 24, 2021
View reviewed changes
compiler/rustc_hir/src/hir.rs Outdated
Copy link
Member

@bjorn3 bjorn3 May 24, 2021

Choose a reason for hiding this comment

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

I think owner would be a better name now for this field here and elsewhere.

Copy link
Contributor Author

@ABouttefeux ABouttefeux May 25, 2021

Choose a reason for hiding this comment

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

There is already a struct Owner here

rust/compiler/rustc_middle/src/hir/mod.rs

Lines 34 to 37 in fbf1b1a

#[derive(Debug)]
pub struct Owner<'tcx> {
node: Node<'tcx>,
}

bjorn3
bjorn3 reviewed May 24, 2021
View reviewed changes
src/tools/clippy/clippy_utils/src/usage.rs Outdated
Copy link
Member

@bjorn3 bjorn3 May 24, 2021

Choose a reason for hiding this comment

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

Can you please revert all those formatting changes?

Copy link
Contributor Author

@ABouttefeux ABouttefeux May 24, 2021

Choose a reason for hiding this comment

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

I didn't notice I am reverting it.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ABouttefeux ABouttefeux force-pushed the separate-hir-owner branch from cf4d985 to 70920c6 Compare May 25, 2021 15:31
@bors
Copy link
Collaborator

bors commented May 31, 2021

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

@crlf0710
Copy link
Member

crlf0710 commented Jun 26, 2021

@ABouttefeux Ping from triage. There's merge conflict now.

@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 26, 2021
cjgillot
cjgillot requested changes Jun 27, 2021
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @ABouttefeux and sorry for the delay!
Overall, this is going in the right direction.
There is still a lot of back-and-forth between HirOwner and LocalDefId. I left a few comment to reduce the conversion glue.

compiler/rustc_ast_lowering/src/lib.rs
trace!("lower_opaque_impl_trait: {:#?}", opaque_ty_def_id);
lctx.generate_opaque_type(opaque_ty_def_id, opaque_ty_item, span, opaque_ty_span);
lctx.generate_opaque_type(
HirOwner { def_id: opaque_ty_def_id },
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

allocate_hir_id_counter should return a HirOwner you can use here instead of constructing it each time.

compiler/rustc_ast_lowering/src/lib.rs
trace!("exist ty from async fn def id: {:#?}", opaque_ty_def_id);
this.generate_opaque_type(opaque_ty_def_id, opaque_ty_item, span, opaque_ty_span);
this.generate_opaque_type(
HirOwner { def_id: opaque_ty_def_id },
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

Likewise

compiler/rustc_hir/src/hir_id.rs
use std::fmt;

#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub struct HirOwner {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

bikeshed: would OwnerId be more consistent?

compiler/rustc_ast_lowering/src/lib.rs
type_def_lifetime_params: Default::default(),
current_module: CRATE_DEF_ID,
current_hir_id_owner: (CRATE_DEF_ID, 0),
current_hir_id_owner: (HirOwner { def_id: CRATE_DEF_ID }, 0),
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

A CRATE_OWNER_ID const could be handy.

compiler/rustc_hir/src/hir_id.rs
}
}

impl rustc_index::vec::Idx for HirOwner {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

HirOwner shoud not implement Idx, since not all HirOwner values are semantically valid.

compiler/rustc_hir/src/hir_id.rs
self.def_id
}

pub fn to_def_id(&self) -> DefId {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

This deserves to be #[inline].

compiler/rustc_hir/src/hir_id.rs
}

impl HirOwner {
pub fn def_id(&self) -> LocalDefId {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

Is this method useful?
It deserves to be #[inline].

compiler/rustc_middle/src/ty/query/mod.rs
}
}

impl IntoQueryParam<DefId> for HirOwner {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

Great idea!
An impl IntoQueryParam<LocalDefId> can also help a lot.

compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs
let opt_suggestions = path_segment
.hir_id
.map(|path_hir_id| self.infcx.tcx.typeck(path_hir_id.owner))
.map(|path_hir_id| self.infcx.tcx.typeck(path_hir_id.owner.def_id))
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

Can typeck take a HirOwner?

compiler/rustc_mir/src/borrow_check/universal_regions.rs
@@ -804,7 +804,7 @@ fn for_each_late_bound_region_defined_on<'tcx>(
) {
if let Some((owner, late_bounds)) = tcx.is_late_bound_map(fn_def_id.expect_local()) {
Copy link
Contributor

@cjgillot cjgillot Jun 27, 2021

Choose a reason for hiding this comment

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

Can this owner be a HirOwner directly?

@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented Jun 27, 2021

Sorry, currently I am hospitalized and I am not able to work in this PR.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@bstrie
Copy link
Contributor

bstrie commented Jul 28, 2021

@ABouttefeux Hope you feel better soon!

@crlf0710
Copy link
Member

crlf0710 commented Aug 14, 2021

@ABouttefeux Ping from triage! Any updates on this?

@ABouttefeux
Copy link
Contributor Author

ABouttefeux commented Aug 17, 2021

I am still in the hospital and unable to work on it. It may take a while before I am able to work on it.

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Sep 6, 2021
@JohnCSimon
Copy link
Member

JohnCSimon commented Sep 6, 2021

Ping from triage:

@ABouttefeux closing this as inactive. Please feel free to reopen when you are ready to continue.

@JohnCSimon JohnCSimon closed this Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@cjgillot cjgillot cjgillot requested changes

Assignees

@cjgillot cjgillot

Labels

S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Separate definitions and HIR owners in the type system

10 participants

@ABouttefeux @rust-highfive @rust-log-analyzer @bors @crlf0710 @bstrie @JohnCSimon @cjgillot @bjorn3 @jackh726