Skip to content

Contextualize owners in reflection API #10394

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
Closed
wants to merge 1 commit into from
Closed

Contextualize owners in reflection API #10394

wants to merge 1 commit into from

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Nov 19, 2020 •
edited
Loading

  • Introduce an Owner abstraction
  • Replace Tree.changeOwner with contextualized Tree.adaptOwner
  • Contextualize owner in Lambda.apply
  • Contextualize owner in Term.etaExpand
  • Contextualize owner in Symbol.{newVal, newMethod, newBind}
  • Recontextualize TreeMap, TreeAccumulator, TreeTaverser
  • Fix owner in ValDef.let

@nicolasstucki nicolasstucki force-pushed the contextual-owners branch 3 times, most recently from 2e48405 to ee64418 Compare November 19, 2020 15:49
@nicolasstucki nicolasstucki mentioned this pull request Nov 19, 2020
Closed
@nicolasstucki nicolasstucki self-assigned this Nov 19, 2020
liufengyun
liufengyun reviewed Nov 19, 2020
View reviewed changes
compiler/src/scala/quoted/internal/impl/QuoteContextImpl.scala
@@ -122,7 +122,7 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
end extension

extension [ThisTree <: Tree](self: ThisTree):
def changeOwner(newOwner: Symbol): ThisTree =
def adaptOwner(using newOwner: Symbol): ThisTree =
tpd.TreeOps(self).changeNonLocalOwners(newOwner).asInstanceOf[ThisTree]
Copy link
Contributor

Choose a reason for hiding this comment

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

When a meta-programmer calls this method explicitly, it seems better to use explicit arguments.

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 can try to handle them explicitly, but this would complicate quite a few abstractions. Mainly TreeTraverser, TreeMap, TreeAccumulator and ValDef.let

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it might be better to change using newOwner: Symbol to newOwner: Symbol. The reason is about consistency: meta-programmers are already calling the method explicitly, it does not make sense to make its only important argument implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one parameter that can be passed here which is the current owner. Which is itself looking for this implicit and returning it. This has the same implicitness but requires the user to write the same parameter on every use site.

compiler/src/scala/quoted/internal/impl/QuoteContextImpl.scala
dotc.core.Symbols.newSymbol(owner, name.toTermName, flags, tpe, privateWithin)
def newBind(owner: Symbol, name: String, flags: Flags, tpe: TypeRepr): Symbol =
def newBind(name: String, flags: Flags, tpe: TypeRepr)(using owner: Owner): Symbol =
dotc.core.Symbols.newSymbol(owner, name.toTermName, flags | Case, tpe)
def noSymbol: Symbol = dotc.core.Symbols.NoSymbol
Copy link
Contributor

Choose a reason for hiding this comment

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

Since creating a symbol is untrivial, it might be better to be explicit about the owner instead of being implicit. This way, they have a consistent model about how symbols work, the invariants, how to debug and fix owner-related problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The early owner check has proven to make simplify any bugs in the owners at the point where it is mishandled.

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 part of the ergonomics of the API is a simple and consistent programming model. As we cannot completely hide the technical detail of owners and the owner-invariant is error-prone, it might be better to make them explicit:

  • meta-programmers will learn a consistent model for symbols, where each symbol has an owner.
  • they will be more careful when using the APIs. If they mess up with owners, they will not blame the macro system. If the argument is implicit, part of the responsibility of the owner-errors is due to the macro system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I will try to make a version where we handle owners explicitly. I believe this will not be as simple to handle.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I added a version of the ValDef.let fix in #10402

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added #10406 with a version where owners are handled explicitly

compiler/src/scala/quoted/internal/impl/QuoteContextImpl.scala
@@ -2200,22 +2201,35 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
case _ => None
end AmbiguousImplicitsTypeTest

type Owner = dotc.core.Symbols.Symbol
Copy link
Contributor

Choose a reason for hiding this comment

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

I still find this introduces unnecessary conceptual complexity to meta-programmers. The programming model with explicit owners seems to be better and simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want it explicit we would also need to use it explicitly in the TreeTraverser, TreeMap, TreeAccumulator and ValDef.let. We would also need to remove Symbol.currentOwner. That would be quite annoying.

Note that the current design already has a contextual owner mechanism but it is hidden behind the Context abstraction. No one has understood how to propagate this correctly so far. As this PR shows even I missed the connection between the two concepts in some cases. By making it clear what we actually propagate contextually it was trivially obvious where it was missing.

@nicolasstucki nicolasstucki force-pushed the contextual-owners branch 2 times, most recently from 09250fd to 86b9e37 Compare November 20, 2020 08:22
@nicolasstucki nicolasstucki mentioned this pull request Nov 20, 2020
Merged
@nicolasstucki
Contextualize owners in reflection API
80828c5 
* Introduce an `Owner` abstraction
* Replace `Tree.changeOwner` with contextualized `Tree.adaptOwner`
* Contextualize owner in `Lambda.apply`
* Contextualize owner in `Term.etaExpand`
* Contextualize owner in `Symbol.{newVal, newMethod, newBind}`
* Recontextualize `TreeMap`, `TreeAccumulator`, `TreeTaverser`
* Fix owner in `ValDef.let`
@nicolasstucki
Copy link
Contributor Author

Replaced by #10406

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

@liufengyun liufengyun liufengyun left review comments

Assignees

@nicolasstucki nicolasstucki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicolasstucki @liufengyun