Fix canonical ordering by using the ordering at the declaration site (#859)

This tries to fix issue #849 by NOT canonicalizing AFTER unification.

Instead, we canonicalize FIRST on the function type and then MAINTAIN
the order at the callsite.

This requires us to NOT go into type `Effect`, which is a set and thus
removes duplicates and destroys the ordering...

Sadly, it is very difficult to write a test for this :(

Author: b-studios

effekt/shared/src/main/scala/effekt/Namer.scala

@@ -699,7 +699,7 @@ object Namer extends Phase[Parsed, NameResolved] {
       }
 
       val effs = resolve(effects).distinct
-      effs.canonical.foreach { eff =>
+      CanonicalOrdering(effs.toList) foreach { eff =>
         val cap = CaptureParam(eff.name)
         cps = cps :+ cap
       }

effekt/shared/src/main/scala/effekt/Typer.scala

@@ -374,23 +374,23 @@ object Typer extends Phase[NameResolved, Typechecked] {
           val declaredOperation = interface.operations.find(o => o.name.name == op.name).getOrElse {
             Context.abort(pretty"Operation ${op.name} not defined in ${interface.name}.")
           }
-          val declaredType = Context.lookupFunctionType(d.definition)
+          val declared = Context.lookupFunctionType(d.definition)
 
           def assertArity(kind: String, got: Int, expected: Int): Unit =
             if (got != expected)
               Context.abort(pretty"Number of ${kind} (${got}) does not match declaration of '${op.name}', which expects ${expected}.")
 
           // if we have zero given type parameters, we synthesize them -- no need to check then
-          if (tparams.size != 0) assertArity("type parameters", tparams.size, declaredType.tparams.size - targs.size)
-          assertArity("value parameters", vparams.size, declaredType.vparams.size)
+          if (tparams.nonEmpty) assertArity("type parameters", tparams.size, declared.tparams.size - targs.size)
+          assertArity("value parameters", vparams.size, declared.vparams.size)
 
           //     effect E[A, B, ...] { def op[C, D, ...]() = ... }  !--> op[A, B, ..., C, D, ...]
           // The parameters C, D, ... are existentials
-          val existentialParams: List[TypeVar] = if (tparams.size == declaredType.tparams.size - targs.size) {
+          val existentialParams: List[TypeVar] = if (tparams.size == declared.tparams.size - targs.size) {
             tparams.map { tparam => tparam.symbol.asTypeParam }
           } else {
             // using the invariant that the universals are prepended to type parameters of the operation
-            declaredType.tparams.drop(targs.size).map { tp =>
+            declared.tparams.drop(targs.size).map { tp =>
               // recreate "fresh" type variables
               val name = tp.name
               TypeVar.TypeParam(name)
@@ -400,23 +400,24 @@ object Typer extends Phase[NameResolved, Typechecked] {
 
           Context.annotate(Annotations.TypeParameters, d, existentialParams)
 
-          val canonicalEffects = declaredType.effects.canonical
+          // canonical ordering on annotated effects
+          val canonical = CanonicalOrdering(declared.effects.toList)
 
           // distinguish between handler operation or object operation (which does not capture a cont.)
           val Result(_, effs) = continuationDetails match {
             // normal object: no continuation there
             case None =>
               // block parameters are to be bound by the definition itself instead of by resume when using handlers
-              assertArity("block parameters", bparams.size, declaredType.bparams.size)
+              assertArity("block parameters", bparams.size, declared.bparams.size)
 
               val cparamsForBlocks = bparams.map { p => p.symbol.capture }
               // will be introduced as capabilities in a later phase
-              val cparamsForEffects = canonicalEffects.map { tpe => CaptureParam(tpe.name) }
+              val cparamsForEffects = canonical.map { tpe => CaptureParam(tpe.name) }
               val cparams = cparamsForBlocks ++ cparamsForEffects
 
               // substitute actual type parameter and capture parameters for declared ones
-              val FunctionType(Nil, Nil, vps, bps, tpe, effs) =
-                Context.instantiate(declaredType, targs ++ existentials, cparams.map(cap => CaptureSet(cap))) : @unchecked
+              val (vps, bps, tpe, effs) =
+                Context.instantiate(declared, targs ++ existentials, cparams.map(cap => CaptureSet(cap))) : @unchecked
 
               (vparams zip vps).foreach {
                 case (param, declaredType) =>
@@ -435,29 +436,29 @@ object Typer extends Phase[NameResolved, Typechecked] {
               }
 
               // these capabilities are later introduced as parameters in capability passing
-              val capabilities = (effs.canonical zip cparamsForEffects).map {
+              val capabilities = (CanonicalOrdering(effs) zip cparamsForEffects).map {
                 case (tpe, capt) => Context.freshCapabilityFor(tpe, CaptureSet(capt))
               }
 
               val Result(bodyType, bodyEffs) = Context.bindingCapabilities(d, capabilities) {
                 body checkAgainst tpe
               }
-              Result(bodyType, bodyEffs -- effs)
+              Result(bodyType, bodyEffs -- Effects(effs))
 
             // handler implementation: we have a continuation
             case Some(ret, continuationCapt) =>
 
               if (bparams.nonEmpty)
                 Context.error("Block parameters are bound by resume and not the effect operation itself")
 
-              def isBidirectional = canonicalEffects.nonEmpty || declaredType.bparams.nonEmpty
+              def isBidirectional = canonical.nonEmpty || declared.bparams.nonEmpty
 
               val cparamsForBlocks = declaredOperation.bparams.map { p => CaptureParam(p.name) } // use the original name
-              val cparamsForEffects = canonicalEffects.map { tpe => CaptureParam(tpe.name) } // use the type name
+              val cparamsForEffects = canonical.map { tpe => CaptureParam(tpe.name) } // use the type name
               val cparams = cparamsForBlocks ++ cparamsForEffects
 
-              val FunctionType(Nil, Nil, vps, bps, tpe, effs) =
-                Context.instantiate(declaredType, targs ++ existentials, cparams.map(cap => CaptureSet(cap))) : @unchecked
+              val (vps, bps, tpe, effs) =
+                Context.instantiate(declared, targs ++ existentials, cparams.map(cap => CaptureSet(cap))) : @unchecked
 
               (vparams zip vps).foreach {
                 case (param, declaredType) =>
@@ -470,7 +471,7 @@ object Typer extends Phase[NameResolved, Typechecked] {
               // (4) synthesize type of continuation
               val resumeType = if (isBidirectional) {
                 // resume { {f} => e }
-                val resumeType = FunctionType(Nil, cparams, Nil, bps, tpe, effs)
+                val resumeType = FunctionType(Nil, cparams, Nil, bps, tpe, Effects(effs))
                 val resumeCapt = CaptureParam(Name.local("resumeBlock"))
                 FunctionType(Nil, List(resumeCapt), Nil, List(resumeType), ret, Effects.Pure)
               } else {
@@ -574,7 +575,7 @@ object Typer extends Phase[NameResolved, Typechecked] {
 
       // (4) Compute blocktype of this constructor with rigid type vars
       // i.e. Cons : `(?t1, List[?t1]) => List[?t1]`
-      val FunctionType(_, _, vps, _, ret, _) = Context.instantiate(sym.toType, targs, Nil)
+      val (vps, _, ret, _) = Context.instantiate(sym.toType, targs, Nil)
 
       // (5) given a scrutinee of `List[Int]`, we learn `?t1 -> Int`
       matchPattern(sc, ret, p)
@@ -1205,7 +1206,7 @@ object Typer extends Phase[NameResolved, Typechecked] {
 
     // (1) Instantiate blocktype
     // e.g. `[A, B] (A, A) => B` becomes `(?A, ?A) => ?B`
-    val (typeArgs, captArgs, bt @ FunctionType(_, _, vps, bps, ret, retEffs)) = Context.instantiateFresh(funTpe)
+    val (typeArgs, captArgs, (vps, bps, ret, retEffs)) = Context.instantiateFresh(CanonicalOrdering(funTpe))
 
     // provided type arguments flow into the fresh unification variables (i.e., Int <: ?A)
     if (targs.nonEmpty) (targs zip typeArgs).foreach { case (targ, tvar) => matchExpected(tvar, targ) }
@@ -1240,22 +1241,16 @@ object Typer extends Phase[NameResolved, Typechecked] {
 
     // We add return effects last to have more information at this point to
     // concretize the effect.
-    effs = effs ++ retEffs
+    effs = effs ++ Effects(retEffs)
 
     // annotate call node with inferred type arguments
     Context.annotateTypeArgs(call, typeArgs)
 
     // Annotate the call target tree with the additional capabilities
-    // We need to establish the canonical ordering of capabilities.
-    // 1) we have to use the capabilities, which are annotated on the original function type
-    // 2) we need to dealias
-    // 3) we need to compute distinct effects on the dealiased list
-    // 3) and only then substitute
-    //
-    // This is important since
-    //   [A, B](): Unit / { State[A], State[B] }
-    // with A := Int and B := Int requires us to pass two capabilities.
-    val capabilities = Context.provideCapabilities(call, retEffs.canonical.map(Context.unification.apply))
+    // The canonical ordering of capabilities is defined by the ordering of the definition site,
+    // not the callsite, so we need to be careful to not use `distinct` on `retEffs`
+    // since this might conflate State[A] and State[B] after A -> Int, B -> Int.
+    val capabilities = Context.provideCapabilities(call, retEffs.map(Context.unification.apply))
 
     val captParams = captArgs.drop(bargs.size)
     (captParams zip capabilities) foreach { case (param, cap) =>

effekt/shared/src/main/scala/effekt/core/Transformer.scala

@@ -738,7 +738,7 @@ object Transformer extends Phase[Typechecked, CoreTransformed] {
         // TODO this is exactly like in [[Callable.toType]] -- TODO repeated here:
         // TODO currently the return type cannot refer to the annotated effects, so we can make up capabilities
         //   in the future namer needs to annotate the function with the capture parameters it introduced.
-        val cparams = bparams.map(b => b.capture) ++ effects.canonical.map { tpe => symbols.CaptureParam(tpe.name) }
+        val cparams = bparams.map(b => b.capture) ++ CanonicalOrdering(effects.toList).map { tpe => symbols.CaptureParam(tpe.name) }
         val vparamTpes = vparams.map(t => substitution.substitute(t.tpe.getOrElse {
           INTERNAL_ERROR("Operation value parameters should have an annotated type.")
         }))
@@ -756,7 +756,7 @@ object Transformer extends Phase[Typechecked, CoreTransformed] {
   def operationAtDeclaration(tparamsInterface: List[Id], op: symbols.Operation)(using Context): core.BlockType = op match {
     case symbols.Operation(name, tps, vps, bps, resultType, effects, interface) =>
       // like in asSeenFrom we need to make up cparams, they cannot occur free in the result type
-      val capabilities = effects.canonical
+      val capabilities = CanonicalOrdering(effects.toList)
       val tparams = tps.drop(tparamsInterface.size)
       val bparamsBlocks = bps.map(b => transform(b.tpe.getOrElse {
         INTERNAL_ERROR("Interface declarations should have annotated types.")
@@ -817,7 +817,7 @@ object Transformer extends Phase[Typechecked, CoreTransformed] {
   def transform(tpe: BlockType)(using Context): core.BlockType = tpe match {
     case BlockType.FunctionType(tparams, cparams, vparams, bparams, result, effects) =>
 
-      val capabilityTypes = effects.canonical.map(transform)
+      val capabilityTypes = CanonicalOrdering(effects.toList).map(transform)
       val allBlockParams = bparams.map(transform) ++ capabilityTypes
 
       assert(cparams.size == allBlockParams.size,

effekt/shared/src/main/scala/effekt/symbols/kinds.scala

@@ -37,7 +37,7 @@ package object kinds {
   def wellformed(b: FunctionType)(using C: Context): Unit = b match {
     case FunctionType(tps, cps, vps, bps, res, effs) =>
       // TODO we could also check whether the same type variable shows up twice
-      if (cps.size != (bps.size + effs.canonical.size)) {
+      if (cps.size != (bps.size + effs.distinct.size)) {
         C.panic(s"Compiler invariant violated: different size of capture parameters and block parameters: ${b}")
       }
       vps.foreach { tpe => wellformed(tpe) }

effekt/shared/src/main/scala/effekt/symbols/symbols.scala

@@ -152,7 +152,7 @@ trait Callable extends BlockSymbol {
       effects = effs.distinct
       // TODO currently the return type cannot refer to the annotated effects, so we can make up capabilities
       //   in the future namer needs to annotate the function with the capture parameters it introduced.
-      capt = effects.canonical.map { tpe => CaptureParam(tpe.name) }
+      capt = CanonicalOrdering(effects.toList).map { tpe => CaptureParam(tpe.name) }
     } yield toType(ret, effects, capt)
 }
 

effekt/shared/src/main/scala/effekt/symbols/types.scala

@@ -69,7 +69,7 @@ extension (i: BlockType.InterfaceType) {
 }
 
 /**
- * Represents effect sets on function types.
+ * Represents effect _sets_ (no order, no duplicates) on function types.
  *
  * All effects are dealiased by namer. Effects are inferred via [[typer.ConcreteEffects]] so
  * by construction all entries in the set of effects here should be concrete (no unification variables).
@@ -96,7 +96,7 @@ case class Effects(effects: List[BlockType.InterfaceType]) {
   def forall(p: InterfaceType => Boolean): Boolean = effects.forall(p)
   def exists(p: InterfaceType => Boolean): Boolean = effects.exists(p)
 
-  lazy val canonical: List[InterfaceType] = effects.sorted(using CanonicalOrdering)
+  def size: Int = effects.size
 
   def distinct: Effects = Effects(effects.distinct)
 }
@@ -122,6 +122,8 @@ object CanonicalOrdering extends Ordering[InterfaceType] {
   def compare(tpe1: InterfaceType, tpe2: InterfaceType): Int = compareStructural(tpe1, tpe2)
 
   def compareStructural(tpe1: Any, tpe2: Any): Int = (tpe1, tpe2) match {
+    case (_: UnificationVar, _) | (_, _: UnificationVar) =>
+      effekt.util.messages.INTERNAL_ERROR("Cannot compute canonical ordering, since type still contains unification variables.")
     case (sym1: Symbol, sym2: Symbol) =>
       sym1.id - sym2.id
     case (p1: Product, p2: Product) if p1.getClass == p2.getClass =>
@@ -133,6 +135,15 @@ object CanonicalOrdering extends Ordering[InterfaceType] {
   }
 
   def fallback(tpe1: Any, tpe2: Any): Int = tpe1.hashCode - tpe2.hashCode
+
+  def apply(l: List[InterfaceType]): List[InterfaceType] = l.sorted(using this)
+
+  def apply(f: FunctionType): FunctionType = f match {
+    case FunctionType(tparams, cparams, vparams, bparams, result, effects) =>
+      val (cparamsBlocks, cparamsEffects) = cparams.splitAt(bparams.size)
+      val (cparams2, effects2) = (cparamsEffects zip effects.toList).sortBy(_._2)(using this).unzip
+      FunctionType(tparams, cparamsBlocks ++ cparams2, vparams, bparams, result, Effects(effects2))
+  }
 }
 
 

effekt/shared/src/main/scala/effekt/typer/ConcreteEffects.scala

@@ -10,6 +10,8 @@ import effekt.util.messages.ErrorMessageReifier
  * All effects inferred by Typer are required to be concrete and dealiased.
  *
  * This way, we can easily compare them for equality.
+ *
+ * This is a _set_ of effects and does not guarantee order!
  */
 class ConcreteEffects private[typer] (protected val effects: List[InterfaceType]) {
 

effekt/shared/src/main/scala/effekt/typer/Unification.scala

@@ -188,16 +188,16 @@ class Unification(using C: ErrorReporter) extends TypeUnifier, TypeMerger, TypeI
   /**
    * Instantiate a typescheme with provided type and capture arguments.
    */
-  def instantiate(tpe: FunctionType, targs: List[ValueType], cargs: List[Captures]): FunctionType = {
+  def instantiate(tpe: FunctionType, targs: List[ValueType], cargs: List[Captures]): (List[ValueType], List[BlockType], ValueType, List[InterfaceType]) = {
     val position = C.focus
     val FunctionType(tparams, cparams, vparams, bparams, ret, eff) = substitution.substitute(tpe)
 
     assert(targs.size == tparams.size,
       pp"Type argument and parameter size mismatch: ${targs.size} vs ${tparams.size} ($targs, $tparams)")
     assert(cargs.size == cparams.size,
       pp"Capture arguments and parameter size mismatch: ${cargs.size} vs ${cparams.size} ($cargs, $cparams)")
-    assert(cparams.size == (bparams.size + eff.canonical.size),
-      pp"Capture param count ${cparams.size} is not equal to bparam ${bparams.size} + controleffects ${eff.canonical.size}.\n  ${tpe}")
+    assert(cparams.size == (bparams.size + eff.distinct.size),
+      pp"Capture param count ${cparams.size} is not equal to bparam ${bparams.size} + controleffects ${eff.size}.\n  ${tpe}")
 
     given Instantiation = Instantiation((tparams zip targs).toMap, (cparams zip cargs).toMap)
 
@@ -207,15 +207,15 @@ class Unification(using C: ErrorReporter) extends TypeUnifier, TypeMerger, TypeI
 
     val substitutedEffects = instantiate(eff)
 
-    FunctionType(Nil, Nil, substitutedVparams, substitutedBparams, substitutedReturn, substitutedEffects)
+    (substitutedVparams, substitutedBparams, substitutedReturn, substitutedEffects)
   }
 
   /**
    * Instantiate a typescheme with fresh, rigid type variables
    *
    * i.e. `[T1, T2, C] (T1, T1) {sigma} => T2` becomes `(?T1, ?T1){} => ?T2[C !-> ?C]`
    */
-  def instantiateFresh(tpe: FunctionType): (List[ValueType], List[Captures], FunctionType) = {
+  def instantiateFresh(tpe: FunctionType): (List[ValueType], List[Captures], (List[ValueType], List[BlockType], ValueType, List[InterfaceType])) = {
     val position = C.focus
     val FunctionType(tparams, cparams, vparams, bparams, ret, eff) = substitution.substitute(tpe)
 
@@ -341,7 +341,7 @@ trait TypeInstantiator { self: Unification =>
       BoxedType(instantiate(tpe), instantiate(capt))
   }
 
-  def instantiate(t: Effects)(using Instantiation): Effects = Effects(t.toList.map(instantiate))
+  def instantiate(t: Effects)(using Instantiation): List[InterfaceType] = t.toList.map(instantiate)
 
   def instantiate(t: BlockType)(using Instantiation): BlockType = t match {
     case e: InterfaceType => instantiate(e)
@@ -362,6 +362,6 @@ trait TypeInstantiator { self: Unification =>
         vps map instantiate,
         bps map instantiate,
         instantiate(ret),
-        instantiate(eff))
+        Effects(instantiate(eff)))
   }
 }