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

small normalization improvement #127570

Merged
merged 1 commit into from
Jul 10, 2024
Merged

small normalization improvement #127570

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
13 changes: 5 additions & 8 deletions compiler/rustc_trait_selection/src/traits/normalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_type_ir/src/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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();

Expand All @@ -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();

Expand Down Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_type_ir/src/visit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ pub trait TypeVisitableExt<I: Interner>: TypeVisitable<I> {
}

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

@fmease fmease Jul 10, 2024 •
edited
Loading

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 {
Expand Down
Loading