Skip to content

Commit

Permalink
Fix discovery of targets whose names get mangled due to their pseudo-…
Browse files Browse the repository at this point in the history
…private nature (#2883)

Fixes #2844

Adds an integration test to cover this edge case, a few other
permutations of it that I could come up with, and other
`private`-related behavior. I'm not sure if I managed to catch all
different ways these methods can be mangled, but if I missed any we can
discover/add them later. This new test fails if I remove any part of the
code change in `GroupEvaluator.scala`

I'm actually not sure if this should be an integration test or a unit
test.

* Technically this code path should be exercised in unit tests as well I
think, and I think this failure mode should be triggerable.
* But it might be affected by exactly how the `def`s are wrapped in
nested `object`s/`trait`s/`class`es, which is something that unit tests
do not do realistically.
* Also, a unit test cannot exercise the failure code path when you try
and specify a `private` target from the command, since it will simply
not compile (being `private` and all), and AFAICT this is somewhere
we're missing coverage (unrelated to this specific failure mode)

So I went with integration test.

Pull request: #2883
  • Loading branch information
lihaoyi committed Nov 19, 2023
1 parent bc84d5b commit 69b2199
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 2 deletions.
43 changes: 43 additions & 0 deletions integration/feature/private-methods/repo/build.sc
@@ -0,0 +1,43 @@
import mill._

def pub = T {
priv()
}

private def priv = T {
"priv"
}

object foo extends Module {
def bar = T {
baz()
}
}

private def baz = T {
"bazOuter"
}

object qux extends Module {
object foo extends Module {
def bar = T {
baz()
}
}
private def baz = T {
"bazInner"
}
}

object cls extends cls
class cls extends Module {
object foo extends Module {
def bar = T {
baz()
}
}

private def baz = T {
"bazCls"
}
}
@@ -0,0 +1,46 @@
package mill.integration

import utest._

object PrivateMethodsTests extends IntegrationTestSuite {
val tests = Tests {
val wsRoot = initWorkspace()
"simple" - {
// Simple public target depending on private target works
val pub = evalStdout("show", "pub")
assert(pub.out == "\"priv\"")
assert(pub.isSuccess)

// Make sure calling private methods indirectly in a variety of scenarios works
// even when they're "not really private" due to
val fooBar = evalStdout("show", "foo.bar")
assert(fooBar.out == "\"bazOuter\"")
assert(fooBar.isSuccess)

val quxFooBar = evalStdout("show", "qux.foo.bar")
assert(quxFooBar.out == "\"bazInner\"")
assert(quxFooBar.isSuccess)

val clsFooBar = evalStdout("show", "cls.foo.bar")
assert(clsFooBar.out == "\"bazCls\"")
assert(clsFooBar.isSuccess)

// Make sure calling private methods directly fails
val priv = evalStdout("show", "priv")
assert(priv.err.contains("Cannot resolve priv"))
assert(priv.isSuccess == false)

val baz = evalStdout("show", "baz")
assert(baz.err.contains("Cannot resolve baz"))
assert(baz.isSuccess == false)

val quxBaz = evalStdout("show", "qux.baz")
assert(quxBaz.err.contains("Cannot resolve qux.baz"))
assert(quxBaz.isSuccess == false)

val clsBaz = evalStdout("show", "cls.baz")
assert(clsBaz.err.contains("Cannot resolve cls.baz"))
assert(clsBaz.isSuccess == false)
}
}
}
10 changes: 8 additions & 2 deletions main/eval/src/mill/eval/GroupEvaluator.scala
Expand Up @@ -8,7 +8,7 @@ import mill.eval.Evaluator.TaskResult
import mill.util._

import scala.collection.mutable
import scala.reflect.NameTransformer.decode
import scala.reflect.NameTransformer.{encode, decode}
import scala.util.DynamicVariable
import scala.util.control.NonFatal

Expand Down Expand Up @@ -98,7 +98,13 @@ private[mill] trait GroupEvaluator {
val methods = for {
c <- transitiveParents
m <- c.getDeclaredMethods
if decode(m.getName) == namedTask.ctx.segment.pathSegments.head
encodedTaskName = encode(namedTask.ctx.segment.pathSegments.head)
if m.getName == encodedTaskName ||
// Handle scenarios where private method names get mangled when they are
// not really JVM-private due to being accessed by Scala nested objects
// or classes https://github.com/scala/bug/issues/9306
m.getName == (c.getName.replace('.', '$') + "$$" + encodedTaskName) ||
m.getName == (c.getName.replace('.', '$') + "$" + encodedTaskName)
} yield m

val methodClass = methods.head.getDeclaringClass.getName
Expand Down

0 comments on commit 69b2199

Please sign in to comment.