Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Scrooge simply bombs out when an invalid type is used #9

Closed
sodabrew opened this Issue Mar 30, 2012 · 6 comments

Comments

Projects
None yet
2 participants

I accidentally used String as a data type in a thrift file, rather the string. Since I was previously compiling to Java, turns out this just passed through transparently and worked as a Java String type! However, when compiling to Scala, scrooge did this helpful thing:

[error] Exception in thread "main" com.twitter.scrooge.TypeNotFoundException: String
[error]     at com.twitter.scrooge.TypeResolver$EntityResolver$$anonfun$apply$1.apply(TypeResolver.scala:40)
[error]     at com.twitter.scrooge.TypeResolver$EntityResolver$$anonfun$apply$1.apply(TypeResolver.scala:40)
[error]     at scala.Option.getOrElse(Option.scala:104)
[error]     at com.twitter.scrooge.TypeResolver$EntityResolver.apply(TypeResolver.scala:40)
[error]     at com.twitter.scrooge.TypeResolver.apply(TypeResolver.scala:232)
[error]     at com.twitter.scrooge.TypeResolver.apply(TypeResolver.scala:182)
[error]     at com.twitter.scrooge.TypeResolver.apply(TypeResolver.scala:170)
[error]     at com.twitter.scrooge.TypeResolver$$anonfun$apply$2.apply(TypeResolver.scala:154)
[error]     at com.twitter.scrooge.TypeResolver$$anonfun$apply$2.apply(TypeResolver.scala:154)
[error]     at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:206)
[error]     at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:206)
[error]     at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:61)
[error]     at scala.collection.immutable.List.foreach(List.scala:45)
[error]     at scala.collection.TraversableLike$class.map(TraversableLike.scala:206)
[error]     at scala.collection.immutable.List.map(List.scala:45)
[error]     at com.twitter.scrooge.TypeResolver.apply(TypeResolver.scala:154)
[error]     at com.twitter.scrooge.TypeResolver.resolve(TypeResolver.scala:118)
[error]     at com.twitter.scrooge.TypeResolver$$anonfun$resolve$2.apply(TypeResolver.scala:96)
[error]     at com.twitter.scrooge.TypeResolver$$anonfun$resolve$2.apply(TypeResolver.scala:95)
[error]     at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:61)
[error]     at scala.collection.immutable.List.foreach(List.scala:45)
[error]     at com.twitter.scrooge.TypeResolver.resolve(TypeResolver.scala:95)
[error]     at com.twitter.scrooge.Main$$anonfun$main$1.apply(Main.scala:92)
[error]     at com.twitter.scrooge.Main$$anonfun$main$1.apply(Main.scala:78)
[error]     at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:61)
[error]     at scala.collection.immutable.List.foreach(List.scala:45)
[error]     at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:44)
[error]     at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:42)
[error]     at com.twitter.scrooge.Main$.main(Main.scala:78)
[error]     at com.twitter.scrooge.Main.main(Main.scala)

Expected: some kind of error message with a line number.

sodabrew commented Apr 2, 2012

I read through the code over the weekend, and the key problem in the type checking phase is that the source code has already been fully converted to an AST at that point. I'm thinking of adding a Pos class to mixin with the Node class in AST.scala. That way, the node that causes a problem can be turned into an error message with a file position.

Collaborator

robey commented Apr 3, 2012

that sounds like a good idea. i think the scala parser-combinator library (which we're using to parse the thrift) has a feature for this, but i haven't looked too deeply into it.

sodabrew commented Apr 3, 2012

Yes, Scala's Parser has a position field, but it's not copied to the nodes in the AST. I did some work on this, but didn't grok the actual point at which the parser creates each node when generating the AST - I was naively looking for something like "TheAST ::= new Thingy()" but didn't see that.

Collaborator

robey commented Apr 3, 2012

the way the parser-combinator library works, each rule "converts" a chunk of text into an AST object, so something like:

def rule: Parser[Node] = regex ~ regex ^^ { case a ~ b => Add(a, b) }

matches 2 regex in sequence, and the code block converts them into an Add object. (you don't need to "new" a case class in scala, you can just call it to get a new instance, like python.)

sodabrew commented Apr 3, 2012

Oh rad. Didn't know that. (I'm about a week into learning Scala.) I'll have to look again, but I also wasn't clear on the scoping of the Parser object at hand. I want it to look something like this:

def rule: Parser[Node] = regex ~ regex ^^ { case a ~ b => Add(a, b).pos(foo.pos) }

I coudn't find where the foo would come from ;) -- pos is Positional from http://www.scala-lang.org/api/current/scala/util/parsing/input/Positional.html which decorates http://www.scala-lang.org/api/current/scala/util/parsing/combinator/Parsers.html

@sodabrew sodabrew added a commit to sodabrew/scrooge that referenced this issue Apr 4, 2012

@sodabrew sodabrew Issue #9 switch parser input from String to StreamReader and begin an…
…notating some AST objects with position. Compiles, works, but unit tests fail due to signature changes.
5bf7f53

@sodabrew sodabrew added a commit to sodabrew/scrooge that referenced this issue Apr 4, 2012

@sodabrew sodabrew Issue #9 annotate type exceptions with position information. a76819a

@sodabrew sodabrew added a commit to sodabrew/scrooge that referenced this issue Apr 4, 2012

@sodabrew sodabrew Issue #9 pretty print exceptions by wrapping in a try/catch, and impr…
…ove verbose output to better identify which file is being processed.
a8c70c5

@sodabrew sodabrew added a commit to sodabrew/scrooge that referenced this issue Apr 4, 2012

@sodabrew sodabrew Issue #9 add adapters for String arguments, which the unit tests use,…
… and are nice to keep handlebar compatible if used elsewhere.
c079a19
Collaborator

robey commented Apr 10, 2012

the work for this is now being done in issue #11.

@robey robey closed this Apr 10, 2012

@sodabrew sodabrew added a commit to sodabrew/scrooge that referenced this issue Apr 12, 2012

@sodabrew sodabrew Issue #9 add more dynamic variable blocks in the type resolver. 94907f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment