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

Don't over-constrain projections in generic method signatures #92728

Closed
wants to merge 2 commits into from

Conversation

jackh726
Copy link
Member

Fixes #91762

This is likely not the "right" way to do this. Just opening for thoughts

r? @nikomatsakis

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022
self.depth,
&mut self.obligations,
);
let normalized_ty = if self.selcx.normalization_mode.eager_inference_replacement {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same as to what is done in #90887, but puts the flag on SelectionContext instead of AssocTypeNormalizer

Comment on lines +1828 to +1913
let any_instantiations = infcx.any_instantiations(&snapshot);

if any_instantiations && !selcx.normalization_mode.allow_infer_constraint_during_projection
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I think about it, this could probably be done during candidate assembly.

let mut selcx = traits::SelectionContext::new(self);
selcx.normalization_mode.eager_inference_replacement = false;
let mut normalize_obligations = Vec::new();

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with doing normalizing like this, is that maybe one type can be normalized, but the other can't. We probably should allow coercing by relating the projection itself.

Another problem with doing it like this: what happens if the result of a normalization is itself a projection. You could imagine that we might want to be able to coerce to any types in the "chain" of normalizations from either side. But we can't just relate types, because we also allow e.g. unsizing. Otherwise, lazy norm might be able to solve this.

Imo, the other half of this PR (not constraining inference vars when confirming a method call) is "more correct" than this one. There might be some version of this change that can be more conservative, where we could just require a ::<T, U> to be specified.

_ => a,
}
}
_ => traits::normalize_with_depth_to(
Copy link
Member Author

Choose a reason for hiding this comment

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

Problem here: We don't have a way to "project_and_unify_type" for generalized type (essentially a Relate instead of a Fold). This hack fails as soon as we wrap the projection in an adt, for example.

@@ -0,0 +1,32 @@
// check-pass
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the relevant test. (Don't mind the no_core MCVE; I made this because otherwise the logs where too long)

@jackh726
Copy link
Member Author

Tried to rebase, but my fix no longer works. Have to think about this more I guess.

@bors
Copy link
Contributor

bors commented Mar 28, 2022

☔ The latest upstream changes (presumably #95333) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726 jackh726 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2022
@JohnCSimon
Copy link
Member

@jackh726
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. 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.

Typecheck error when using a equality-constrained GAT in a trait.
6 participants