Skip to content

Issue 9 - Make the AST position aware, use that for TypeNotFoundException, and pretty print exceptions in general #11

Closed
wants to merge 11 commits into from
+126 −79
View
1 README.md
@@ -163,3 +163,4 @@ included file `LICENSE`.
- Robey Pointer
- Ian Ownbey
- Jeremy Cloud
+- Aaron Stone
View
9 src/main/scala/com/twitter/handlebar/Handlebar.scala
@@ -16,6 +16,9 @@
package com.twitter.handlebar
+import scala.util.parsing.input.StreamReader
+import java.io.StringReader
+
case class Unpacker[T](
document: AST.Document,
unpacker: T => Dictionary,
@@ -29,6 +32,9 @@ object Handlebar {
import Dictionary._
def generate(template: String, dictionary: Dictionary): String =
+ generate(Parser(StreamReader(new StringReader(template))), dictionary)
+
+ def generate(template: StreamReader, dictionary: Dictionary): String =
generate(Parser(template), dictionary)
def generate(document: Document, dictionary: Dictionary): String = {
@@ -61,7 +67,8 @@ object Handlebar {
}
case class Handlebar(document: AST.Document) {
- def this(template: String) = this(Parser(template))
+ def this(template: StreamReader) = this(Parser(template))
+ def this(template: String) = this(StreamReader(new StringReader(template)))
/**
* Create a string out of a template, using a dictionary to fill in the blanks.
View
5 src/main/scala/com/twitter/handlebar/Parser.scala
@@ -19,6 +19,8 @@ package com.twitter.handlebar
import scala.collection.mutable
import scala.util.parsing.combinator._
import scala.util.parsing.combinator.lexical._
+import scala.util.parsing.input.StreamReader
+import java.io.StringReader
class ParseException(reason: String, cause: Throwable) extends Exception(reason, cause) {
def this(reason: String) = this(reason, null)
@@ -64,7 +66,7 @@ object Parser extends RegexParsers {
def id = """[A-Za-z0-9_\.]+""".r
// rawr.
- def apply(in: String): Document = {
+ def apply(in: StreamReader): Document = {
CleanupWhitespace {
parseAll(document, in) match {
case Success(result, _) => result
@@ -73,6 +75,7 @@ object Parser extends RegexParsers {
}
}
}
+ def apply(in: String): Document = apply(StreamReader(new StringReader(in)))
}
/**
View
4 src/main/scala/com/twitter/scrooge/AST.scala
@@ -16,8 +16,10 @@
package com.twitter.scrooge
+import scala.util.parsing.input.Positional
+
object AST {
- sealed abstract class Node
+ sealed abstract class Node extends Positional
sealed abstract class Requiredness extends Node {
def isOptional = this eq Requiredness.Optional
def isRequired = this eq Requiredness.Required
View
42 src/main/scala/com/twitter/scrooge/Main.scala
@@ -90,22 +90,36 @@ object Main {
}
for (inputFile <- thriftFiles) {
- val inputFileDir = new File(inputFile).getParent
- val importer = Importer.fileImporter(inputFileDir :: importPaths.toList)
- val parser = new ScroogeParser(importer)
- val doc0 = parser.parseFile(inputFile).mapNamespaces(namespaceMappings.toMap)
+ try {
+ if (verbose) print("+ Compiling %s".format(inputFile))
@robey
robey added a note Apr 5, 2012

we want to avoid printing this unless the file is going to be compiled.

@sodabrew
sodabrew added a note Apr 5, 2012
@robey
robey added a note Apr 5, 2012

oh, i see, because an exception could happen in the parsing even if we don't plan to rewrite the scala files.

i guess that bug's always been there. :)

@sodabrew
sodabrew added a note Apr 5, 2012

Yes, and caught me unawares a few times already as I was trying to figure out what scrooge was looking at when it bombed out :)

Prepending the filename to exceptions also helps, particularly in non-verbose mode where it's the only output you get on an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- val outputFile = outputFilename map { new File(_) } getOrElse generator.outputFile(destFolder, doc0, inputFile)
- val lastModified = importer.lastModified(inputFile).getOrElse(Long.MaxValue)
- if (!(skipUnchanged && isUnchanged(outputFile, lastModified))) {
- if (verbose) println("+ Compiling %s".format(inputFile))
- val doc1 = TypeResolver().resolve(doc0).document
- val content = generator(doc1, flags.toSet)
+ val inputFileDir = new File(inputFile).getParent
+ val importer = Importer.fileImporter(inputFileDir :: importPaths.toList)
+ val parser = new ScroogeParser(importer)
+ val doc0 = parser.parseFile(inputFile).mapNamespaces(namespaceMappings.toMap)
+ val outputFile = outputFilename map { new File(_) } getOrElse generator.outputFile(destFolder, doc0, inputFile)
+ val lastModified = importer.lastModified(inputFile).getOrElse(Long.MaxValue)
- Option(outputFile.getParentFile).foreach { _.mkdirs() }
- val out = new FileWriter(outputFile)
- out.write(content)
- out.close()
+ if (skipUnchanged && isUnchanged(outputFile, lastModified)) {
+ if (verbose) print(" (unchanged)")
+
+ } else {
+ val doc1 = TypeResolver().resolve(doc0).document
+ val content = generator(doc1, flags.toSet)
+
+ Option(outputFile.getParentFile).foreach { _.mkdirs() }
+ val out = new FileWriter(outputFile)
+ out.write(content)
+ out.close()
+ }
+ } catch {
+ case ex: Exception => {
+ if (verbose) println("")
+ System.err.println(inputFile + ": " + ex)
+ System.exit(1)
+ }
+ } finally {
+ if (verbose) println("")
}
}
}
View
4 src/main/scala/com/twitter/scrooge/MustacheLoader.scala
@@ -20,6 +20,8 @@ import com.twitter.conversions.string._
import com.twitter.handlebar.Handlebar
import scala.collection.mutable.HashMap
import scala.io.Source
+import scala.util.parsing.input.StreamReader
+import java.io.{FileInputStream, InputStreamReader}
class HandlebarLoader(prefix: String, suffix: String = ".scala") {
private val cache = new HashMap[String, Handlebar]
@@ -32,7 +34,7 @@ class HandlebarLoader(prefix: String, suffix: String = ".scala") {
throw new NoSuchElementException("template not found: " + fullName)
}
case inputStream => {
- new Handlebar(Source.fromInputStream(inputStream).getLines().mkString("\n"))
+ new Handlebar(StreamReader(new InputStreamReader(inputStream)))
}
}
)
View
18 src/main/scala/com/twitter/scrooge/ScroogeParser.scala
@@ -19,6 +19,8 @@ package com.twitter.scrooge
import scala.collection.mutable
import scala.util.parsing.combinator._
import scala.util.parsing.combinator.lexical._
+import scala.util.parsing.input.{Positional, StreamReader}
+import java.io.{FileInputStream, InputStreamReader, StringReader}
class ParseException(reason: String, cause: Throwable) extends Exception(reason, cause) {
def this(reason: String) = this(reason, null)
@@ -77,15 +79,15 @@ class ScroogeParser(importer: Importer) extends RegexParsers {
MapConstant(Map(list.map { case k ~ x ~ v => (k, v) }: _*))
}
- def identifier = "[A-Za-z_][A-Za-z0-9\\._]*".r ^^ { x => Identifier(x) }
+ def identifier = positioned("[A-Za-z_][A-Za-z0-9\\._]*".r ^^ { x => Identifier(x) })
// types
- def fieldType: Parser[FieldType] = baseType | containerType | referenceType
+ def fieldType: Parser[FieldType] = positioned(baseType) | positioned(containerType) | positioned(referenceType)
def referenceType = identifier ^^ { x => ReferenceType(x.name) }
- def definitionType = baseType | containerType
+ def definitionType = positioned(baseType) | positioned(containerType)
def baseType: Parser[BaseType] = (
"bool" ^^^ TBool |
@@ -155,7 +157,7 @@ class ScroogeParser(importer: Importer) extends RegexParsers {
// definitions
- def definition = const | typedef | enum | senum | struct | exception | service
+ def definition = positioned(const) | positioned(typedef) | positioned(enum) | positioned(senum) | positioned(struct) | positioned(exception) | positioned(service)
def const = "const" ~> fieldType ~ identifier ~ ("=" ~> constant) ~ opt(listSeparator) ^^ {
case ftype ~ id ~ const ~ _ => Const(id.name, ftype, const)
@@ -222,14 +224,14 @@ class ScroogeParser(importer: Importer) extends RegexParsers {
def namespaceScope = "*" | (identifier ^^ { id => id.name })
// rawr.
-
- def parse[T](in: String, parser: Parser[T]): T = {
+ def parse[T](in: StreamReader, parser: Parser[T]): T = {
parseAll(parser, in) match {
case Success(result, _) => result
- case x @ Failure(msg, z) => throw new ParseException(x.toString)
+ case x @ Failure(msg, _) => throw new ParseException(x.toString)
case x @ Error(msg, _) => throw new ParseException(x.toString)
}
}
+ def parse[T](in: String, parser: Parser[T]): T = parse(StreamReader(new StringReader(in)), parser)
- def parseFile(filename: String) = parse(importer(filename), document)
@robey
robey added a note Apr 9, 2012

do we not use the importer anymore? (i guess not, or the tests would have failed)

@sodabrew
sodabrew added a note Apr 9, 2012

Maybe not! I didn't look carefully to see if I could remove it.

@robey
robey added a note May 4, 2012

oh, we just fixed a bug in this, so we DO use it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ def parseFile(filename: String) = parse(StreamReader(new InputStreamReader(new FileInputStream((filename)))), document)
}
View
120 src/main/scala/com/twitter/scrooge/TypeResolver.scala
@@ -18,10 +18,17 @@ package com.twitter.scrooge
import AST._
import scala.collection.mutable.ArrayBuffer
+import scala.util.DynamicVariable
+import scala.util.parsing.input.{Position, NoPosition}
-class TypeNotFoundException(name: String) extends Exception(name)
-class UndefinedSymbolException(name: String) extends Exception(name)
-class TypeMismatchException(name: String) extends Exception(name)
+class ExceptionAt(name: String)(implicit node: DynamicVariable[Node]) extends Exception(name) {
+ val pos: Position = if (node.value eq null) NoPosition else node.value.pos
+ override def toString: String = super.toString + " at " + pos
+}
+
+class TypeNotFoundException(name: String)(implicit node: DynamicVariable[Node]) extends ExceptionAt(name)
+class UndefinedSymbolException(name: String)(implicit node: DynamicVariable[Node]) extends ExceptionAt(name)
+class TypeMismatchException(name: String)(implicit node: DynamicVariable[Node]) extends ExceptionAt(name)
case class ResolvedDocument(document: Document, resolver: TypeResolver)
case class ResolvedDefinition(definition: Definition, resolver: TypeResolver)
@@ -31,6 +38,7 @@ object TypeResolver {
typeMap: Map[String,T],
includeMap: Map[String, ResolvedDocument],
next: TypeResolver => EntityResolver[T])
+ (implicit node: DynamicVariable[Node])
{
def apply(name: String): T = {
name match {
@@ -75,6 +83,8 @@ case class TypeResolver(
{
import TypeResolver._
+ implicit val dynCurrentNode: DynamicVariable[Node] = new DynamicVariable[Node](null)
+
lazy val fieldTypeResolver: EntityResolver[FieldType] =
new EntityResolver(typeMap, includeMap, _.fieldTypeResolver)
lazy val serviceResolver: EntityResolver[Service] =
@@ -114,7 +124,7 @@ case class TypeResolver(
* typeMap, and then returns an updated TypeResolver with the new
* definition bound, plus the resolved definition.
*/
- def resolve(definition: Definition): ResolvedDefinition = {
+ def resolve(definition: Definition): ResolvedDefinition = dynCurrentNode.withValue(definition) {
@robey
robey added a note Apr 9, 2012

does this mean the error messages will only report on lines with the beginning of a definition? makes it seem like there's no need to attach the position to any other node.

@sodabrew
sodabrew added a note Apr 9, 2012

Good point. I went back and forth a lot attaching position() combinators and looking at where to put a withValue { } enclosure. To get pinpoint type error reporting, there would need to be more withValue { } blocks around many of the apply methods. Open to direction on how detailed to get here.

@robey
robey added a note Apr 10, 2012

i'm ... really not sure. :)

it would be good to have the position information for the actual token causing problems when we throw an exception, but it doesn't seem like that's possible without threading the AST node through the TypeResolver, either explicitly, or by calling withNode in a bunch of places whenever the focused AST node changes.

i'm leaning toward the explicit passing around of node objects, because the withValue() thing is a bit magickal to me -- i had to go look up what that is. but if it can be done with a few well-placed withValue() calls vs changing the argument list to every method in TypeResolver, then that's probably better.

@sodabrew
sodabrew added a note Apr 10, 2012

http://www.scala-lang.org/api/current/scala/util/DynamicVariable.html

Think of it as a stack. Everyone with lexical scope (e.g. is in the same class) to see the variable sees just the topmost value. Calls to withValue{ } push onto the stack going into the block, and pop off the stack when exiting the block. This way, you can have code inside your class that just calls for the current value and doesn't need to worry about the particular code path taken on the way there.

I was thinking about adding a withValue { } block around most of the apply() methods, and would be happy to do that.

@robey
robey added a note Apr 10, 2012

well, let's try that then. :)

@sodabrew
sodabrew added a note Apr 12, 2012

Just pushed some more withValue blocks. Error output is slightly more specific now (was previously reporting line x column 1, now reporting line x column y).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
apply(definition) match {
case d @ Typedef(name, t) => ResolvedDefinition(d, define(name, t))
case e @ Enum(name, _) => ResolvedDefinition(e, define(name, EnumType(e)))
@@ -148,7 +158,7 @@ case class TypeResolver(
copy(serviceMap = serviceMap + (service.name -> service))
}
- def apply(definition: Definition): Definition = {
+ def apply(definition: Definition): Definition = dynCurrentNode.withValue(definition) {
definition match {
case d @ Typedef(name, t) => d.copy(`type` = apply(t))
case s @ Struct(_, fs) => s.copy(fields = fs.map(apply))
@@ -173,57 +183,63 @@ case class TypeResolver(
default = f.default.map { const => apply(const, fieldType) })
}
- def apply(t: FunctionType): FunctionType = t match {
- case Void => Void
- case t: FieldType => apply(t)
+ def apply(t: FunctionType): FunctionType = dynCurrentNode.withValue(t) {
+ t match {
+ case Void => Void
+ case t: FieldType => apply(t)
+ }
}
- def apply(t: FieldType): FieldType = t match {
- case ReferenceType(name) => apply(name)
- case m @ MapType(k, v, _) => m.copy(keyType = apply(k), valueType = apply(v))
- case s @ SetType(e, _) => s.copy(eltType = apply(e))
- case l @ ListType(e, _) => l.copy(eltType = apply(e))
- case _ => t
+ def apply(t: FieldType): FieldType = dynCurrentNode.withValue(t) {
+ t match {
+ case ReferenceType(name) => apply(name)
+ case m @ MapType(k, v, _) => m.copy(keyType = apply(k), valueType = apply(v))
+ case s @ SetType(e, _) => s.copy(eltType = apply(e))
+ case l @ ListType(e, _) => l.copy(eltType = apply(e))
+ case _ => t
+ }
}
- def apply(c: Constant, fieldType: FieldType): Constant = c match {
- case l @ ListConstant(elems) =>
- fieldType match {
- case ListType(eltType, _) => l.copy(elems = elems map { e => apply(e, eltType) } )
- case SetType(eltType, _) => SetConstant(elems map { e => apply(e, eltType) } toSet)
- case _ => throw new TypeMismatchException("Expecting " + fieldType + ", found " + l)
- }
- case m @ MapConstant(elems) =>
- fieldType match {
- case MapType(keyType, valType, _) =>
- m.copy(elems = elems.map { case (k, v) => (apply(k, keyType), apply(v, valType)) })
- case _ => throw new TypeMismatchException("Expecting " + fieldType + ", found " + m)
- }
- case i @ Identifier(name) =>
- fieldType match {
- case EnumType(enum, _) =>
- val valueName = name match {
- case QualifiedName(scope, QualifiedName(enumName, valueName)) =>
- if (fieldTypeResolver(scope, enumName) != fieldType) {
- throw new UndefinedSymbolException(scope + "." + enumName)
- } else {
- valueName
- }
- case QualifiedName(enumName, valueName) =>
- if (enumName != enum.name) {
- throw new UndefinedSymbolException(enumName)
- } else {
- valueName
- }
- case _ => name
- }
- enum.values.find(_.name == valueName) match {
- case None => throw new UndefinedSymbolException(name)
- case Some(value) => EnumValueConstant(enum, value)
- }
- case _ => throw new UndefinedSymbolException(name)
- }
- case _ => c
+ def apply(c: Constant, fieldType: FieldType): Constant = dynCurrentNode.withValue(c) {
+ c match {
+ case l @ ListConstant(elems) =>
+ fieldType match {
+ case ListType(eltType, _) => l.copy(elems = elems map { e => apply(e, eltType) } )
+ case SetType(eltType, _) => SetConstant(elems map { e => apply(e, eltType) } toSet)
+ case _ => throw new TypeMismatchException("Expecting " + fieldType + ", found " + l)
+ }
+ case m @ MapConstant(elems) =>
+ fieldType match {
+ case MapType(keyType, valType, _) =>
+ m.copy(elems = elems.map { case (k, v) => (apply(k, keyType), apply(v, valType)) })
+ case _ => throw new TypeMismatchException("Expecting " + fieldType + ", found " + m)
+ }
+ case i @ Identifier(name) =>
+ fieldType match {
+ case EnumType(enum, _) =>
+ val valueName = name match {
+ case QualifiedName(scope, QualifiedName(enumName, valueName)) =>
+ if (fieldTypeResolver(scope, enumName) != fieldType) {
+ throw new UndefinedSymbolException(scope + "." + enumName)
+ } else {
+ valueName
+ }
+ case QualifiedName(enumName, valueName) =>
+ if (enumName != enum.name) {
+ throw new UndefinedSymbolException(enumName)
+ } else {
+ valueName
+ }
+ case _ => name
+ }
+ enum.values.find(_.name == valueName) match {
+ case None => throw new UndefinedSymbolException(name)
+ case Some(value) => EnumValueConstant(enum, value)
+ }
+ case _ => throw new UndefinedSymbolException(name)
+ }
+ case _ => c
+ }
}
def apply(parent: ServiceParent): ServiceParent = {
View
2 src/test/scala/com/twitter/scrooge/ScroogeParserSpec.scala
@@ -162,7 +162,7 @@ class ScroogeParserSpec extends Specification {
"standard test file" in {
val parser = new ScroogeParser(Importer.resourceImporter(getClass))
- val doc = parser.parseFile("/test.thrift")
+ val doc = parser.parseFile("test.thrift")
// i guess not blowing up is a good first-pass test.
// might be nice to verify parts of it tho.
doc.headers.size mustEqual 13
Something went wrong with that request. Please try again.