Instantiate more type variables to hard unions #15632

odersky · 2022-07-09T12:10:13Z

odersky · 2022-07-09T18:35:47Z

I was hoping this would give some leverage for avoiding errors caused by level checking, but alas that's not the case. Nevertheless it seems useful to support the scenario of #14770.

smarter · 2022-07-09T19:10:50Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+
+        val res = widenOK
+          || joinOK
+          || (tp1.isSoft || constrainRHSVars(tp2)) && recur(tp11, tp2) && recur(tp12, tp2)


Can this mechanism replaces the existing constrainRHSVars hack? As mentioned in #14770 (comment) this is supposed to handle this sort of cases, but it's incomplete and break internal invariants. I'd feel more comfortable adding new code here if we could remove the existing hacks first.

I tried now, and the answer is no. We already can't build the compiler itself if we disable constrainRHSVars.

smarter

I won't have time reviewing this in more details but I'm not comfortable merging this as is. I think a TypeComparer-wide mode that gets set temporarily as we add the constraint as I hypothesized in #14770 would be easier to reason about.

smarter · 2022-07-09T19:16:49Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

-            // loop for a very long time without the recursion brake.
+        def hardenTypeVars(tp2: Type): Unit = tp2.dealiasKeepRefiningAnnots match
+          case tvar: TypeVar if constraint.contains(tvar.origin) =>
+            tvar.widenUnions = false


This seems very dangerous to me. The particular subtype check we're doing might be part of a TyperState that ends up being rolled back, but the mutation here will be preserved. This will make it extremely hard to debug any issue involving widening, and any change in inference could break existing code because this flag happens to be toggled.

bccbaf7 now keeps this info in TyperState.

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala

odersky · 2022-07-09T20:08:52Z

I think a TypeComparer-wide mode that gets set temporarily as we add the constraint as I hypothesized in #14770 would be easier to reason about.

I think that would be harder to reason about since it is a global side effect, and you don't know what it will influence. Propagating windenUnion parameters is arduous, but at least we know exactly what they influence.

smarter · 2022-07-10T11:29:28Z

compiler/src/dotty/tools/dotc/core/TyperState.scala

+   *  to hard unions instead of being widened in `widenOr`.
+   */
+  private var myRigidVars: TypeVars = _
+  def rigidVars: TypeVars = myRigidVars


I like this much more 😅, but I think the name could be misleading: usually "rigid variables" are variables that cannot be constrained at all (like the type parameters of a method as seen from inside that method). Maybe go with hardVars instead in keeping with hardenVars?

smarter · 2022-07-10T11:55:03Z

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala

