Skip to content
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

Move reflection inside QuoteContext #6723

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Jun 21, 2019

Homogenize API used for macros and runtime staging. Support QuoteContext instead of Reflection as the implicit that macros take as parameter. We still a keep deprecated support for Reflection in macros. Note that the macros in scalatest, sourcecode and xml-interpolator did not need to change, they only get the warning suggesting to use QuoteContext.

@nicolasstucki nicolasstucki self-assigned this Jun 21, 2019
@nicolasstucki nicolasstucki mentioned this pull request Jun 21, 2019
16 tasks
@nicolasstucki nicolasstucki force-pushed the move-reflection-inside-quote-context branch 2 times, most recently from def3c4e to 88b0607 Compare June 21, 2019 13:53
@nicolasstucki nicolasstucki mentioned this pull request Jun 23, 2019
@nicolasstucki nicolasstucki force-pushed the move-reflection-inside-quote-context branch 5 times, most recently from c0d7981 to b84d9b8 Compare June 28, 2019 09:00
@nicolasstucki nicolasstucki force-pushed the move-reflection-inside-quote-context branch 3 times, most recently from 9af660c to 9ec88d7 Compare June 28, 2019 10:23
@nicolasstucki nicolasstucki marked this pull request as ready for review June 28, 2019 10:55
@nicolasstucki nicolasstucki force-pushed the move-reflection-inside-quote-context branch from 9ec88d7 to 34cfb6a Compare June 30, 2019 08:51
@nicolasstucki nicolasstucki added this to the 0.17 Tech Preview milestone Jun 30, 2019
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

There are many places where we change import refl._ to import qctx.tasty._, which is a small regression in usability at first glance. This change is only justified by the hypothesis that most macros will be quote macros (which does not need the import), reflection macros are less common.

It would be good to have more evidence to support the hypothesis in real-world projects before deprecating the old syntax for reflection macros.

docs/docs/reference/metaprogramming/tasty-inspect.md Outdated Show resolved Hide resolved
@@ -2,16 +2,26 @@ package scala.quoted

import scala.quoted.show.SyntaxHighlight

class QuoteContext(reflection: tasty.Reflection) {
class QuoteContext(val tasty: scala.tasty.Reflection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick : I'm not sure if tasty is a good name: (1) as a meta-programmer, I don't care about whether tasty or not, but only if the APIs are stable, useful and friendly; and it's easy to debug compiler crashes when using reflect API; (2) Reflection is more like an interface to compiler, reflect might be a better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case tasty should be read as the Typed AST API. I will add some documentation for it.

We cannot use reflect as it conflicts with the scala.reflect package and also the expectation of the users that know about scala.reflect. Calling scala.tasty.Reflection has already proven to lead the confusion and we should probably also rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least in scalatest it does not affect the code much, we can even remove one import per file.

@nicolasstucki nicolasstucki force-pushed the move-reflection-inside-quote-context branch from 34cfb6a to 031110b Compare July 1, 2019 07:45
@nicolasstucki
Copy link
Contributor Author

Note that using import qctx.tasty._ instead of import refl._ is a necessary consequence of having a division of concerns between QuoteContext and Reflect. The latter contains the APIs related with TASTy trees while the first contains the APIs needed specifically for quotes and splices.

@nicolasstucki nicolasstucki merged commit e2f4070 into scala:master Jul 1, 2019
@liufengyun liufengyun deleted the move-reflection-inside-quote-context branch July 9, 2019 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants