New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tree-based code generation #3321

Merged
merged 8 commits into from Jan 15, 2014

Conversation

Projects
None yet
5 participants
@VladimirNik
Contributor

VladimirNik commented Dec 31, 2013

New printer (ParsedTreePrinter) is intended for unattributed trees and generates the code based on the passed AST.
Result code can be later compiled by scalac retaining the same meaning.
ParsedTreePrinter is based on Sprinter project.

VladimirNik added some commits Dec 31, 2013

Variance annotations printing
def printTypeParams is modified. Tests are updated.
val showOuterTests is removed
val showOuterTests wasn't used in Printers

@ghost ghost assigned retronym Jan 1, 2014

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 1, 2014

Yay! Happy New Year!

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 1, 2014

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 1, 2014

Great job! It's very cool to see this kind of effort, and I'm looking forward to having toCode in trunk! Review @retronym /cc @densh

@retronym

This comment has been minimized.

Member

retronym commented Jan 1, 2014

Similar functionality exists over in scala-refactoring:

https://github.com/scala-ide/scala-refactoring/blob/master/org.scala-refactoring.library/src/main/scala/scala/tools/refactoring/sourcegen/PrettyPrinter.scala

Could you compare please make sure that your test suite covers at least the cases in PrettyPrinterTest ?

/cc @misto

@VladimirNik

This comment has been minimized.

Contributor

VladimirNik commented Jan 3, 2014

@retronym Yes, as far as I know tree printing in scala-refactoring is targeted on typed trees produced by presentation compiler. Our target in Sprinter was to generate semantically equivalent code using mainly tree structure (we used only dependency on scala.reflect for our main printer), that's why there's a lot of desugaring in result code and we plan to improve it in the future.

I added to my test suite new tests with some code snippets from scala-refactoring tests. Also to test new ParsedTreePrinter I used scalaz (scalaz sources were regenerated using printPlugin, recompiled, and then all project's tests were passed successfully).

VladimirNik added some commits Jan 5, 2014

toCode is added to Printers
1. Printers API is updated (def toCode)
2. ParsedTreePrinter is added to internal.Printers. ParsedTreePrinter generates the code for passed unattributed trees.
Generated code can be later compiled by scalac retaining the same meaning.
@VladimirNik

This comment has been minimized.

Contributor

VladimirNik commented Jan 5, 2014

Version with fixes from comments is added. Commits are updated.

case anns => anns
}
annots foreach (annot => print("@"+annot+" "))
annots foreach (annot => print("@" + annot + " "))

This comment has been minimized.

@densh

densh Jan 6, 2014

Contributor

s"@$annot " ?

This comment has been minimized.

@retronym

retronym Jan 6, 2014

Member

More to the point, does annotation.toString always produce re-typecheckable code? What about back-ticks for special characters? How are the arguments to the annotation printed?

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

@densh Code fixed
@retronym It's a version of printAnnotations method for TreePrinter. ParsedTreePrinter overrides this method and process back-ticks for special characters. In the case of TreePrinter arguments to the annotation are printed as a part of tree because toString results in invocation of TreePrinter's printTree(annot).

@@ -459,11 +525,496 @@ trait Printers extends api.Printers { self: SymbolTable =>
out.print(if (arg == null) "null" else arg.toString)
}
}
//printer for trees after parser and before typer phases

This comment has been minimized.

@densh

densh Jan 6, 2014

Contributor

Maybe one more space after // and before beginning of the comment?

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

Fixed

@xeno-by

This comment has been minimized.

You wanted to say "TODO: ..."? Some clarifications of the future plans would be of help here.

This comment has been minimized.

Owner

VladimirNik replied Jan 7, 2014

Fixed

printBlock(rhs)
}
case imp @ Import(expr, selectors) =>

This comment has been minimized.

@densh

densh Jan 6, 2014

Contributor

selectors not used? use _ instead?

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

Fixed

@@ -104,9 +102,14 @@ trait Printers extends api.Printers { self: SymbolTable =>
def printRow(ts: List[Tree], sep: String) { printRow(ts, "", sep, "") }
def printTypeParams(ts: List[TypeDef]) {

This comment has been minimized.

@densh

densh Jan 6, 2014

Contributor

Procedure syntax is planned to be deprecated in the future, use def f: Unit = ... instead.

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

Fixed

@densh

This comment has been minimized.

Contributor

densh commented Jan 6, 2014

I see a lot of promise in this work. There are a few low hanging fruits that could be improved by using Syntactic extractors but it would be extremely easy to add them as follow-ups later on.

case th @ This(qual) =>
printThis(th, printedName(qual))
case Select(qual @ New(tpe), name) =>

This comment has been minimized.

@densh

densh Jan 6, 2014

Contributor

tpe not used, maybe qual: New subpattern instead?

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

Fixed

}
protected def printAnnotated(tree: Annotated)(printArg: => Unit) {
val Annotated(Apply(Select(New(tpt), nme.CONSTRUCTOR), args), atree) = tree

This comment has been minimized.

@retronym

retronym Jan 6, 2014

Member

Does this handle annotations with multiple parameter lists?

This comment has been minimized.

@VladimirNik

VladimirNik Jan 7, 2014

Contributor

I resolved this issue, new version handles Annotated trees that contain annotations with multiple parameter lists.

VladimirNik added some commits Jan 6, 2014

Annotated trees processing is modified
1. Problem with multiple parameter lists in annotations is resolved
2. Tests for Annotated trees are added
@VladimirNik

This comment has been minimized.

Contributor

VladimirNik commented Jan 7, 2014

@xeno-by @densh @retronym Thanks for the comments! Fixed version is committed.

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 12, 2014

@VladimirNik You know, I've been thinking now, and how about we turn Tree.toCode into Universe.showCode in order to be consistent with show and showRaw (https://github.com/xeno-by/scala/blob/master/src/reflect/scala/reflect/api/Printers.scala#L196)?

@VladimirNik

This comment has been minimized.

Contributor

VladimirNik commented Jan 12, 2014

@xeno-by I agree, the only thing - before we had toCode in Printers (and could access it from Universe, not Tree). In my last commit toCode(tree: Tree) is changed to showCode(tree: Tree)

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jan 14, 2014

awesome! could one of the reviewers sign off with an LGTM? (remember, it has to be the first word of your comment in order to count)

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 15, 2014

LGTM

@xeno-by

This comment has been minimized.

Member

xeno-by commented Jan 15, 2014

Shall we? :)

retronym added a commit that referenced this pull request Jan 15, 2014

Merge pull request #3321 from VladimirNik/sprinter
Add tree-based code generation

@retronym retronym merged commit 07e823a into scala:master Jan 15, 2014

1 check passed

default pr-scala Took 53 min.
Details
@densh

This comment has been minimized.

Contributor

densh commented Jan 15, 2014

🍰 to the @VladimirNik, keep up the good stuff.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jan 16, 2014

Cool stuff indeed, but it breaks on windows unfortunately: https://scala-webapps.epfl.ch/jenkins/job/scala-nightly-windows/1945/console

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jan 16, 2014

@adriaanm

This comment has been minimized.

Member

adriaanm commented Jan 16, 2014

Since we unfortunately don't archive test output on nightlies, I quickly went ahead and capture the relevant log file: https://gist.github.com/adriaanm/8449000

It looks like the failures are related to the windowsy EOL

@VladimirNik

This comment has been minimized.

Contributor

VladimirNik commented Jan 17, 2014

@adriaanm Thank you for log file. Yes, these problems are caused by windows EOL character. Newline processing and test suite are fixed in #3377.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment