Skip to content

Implement Expr.{run|show} #3685

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jan 13, 2018
Merged

Implement Expr.{run|show} #3685

merged 14 commits into from
Jan 13, 2018

Conversation

nicolasstucki
Copy link
Contributor

  • Create implicit Runner for run and show
  • Implement quote compiler for run
  • Implement quote decompiler for show
  • Add -with-compiler flag for dotr/repl to setup the compiler dependencies in the classpath

Based on #3662

@nicolasstucki nicolasstucki force-pushed the implemet-quote-run branch 2 times, most recently from ce79e35 to 69efd50 Compare December 21, 2017 14:26
@nicolasstucki nicolasstucki added this to the 0.6 Tech Preview milestone Dec 21, 2017
@nicolasstucki nicolasstucki mentioned this pull request Dec 22, 2017
Closed
@nicolasstucki nicolasstucki force-pushed the implemet-quote-run branch 2 times, most recently from 31b54f4 to 73a6816 Compare December 23, 2017 09:35
@nicolasstucki nicolasstucki mentioned this pull request Dec 23, 2017
Merged
@nicolasstucki
Copy link
Contributor Author

Rebased

@nicolasstucki nicolasstucki force-pushed the implemet-quote-run branch 3 times, most recently from 86c58ff to 6b5dda5 Compare January 8, 2018 16:49
nicolasstucki
nicolasstucki commented Jan 8, 2018
View reviewed changes
compiler/test/dotty/tools/dotc/FromTastyTests.scala Outdated
@@ -89,6 +89,7 @@ class FromTastyTests extends ParallelTesting {
"t493.scala",
"t8395.scala",
"t3613.scala",
"quote-no-splices.scala",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this issue in another PR, it is unrelated to theExpr.{run|show} implementation.

@nicolasstucki nicolasstucki requested a review from odersky January 8, 2018 16:56
@nicolasstucki
Copy link
Contributor Author

Some improvements on the format of the output of Expr.show will come with PR #3691.

@nicolasstucki
Copy link
Contributor Author

Test performance please

@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Jan 8, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Jan 8, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3685/ to see the changes.

Benchmarks is based on merging with master (39e466d)

odersky
odersky approved these changes Jan 12, 2018
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM. Nice work!

compiler/src/dotty/tools/dotc/quoted/ExprCompiler.scala
/** Compiler that takes the contents of a quoted expression `expr` and produces
* a class file with `class ' { def apply: Object = expr }`.
*/
class ExprCompiler(directory: VirtualDirectory) extends Compiler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are all small files, should ExprRun and ExprFrontEnd be made part of ExprCompiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Done.

compiler/src/dotty/tools/dotc/transform/PostTyper.scala Outdated
@@ -203,7 +203,9 @@ class PostTyper extends MacroTransform with IdentityDenotTransformer { thisPhase
// might be a type constructor.
Checking.checkInstantiable(tree.tpe, nu.pos)
withNoCheckNews(nu :: Nil)(super.transform(tree))
case _ =>
case meth =>
if (meth.symbol.name.eq(nme.QUOTE) && meth.symbol.owner.eq(defn.OpsPackageClass))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just meth.symbol.isQuote should do. But I don't see why this is needed. Does handleMeta in the Select case earlier not catch this? Or maybe we need a handleMeta for Idents as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is half of the fix in #3777. I will explore the different solutions in that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleMeta seems to be the simplest solution. I will merge #3777 into this PR.

@nicolasstucki nicolasstucki mentioned this pull request Jan 12, 2018
Closed
@nicolasstucki
Copy link
Contributor Author

Needs review for the last commits.

@nicolasstucki
Copy link
Contributor Author

test performance please

1 similar comment
@nicolasstucki
Copy link
Contributor Author

test performance please

@liufengyun
Copy link
Contributor

test performance please

(The machine powered off for unknown reasons, maybe due to electricity problem)

@dottybot
Copy link
Member

performance test scheduled: 6 job(s) in queue, 1 running.

@nicolasstucki nicolasstucki force-pushed the implemet-quote-run branch 2 times, most recently from 2065a38 to 7aeb306 Compare January 12, 2018 15:52
@nicolasstucki
Copy link
Contributor Author

Needs review for the last 4 commits.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3685/ to see the changes.

Benchmarks is based on merging with master (8817f4c)

odersky
odersky approved these changes Jan 12, 2018
View reviewed changes
compiler/src/dotty/tools/dotc/CompilationUnit.scala
unit1
}

/** Force the tree to be loaded */
private object force extends TreeTraverser {
def traverse(tree: Tree)(implicit ctx: Context): Unit = traverseChildren(tree)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the change here is necessary. forceTrees is true only in the IDE,, right? But in the IDE we never get to ReifyQuotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, force tree is also used when compiling from TASTY.
This covers the case when we recompile from TASTY a file with a quote and no splices.

@nicolasstucki nicolasstucki mentioned this pull request Jan 13, 2018
Merged
@odersky odersky merged commit d7deabf into scala:master Jan 13, 2018
@allanrenucci allanrenucci deleted the implemet-quote-run branch January 13, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
0.6 Tech Preview
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolasstucki @dottybot @liufengyun @odersky