@@ -548,24 +554,41 @@ trait ConstraintHandling {
        wideInst.dropRepeatedAnnot
  end widenInferred

+  /** Convert all toplevel union types in `tp` to hard unions */
+  extension (tp: Type) private def hardenUnions(using Context): Type = tp.widen match


My original idea was to change https://github.com/lampepfl/dotty/blob/6540ad9154797164561281080b6a4e24a191691d/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala#L266 which currently uses | (and thus creates soft unions) to instead check a flag to decide whether to use a soft or hard union. This would avoid having to convert unions after creating them, and it means we can reset the flag after adding the constraint.

The problem I see with this is that the lower bound might be a union with type variables in it. In that case the type variables would end up in the ordering constraint and there would be no union to harden until the time we instantiate the type variable.

I see, so we couldn't reset the flag but could check hardVars in ConstraintHandling when we create the unions I think, that would cover unions created later from propagating constraints based on ordering.

I think hardVars belongs in TyperState. If it's in ConstraintHandling we would lose it when we commit a constraint.

Right, I meant we could leave it in TyperState but check it in ConstraintHandling#addOneBound instead of checking it in TypeVar#instantiate, but I don't know if we're actually supposed to access the currrent TyperState from ConstraintHandling.

But addOneBound does not necessarily create a union. The union might be created by fullBounds when we instantiate the variable.

But as far as I can tell we don't use fullBounds to instantiate a type variable (except for GADTs but we know that's problematic: #14288)

We do use fullLowerBound in approximation.

Ah that's right, so we'd have to check hardVars both in addOneBound and fullLowerBound, I'm neutral on whether this is better than the current scheme.

I think it's better to tie it to typevariable instantiation, since we do fullLowerBound also elsewhere. We could possibly add it to approximate, but maybe that mixes up things too much.

dwijnand · 2022-08-28T08:00:39Z

This looks too involved for me to review with little time, but I'll get back to this when I'm back - sorry for the delay.

Fixes scala#14770

Try alternative to track hard typevars in constraint instead of typer state. This avoids the problem that a failing subtype comparison can also mark type variables as hard.

It seems constrainRHSVars is no longer needed with the new way to keep track of hard type variables.

odersky · 2022-08-30T15:58:46Z

@smarter I think it now addresses your concerns. Do you have time to take a look again?

compiler/src/dotty/tools/dotc/core/TyperState.scala

smarter · 2022-08-30T19:11:13Z

Nice! I'll have a look tomorrow.

smarter

Otherwise LGTM!

smarter · 2022-08-31T12:11:36Z

compiler/src/dotty/tools/dotc/core/TyperState.scala

+            tvars.foreach(tvar =>
+              if !tvar.inst.exists then
+                if !isOwnedAnywhere(this, tvar) then includeVar(tvar)
+                if other.isHard(tvar) then constraint = constraint.withHard(tvar))


This code path is only executed if constraint.contains(tl) is false above, but if the same type lambda appears in both constraints, they could still have different sets of hardened tvars. So I think this logic needs to be moved below inside the argument to constraint.uninstVars.forall.

Ah yes, well spotted.

odersky marked this pull request as draft July 9, 2022 12:10

odersky force-pushed the fix-14770 branch from fe8d39a to cdce995 Compare July 9, 2022 13:26

odersky marked this pull request as ready for review July 9, 2022 18:34

odersky requested a review from smarter July 9, 2022 18:34

odersky assigned smarter and odersky Jul 9, 2022

smarter reviewed Jul 9, 2022

View reviewed changes

smarter requested changes Jul 9, 2022

View reviewed changes

smarter removed their assignment Jul 9, 2022

dwijnand reviewed Jul 9, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala Show resolved Hide resolved

smarter reviewed Jul 10, 2022

View reviewed changes

odersky assigned odersky and smarter and unassigned odersky Jul 11, 2022

odersky force-pushed the fix-14770 branch from b0d0611 to aa8032d Compare August 20, 2022 09:58

odersky assigned dwijnand and unassigned smarter Aug 20, 2022

odersky added 2 commits August 29, 2022 19:35

Instantiate more type variables to hard unions

c1f35aa

Fixes scala#14770

Refactor hardenedVars handling

f177266

odersky force-pushed the fix-14770 branch from aa8032d to f177266 Compare August 29, 2022 17:55

odersky added 2 commits August 30, 2022 13:28

Try alternative to track hard typevars in constraint

d15558d

Try alternative to track hard typevars in constraint instead of typer state. This avoids the problem that a failing subtype comparison can also mark type variables as hard.

Drop old TyperState based scheme and drop constrainRHSVars

d7521bf

It seems constrainRHSVars is no longer needed with the new way to keep track of hard type variables.

odersky assigned smarter and unassigned dwijnand Aug 30, 2022

odersky commented Aug 30, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/core/TyperState.scala Outdated Show resolved Hide resolved

smarter requested changes Aug 31, 2022

View reviewed changes

smarter assigned odersky and unassigned smarter Aug 31, 2022

odersky force-pushed the fix-14770 branch from 9f9218b to 8caff95 Compare August 31, 2022 16:17

Address review comment

dfcfb6b

odersky force-pushed the fix-14770 branch from 8caff95 to dfcfb6b Compare August 31, 2022 16:23

odersky merged commit bf03086 into scala:main Sep 1, 2022

odersky deleted the fix-14770 branch September 1, 2022 07:58

Kordyjan added this to the 3.2.1 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instantiate more type variables to hard unions #15632

Instantiate more type variables to hard unions #15632

odersky commented Jul 9, 2022

odersky commented Jul 9, 2022

smarter Jul 9, 2022

odersky Aug 20, 2022 •

edited

Loading

smarter left a comment

smarter Jul 9, 2022

odersky Jul 10, 2022 •

edited

Loading

odersky commented Jul 9, 2022

smarter Jul 10, 2022

smarter Jul 10, 2022

odersky Jul 10, 2022

smarter Jul 10, 2022

odersky Jul 10, 2022

smarter Jul 10, 2022

odersky Jul 10, 2022

smarter Jul 10, 2022

odersky Jul 10, 2022

smarter Jul 10, 2022

odersky Jul 10, 2022

dwijnand commented Aug 28, 2022

odersky commented Aug 30, 2022

smarter commented Aug 30, 2022

smarter left a comment

smarter Aug 31, 2022

odersky Aug 31, 2022

Instantiate more type variables to hard unions #15632

Instantiate more type variables to hard unions #15632

Conversation

odersky commented Jul 9, 2022

odersky commented Jul 9, 2022

Choose a reason for hiding this comment

odersky Aug 20, 2022 • edited Loading

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

odersky commented Jul 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Aug 28, 2022

odersky commented Aug 30, 2022

smarter commented Aug 30, 2022

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky Aug 20, 2022 •

edited

Loading

odersky Jul 10, 2022 •

edited

Loading