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

Incompleteness in preferring subst-relate candidate in alias-relate goals #26

Closed
compiler-errors opened this issue May 31, 2023 · 3 comments
Labels
A-incomplete incorrectly return `NoSolution`, unsound during coherence

Comments

@compiler-errors
Copy link
Member

compiler-errors commented May 31, 2023

There are 3 ways to prove AliasRelate goals: lhs normalize-to rhs, rhs normalize-to lhs, and substs-relate.

If merging the three options via fn try_merge_responses fails, we incompletely prefer substs-relate over normalizing. https://github.com/rust-lang/rust/blob/a482149598f5aacf3837eee87026dd634f08641c/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L84-L86

It seems like this incompleteness is not necessary if we go with #25 instead.


edit: original description

ill write this tomorrow since it's 21:50 rn and i want to sleep

tl;dr here's the list of failing tests before and after -- red (left-hand side) tests are those that are fixed by the incompleteness, and green (right-hand side) are those that begin to fail after the incompleteness: https://www.diffchecker.com/NdEJkqfr/

@compiler-errors compiler-errors added the A-incomplete incorrectly return `NoSolution`, unsound during coherence label May 31, 2023
@compiler-errors
Copy link
Member Author

compiler-errors commented May 31, 2023

if we were to change this incompleteness to apply only if the response has no infer/opaques, this is the list of failing tests for each scenario: https://www.diffchecker.com/PbEqtAfC/ -- given that these two incompletenesses only differ by ~10 tests or so, it suggests most of the test fail -> pass come from preferring subst-relate only when regions are involved.

this suggests that we may be able to get away (for now) with only preferring a subst-relate candidate if it has_only_region_constraints.

@lcnr
Copy link
Contributor

lcnr commented Jun 12, 2023

This is used to fix #2

@compiler-errors
Copy link
Member Author

We don't do this anymore as of rust-lang/rust#113901 -- bidirectional normalizes-to is both more conservative and is sufficient in most cases, and #2 was also fixed rust-lang/rust#112399.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 22, 2023
…lcnr

Get rid of subst-relate incompleteness in new solver

We shouldn't need subst-relate if we have bidirectional-normalizes-to in the new solver.

The only potential issue may happen if we have an unconstrained projection like `<Wrapper<?0> as Trait>::Assoc == <Wrapper<T> as Trait>::Assoc` where they both normalize to something that doesn't mention any substs, which would possibly prefer `?0 = T` if we fall back to subst-relate. But I'd prefer if we remove incompleteness until we can determine some case where we need them, and the bidirectional-normalizes-to seems better to have in general.

I can update rust-lang/trait-system-refactor-initiative#26 and rust-lang/trait-system-refactor-initiative#25 once this lands.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incomplete incorrectly return `NoSolution`, unsound during coherence
Projects
None yet
Development

No branches or pull requests

2 participants