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

Include parent substs in substs argument in nominal_obligations when const param defaults exist #106847

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Jan 14, 2023

Fixes #106994

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2023
@WaffleLapkin
Copy link
Member

I have no idea how this code works ":)
r? compiler

@rustbot rustbot assigned jackh726 and unassigned WaffleLapkin Jan 14, 2023
@jackh726
Copy link
Member

Wait...why? Can you explain a bit (definitely at least in a comment, also maybe here) why we need to skip nominal obligations?

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jan 14, 2023

@jackh726 I found that we don't have to avoid nominal_obligations, so I fixed it to include parent substs and call nominal_obligations .

Comment on lines 468 to 470
InternalSubsts::identity_for_item(self.tcx, did)
.into_iter()
.chain(uv.substs),
Copy link
Member

Choose a reason for hiding this comment

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

This needs an explanation of what it's doing

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a comment.

Copy link
Member

@compiler-errors compiler-errors Jan 15, 2023

Choose a reason for hiding this comment

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

Sorry, specifically I was asking about a comment that explains why you're chaining the identity parent substs with the (partially constructed) substs you have in uv.substs.

It doesn't seem correct to me at first glance.

@BoxyUwU BoxyUwU added the A-const-generics Area: const generics (parameters and arguments) label Jan 14, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 14, 2023

r? @BoxyUwU

@rustbot rustbot assigned BoxyUwU and unassigned jackh726 Jan 14, 2023
@TaKO8Ki TaKO8Ki changed the title Avoid nominal_obligations when const param defaults exist Include parent substs in substs argument in nominal_obligations when const param defaults exist Jan 15, 2023
@TaKO8Ki TaKO8Ki force-pushed the avoid-nominal_obligations-when-const-param-defaults-exist branch from 23c6ce5 to 78d2dea Compare January 16, 2023 09:50
@TaKO8Ki TaKO8Ki requested a review from BoxyUwU January 16, 2023 10:15
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 17, 2023

I don't feel super comfortable merging more hacks to make generic_const_exprs + const_generics_defaults work in the presence of each other. #86580 alone was already rather hacky and I think this PR just makes it way too much to be reasonable. It's also unclear to me whether this is going to interfere with fixing some of the other issues we have with const generics and I don't want to make that any harder than necessary.

feature(generic_const_exprs) is an incomplete feature (both literally and according to the incomplete_features lint), having it in a broken state because we are not currently in a good position to properly fix this seems fine to me. Thanks for making the PR anyway but I expect this issue will stay open until we make a lot more progress on generic_const_exprs 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generic_const_exprs does not work properly with const/type param defaults
6 participants