From c627dd1c20577115a111b293296dd06392220880 Mon Sep 17 00:00:00 2001 From: "Joseph K. Strauss" Date: Wed, 6 Jun 2018 23:39:08 -0400 Subject: Allow hyphens in module and task names (#362) * Allow bacticked tasks * Prevent stack overflow * Test for illegal bacticked identifiers * Filter out illegal backticked identifiers The only legal identifiers are aplanumeric, unserscore (_), and hyphens (-). * Remove unused method that is invalid * Document valid characters for module/task names --- core/src/mill/define/Module.scala | 29 ++++++++++---------- core/src/mill/util/ParseArgs.scala | 8 ++++-- docs/pages/2 - Configuring Mill.md | 38 +++++++++++++++++++++++++++ main/src/mill/main/ReplApplyHandler.scala | 8 +++--- main/src/mill/main/Resolve.scala | 12 ++++----- main/test/src/mill/define/BasePathTests.scala | 6 +++++ main/test/src/mill/define/DiscoverTests.scala | 3 +++ main/test/src/mill/define/GraphTests.scala | 12 +++++++++ main/test/src/mill/eval/EvaluationTests.scala | 12 +++++++++ main/test/src/mill/eval/FailureTests.scala | 33 +++++++++++++++++++++++ main/test/src/mill/main/MainTests.scala | 14 ++++++++++ main/test/src/mill/util/TestGraphs.scala | 9 +++++++ 12 files changed, 157 insertions(+), 27 deletions(-) diff --git a/core/src/mill/define/Module.scala b/core/src/mill/define/Module.scala index 3f91b524..f72ec8ca 100644 --- a/core/src/mill/define/Module.scala +++ b/core/src/mill/define/Module.scala @@ -3,9 +3,12 @@ package mill.define import java.lang.reflect.Modifier import ammonite.ops.Path +import mill.util.ParseArgs import scala.language.experimental.macros import scala.reflect.ClassTag +import scala.reflect.NameTransformer.decode + /** * `Module` is a class meant to be extended by `trait`s *only*, in order to @@ -44,7 +47,7 @@ object Module{ lazy val modules = traverse(Seq(_)) lazy val segmentsToModules = modules.map(m => (m.millModuleSegments, m)).toMap - lazy val targets = traverse{_.millInternal.reflect[Target[_]]}.toSet + lazy val targets = traverse{_.millInternal.reflectAll[Target[_]]}.toSet lazy val segmentsToTargets = targets .map(t => (t.ctx.segments, t)) @@ -56,32 +59,30 @@ object Module{ lazy val millModuleEnclosing = outer.millOuterCtx.enclosing lazy val millModuleLine = outer.millOuterCtx.lineNum - def reflect[T: ClassTag] = { + private def reflect[T: ClassTag](filter: (String) => Boolean): Array[T] = { val runtimeCls = implicitly[ClassTag[T]].runtimeClass for{ - m <- outer.getClass.getMethods + m <- outer.getClass.getMethods.sortBy(_.getName) + n = decode(m.getName) if - !m.getName.contains('$') && + filter(n) && + ParseArgs.isLegalIdentifier(n) && m.getParameterCount == 0 && (m.getModifiers & Modifier.STATIC) == 0 && (m.getModifiers & Modifier.ABSTRACT) == 0 && runtimeCls.isAssignableFrom(m.getReturnType) } yield m.invoke(outer).asInstanceOf[T] } - def reflectNames[T: ClassTag] = { - val runtimeCls = implicitly[ClassTag[T]].runtimeClass - for{ - m <- outer.getClass.getMethods - if - (m.getModifiers & Modifier.STATIC) == 0 && - runtimeCls.isAssignableFrom(m.getReturnType) - } yield m.getName - } + + def reflectAll[T: ClassTag]: Array[T] = reflect(Function.const(true)) + + def reflectSingle[T: ClassTag](label: String): Option[T] = reflect(_ == label).headOption + // For some reason, this fails to pick up concrete `object`s nested directly within // another top-level concrete `object`. This is fine for now, since Mill's Ammonite // script/REPL runner always wraps user code in a wrapper object/trait def reflectNestedObjects[T: ClassTag] = { - (reflect[T] ++ + (reflectAll[T] ++ outer .getClass .getClasses diff --git a/core/src/mill/util/ParseArgs.scala b/core/src/mill/util/ParseArgs.scala index 274f6449..ae3b1685 100644 --- a/core/src/mill/util/ParseArgs.scala +++ b/core/src/mill/util/ParseArgs.scala @@ -112,9 +112,13 @@ object ParseArgs { case Parsed.Success(selector, _) => Right(selector) } + private val identChars = ('a' to 'z') ++ ('A' to 'Z') ++ ('0' to '9') ++ Seq('_', '-') + private val ident = P( CharsWhileIn(identChars) ).! + + def isLegalIdentifier(identifier: String): Boolean = + (Start ~ ident ~ End).parse(identifier).isInstanceOf[Parsed.Success[_]] + private def parseSelector(input: String) = { - val identChars = ('a' to 'z') ++ ('A' to 'Z') ++ ('0' to '9') ++ Seq('_', '-') - val ident = P( CharsWhileIn(identChars) ).! val ident2 = P( CharsWhileIn(identChars ++ ".") ).! val segment = P( ident ).map( Segment.Label) val crossSegment = P("[" ~ ident2.rep(1, sep = ",") ~ "]").map(Segment.Cross) diff --git a/docs/pages/2 - Configuring Mill.md b/docs/pages/2 - Configuring Mill.md index 43191950..03489f68 100644 --- a/docs/pages/2 - Configuring Mill.md +++ b/docs/pages/2 - Configuring Mill.md @@ -372,6 +372,44 @@ object foo extends MySpecialModule object bar extends MySpecialModule ``` +## Module/Task Names + +```scala +// build.sc +import mill._ +import mill.scalalib._ + +object `hyphenated-module` extends Module { + def `hyphenated-target` = T{ + println("This is a hyphenated target in a hyphenated module.") + } +} + +object unhyphenatedModule extends Module { + def unhyphenated_target = T{ + println("This is an unhyphenated target in an unhyphenated module.") + } + def unhyphenated_target2 = T{ + println("This is the second unhyphenated target in an unhyphenated module.") + } +} +``` + +Mill modules and tasks may be composed of any of the following characters types: + - Alphanumeric (A-Z, a-z, and 0-9) + - Underscore (_) + - Hyphen (-) + +Due to Scala naming restrictions, module and task names with hyphens must be surrounded by back-ticks (`). + +Using hyphenated names at the command line is unaffected by these restrictions. + +```bash +mill hyphenated-module.hyphenated-target +mill unhyphenatedModule.unhyphenated_target +mill unhyphenatedModule.unhyphenated_target2 +``` + ## Overriding Tasks ```scala diff --git a/main/src/mill/main/ReplApplyHandler.scala b/main/src/mill/main/ReplApplyHandler.scala index 9ce30142..22a247cc 100644 --- a/main/src/mill/main/ReplApplyHandler.scala +++ b/main/src/mill/main/ReplApplyHandler.scala @@ -44,10 +44,10 @@ object ReplApplyHandler{ def pprintModule(m: mill.define.Module, evaluator: Evaluator[_]) = { pprint.Tree.Lazy( ctx => Iterator(m.millInternal.millModuleEnclosing, ":", m.millInternal.millModuleLine.toString) ++ - (if (m.millInternal.reflect[mill.Module].isEmpty) Nil + (if (m.millInternal.reflectAll[mill.Module].isEmpty) Nil else ctx.applyPrefixColor("\nChildren:").toString +: - m.millInternal.reflect[mill.Module].map("\n ." + _.millOuterCtx.segment.pathSegments.mkString("."))) ++ + m.millInternal.reflectAll[mill.Module].map("\n ." + _.millOuterCtx.segment.pathSegments.mkString("."))) ++ (evaluator.rootModule.millDiscover.value.get(m.getClass) match{ case None => Nil case Some(commands) => @@ -57,10 +57,10 @@ object ReplApplyHandler{ ")()" } }) ++ - (if (m.millInternal.reflect[Target[_]].isEmpty) Nil + (if (m.millInternal.reflectAll[Target[_]].isEmpty) Nil else { Seq(ctx.applyPrefixColor("\nTargets:").toString) ++ - m.millInternal.reflect[Target[_]].sortBy(_.label).map(t => + m.millInternal.reflectAll[Target[_]].map(t => "\n ." + t.label + "()" ) }) diff --git a/main/src/mill/main/Resolve.scala b/main/src/mill/main/Resolve.scala index 4baac312..d0a08c87 100644 --- a/main/src/mill/main/Resolve.scala +++ b/main/src/mill/main/Resolve.scala @@ -15,7 +15,7 @@ object ResolveMetadata extends Resolve[String]{ val targets = obj .millInternal - .reflect[Target[_]] + .reflectAll[Target[_]] .map(_.toString) val commands = for{ (cls, entryPoints) <- discover.value @@ -127,8 +127,7 @@ object ResolveSegments extends Resolve[Segments] { val target = obj .millInternal - .reflect[Target[_]] - .find(_.label == last) + .reflectSingle[Target[_]](last) .map(t => Right(t.ctx.segments)) val command = @@ -193,16 +192,15 @@ object ResolveTasks extends Resolve[NamedTask[Any]]{ Right( obj.millInternal.modules .filter(_ != obj) - .flatMap(m => m.millInternal.reflect[Target[_]]) + .flatMap(m => m.millInternal.reflectAll[Target[_]]) ) - case "_" => Right(obj.millInternal.reflect[Target[_]]) + case "_" => Right(obj.millInternal.reflectAll[Target[_]]) case _ => val target = obj .millInternal - .reflect[Target[_]] - .find(_.label == last) + .reflectSingle[Target[_]](last) .map(Right(_)) val command = Resolve.invokeCommand(obj, last, discover, rest).headOption diff --git a/main/test/src/mill/define/BasePathTests.scala b/main/test/src/mill/define/BasePathTests.scala index 1f5b4037..d5167081 100644 --- a/main/test/src/mill/define/BasePathTests.scala +++ b/main/test/src/mill/define/BasePathTests.scala @@ -14,6 +14,12 @@ object BasePathTests extends TestSuite{ 'singleton - { check(testGraphs.singleton)(identity) } + 'backtickIdentifiers - { + check(testGraphs.bactickIdentifiers)( + _.`nested-module`, + "nested-module" + ) + } 'separateGroups - { check(TestGraphs.triangleTask)(identity) } diff --git a/main/test/src/mill/define/DiscoverTests.scala b/main/test/src/mill/define/DiscoverTests.scala index cd9939f0..248d6afe 100644 --- a/main/test/src/mill/define/DiscoverTests.scala +++ b/main/test/src/mill/define/DiscoverTests.scala @@ -14,6 +14,9 @@ object DiscoverTests extends TestSuite{ 'singleton - { check(testGraphs.singleton)(_.single) } + 'backtickIdentifiers { + check(testGraphs.bactickIdentifiers)(_.`up-target`, _.`a-down-target`, _.`nested-module`.`nested-target`) + } 'separateGroups - { check(TestGraphs.triangleTask)(_.left, _.right) } diff --git a/main/test/src/mill/define/GraphTests.scala b/main/test/src/mill/define/GraphTests.scala index 7e6680be..224ce59f 100644 --- a/main/test/src/mill/define/GraphTests.scala +++ b/main/test/src/mill/define/GraphTests.scala @@ -25,6 +25,10 @@ object GraphTests extends TestSuite{ targets = Agg(singleton.single), expected = Agg(singleton.single) ) + 'backtickIdentifiers - check( + targets = Agg(bactickIdentifiers.`a-down-target`), + expected = Agg(bactickIdentifiers.`up-target`, bactickIdentifiers.`a-down-target`) + ) 'pair - check( targets = Agg(pair.down), expected = Agg(pair.up, pair.down) @@ -95,6 +99,14 @@ object GraphTests extends TestSuite{ Agg(_.single), Agg(singleton.single -> 1) ) + 'backtickIdentifiers - check(bactickIdentifiers)( + _.`a-down-target`, + Agg(_.`up-target`, _.`a-down-target`), + Agg( + bactickIdentifiers.`up-target` -> 1, + bactickIdentifiers.`a-down-target` -> 1 + ) + ) 'pair - check(pair)( _.down, Agg(_.up, _.down), diff --git a/main/test/src/mill/eval/EvaluationTests.scala b/main/test/src/mill/eval/EvaluationTests.scala index 9c215086..75a5bbe3 100644 --- a/main/test/src/mill/eval/EvaluationTests.scala +++ b/main/test/src/mill/eval/EvaluationTests.scala @@ -67,6 +67,18 @@ object EvaluationTests extends TestSuite{ // After incrementing the counter, it forces re-evaluation check(single, expValue = 1, expEvaled = Agg(single)) } + 'backtickIdentifiers - { + import graphs.bactickIdentifiers._ + val check = new Checker(bactickIdentifiers) + + check(`a-down-target`, expValue = 0, expEvaled = Agg(`up-target`, `a-down-target`)) + + `a-down-target`.counter += 1 + check(`a-down-target`, expValue = 1, expEvaled = Agg(`a-down-target`)) + + `up-target`.counter += 1 + check(`a-down-target`, expValue = 2, expEvaled = Agg(`up-target`, `a-down-target`)) + } 'pair - { import pair._ val check = new Checker(pair) diff --git a/main/test/src/mill/eval/FailureTests.scala b/main/test/src/mill/eval/FailureTests.scala index 244b084d..22021079 100644 --- a/main/test/src/mill/eval/FailureTests.scala +++ b/main/test/src/mill/eval/FailureTests.scala @@ -80,6 +80,39 @@ object FailureTests extends TestSuite{ expectedRawValues = Seq(Result.Skipped) ) } + 'evaluateBacktickIdentifiers - { + val check = new TestEvaluator(bactickIdentifiers) + import bactickIdentifiers._ + check.fail( + `a-down-target`, + expectedFailCount = 0, + expectedRawValues = Seq(Result.Success(0)) + ) + + `up-target`.failure = Some("lols") + + check.fail( + `a-down-target`, + expectedFailCount = 1, + expectedRawValues = Seq(Result.Skipped) + ) + + `up-target`.failure = None + + check.fail( + `a-down-target`, + expectedFailCount = 0, + expectedRawValues = Seq(Result.Success(0)) + ) + + `up-target`.exception = Some(new IndexOutOfBoundsException()) + + check.fail( + `a-down-target`, + expectedFailCount = 1, + expectedRawValues = Seq(Result.Skipped) + ) + } 'multipleUsesOfDest - { object build extends TestUtil.BaseModule { // Using `T.ctx( ).dest` twice in a single task is ok diff --git a/main/test/src/mill/main/MainTests.scala b/main/test/src/mill/main/MainTests.scala index f9bf7aec..e836099c 100644 --- a/main/test/src/mill/main/MainTests.scala +++ b/main/test/src/mill/main/MainTests.scala @@ -34,6 +34,20 @@ object MainTests extends TestSuite{ 'neg6 - check("single.doesntExist", Left("Task single is not a module and has no children.")) 'neg7 - check("", Left("Selector cannot be empty")) } + 'backtickIdentifiers - { + val check = MainTests.check(bactickIdentifiers) _ + 'pos1 - check("up-target", Right(Seq(_.`up-target`))) + 'pos2 - check("a-down-target", Right(Seq(_.`a-down-target`))) + 'neg1 - check("uptarget", Left("Cannot resolve uptarget. Did you mean up-target?")) + 'neg2 - check("upt-arget", Left("Cannot resolve upt-arget. Did you mean up-target?")) + 'neg3 - check("up-target.doesntExist", Left("Task up-target is not a module and has no children.")) + 'neg4 - check("", Left("Selector cannot be empty")) + 'neg5 - check("invisible&", Left("Cannot resolve invisible. Try `mill resolve _` to see what's available.")) + 'nested - { + 'pos - check("nested-module.nested-target", Right(Seq(_.`nested-module`.`nested-target`))) + 'neg - check("nested-module.doesntExist", Left("Cannot resolve nested-module.doesntExist. Try `mill resolve nested-module._` to see what's available.")) + } + } 'nested - { val check = MainTests.check(nestedModule) _ 'pos1 - check("single", Right(Seq(_.single))) diff --git a/main/test/src/mill/util/TestGraphs.scala b/main/test/src/mill/util/TestGraphs.scala index 83e03576..d3b35ddc 100644 --- a/main/test/src/mill/util/TestGraphs.scala +++ b/main/test/src/mill/util/TestGraphs.scala @@ -20,6 +20,15 @@ class TestGraphs(){ val single = test() } + object bactickIdentifiers extends TestUtil.BaseModule { + val `up-target` = test() + val `a-down-target` = test(`up-target`) + val `invisible&` = test() + object `nested-module` extends TestUtil.BaseModule { + val `nested-target` = test() + } + } + // up---down object pair extends TestUtil.BaseModule{ val up = test() -- cgit v1.2.3