From 057730acd3e391dd4addd29feb38ff95b0717989 Mon Sep 17 00:00:00 2001 From: Tomas Janousek Date: Tue, 15 Sep 2015 11:16:26 +0200 Subject: [PATCH 1/3] Fix whitespace in Parsers.phrase --- .../scala/scala/util/parsing/combinator/Parsers.scala | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/scala/scala/util/parsing/combinator/Parsers.scala b/src/main/scala/scala/util/parsing/combinator/Parsers.scala index f6011d3a..18bfc438 100644 --- a/src/main/scala/scala/util/parsing/combinator/Parsers.scala +++ b/src/main/scala/scala/util/parsing/combinator/Parsers.scala @@ -880,11 +880,11 @@ trait Parsers { def phrase[T](p: Parser[T]) = new Parser[T] { def apply(in: Input) = lastNoSuccessVar.withValue(None) { p(in) match { - case s @ Success(out, in1) => - if (in1.atEnd) - s - else - lastNoSuccessVar.value filterNot { _.next.pos < in1.pos } getOrElse Failure("end of input expected", in1) + case s @ Success(out, in1) => + if (in1.atEnd) + s + else + lastNoSuccessVar.value filterNot { _.next.pos < in1.pos } getOrElse Failure("end of input expected", in1) case ns => lastNoSuccessVar.value.getOrElse(ns) } } From 72ea0d78bd5ababc8b352dcfc95212916068d609 Mon Sep 17 00:00:00 2001 From: Tomas Janousek Date: Tue, 15 Sep 2015 11:27:54 +0200 Subject: [PATCH 2/3] Fix lastNoSuccessVar memory leak Fixes https://issues.scala-lang.org/browse/SI-9010, see https://issues.scala-lang.org/browse/SI-4929 for discussion. The leak happened when parsers are used outside of phrase, which seems to be not so common until one realizes that that's exactly how lexers are used in Scanner, so every lexer leaked. This should fix it. --- .../scala/util/parsing/combinator/Parsers.scala | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/scala/scala/util/parsing/combinator/Parsers.scala b/src/main/scala/scala/util/parsing/combinator/Parsers.scala index 18bfc438..115ee135 100644 --- a/src/main/scala/scala/util/parsing/combinator/Parsers.scala +++ b/src/main/scala/scala/util/parsing/combinator/Parsers.scala @@ -156,14 +156,19 @@ trait Parsers { val successful = true } - private lazy val lastNoSuccessVar = new DynamicVariable[Option[NoSuccess]](None) + /* two layers of Option: + * outer Option is None if lastNoSuccess tracking is disabled (outside of + * phrase) and Some if tracking is enabled + * inner Option is None if NoSuccess hasn't been seen yet, Some otherwise + * this is necessary to avoid leaking NoSuccesses in thread locals */ + private lazy val lastNoSuccessVar = new DynamicVariable[Option[Option[NoSuccess]]](None) /** A common super-class for unsuccessful parse results. */ sealed abstract class NoSuccess(val msg: String, override val next: Input) extends ParseResult[Nothing] { // when we don't care about the difference between Failure and Error val successful = false - if (lastNoSuccessVar.value forall (v => !(next.pos < v.next.pos))) - lastNoSuccessVar.value = Some(this) + if (lastNoSuccessVar.value exists (_ forall (v => !(next.pos < v.next.pos)))) + lastNoSuccessVar.value = Some(Some(this)) def map[U](f: Nothing => U) = this def mapPartial[U](f: PartialFunction[Nothing, U], error: Nothing => String): ParseResult[U] = this @@ -878,14 +883,14 @@ trait Parsers { * if `p` consumed all the input. */ def phrase[T](p: Parser[T]) = new Parser[T] { - def apply(in: Input) = lastNoSuccessVar.withValue(None) { + def apply(in: Input) = lastNoSuccessVar.withValue(Some(None)) { p(in) match { case s @ Success(out, in1) => if (in1.atEnd) s else - lastNoSuccessVar.value filterNot { _.next.pos < in1.pos } getOrElse Failure("end of input expected", in1) - case ns => lastNoSuccessVar.value.getOrElse(ns) + lastNoSuccessVar.value flatMap (_ filterNot { _.next.pos < in1.pos }) getOrElse Failure("end of input expected", in1) + case ns => lastNoSuccessVar.value.flatten.getOrElse(ns) } } } From 01c5bacbf8b062912487f719a5bab6147280d6d2 Mon Sep 17 00:00:00 2001 From: Tomas Janousek Date: Tue, 15 Sep 2015 15:58:19 +0200 Subject: [PATCH 3/3] Add test for SI-9010 This is extremely ugly (not only does it use reflection, is uses Java reflection with hard-coded mangled symbol name because of a bug in Scala reflection), but I don't know of a better solution. :-( --- .../scala/util/parsing/combinator/t9010.scala | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 src/test/scala/scala/util/parsing/combinator/t9010.scala diff --git a/src/test/scala/scala/util/parsing/combinator/t9010.scala b/src/test/scala/scala/util/parsing/combinator/t9010.scala new file mode 100644 index 00000000..61fafd3d --- /dev/null +++ b/src/test/scala/scala/util/parsing/combinator/t9010.scala @@ -0,0 +1,51 @@ +import scala.util.parsing.combinator._ +import scala.util.DynamicVariable + +import org.junit.Test + +class t9010 { + @Test + def test: Unit = { + val p = new grammar + val lastNoSuccessVar = getLastNoSuccessVar(p) + import p._ + + val res1 = parse(x, "x") + assert(res1.successful) + assert(lastNoSuccessVar.value == None) + + val res2 = parse(x, "y") + assert(!res2.successful) + assert(lastNoSuccessVar.value == None) + + val res3 = parseAll(x, "x") + assert(res3.successful) + assert(lastNoSuccessVar.value == None) + + val res4 = parseAll(x, "y") + assert(!res4.successful) + assert(lastNoSuccessVar.value == None) + } + + private def getLastNoSuccessVar(p: Parsers): DynamicVariable[Option[_]] = { + // use java reflection instead of scala (see below) because of + // https://issues.scala-lang.org/browse/SI-9306 + val fn = "scala$util$parsing$combinator$Parsers$$lastNoSuccessVar" + val f = p.getClass.getDeclaredMethod(fn) + f.setAccessible(true) + f.invoke(p).asInstanceOf[DynamicVariable[Option[_]]] + + /* + val ru = scala.reflect.runtime.universe + val mirror = ru.runtimeMirror(getClass.getClassLoader) + val lastNoSuccessVarField = + ru.typeOf[Parsers].decl(ru.TermName("lastNoSuccessVar")).asTerm.accessed.asTerm + mirror.reflect(p).reflectField(lastNoSuccessVarField).get. + asInstanceOf[DynamicVariable[Option[_]]] + */ + } + + private final class grammar extends RegexParsers { + val x: Parser[String] = "x" + } +}