Skip to content

Don't opt_rpitit_info as a separate query #109057

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

No Sign up for GitHub

By clicking “No 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? No Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2023
+31 −23
Merged

Don't opt_rpitit_info as a separate query #109057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/src/method/probe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,7 @@ impl<'tcx> Pick<'tcx> {
container: _,
trait_item_def_id: _,
fn_has_self_parameter: _,
opt_rpitit_info: _,
},
kind: _,
import_ids: _,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,6 +1096,8 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
trait_item_def_id: self.get_trait_item_def_id(id),
container,
fn_has_self_parameter: has_self,
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty): We need to encode this
opt_rpitit_info: None,
}
}

Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/hir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ pub fn provide(providers: &mut Providers) {
}
};
providers.opt_def_kind = |tcx, def_id| tcx.hir().opt_def_kind(def_id.expect_local());
providers.opt_rpitit_info = |_, _| None;
providers.all_local_trait_impls = |tcx, ()| &tcx.resolutions(()).trait_impls;
providers.expn_that_defined = |tcx, id| {
let id = id.expect_local();
Expand Down
8 changes: 0 additions & 8 deletions compiler/rustc_middle/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,14 +1161,6 @@ rustc_queries! {
feedable
}

/// The `opt_rpitit_info` query returns the pair of the def id of the function where the RPIT
/// is defined and the opaque def id if any.
query opt_rpitit_info(def_id: DefId) -> Option<ty::ImplTraitInTraitData> {
desc { |tcx| "opt_rpitit_info `{}`", tcx.def_path_str(def_id) }
cache_on_disk_if { def_id.is_local() }
feedable
}

/// Gets the span for the definition.
query def_span(def_id: DefId) -> Span {
desc { |tcx| "looking up span for `{}`", tcx.def_path_str(def_id) }
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/ty/assoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ pub struct AssocItem {
/// Whether this is a method with an explicit self
/// as its first parameter, allowing method calls.
pub fn_has_self_parameter: bool,

/// `Some` if the associated item (an associated type) comes from the
/// return-position `impl Trait` in trait desugaring. The `ImplTraitInTraitData`
/// provides additional information about its source.
pub opt_rpitit_info: Option<ty::ImplTraitInTraitData>,
}

impl AssocItem {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2452,7 +2452,7 @@ impl<'tcx> TyCtxt<'tcx> {

pub fn is_impl_trait_in_trait(self, def_id: DefId) -> bool {
if self.lower_impl_trait_in_trait_to_assoc_ty() {
self.def_kind(def_id) == DefKind::AssocTy && self.opt_rpitit_info(def_id).is_some()
self.opt_rpitit_info(def_id).is_some()
} else {
self.def_kind(def_id) == DefKind::ImplTraitPlaceholder
}
Expand Down
15 changes: 14 additions & 1 deletion compiler/rustc_middle/src/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2070,7 +2070,9 @@ pub enum ImplOverlapKind {
Issue33140,
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, TyEncodable, TyDecodable, HashStable)]
/// Useful source information about where a desugared associated type for an
/// RPITIT originated from.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Encodable, Decodable, HashStable)]
pub enum ImplTraitInTraitData {
Trait { fn_def_id: DefId, opaque_def_id: DefId },
Impl { fn_def_id: DefId },
Expand Down Expand Up @@ -2213,6 +2215,17 @@ impl<'tcx> TyCtxt<'tcx> {
}
}

/// If the def-id is an associated type that was desugared from a
/// return-position `impl Trait` from a trait, then provide the source info
/// about where that RPITIT came from.
pub fn opt_rpitit_info(self, def_id: DefId) -> Option<ImplTraitInTraitData> {
if let DefKind::AssocTy = self.def_kind(def_id) {
self.associated_item(def_id).opt_rpitit_info
} else {
None
}
}

pub fn find_field_index(self, ident: Ident, variant: &VariantDef) -> Option<usize> {
variant
.fields
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_passes/src/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}

// Avoid accessing the HIR for the synthesized associated type generated for RPITITs.
if self.tcx.opt_rpitit_info(id).is_some() {
if self.tcx.opt_rpitit_info(id.to_def_id()).is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's makes the opt_rpitit_info query be fired that much, if it's this call or the other one (because there are just 2), but this call could be the one. I was wondering, what if we make find_by_def_id called below return None for rpitits and the code on line 256 would just be ...

self.live_symbols.insert(id);

if let Some(node) = self.tcx.hir().find_by_def_id(id) {
    self.visit_node(node);
}

I'm not sure if there's any other situation where find_by_def_id returns None and we don't want to do self.live_symbols.insert(id), though.

Copy link
Member

Choose a reason for hiding this comment

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

With this patch ...

diff --git a/compiler/rustc_middle/src/hir/map/mod.rs b/compiler/rustc_middle/src/hir/map/mod.rs
index 746cf488589..e76de89e432 100644
--- a/compiler/rustc_middle/src/hir/map/mod.rs
+++ b/compiler/rustc_middle/src/hir/map/mod.rs
@@ -316,7 +316,7 @@ pub fn find(self, id: HirId) -> Option<Node<'hir>> {
     /// Retrieves the `Node` corresponding to `id`, returning `None` if cannot be found.
     #[inline]
     pub fn find_by_def_id(self, id: LocalDefId) -> Option<Node<'hir>> {
-        self.find(self.local_def_id_to_hir_id(id))
+        self.find(self.tcx.opt_local_def_id_to_hir_id(id)?)
     }

     /// Retrieves the `Node` corresponding to `id`, panicking if it cannot be found.
diff --git a/compiler/rustc_passes/src/dead.rs b/compiler/rustc_passes/src/dead.rs
index 73ee51d5f3a..1d713bbd055 100644
--- a/compiler/rustc_passes/src/dead.rs
+++ b/compiler/rustc_passes/src/dead.rs
@@ -243,18 +243,12 @@ fn mark_live_symbols(&mut self) {
                 continue;
             }

-            // Avoid accessing the HIR for the synthesized associated type generated for RPITITs.
-            if self.tcx.opt_rpitit_info(id).is_some() {
-                self.live_symbols.insert(id);
-                continue;
-            }
-
             // in the case of tuple struct constructors we want to check the item, not the generated
             // tuple struct constructor function
             let id = self.struct_constructors.get(&id).copied().unwrap_or(id);

+            self.live_symbols.insert(id);
             if let Some(node) = self.tcx.hir().find_by_def_id(id) {
-                self.live_symbols.insert(id);
                 self.visit_node(node);
             }
         }

all the tests pass. May worth running perf on this?.

Copy link
Member Author

Choose a reason for hiding this comment

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

That change seems orthogonal to this one -- this PR still demonstrates that we don't need opt_rpitit_info as a query, which is a change I want anyways because storing it in the AssocTy just seems cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, if we keep opt_rpitit_info around, other PRs may add other calls to the query that may cause regressions. Let's avoid this by fixing the problem at its source. This optimization may also be helpful, feel free to test it if you want, but I'm gonna land this PR I think.

if it's this call or the other one (because there are just 2)

There are 5 calls to opt_rpitit_info that I can see in the codebase.

self.live_symbols.insert(id);
continue;
}
Expand Down
18 changes: 7 additions & 11 deletions compiler/rustc_ty_utils/src/assoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ fn associated_item_from_trait_item_ref(trait_item_ref: &hir::TraitItemRef) -> ty
trait_item_def_id: Some(owner_id.to_def_id()),
container: ty::TraitContainer,
fn_has_self_parameter: has_self,
opt_rpitit_info: None,
}
}

