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

Is TypeFlags::NOMINAL_FLAGS unnecessary now? #70836

Closed
eddyb opened this issue Apr 6, 2020 · 7 comments · Fixed by #70915
Closed

Is TypeFlags::NOMINAL_FLAGS unnecessary now? #70836

eddyb opened this issue Apr 6, 2020 · 7 comments · Fixed by #70915
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Apr 6, 2020

This seems to contain all the flags in TypeFlags. If I'm guessing right, the flags that used to not be in here, all turned into queries? So they're now cached as queries, instead of by mutating Ty itself?

/// Flags representing the nominal content of a type,
/// computed by FlagsComputation. If you add a new nominal
/// flag, it should be added here too.
const NOMINAL_FLAGS = TypeFlags::HAS_TY_PARAM.bits
| TypeFlags::HAS_RE_PARAM.bits
| TypeFlags::HAS_CT_PARAM.bits
| TypeFlags::HAS_TY_INFER.bits
| TypeFlags::HAS_RE_INFER.bits
| TypeFlags::HAS_CT_INFER.bits
| TypeFlags::HAS_TY_PLACEHOLDER.bits
| TypeFlags::HAS_RE_PLACEHOLDER.bits
| TypeFlags::HAS_CT_PLACEHOLDER.bits
| TypeFlags::HAS_FREE_LOCAL_REGIONS.bits
| TypeFlags::HAS_TY_PROJECTION.bits
| TypeFlags::HAS_TY_OPAQUE.bits
| TypeFlags::HAS_CT_PROJECTION.bits
| TypeFlags::KEEP_IN_LOCAL_TCX.bits
| TypeFlags::HAS_TY_ERR.bits
| TypeFlags::HAS_FREE_REGIONS.bits
| TypeFlags::HAS_RE_LATE_BOUND.bits
| TypeFlags::HAS_RE_ERASED.bits
| TypeFlags::STILL_FURTHER_SPECIALIZABLE.bits;

cc @nikomatsakis @Zoxc

@eddyb eddyb added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 6, 2020
@nikomatsakis
Copy link
Contributor

If it contains all the flags, then it seems to have no purpose. I'm honestly not sure what the name "nominal" is even meant to refer to here exactly -- I'm sure I gave it that name, but it's not very clear. I guess the point is "flags that should be inherited by containing types; ie., if the type T has some flag F, and F is "nominal", then Vec<T> should also have F".

Are there any flags that don't meet that definition anymore?

@eddyb
Copy link
Member Author

eddyb commented Apr 6, 2020

@nikomatsakis I think there were "nominal flags" and "cached properties" (using interior mutability), but now the latter is completely gone.

@nikomatsakis
Copy link
Contributor

that sounds right. I'd be in favor of removing NOMINAL_FLAGS then.

@varkor varkor added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Apr 6, 2020
@tsandstr
Copy link
Contributor

tsandstr commented Apr 8, 2020

I would love to help with this one. A quick search shows that NOMINAL_FLAGS is only used once in the entire code base, here:

fn add_flags(&mut self, flags: TypeFlags) {
self.flags = self.flags | (flags & TypeFlags::NOMINAL_FLAGS);
}

Looks like this could just be changed to
self.flags = self.flags | flags;

Is that all it will take?

@eddyb
Copy link
Member Author

eddyb commented Apr 8, 2020

Sounds correct, yeah.

@tsandstr
Copy link
Contributor

tsandstr commented Apr 8, 2020

Cool great

@tsandstr
Copy link
Contributor

tsandstr commented Apr 8, 2020

Oops, I'm still learning git. Sorry for the spam there. Only 4cdb206 is relevant.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 8, 2020
Remove unnecessary TypeFlags::NOMINAL_FLAGS

This was a relic from when we had "nominal flags" and "cached
properties." The latter no longer exists, so nominal flags are no
longer necessary. In fact, every flag is considered a nominal flag. I
went ahead and removed all references to NOMINAL_FLAGS.

Fixes rust-lang#70836
@bors bors closed this as completed in 4cdb206 Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants