Skip to content

Add Expr.asTerm #10694

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 1 commit into from
Dec 8, 2020
Merged

Add Expr.asTerm #10694

merged 1 commit into from
Dec 8, 2020
+195 −190

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Dec 8, 2020 •
edited
Loading

  • Add Expr.asTerm under reflect
  • Deprecate Tree.of and Term.of for simpler migration from M2 to M3

This change make the intent of the operation clearer. We call asTerm to see the expression as a Term just like we call asExpr make the Term and Expr[Any].
It will simplify code that needs to access the trees.

As these are extension methods, they can also be accessed explicitly using reflect.asTerm(expr) or reflect.asTree(expr) which can be useful in some situations.

@nicolasstucki nicolasstucki mentioned this pull request Dec 8, 2020
Closed
@smarter
Copy link
Member

smarter commented Dec 8, 2020

Should we also replace TypeRepr.of and TypeTree.of ?

@nicolasstucki
Copy link
Contributor Author

No, TypeRepr and TypeTree cannot be replaced. Those two are contextual and therefore we cannot call methods directly on them directly.

@nicolasstucki
Copy link
Contributor Author

I meant Type is contextual and those depend on it.

@nicolasstucki nicolasstucki force-pushed the add-Expr-asTerm-and-asTree branch from a9ea7c8 to 53f0bcc Compare December 8, 2020 01:23
@smarter
Copy link
Member

smarter commented Dec 8, 2020

I don't really understand what contextual means here, could you give an example of what would go wrong if we tried defining def asTypeRepr(using Quotes): quotes.reflect.TypeRepr on quoted.Type (either directly or as an extension method) ?

@nicolasstucki
Copy link
Contributor Author

It means we only have the name of the type T bit not a name for the parameter Type[T]. Therefore we cannot call a method on Type[T] directly.

All that is unrelated with the change in this PR.

@nicolasstucki nicolasstucki marked this pull request as ready for review December 8, 2020 07:30
@nicolasstucki
Copy link
Contributor Author

This PR addresses the main usability concern described in #10685 while keeping an intention based naming convention.

liufengyun
liufengyun approved these changes Dec 8, 2020
View reviewed changes
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

To echo the concern raised by @smarter , maybe we should do the same for Type[T] for consistency.

library/src/scala/quoted/Quotes.scala Outdated
/** Returns the `Tree` representation this expression.
* Same as `asTerm` but typed as a `Tree`.
*/
def asTree: Tree
Copy link
Contributor

Choose a reason for hiding this comment

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

An Expr[T] is always a Term, and Term <: Tree, maybe asTerm is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is sufficient. I added this one because it is an intuitive name. Maybe it is better to remove it. Users can always define their extension method that does the same in a couple of lines of they prefer asTree in their use cases. It can also be added back in a future version of the library.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to remove and only keep asTerm. For IDE support, the two versions make no difference, which is different from the case for size and length.

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 remove asTree

@nicolasstucki
Copy link
Contributor Author

We cannot do the same for Type[T]. That one is used in a fundamentally different way in which we do/should not have access to their instances directly named.

@nicolasstucki nicolasstucki linked an issue Dec 8, 2020 that may be closed by this pull request
Term.of and .asExprOf is confusing to new users, please add back seal/unseal #10685
Closed
@nicolasstucki nicolasstucki changed the title Add Expr.asTree and Expr.asTerm Add Expr.asTerm Dec 8, 2020
@nicolasstucki nicolasstucki force-pushed the add-Expr-asTerm-and-asTree branch from 5d46d33 to bbb5209 Compare December 8, 2020 14:19
@nicolasstucki
Add Expr.asTerm
257716e 
* Add `Expr.asTerm` under `reflect`
* Deprecate `Tree.of` and `Term.of` for simpler migration from M2 to M3

This change make the intent of the operation clearer. We call `asTerm` to see the expression as a `Term` just like we call `asExpr` make the `Term` and `Expr[Any]`.
t It will simplify code that needs to acceess the trees.

As these are extension methods, they can also be accessed explicilty using `reflect.asTerm(expr)`  which can be useful in some situations.

Remove asTree
@nicolasstucki nicolasstucki force-pushed the add-Expr-asTerm-and-asTree branch from bbb5209 to 257716e Compare December 8, 2020 16:20
@nicolasstucki nicolasstucki merged commit 7adb69d into scala:master Dec 8, 2020
@nicolasstucki nicolasstucki deleted the add-Expr-asTerm-and-asTree branch December 8, 2020 19:25
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liufengyun liufengyun liufengyun approved these changes

Assignees

@liufengyun liufengyun

Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

Term.of and .asExprOf is confusing to new users, please add back seal/unseal
4 participants
@nicolasstucki @smarter @liufengyun @Kordyjan