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.

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

Allow nested Quotes with a different owners #13652

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Allow nested Quotes with a different owners #13652

merged 1 commit into from
Mar 23, 2022

Conversation

nicolasstucki
Copy link
Contributor

This makes it possible to create Exprs with different owners to avoid a call to changeOwner.

See #13571

@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch 2 times, most recently from 28ea733 to ae85c48 Compare October 11, 2021 12:47
@nicolasstucki nicolasstucki marked this pull request as ready for review October 11, 2021 15:05
@nicolasstucki nicolasstucki self-assigned this Oct 11, 2021
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from ae85c48 to bf5b2b5 Compare October 12, 2021 08:16
@nicolasstucki nicolasstucki requested a review from smarter October 12, 2021 08:17
smarter
smarter reviewed Oct 12, 2021
View reviewed changes
library/src/scala/quoted/Quotes.scala
* override def transformTerm(tree: Term)(owner: Symbol): Term =
* tree match
* case ... =>
* given Quotes = owner.asQuotes
Copy link
Member

Choose a reason for hiding this comment

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

This still looks easy too easy to miss and hard to teach to me, is there no way to automatically use the correct Quotes?

Copy link
Member

Choose a reason for hiding this comment

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

(also if we do go with this solution, it would need to be explained in details in the documentation)

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 still looks easy too easy to miss and hard to teach to me, is there no way to automatically use the correct Quotes?

We can do this only if we use the quotes API alone. We could not find a good way to do this when reflection is involved. That is one of the reasons why we introduced the changeOwner. This is an alternative to changeOwner that does not require post-processing. Both of these need be explained in detail in the documentation and both require the same understanding of the API.

Copy link
Member

Choose a reason for hiding this comment

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

We can do this only if we use the quotes API alone.

What about the discussion in #12309 (comment) ? /cc @odersky, @LPTK

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 adds a way to avoid the costly change of owners mentioned in #12309 (comment). It provides a way to construct the quotes under the correct owner which means that when we will only need to check that the owner is correct and not change it. Currently, if we create a quote that goes into a reflection constructor we always end up taking the slow path and need to change the owners.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, if we create a quote that goes into a reflection constructor we always end up taking the slow path and need to change the owners.

But presumably this isn't always working if #13571 emits an error?

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 #13571 -Xcheck-macros detects the bug in the user code. There we show how they can fix the owner after the fact. This addition allows the users to set the correct owner before the creation of the quote and avoid having to do the extra work to fix the owner.

@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from bf5b2b5 to 50d5fc3 Compare October 12, 2021 09:56
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from 50d5fc3 to 6a81479 Compare November 3, 2021 15:50
@nicolasstucki nicolasstucki mentioned this pull request Nov 11, 2021
Closed
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from 6a81479 to 8015e82 Compare November 11, 2021 08:59
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch 2 times, most recently from 84ca549 to ee74a94 Compare November 19, 2021 13:29
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from ee74a94 to 2610b91 Compare December 15, 2021 15:20
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch 2 times, most recently from 59c27fb to a108104 Compare January 4, 2022 08:53
@nicolasstucki nicolasstucki added this to the 3.2.0 milestone Jan 13, 2022
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from a108104 to 663d77b Compare January 13, 2022 08:19
smarter
smarter approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Should the documentation of TreeMap and/or the general meta-programming doc point to this method? Otherwise it will be hard for users to discover.

@smarter smarter assigned nicolasstucki and unassigned smarter Mar 7, 2022
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch 3 times, most recently from 25e41b9 to 1c6d96d Compare March 18, 2022 08:07
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch 2 times, most recently from 8ea64e1 to ad7f2d2 Compare March 21, 2022 11:04
@nicolasstucki
Copy link
Contributor Author

Updated documentation

@nicolasstucki
Copy link
Contributor Author

@smarter could you have a quick look before merging?

smarter
smarter approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@smarter smarter 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!

library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala
* }
* }
* ```
* @syntax markdown
Copy link
Member

Choose a reason for hiding this comment

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

I don't think @syntax markdown is required in the library doc anymore, it should be the default.

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 it in a separate PR. There is a total of 27 of these in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up in #14759

library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
@smarter smarter assigned nicolasstucki and unassigned smarter Mar 21, 2022
@nicolasstucki
Allow nested Quotes with a different owners
e0989a0 
This makes it possible to create `Expr`s with different owners to avoid
a call to `changeOwner`.

Closes scala#13922
@nicolasstucki nicolasstucki force-pushed the add-quotes-change-owner branch from ad7f2d2 to e0989a0 Compare March 22, 2022 15:22
@nicolasstucki nicolasstucki merged commit f88d996 into scala:main Mar 23, 2022
@nicolasstucki nicolasstucki deleted the add-quotes-change-owner branch March 23, 2022 12:33
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Mar 23, 2022
@nicolasstucki
Remove @syntax markdown from Quotes
89b3074 
See scala#13652 (comment)
@nicolasstucki nicolasstucki mentioned this pull request Mar 23, 2022
Merged
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.1.3 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

Assignees

@nicolasstucki nicolasstucki

Labels
None yet
Projects
None yet
Milestone
3.1.3
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolasstucki @smarter @Kordyjan