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

Two fixes to constraint solving #16353

Merged
merged 2 commits into from
Nov 18, 2022
Merged

Two fixes to constraint solving #16353

merged 2 commits into from
Nov 18, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 16, 2022

1. Fix replace operation

In OrderingConstraint#replace we moved the actual replacement of a parameter with a type from the start of replace to its end, since that was more convenient for dependency adjustments. It turns out that doing so causes infinite recursion in instantiations in some cases, specifically if a parameter contains itself as an indirect lower bound that goes through an alias. Here is a situation that arises in i16311.scala:

  type WithTag[T, U] = T & Tagged[U]

  T1 >: WithTag[T2, Int]
  T2 >: T1 & Tagged[Int]

The correct instantiation for T1 and T2 is Nothing. But we ran into a cycle instead.

The fix is to move the parameter replacement back to the start of replace, and to account for that in the dependency adjustment logic.

Fixes #16311 (with failing Ycheck)

2. See through aliases before decomposing And/Or in isSubType

There seem to be two missing cases in TypeComparer where we
have a TypeParamRef on one side and an And/Or type under an alias on the other.
Examples:

type AND = A & B
type OR  = A | B
p <:< AND
OR <:< p

In this case we missed the decomposition into smaller types that would happen
otherwise. This broke i16311.scala in Ycheck and broke i15365.scala with an
infinite recursion in avoidance.

I verified that having an AndType as super or subtype of an abstract type works
as expected. So if in the example above

type AND >: A & B

or

type AND <: A & B

it worked before. It was just aliases that were the problem (I assume it's the same for OrTypes
as lower bounds).

This fixes #16311 completely and also
Fixes #15365

In OrderingConstraint#replace we moved the actual replacement
of a parameter with a type from the start of replace to its end,
since that was more convenient for dependency adjustments. It turns
out that doing so causes infinite recursion in instantiations in
some cases, specifically if a parameter contains itself as an indirect
lower bound that goes through an alias. Here is a situation that arises
in i16311.scala:
```
type WithTag[T, U] = T & Tagged[U]

  T1 >: WithTag[T2, Int]
  T2 >: T1 & Tagged[Int]
```
The current instantiation for T1 and T2 is Nothing. But we ran into a cycle
instead.

The fix is to move the parameter replacement back to the start of `replace`,
and to account for that in the dependency adjustment logic.

Fixes scala#16311
@odersky
Copy link
Contributor Author

odersky commented Nov 16, 2022

The test now compiles but fails -Ycheck.

I noted that all previous Scala 3 versions also fail Ycheck with that test. In each case the error is:

Error: Unexpected error when compiling project_e61661fc0e_e61661fc0e-dc052a6269: 'assertion failed: 
Found:    test.InputType[Boolean & test.Tagged[Int]]
Required: test.InputType[Nothing & test.Tagged[Int] & test.Tagged[Int]]
found: ??
expected: ??
tree = ??? :test.InputType[Boolean & test.Tagged[Int]]'

There seem to be two missing cases in TypeComparer where we
have a TypeParamRef on one side and an And/Or type under an aloas on the other.
Examples:

    type AND = A & B
    type OR  = A | B
    p <:< AND
    OR <:< p

In this case we missed the decomposition into smaller types that would happen
otherwise. This broke i16311.scala in Ycheck and broke i15365.scala with an
infinite recursion in avoidance.

I verified that having an AndType as super or subtype of an abstract type works
as expected. So if in the example above

    type AND >: A & B

or

    type AND <: A & B

it worked before. It was just aliases that were the problem (I assume it's the same for OrTypes
as lower bounds).
@odersky odersky changed the title Fix replace operation Two fixes to constraint solving Nov 16, 2022
@odersky
Copy link
Contributor Author

odersky commented Nov 16, 2022

The second commit (See through aliases before decomposing And/Or in isSubType) makes the first redundant. The test cases now succeed with both the old and the new replace algorithm. But when looking at the constraint generated without the second commit, the revised replace algorithm computes the correct solution, whereas the previous one looped.

On the other hand, the previous Ycheck failure seems to indicate that we did not generate the correct constraints. I won't have time to dig deeper why that was the case, though.

@odersky odersky requested a review from smarter November 16, 2022 20:08
@odersky odersky assigned odersky and smarter and unassigned odersky Nov 16, 2022
@smarter smarter merged commit 57808b6 into scala:main Nov 18, 2022
@smarter smarter deleted the fix-16311 branch November 18, 2022 13:27
@Kordyjan Kordyjan added this to the 3.2.2 backports milestone Dec 22, 2022
@Kordyjan Kordyjan added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Dec 22, 2022
@Kordyjan Kordyjan modified the milestones: 3.2.2 backports, 3.3.0-RC1 Dec 22, 2022
@Kordyjan
Copy link
Contributor

Apparently, #16042 is not part of 3.2.2, so there is no need to backport this.

@Kordyjan Kordyjan modified the milestones: 3.3.0-RC1, 3.3.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants