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.

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

Seperate HIR owner from LocalDefId in the type system #85587

Closed
wants to merge 8 commits into from
Closed

Seperate HIR owner from LocalDefId in the type system #85587

wants to merge 8 commits into from

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
Collaborator

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

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned jackh726 May 22, 2021
@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
@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
@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 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
@@ -2890,7 +2890,7 @@ pub struct ForeignItemRef<'hir> {
pub struct ForeignItem<'hir> {
pub ident: Ident,
pub kind: ForeignItemKind<'hir>,
pub def_id: LocalDefId,
pub def_id: HirOwner,
Copy link
Member

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

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
@@ -189,7 +194,7 @@ impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor {
} else {
rustc_hir::intravisit::walk_expr(self, ex);
}
},
}
Copy link
Member

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

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

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

@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
@@ -1512,18 +1514,26 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
};

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

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
@@ -2015,7 +2025,12 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
};

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

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

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
@@ -322,7 +322,7 @@ pub fn lower_crate<'a, 'hir>(
anonymous_lifetime_mode: AnonymousLifetimeMode::PassThrough,
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

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

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

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

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
@@ -308,6 +308,13 @@ mod sealed {
self.to_def_id()
}
}

impl IntoQueryParam<DefId> for HirOwner {
Copy link
Contributor

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
@@ -698,7 +698,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
{
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

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

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

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

@ABouttefeux Ping from triage! Any updates on this?

@ABouttefeux
Copy link
Contributor Author

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

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