Skip to content

Better type inference in harmonizeUnion #2111

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

Merged
merged 1 commit into from
Mar 18, 2017

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 16, 2017

NOTE: This PR depends on #2110 because the improved type inference revealed the bug fixed by #2110, only the last commit is new.


Before this commit, the added testcase failed because the type of inv
was inferred to be Inv[Any] instead of Inv[Int]. The situation looks
like this:

def inv(cond: Boolean) =
    if (cond)
      new Inv(1) // : Inv[A] where A >: Int
    else
      Inv.empty // : Inv[A'] where A' unconstrained
    // : Inv[A] | Inv[A']

To get the type of inv, we call harmonizeUnion which will take the
lub of Inv[A] and Inv[A'], eventually this mean that we do:

  A' <:< A

But since harmonizeUnion uses fluidly, this does not result in A'
getting constrained to be a subtype of A, instead we constrain A to
the upper bound of A':

  Any <:< A

We use fluidly to avoid creating OrTypes in lub, but it turns out
that there is a less aggressive solution: lub calls mergeIfSuper
which then calls isSubTypeWhenFrozen, if we just make these subtype
calls non-frozen, we can achieve what we want. This is what the new
method fluidLub does.

@smarter smarter requested a review from odersky March 16, 2017 19:46
@smarter
Copy link
Member Author

smarter commented Mar 16, 2017

There is a single test that fails with a deep subtype error, it reduces to:

object Test {
  def test: Unit = {
    Seq(Set(1),Set(2),Set(3))
  }
}

By adding more sets to the Seq, you can get it to stackoverflow with deep subtype checking turned off. It doesn't happen on master but type inference is broken there (it is inferred to have type Seq[Set[Any]] whereas in this branch it has type Seq[Set[Int]] as expected).

@odersky Any idea why this happens? Here's the trace: https://gist.github.com/smarter/9dc1c8952c28886c0758acdff48c7e79

@smarter smarter force-pushed the fix/better-lub branch 2 times, most recently from e824d54 to 1b0865a Compare March 18, 2017 11:12
@smarter
Copy link
Member Author

smarter commented Mar 18, 2017

I've replaced fluidLub by an even more constrained mechanism: a parameter canConstrain to lub. This is nicer because it doesn't require a global variable and it fixes the deep recursion seen above.

@odersky
Copy link
Contributor

odersky commented Mar 18, 2017

Nice work! LGTM

Before this commit, the added testcase failed because the type of `inv`
was inferred to be `Inv[Any]` instead of `Inv[Int]`. The situation looks
like this:

def inv(cond: Boolean) =
    if (cond)
      new Inv(1) // : Inv[A] where A >: Int
    else
      Inv.empty // : Inv[A'] where A' unconstrained
    // : Inv[A] | Inv[A']

To get the type of `inv`, we call `harmonizeUnion` which will take the
lub of `Inv[A]` and `Inv[A']`, eventually this mean that we do:
  A' <:< A

But since `harmonizeUnion` uses `fluidly`, this does not result in `A'`
getting constrained to be a subtype of `A`, instead we constrain `A` to
the upper bound of `A'`:
  Any <:< A

We use `fluidly` to avoid creating OrTypes in `lub`, but it turns out
that there is a less aggressive solution: `lub` calls `mergeIfSuper`
which then calls `isSubTypeWhenFrozen`, if we just make these subtype
calls non-frozen, we can achieve what we want. This is what the new
`lub` parameter `canConstrain` allows.
@smarter smarter merged commit e93b78f into scala:master Mar 18, 2017
@allanrenucci allanrenucci deleted the fix/better-lub branch December 14, 2017 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants