sbt / sbt-projectmatrix

MIT License
124 stars 14 forks source link

Inner class WeakAxis #49

Closed htmldoug closed 3 years ago

htmldoug commented 3 years ago

Problem definition

This may not be a problem with sbt-projectmatrix at all. However, the scripted test demonstrates what I wanted to do, but failed in a surprising way. The project fails to load with:

[info] [error] no rows were found in akka matching ProjectRow(true, List(play27(2.7.4), akka25(2.5.23), PlatformAxis(jvm,JVM,jvm), ScalaVersionAxis(2.12.12,2.12))):
[info] [error] ProjectRow(true, List(akka25(2.5.23), PlatformAxis(jvm,JVM,jvm), ScalaVersionAxis(2.12.12,2.12)))
[info] [error] ProjectRow(true, List(akka26(2.6.8), PlatformAxis(jvm,JVM,jvm), ScalaVersionAxis(2.12.12,2.12)))
[info] [error] ProjectRow(true, List(akka26(2.6.8), PlatformAxis(jvm,JVM,jvm), ScalaVersionAxis(2.13.3,2.13)))

Note that play27(2.7.4) is a WeakAxis.

The problem seems to be that at compile time, the inner classes have different types, but at runtime, they eq each other. That makes isStronglyCompatible conclude (undesirably) that the "play" WeakAxis must have values of the same axis type defined in the "akka" project.

Slightly related, the same problem seems to exist with enums.

scala> object Apple extends Enumeration {
     |   val GrannySmith = Value
     | }
object Apple

scala> object Orange extends Enumeration {
     |   val Valencia = Value
     | }
object Orange

scala> Apple.GrannySmith.getClass eq Orange.Valencia.getClass
val res1: Boolean = true

Workaround

sbt-projectmatrix works as expected if I redefine the axis class in each of the inner class instances.

object AkkaAxis extends CustomAxis("akka") {
  type V = AkkaAxis

  case class AkkaAxis(version: String) extends Value(version)
}

object PlayAxis extends CustomAxis("play") {
  type V = PlayAxis

  case class PlayAxis(version: String) extends Value(version)
}

abstract class CustomAxis(prefix: String) {
  type V
  // ...
}

The duplication is unsatisfying.

Paths forward

I don't have a good suggestion for what could be used as an axis identifier instead of weakAxis.getClass, e.g. using a TypeTag seems difficult. Maybe VirtualAxis could expose an overridable def axisIdentifier: Any = getClass. This would not fix the problem of the shared class instance, but it would at least expose an escape hatch that could be used to eliminate the duplication.

Or perhaps there's a better workaround I missed that would make something like that unnecessary? WDYT?

eed3si9n commented 3 years ago

@htmldoug Thanks for this bug report! I'd be curious to minimize the actual eq situation.

eed3si9n commented 3 years ago
scala> :paste
// Entering paste mode (ctrl-D to finish)

object AkkaAxis extends InnerClassAxis("akka")

object PlayAxis extends InnerClassAxis("play")

class InnerClassAxis(prefix: String) {
  case class Value(version: String)
}

// Exiting paste mode, now interpreting.

defined object AkkaAxis
defined object PlayAxis
defined class InnerClassAxis

scala> AkkaAxis.Value("2").getClass == PlayAxis.Value("2").getClass
res0: Boolean = true
eed3si9n commented 3 years ago

I guess the idiomatic workaround for this is using reflection API?

scala> import scala.reflect.runtime.universe._
import scala.reflect.runtime.universe._

scala> def axisTag[A: TypeTag](a: A) = implicitly[TypeTag[A]]
axisTag: [A](a: A)(implicit evidence$1: reflect.runtime.universe.TypeTag[A])reflect.runtime.universe.TypeTag[A]

scala> axisTag(AkkaAxis.Value("2"))
res0: reflect.runtime.universe.TypeTag[AkkaAxis.Value] = TypeTag[AkkaAxis.Value]

scala> axisTag(AkkaAxis.Value("2")) == axisTag(PlayAxis.Value("2"))
res1: Boolean = false
htmldoug commented 3 years ago

Thanks for the response. That's about as far as I got, too. The next question: how do we make a TypeTag available to isStronglyCompatible?

eed3si9n commented 3 years ago

If I recall correctly, there's an interesting use for the loathed Scala feature auto-tupling, and it's that we can use it to capture the individual types given to a method that looks like a parameter but in fact accepts tuples. So we can make Seq(...) wrapper called TypedSeq(...) with:

def apply[A1: TypeTag, A2: TypeTag]((a1: A1, a2: A2)): Seq[(VirtualAxis, TypeTag[_])]

That sounds potentially too much work to support inherited inner classes, but it's a potential solution.

htmldoug commented 3 years ago

Wow, TIL. That does seem like overkill.

My workaround has improved into:

case class AkkaAxis(version: String) extends PrefixedWeakAxis("akka")
object AkkaAxis extends CurrentAxis[AkkaAxis] {
  val Akka24 = AkkaAxis("2.4.20")
  val Akka25 = AkkaAxis("2.5.23")
  val Akka26 = AkkaAxis("2.6.3")
}

case class PlayAxis(version: String) extends PrefixedWeakAxis("play")
object PlayAxis extends CurrentAxis[PlayAxis] {
  val Play25 = PlayAxis("2.5.19")
  val Play27 = PlayAxis("2.7.4")
  val Play28 = PlayAxis("2.8.1")
}

abstract class CurrentAxis[T: ClassTag] {
  def current: Def.Initialize[T] = Def.setting {
    virtualAxes.value.collectFirst {
      case a: T => a
    }.get
  }
}

abstract class PrefixedWeakAxis(prefix: String) extends VirtualAxis.WeakAxis {
  def version: String
  val majorMinor: String = version.split('.').take(2).mkString
  def nameComponent: String = s"$prefix$majorMinor"
  override def directorySuffix: String = s"-$nameComponent"
  override def idSuffix: String = directorySuffix
  override def toString: String = s"$nameComponent($version)"
}

Stepping back, I care more that there's a convenient way to get the current value of an axis than that inner classes are supported.

I'm going to close this PR, since it seems like inner classes are a dead end, and the workaround is good enough. Thanks for reviewing.