From 2563f6c013cfad5f73b5ecbdbcb2445d3224ced5 Mon Sep 17 00:00:00 2001 From: LPTK Date: Wed, 28 Nov 2018 11:59:57 +0100 Subject: [PATCH 1/2] Check that parameters used in parsers are by-name --- .../src/fastparse/internal/MacroImpls.scala | 32 +++++++++++++++++++ .../src/fastparse/internal/RepImpls.scala | 2 +- .../test/src/fastparse/ParsingTests.scala | 23 +++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/fastparse/src/fastparse/internal/MacroImpls.scala b/fastparse/src/fastparse/internal/MacroImpls.scala index b09debef..b020b041 100644 --- a/fastparse/src/fastparse/internal/MacroImpls.scala +++ b/fastparse/src/fastparse/internal/MacroImpls.scala @@ -15,11 +15,32 @@ import scala.reflect.macros.blackbox.Context * String/Char values are known at compile time. */ object MacroImpls { + + def checkByNames[T](c: Context)(p: c.Expr[T]): Unit = { + import c.universe._ + val ParsingRunSym = typeOf[ParsingRun[_]].typeSymbol + + // Note: it is not enough to just check the top-level tree, as sometimes it may be wrapped in EagerOps or others. + + new Traverser { + override def traverse(tree: Tree): Unit = { + if (tree.tpe != null && tree.tpe.baseType(ParsingRunSym) != NoType) { + val sym = tree.symbol + if (sym != null && sym.isParameter && sym.isTerm && !sym.asTerm.isByNameParam && !sym.asTerm.isImplicit) + c.error(tree.pos, s"Parameters passed to parsers should be by-name: $tree") + } + super.traverse(tree) + } + } traverse p.tree + + } + def filterMacro[T: c.WeakTypeTag](c: Context) (f: c.Expr[T => Boolean]) (ctx: c.Expr[ParsingRun[_]]) = { import c.universe._ val lhs = c.prefix.asInstanceOf[c.Expr[EagerOps[T]]] + checkByNames(c)(lhs) reify{ ctx.splice match{ case ctx1 => lhs.splice @@ -141,6 +162,7 @@ object MacroImpls { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[T]]] + checkByNames(c)(lhs0) reify { lhs0.splice.parse0 match{ case lhs => if (!lhs.isSuccess) lhs.asInstanceOf[ParsingRun[V]] @@ -160,6 +182,7 @@ object MacroImpls { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[T]]] + checkByNames(c)(lhs0) reify { val lhs = lhs0.splice.parse0 if (!lhs.isSuccess) lhs.asInstanceOf[ParsingRun[V]] @@ -174,6 +197,7 @@ object MacroImpls { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[T]]] + checkByNames(c)(lhs0) reify { val lhs = lhs0.splice.parse0 whitespace.splice match{ case ws => @@ -198,6 +222,8 @@ object MacroImpls { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[T]]] + checkByNames(c)(lhs0) + checkByNames(c)(other) reify { ctx.splice match { case ctx5 => val oldCut = ctx5.cut @@ -244,6 +270,7 @@ object MacroImpls { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[_]]] + checkByNames(c)(lhs0) reify { ctx.splice match{ case ctx6 => @@ -427,6 +454,9 @@ object MacroImpls { val lhs = c.prefix.asInstanceOf[Expr[EagerOps[T]]] val cut1 = c.Expr[Boolean](if(cut) q"true" else q"ctx1.cut") + checkByNames(c)(lhs) + checkByNames(c)(other) + val rhs = q""" if (!ctx1.isSuccess && ctx1.cut) ctx1 else { @@ -493,6 +523,7 @@ object MacroImpls { def cutMacro[T: c.WeakTypeTag](c: Context)(ctx: c.Expr[ParsingRun[_]]): c.Expr[ParsingRun[T]] = { import c.universe._ val lhs = c.prefix.asInstanceOf[c.Expr[EagerOps[_]]] + checkByNames(c)(lhs) reify{ ctx.splice match { case ctx0 => val startIndex = ctx0.index @@ -584,6 +615,7 @@ object MacroImpls { ctx: c.Expr[ParsingRun[Any]]): c.Expr[ParsingRun[V]] = { import c.universe._ val lhs0 = c.prefix.asInstanceOf[c.Expr[EagerOps[_]]] + checkByNames(c)(lhs0) reify{ ctx.splice match{ case ctx1 => optioner.splice match { case optioner1 => diff --git a/fastparse/src/fastparse/internal/RepImpls.scala b/fastparse/src/fastparse/internal/RepImpls.scala index ea6088eb..93e9c579 100644 --- a/fastparse/src/fastparse/internal/RepImpls.scala +++ b/fastparse/src/fastparse/internal/RepImpls.scala @@ -14,7 +14,7 @@ import language.experimental.macros * due to https://github.com/scala/bug/issues/5920). * * Even the normal method overloads are manually-specialized to some extent - * for various sorts of inputs as a best-effort attempt ot minimize branching + * for various sorts of inputs as a best-effort attempt to minimize branching * in the hot paths. */ object MacroRepImpls{ diff --git a/fastparse/test/src/fastparse/ParsingTests.scala b/fastparse/test/src/fastparse/ParsingTests.scala index e1d813f2..940cd9bf 100644 --- a/fastparse/test/src/fastparse/ParsingTests.scala +++ b/fastparse/test/src/fastparse/ParsingTests.scala @@ -219,6 +219,29 @@ object ParsingTests extends TestSuite{ checkWhitespaceFlatMap() checkNonWhitespaceFlatMap() } + 'parameterized{ + def digit[_: P] = CharIn("0-9") + def letter[_: P] = CharIn("A-Z") + + assert { + val msg = compileError("def twice[T, _: P](p: P[T]): P[(T,T)] = p ~ p").msg + msg == "Parameters passed to parsers should be by-name: p" + } + def twice[T, _: P](p: => P[T]): P[(T,T)] = p ~ p + + def p[_: P] = P( twice(digit) ~ twice(letter) ) + val Parsed.Success(_, _) = parse("12AB", p(_)) // Note: this would fail if 'p' was not passed by name + + assert { + val msg = compileError("def twice2[T, _: P](l: P[T], r: => P[T]): P[(T,(T,T))] = l ~ (l ~ r)").msg + msg == "Parameters passed to parsers should be by-name: l" + } + def concats[T, _: P](l: => P[T], r: => P[T]): P[(T,(T,T))] = l ~ (l ~ r) + + def p2[_: P] = P( concats(digit,letter) ~ concats(letter,digit) ) + val Parsed.Success(_, _) = parse("12ABC3", p2(_)) + + } } def checkWhitespaceFlatMap() = { From b67c367de726987e74fdcd6c633d05d338db6620 Mon Sep 17 00:00:00 2001 From: LPTK Date: Wed, 28 Nov 2018 16:01:53 +0100 Subject: [PATCH 2/2] Allow non-implicit non-by-name wildcard-ed P[_] parameters --- .../src/fastparse/internal/MacroImpls.scala | 29 ++++++++++++------- .../src/fastparse/internal/RepImpls.scala | 5 ++++ .../test/src/fastparse/ParsingTests.scala | 4 ++- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/fastparse/src/fastparse/internal/MacroImpls.scala b/fastparse/src/fastparse/internal/MacroImpls.scala index b020b041..25a98a6a 100644 --- a/fastparse/src/fastparse/internal/MacroImpls.scala +++ b/fastparse/src/fastparse/internal/MacroImpls.scala @@ -16,23 +16,32 @@ import scala.reflect.macros.blackbox.Context */ object MacroImpls { - def checkByNames[T](c: Context)(p: c.Expr[T]): Unit = { + /** Checks a tree for subexpressions that contain references to parsers passed as parameters, + * and makes sure these parameters are by-name. + * We exclude from the check: + * - implicit parameters, because they are used pervasively to pass context around, as in: `def foo[_: P] = ...` + * - parsers whose type argument is an existential/wildcard, because they can be used safely (their value is not used) + */ + private[fastparse] def checkByNames[T](c: Context)(p: c.Expr[T]): Unit = { import c.universe._ val ParsingRunSym = typeOf[ParsingRun[_]].typeSymbol - + // Note: it is not enough to just check the top-level tree, as sometimes it may be wrapped in EagerOps or others. - - new Traverser { + + new Traverser{ override def traverse(tree: Tree): Unit = { - if (tree.tpe != null && tree.tpe.baseType(ParsingRunSym) != NoType) { - val sym = tree.symbol - if (sym != null && sym.isParameter && sym.isTerm && !sym.asTerm.isByNameParam && !sym.asTerm.isImplicit) - c.error(tree.pos, s"Parameters passed to parsers should be by-name: $tree") + Option(tree.tpe).map(_.baseType(ParsingRunSym)).collect{ + case TypeRef(_, ParsingRunSym, ty :: Nil) => // type ParsingRun[T] has one type parameter + if (ty.typeSymbol.isType && !ty.typeSymbol.asType.isExistential) { + val sym = tree.symbol + if (sym != null && sym.isParameter && sym.isTerm && !sym.asTerm.isByNameParam && !sym.asTerm.isImplicit) + c.error(tree.pos, s"Parameters passed to parsers should be by-name: $tree") + } } super.traverse(tree) } - } traverse p.tree - + }.traverse(p.tree) + } def filterMacro[T: c.WeakTypeTag](c: Context) diff --git a/fastparse/src/fastparse/internal/RepImpls.scala b/fastparse/src/fastparse/internal/RepImpls.scala index 93e9c579..81c76e64 100644 --- a/fastparse/src/fastparse/internal/RepImpls.scala +++ b/fastparse/src/fastparse/internal/RepImpls.scala @@ -18,11 +18,16 @@ import language.experimental.macros * in the hot paths. */ object MacroRepImpls{ + import MacroImpls.checkByNames + def repXMacro0[T: c.WeakTypeTag, V: c.WeakTypeTag](c: Context) (whitespace: Option[c.Tree], min: Option[c.Tree]) (repeater: c.Tree, ctx: c.Tree): c.Tree = { import c.universe._ + + checkByNames(c)(c.prefix) + val repeater1 = TermName(c.freshName("repeater")) val ctx1 = TermName(c.freshName("repeater")) val acc = TermName(c.freshName("acc")) diff --git a/fastparse/test/src/fastparse/ParsingTests.scala b/fastparse/test/src/fastparse/ParsingTests.scala index 940cd9bf..fc999028 100644 --- a/fastparse/test/src/fastparse/ParsingTests.scala +++ b/fastparse/test/src/fastparse/ParsingTests.scala @@ -233,7 +233,7 @@ object ParsingTests extends TestSuite{ val Parsed.Success(_, _) = parse("12AB", p(_)) // Note: this would fail if 'p' was not passed by name assert { - val msg = compileError("def twice2[T, _: P](l: P[T], r: => P[T]): P[(T,(T,T))] = l ~ (l ~ r)").msg + val msg = compileError("def concats[T, _: P](l: P[T], r: => P[T]): P[(T,(T,T))] = l ~ (l ~ r)").msg msg == "Parameters passed to parsers should be by-name: l" } def concats[T, _: P](l: => P[T], r: => P[T]): P[(T,(T,T))] = l ~ (l ~ r) @@ -241,6 +241,8 @@ object ParsingTests extends TestSuite{ def p2[_: P] = P( concats(digit,letter) ~ concats(letter,digit) ) val Parsed.Success(_, _) = parse("12ABC3", p2(_)) + def foo[T, _: P](p: P[_]): P[_] = p ~ digit // compiles because we used a wildcard in P[_] + } }