Expand All @@ -171,6 +172,7 @@ fn associated_item_from_impl_item_ref(impl_item_ref: &hir::ImplItemRef) -> ty::A
trait_item_def_id: impl_item_ref.trait_item_def_id,
container: ty::ImplContainer,
fn_has_self_parameter: has_self,
opt_rpitit_info: None,
}
}

Expand Down Expand Up @@ -262,19 +264,17 @@ fn associated_item_for_impl_trait_in_trait(
// Copy span of the opaque.
trait_assoc_ty.def_ident_span(Some(span));

// Add the def_id of the function and opaque that generated this synthesized associated type.
trait_assoc_ty.opt_rpitit_info(Some(ImplTraitInTraitData::Trait {
fn_def_id,
opaque_def_id: opaque_ty_def_id.to_def_id(),
}));

trait_assoc_ty.associated_item(ty::AssocItem {
name: kw::Empty,
kind: ty::AssocKind::Type,
def_id,
trait_item_def_id: None,
container: ty::TraitContainer,
fn_has_self_parameter: false,
opt_rpitit_info: Some(ImplTraitInTraitData::Trait {
fn_def_id,
opaque_def_id: opaque_ty_def_id.to_def_id(),
}),
});

// Copy visility of the containing function.
Expand Down Expand Up @@ -328,18 +328,14 @@ fn impl_associated_item_for_impl_trait_in_trait(
// `opt_local_def_id_to_hir_id` with `None`.
impl_assoc_ty.opt_local_def_id_to_hir_id(None);

// Add the def_id of the function that generated this synthesized associated type.
impl_assoc_ty.opt_rpitit_info(Some(ImplTraitInTraitData::Impl {
fn_def_id: impl_fn_def_id.to_def_id(),
}));

impl_assoc_ty.associated_item(ty::AssocItem {
name: kw::Empty,
kind: ty::AssocKind::Type,
def_id,
trait_item_def_id: Some(trait_assoc_def_id.to_def_id()),
container: ty::ImplContainer,
fn_has_self_parameter: false,
opt_rpitit_info: Some(ImplTraitInTraitData::Impl { fn_def_id: impl_fn_def_id.to_def_id() }),
});

// Copy param_env of the containing function. The synthesized associated type doesn't have
Expand Down