Skip to content

Commit

Permalink
Fix sneaky binary incompatibility in Discover.scala (#2752)
Browse files Browse the repository at this point in the history
fixes #2749

The basic problem is that the signature of `Discover.apply()` changed,
but it only changed in the argument's generic type parameters, so it was
nominally the same type after erasure, and accepted the `Map[Class[_],
Seq]` even though it was expecting a `Map[Class[_], (Seq, Seq)]`, but
later on in the code it would blow up with a `ClassCastException`

This causes problems with Mill plugins containing external modules,
which have their own `Discover[T]` macro pre-expanded and would not get
re-compiled with the new 0.11.3 version of Mill.

This PR works around the problem by adding a forwarder
`Discover.apply(value: Map[Class, Seq])` that does the right thing and
expands the `Seq` into a `(Seq, Seq)`, while adding a dummy paramter to
`case class Discover` to avoid conflicts. A similar forwarder
`Discover.<init>(value: Map[Class, Seq])` is needed to preserve binary
compatibility (I think?)

Tested manually via `./mill -i dev.run example/basic/1-simple-scala
--import ivy:io.chris-kipp::mill-github-dependency-graph::0.2.6
io.kipp.mill.github.dependency.graph.Graph/generate`, which fails before
this PR and passes after

---------

Co-authored-by: Chris Kipp <ckipp@pm.me>
Co-authored-by: Tobias Roeser <le.petit.fou@web.de>
  • Loading branch information
3 people committed Sep 18, 2023
1 parent 55d04b4 commit 98bbfe8
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
31 changes: 27 additions & 4 deletions main/define/src/mill/define/Discover.scala
Expand Up @@ -16,15 +16,38 @@ import scala.reflect.macros.blackbox
* the `T.command` methods we find. This mapping from `Class[_]` to `MainData`
* can then be used later to look up the `MainData` for any module.
*/
case class Discover[T] private (val value: Map[
Class[_],
(Seq[String], Seq[mainargs.MainData[_, _]])
])
case class Discover[T] private (
value: Map[
Class[_],
(Seq[String], Seq[mainargs.MainData[_, _]])
],
dummy: Int = 0 /* avoid conflict with Discover.apply(value: Map) below*/
) {
@deprecated("Binary compatibility shim", "Mill 0.11.4")
private[define] def this(value: Map[Class[_], Seq[mainargs.MainData[_, _]]]) =
this(value.view.mapValues((Nil, _)).toMap)
// Explicit copy, as we also need to provide an override for bin-compat reasons
def copy[T](
value: Map[
Class[_],
(Seq[String], Seq[mainargs.MainData[_, _]])
] = value,
dummy: Int = dummy /* avoid conflict with Discover.apply(value: Map) below*/
): Discover[T] = new Discover[T](value, dummy)
@deprecated("Binary compatibility shim", "Mill 0.11.4")
private[define] def copy[T](value: Map[Class[_], Seq[mainargs.MainData[_, _]]]): Discover[T] = {
new Discover[T](value.view.mapValues((Nil, _)).toMap, dummy)
}
}

object Discover {
def apply2[T](value: Map[Class[_], (Seq[String], Seq[mainargs.MainData[_, _]])]): Discover[T] =
new Discover[T](value)

@deprecated("Binary compatibility shim", "Mill 0.11.4")
def apply[T](value: Map[Class[_], Seq[mainargs.MainData[_, _]]]): Discover[T] =
new Discover[T](value.view.mapValues((Nil, _)).toMap)

def apply[T]: Discover[T] = macro Router.applyImpl[T]

private class Router(val ctx: blackbox.Context) extends mainargs.Macros(ctx) {
Expand Down
2 changes: 1 addition & 1 deletion main/resolve/src/mill/resolve/Resolve.scala
Expand Up @@ -227,11 +227,11 @@ trait Resolve[T] {
nullCommandDefaults: Boolean
): Either[String, Seq[T]] = {
val rootResolved = ResolveCore.Resolved.Module(Segments(), rootModule.getClass)
val allPossibleNames = rootModule.millDiscover.value.values.flatMap(_._1).toSet
val resolved =
ResolveCore.resolve(rootModule, sel.value.toList, rootResolved, Segments()) match {
case ResolveCore.Success(value) => Right(value)
case ResolveCore.NotFound(segments, found, next, possibleNexts) =>
val allPossibleNames = rootModule.millDiscover.value.values.flatMap(_._1).toSet
Left(ResolveNotFoundHandler(sel, segments, found, next, possibleNexts, allPossibleNames))
case ResolveCore.Error(value) => Left(value)
}
Expand Down

0 comments on commit 98bbfe8

Please sign in to comment.