Skip to content

Remove redundant match arm for RPITITs #113340

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

Closed
wants to merge 2 commits into from
Closed

Remove redundant match arm for RPITITs #113340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7234864
Replace RPITIT current impl with new strategy that lowers as a GAT
spastorino Jun 24, 2023
9547302
Remove redundant match arm for RPITITs
spastorino Jul 4, 2023
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
7 changes: 1 addition & 6 deletions compiler/rustc_hir/src/def.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ pub enum DefKind {
InlineConst,
/// Opaque type, aka `impl Trait`.
OpaqueTy,
/// A return-position `impl Trait` in a trait definition
ImplTraitPlaceholder,
Field,
/// Lifetime parameter: the `'a` in `struct Foo<'a> { ... }`
LifetimeParam,
Expand Down Expand Up @@ -143,7 +141,6 @@ impl DefKind {
DefKind::Ctor(CtorOf::Struct, CtorKind::Fn) => "tuple struct",
DefKind::Ctor(CtorOf::Struct, CtorKind::Const) => "unit struct",
DefKind::OpaqueTy => "opaque type",
DefKind::ImplTraitPlaceholder => "opaque type in trait",
DefKind::TyAlias => "type alias",
DefKind::TraitAlias => "trait alias",
DefKind::AssocTy => "associated type",
Expand Down Expand Up @@ -227,8 +224,7 @@ impl DefKind {
| DefKind::Use
| DefKind::ForeignMod
| DefKind::GlobalAsm
| DefKind::Impl { .. }
| DefKind::ImplTraitPlaceholder => None,
| DefKind::Impl { .. } => None,
}
}

Expand Down Expand Up @@ -262,7 +258,6 @@ impl DefKind {
| DefKind::Use
| DefKind::ForeignMod
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Impl { .. }
| DefKind::Field
| DefKind::TyParam
Expand Down
11 changes: 1 addition & 10 deletions compiler/rustc_hir/src/target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ pub enum Target {
GlobalAsm,
TyAlias,
OpaqueTy,
ImplTraitPlaceholder,
Enum,
Variant,
Struct,
Expand Down Expand Up @@ -80,13 +79,7 @@ impl Target {
ItemKind::ForeignMod { .. } => Target::ForeignMod,
ItemKind::GlobalAsm(..) => Target::GlobalAsm,
ItemKind::TyAlias(..) => Target::TyAlias,
ItemKind::OpaqueTy(ref opaque) => {
if opaque.in_trait {
Target::ImplTraitPlaceholder
} else {
Target::OpaqueTy
}
}
ItemKind::OpaqueTy(..) => Target::OpaqueTy,
ItemKind::Enum(..) => Target::Enum,
ItemKind::Struct(..) => Target::Struct,
ItemKind::Union(..) => Target::Union,
Expand All @@ -110,7 +103,6 @@ impl Target {
DefKind::GlobalAsm => Target::GlobalAsm,
DefKind::TyAlias => Target::TyAlias,
DefKind::OpaqueTy => Target::OpaqueTy,
DefKind::ImplTraitPlaceholder => Target::ImplTraitPlaceholder,
DefKind::Enum => Target::Enum,
DefKind::Struct => Target::Struct,
DefKind::Union => Target::Union,
Expand Down Expand Up @@ -165,7 +157,6 @@ impl Target {
Target::GlobalAsm => "global asm",
Target::TyAlias => "type alias",
Target::OpaqueTy => "opaque type",
Target::ImplTraitPlaceholder => "opaque type in trait",
Target::Enum => "enum",
Target::Variant => "enum variant",
Target::Struct => "struct",
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_analysis/src/astconv/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2128,7 +2128,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {

let span = path.span;
match path.res {
Res::Def(DefKind::OpaqueTy | DefKind::ImplTraitPlaceholder, did) => {
Res::Def(DefKind::OpaqueTy, did) => {
// Check for desugared `impl Trait`.
assert!(tcx.is_type_alias_impl_trait(did));
let item_segment = path.segments.split_last().unwrap();
Expand Down Expand Up @@ -2439,7 +2439,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// If this is an RPITIT and we are using the new RPITIT lowering scheme, we
// generate the def_id of an associated type for the trait and return as
// type a projection.
let def_id = if in_trait && tcx.lower_impl_trait_in_trait_to_assoc_ty() {
let def_id = if in_trait {
tcx.associated_type_for_impl_trait_in_trait(local_def_id).to_def_id()
} else {
local_def_id.to_def_id()
Expand Down
22 changes: 4 additions & 18 deletions compiler/rustc_hir_analysis/src/check/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,11 @@ pub(super) fn check_opaque_for_inheriting_lifetimes(

if let ItemKind::OpaqueTy(&hir::OpaqueTy {
origin: hir::OpaqueTyOrigin::AsyncFn(..) | hir::OpaqueTyOrigin::FnReturn(..),
in_trait,
..
}) = item.kind
{
let substs = InternalSubsts::identity_for_item(tcx, def_id);
let opaque_identity_ty = if in_trait && !tcx.lower_impl_trait_in_trait_to_assoc_ty() {
Ty::new_projection(tcx, def_id.to_def_id(), substs)
} else {
Ty::new_opaque(tcx, def_id.to_def_id(), substs)
};
let opaque_identity_ty = Ty::new_opaque(tcx, def_id.to_def_id(), substs);
let mut visitor = ProhibitOpaqueVisitor {
opaque_identity_ty,
parent_count: tcx.generics_of(def_id).parent_count as u32,
Expand Down Expand Up @@ -576,17 +571,6 @@ fn check_item_type(tcx: TyCtxt<'_>, id: hir::ItemId) {
check_opaque(tcx, id);
}
}
DefKind::ImplTraitPlaceholder => {
let parent = tcx.impl_trait_in_trait_parent_fn(id.owner_id.to_def_id());
// Only check the validity of this opaque type if the function has a default body
if let hir::Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(_)),
..
}) = tcx.hir().get_by_def_id(parent.expect_local())
{
check_opaque(tcx, id);
}
}
DefKind::TyAlias => {
let pty_ty = tcx.type_of(id.owner_id).subst_identity();
let generics = tcx.generics_of(id.owner_id);
Expand Down Expand Up @@ -1485,7 +1469,9 @@ fn opaque_type_cycle_error(
}

for closure_def_id in visitor.closures {
let Some(closure_local_did) = closure_def_id.as_local() else { continue; };
let Some(closure_local_did) = closure_def_id.as_local() else {
continue;
};
let typeck_results = tcx.typeck(closure_local_did);

let mut label_match = |ty: Ty<'_>, span| {
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_hir_analysis/src/collect/item_bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,12 @@ pub(super) fn explicit_item_bounds(
..
}) => associated_type_bounds(tcx, def_id, bounds, *span),
hir::Node::Item(hir::Item {
kind: hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds, in_trait, .. }),
kind: hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds, .. }),
span,
..
}) => {
let substs = InternalSubsts::identity_for_item(tcx, def_id);
let item_ty = if *in_trait && !tcx.lower_impl_trait_in_trait_to_assoc_ty() {
Ty::new_projection(tcx, def_id.to_def_id(), substs)
} else {
Ty::new_opaque(tcx, def_id.to_def_id(), substs)
};
let item_ty = Ty::new_opaque(tcx, def_id.to_def_id(), substs);
opaque_type_bounds(tcx, def_id, bounds, item_ty, *span)
}
hir::Node::Item(hir::Item { kind: hir::ItemKind::TyAlias(..), .. }) => &[],
Expand Down
15 changes: 1 addition & 14 deletions compiler/rustc_hir_analysis/src/impl_wf_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,7 @@ fn enforce_impl_params_are_constrained(tcx: TyCtxt<'_>, impl_def_id: LocalDefId)
vec![]
}
}
ty::AssocKind::Fn => {
if !tcx.lower_impl_trait_in_trait_to_assoc_ty()
&& item.defaultness(tcx).has_value()
&& tcx.impl_method_has_trait_impl_trait_tys(item.def_id)
&& let Ok(table) = tcx.collect_return_position_impl_trait_in_trait_tys(def_id)
{
table.values().copied().flat_map(|ty| {
cgp::parameters_for(&ty.subst_identity(), true)
}).collect()
} else {
vec![]
}
}
ty::AssocKind::Const => vec![],
ty::AssocKind::Fn | ty::AssocKind::Const => vec![],
}
})
.collect();
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_hir_analysis/src/variance/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ fn variances_of(tcx: TyCtxt<'_>, item_def_id: LocalDefId) -> &[ty::Variance] {
let crate_map = tcx.crate_variances(());
return crate_map.variances.get(&item_def_id.to_def_id()).copied().unwrap_or(&[]);
}
DefKind::OpaqueTy | DefKind::ImplTraitPlaceholder => {
DefKind::OpaqueTy => {
return variance_of_opaque(tcx, item_def_id);
}
DefKind::AssocTy => {
Expand Down Expand Up @@ -122,13 +122,6 @@ fn variance_of_opaque(tcx: TyCtxt<'_>, item_def_id: LocalDefId) -> &[ty::Varianc
{
self.visit_opaque(*def_id, substs)
}
// FIXME(-Zlower-impl-trait-in-trait-to-assoc-ty) check whether this is necessary
Copy link
Member

Choose a reason for hiding this comment

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

Are you certain that this is correct to remove, or did you just remove this because no UI tests failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it just equivalent to the previous arm?. Like in this new scheme, def_kind is going to be OpaqueTy so this branch is never gonna be exercised.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it's not being called on any of the GAT def ids?

Copy link
Member Author

@spastorino spastorino Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, going to double check tomorrow. I guess you may be implying that for impls is executed because for traits shouldn't but I wouldn't be sure what would be the effect of calling this on impl's GAT. Going to check again tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, given that all test pass here would be good to come up with a test case that fails with this branch removed and works if we add this back.

Copy link
Member

Choose a reason for hiding this comment

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

For traits, walking the fn_sig will give you back projections, so it may happen. Yeah, this at least needs a bit more checking.

// at all for RPITITs.
ty::Alias(_, ty::AliasTy { def_id, substs, .. })
if self.tcx.is_impl_trait_in_trait(*def_id) =>
{
self.visit_opaque(*def_id, substs)
}
_ => t.super_visit_with(self),
}
}
Expand Down
31 changes: 1 addition & 30 deletions compiler/rustc_metadata/src/rmeta/encoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,7 +824,6 @@ fn should_encode_span(def_kind: DefKind) -> bool {
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Field
| DefKind::Impl { .. }
| DefKind::Closure
Expand Down Expand Up @@ -867,7 +866,6 @@ fn should_encode_attrs(def_kind: DefKind) -> bool {
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::LifetimeParam
| DefKind::GlobalAsm
| DefKind::Generator => false,
Expand Down Expand Up @@ -902,7 +900,6 @@ fn should_encode_expn_that_defined(def_kind: DefKind) -> bool {
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Field
| DefKind::LifetimeParam
| DefKind::GlobalAsm
Expand Down Expand Up @@ -939,7 +936,6 @@ fn should_encode_visibility(def_kind: DefKind) -> bool {
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::GlobalAsm
| DefKind::Impl { .. }
| DefKind::Closure
Expand All @@ -966,7 +962,6 @@ fn should_encode_stability(def_kind: DefKind) -> bool {
| DefKind::ForeignMod
| DefKind::TyAlias
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Enum
| DefKind::Union
| DefKind::Impl { .. }
Expand Down Expand Up @@ -1033,7 +1028,6 @@ fn should_encode_variances(def_kind: DefKind) -> bool {
| DefKind::Enum
| DefKind::Variant
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Fn
| DefKind::Ctor(..)
| DefKind::AssocFn => true,
Expand Down Expand Up @@ -1083,7 +1077,6 @@ fn should_encode_generics(def_kind: DefKind) -> bool {
| DefKind::AnonConst
| DefKind::InlineConst
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::Impl { .. }
| DefKind::Field
| DefKind::TyParam
Expand Down Expand Up @@ -1134,19 +1127,6 @@ fn should_encode_type(tcx: TyCtxt<'_>, def_id: LocalDefId, def_kind: DefKind) ->
}
}

DefKind::ImplTraitPlaceholder => {
let parent_def_id = tcx.impl_trait_in_trait_parent_fn(def_id.to_def_id());
let assoc_item = tcx.associated_item(parent_def_id);
match assoc_item.container {
// Always encode an RPIT in an impl fn, since it always has a body
ty::AssocItemContainer::ImplContainer => true,
ty::AssocItemContainer::TraitContainer => {
// Encode an RPIT for a trait only if the trait has a default body
assoc_item.defaultness(tcx).has_value()
}
}
}

DefKind::AssocTy => {
let assoc_item = tcx.associated_item(def_id);
match assoc_item.container {
Expand Down Expand Up @@ -1192,7 +1172,6 @@ fn should_encode_fn_sig(def_kind: DefKind) -> bool {
| DefKind::Ctor(..)
| DefKind::TyAlias
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::ForeignTy
| DefKind::Impl { .. }
| DefKind::AssocConst
Expand Down Expand Up @@ -1235,7 +1214,6 @@ fn should_encode_constness(def_kind: DefKind) -> bool {
| DefKind::TyAlias
| DefKind::OpaqueTy
| DefKind::Impl { of_trait: false }
| DefKind::ImplTraitPlaceholder
| DefKind::ForeignTy
| DefKind::Generator
| DefKind::ConstParam
Expand Down Expand Up @@ -1268,7 +1246,6 @@ fn should_encode_const(def_kind: DefKind) -> bool {
| DefKind::Static(..)
| DefKind::TyAlias
| DefKind::OpaqueTy
| DefKind::ImplTraitPlaceholder
| DefKind::ForeignTy
| DefKind::Impl { .. }
| DefKind::AssocFn
Expand All @@ -1289,11 +1266,8 @@ fn should_encode_const(def_kind: DefKind) -> bool {
}
}

// We only encode impl trait in trait when using `lower-impl-trait-in-trait-to-assoc-ty` unstable
// option.
fn should_encode_fn_impl_trait_in_trait<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool {
if tcx.lower_impl_trait_in_trait_to_assoc_ty()
&& let Some(assoc_item) = tcx.opt_associated_item(def_id)
if let Some(assoc_item) = tcx.opt_associated_item(def_id)
&& assoc_item.container == ty::AssocItemContainer::TraitContainer
&& assoc_item.kind == ty::AssocKind::Fn
{
Expand Down Expand Up @@ -1447,9 +1421,6 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
.is_type_alias_impl_trait
.set(def_id.index, self.tcx.is_type_alias_impl_trait(def_id));
}
if let DefKind::ImplTraitPlaceholder = def_kind {
self.encode_explicit_item_bounds(def_id);
}
if tcx.impl_method_has_trait_impl_trait_tys(def_id)
&& let Ok(table) = self.tcx.collect_return_position_impl_trait_in_trait_tys(def_id)
{
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_metadata/src/rmeta/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ fixed_size_enum! {
( AnonConst )
( InlineConst )
( OpaqueTy )
( ImplTraitPlaceholder )
( Field )
( LifetimeParam )
( GlobalAsm )
Expand Down
8 changes: 1 addition & 7 deletions compiler/rustc_middle/src/hir/map/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,13 +195,7 @@ impl<'hir> Map<'hir> {
ItemKind::Fn(..) => DefKind::Fn,
ItemKind::Macro(_, macro_kind) => DefKind::Macro(macro_kind),
ItemKind::Mod(..) => DefKind::Mod,
ItemKind::OpaqueTy(ref opaque) => {
if opaque.in_trait && !self.tcx.lower_impl_trait_in_trait_to_assoc_ty() {
DefKind::ImplTraitPlaceholder
} else {
DefKind::OpaqueTy
}
}
ItemKind::OpaqueTy(..) => DefKind::OpaqueTy,
ItemKind::TyAlias(..) => DefKind::TyAlias,
ItemKind::Enum(..) => DefKind::Enum,
ItemKind::Struct(..) => DefKind::Struct,
Expand Down
14 changes: 4 additions & 10 deletions compiler/rustc_middle/src/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,9 @@ impl<'tcx> TyCtxt<'tcx> {
scope_def_id: LocalDefId,
) -> Vec<&'tcx hir::Ty<'tcx>> {
let hir_id = self.hir().local_def_id_to_hir_id(scope_def_id);
let Some(hir::FnDecl { output: hir::FnRetTy::Return(hir_output), .. }) = self.hir().fn_decl_by_hir_id(hir_id) else {
let Some(hir::FnDecl { output: hir::FnRetTy::Return(hir_output), .. }) =
self.hir().fn_decl_by_hir_id(hir_id)
else {
return vec![];
};

Expand Down Expand Up @@ -2002,16 +2004,8 @@ impl<'tcx> TyCtxt<'tcx> {
)
}

pub fn lower_impl_trait_in_trait_to_assoc_ty(self) -> bool {
self.sess.opts.unstable_opts.lower_impl_trait_in_trait_to_assoc_ty
}

pub fn is_impl_trait_in_trait(self, def_id: DefId) -> bool {
if self.lower_impl_trait_in_trait_to_assoc_ty() {
self.opt_rpitit_info(def_id).is_some()
} else {
self.def_kind(def_id) == DefKind::ImplTraitPlaceholder
}
self.opt_rpitit_info(def_id).is_some()
}

/// Named module children from all kinds of items, including imports.
Expand Down
Loading