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

small normalization improvement #127570

Merged
merged 1 commit into from
Jul 10, 2024
Merged
Changes from all commits
Commits
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
13 changes: 5 additions & 8 deletions compiler/rustc_trait_selection/src/traits/normalize.rs
Original file line number Diff line number Diff line change
@@ -109,16 +109,13 @@ pub(super) fn needs_normalization<'tcx, T: TypeVisitable<TyCtxt<'tcx>>>(
value: &T,
reveal: Reveal,
) -> bool {
// This mirrors `ty::TypeFlags::HAS_ALIASES` except that we take `Reveal` into account.

let mut flags = ty::TypeFlags::HAS_TY_PROJECTION
| ty::TypeFlags::HAS_TY_WEAK
| ty::TypeFlags::HAS_TY_INHERENT
| ty::TypeFlags::HAS_CT_PROJECTION;
let mut flags = ty::TypeFlags::HAS_ALIAS;

// Opaques are treated as rigid with `Reveal::UserFacing`,
// so we can ignore those.
match reveal {
Reveal::UserFacing => {}
Reveal::All => flags |= ty::TypeFlags::HAS_TY_OPAQUE,
Reveal::UserFacing => flags.remove(ty::TypeFlags::HAS_TY_OPAQUE),
Reveal::All => {}
Comment on lines -120 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic here now inverted, is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, HAS_TY_OPAQUE wasn't in flags before.

}

value.has_type_flags(flags)
8 changes: 4 additions & 4 deletions compiler/rustc_type_ir/src/flags.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ bitflags::bitflags! {
/// Does this have `ConstKind::Param`?
const HAS_CT_PARAM = 1 << 2;

const HAS_PARAM = TypeFlags::HAS_TY_PARAM.bits()
const HAS_PARAM = TypeFlags::HAS_TY_PARAM.bits()
| TypeFlags::HAS_RE_PARAM.bits()
| TypeFlags::HAS_CT_PARAM.bits();

@@ -27,7 +27,7 @@ bitflags::bitflags! {

/// Does this have inference variables? Used to determine whether
/// inference is required.
const HAS_INFER = TypeFlags::HAS_TY_INFER.bits()
const HAS_INFER = TypeFlags::HAS_TY_INFER.bits()
| TypeFlags::HAS_RE_INFER.bits()
| TypeFlags::HAS_CT_INFER.bits();

@@ -39,7 +39,7 @@ bitflags::bitflags! {
const HAS_CT_PLACEHOLDER = 1 << 8;

/// Does this have placeholders?
const HAS_PLACEHOLDER = TypeFlags::HAS_TY_PLACEHOLDER.bits()
const HAS_PLACEHOLDER = TypeFlags::HAS_TY_PLACEHOLDER.bits()
| TypeFlags::HAS_RE_PLACEHOLDER.bits()
| TypeFlags::HAS_CT_PLACEHOLDER.bits();

@@ -81,7 +81,7 @@ bitflags::bitflags! {
/// Does this have `Alias` or `ConstKind::Unevaluated`?
///
/// Rephrased, could this term be normalized further?
const HAS_ALIASES = TypeFlags::HAS_TY_PROJECTION.bits()
const HAS_ALIAS = TypeFlags::HAS_TY_PROJECTION.bits()
Copy link
Member

Choose a reason for hiding this comment

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

👍

| TypeFlags::HAS_TY_WEAK.bits()
| TypeFlags::HAS_TY_OPAQUE.bits()
| TypeFlags::HAS_TY_INHERENT.bits()
2 changes: 1 addition & 1 deletion compiler/rustc_type_ir/src/visit.rs
Original file line number Diff line number Diff line change
@@ -230,7 +230,7 @@ pub trait TypeVisitableExt<I: Interner>: TypeVisitable<I> {
}

fn has_aliases(&self) -> bool {
Copy link
Member

@fmease fmease Jul 10, 2024

Choose a reason for hiding this comment

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

Suggested change
fn has_aliases(&self) -> bool {
fn has_alias(&self) -> bool {

too (my bad)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interstingly, we do use the plural for the functions, e.g. we have has_inherent_projections and has_opaque_types even though the type flag is HAS_TY_OPAQUE

Copy link
Member

Choose a reason for hiding this comment

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

let's leave this one

self.has_type_flags(TypeFlags::HAS_ALIASES)
self.has_type_flags(TypeFlags::HAS_ALIAS)
}

fn has_inherent_projections(&self) -> bool {
Loading