Add transparent methods #4622

odersky · 2018-06-05T16:01:31Z

0th step, building on #4620: Add transparent methods.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

odersky · 2018-06-05T18:13:21Z

@gsps I think this is the right basis to build on for step 1. Can you review this with the aim to merge early and then we add the TypeOf abstractions on top?

gsps · 2018-06-05T19:31:50Z

Looking through this PR I noticed lots of unrelated commits and finally saw that there's another PR that includes the prefix up to "Addressing reviewer comments". I guess I'll only do my review from that point on.

odersky · 2018-06-05T19:41:03Z

@gsps Yes, everything after #4620.

gsps · 2018-06-05T19:38:03Z

compiler/src/dotty/tools/dotc/core/Flags.scala

@@ -363,7 +363,10 @@ object Flags {
  /** Symbol is a Java default method */
  final val DefaultMethod = termFlag(38, "<defaultmethod>")

-  /** Symbol is a Java enum */
+  /** Labelled with `transparent` modifier */
+  final val Transparent = termFlag(39, "transparent")


Why not make transparent a commonFlag, if we use it that way below anyways?

Since there are no transparent types, it should not apply to them.

gsps · 2018-06-05T19:39:56Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -779,7 +779,13 @@ object SymDenotations {

    def isSkolem: Boolean = name == nme.SKOLEM

-    def isInlineMethod(implicit ctx: Context): Boolean = is(InlineMethod, butNot = Accessor)
+    def isInlinedMethod(implicit ctx: Context): Boolean =


I think the method's and the flag's name should match (so probably isInlineMethod, i.e. minus the d).

I was thinking if we keep something like inline as a modifier we should rename it to inlined instead. Then we don't have a clash with the @inline annotation, and it's a bit cleaner. transparent and inlined are both adjectives, whereas it's a stretch to call inline an adjective.

gsps · 2018-06-05T19:50:53Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

@@ -90,6 +90,11 @@ object Inliner {
    }
  }

+  def isLocal(sym: Symbol, inlineMethod: Symbol)(implicit ctx: Context) =


~~This doesn't seem to be used anywhere.~~ (The following commit does use it.)

gsps · 2018-06-05T20:07:02Z

compiler/src/dotty/tools/dotc/typer/Namer.scala

@@ -1113,7 +1113,7 @@ class Namer { typer: Typer =>

      // println(s"final inherited for $sym: ${inherited.toString}") !!!
      // println(s"owner = ${sym.owner}, decls = ${sym.owner.info.decls.show}")
-      def isInline = sym.is(FinalOrInline, butNot = Method | Mutable)
+      def isInline = sym.is(FinalOrInlineOrTransparent, butNot = Method | Mutable)


Not sure if it's worth adding Transparent to the exception here, since the handling of return types of transparent methods in widenRhs will have to be more different anyways (it should be allowed to infer a TermRef, for instance; think transparent def unapply(x: Foo) = x)

Yes, that was only done as a temporary measure because I could not decide yet whether Java final constants should be called inline(d) or transparent. Probably transparent, but for now it's inline.

gsps · 2018-06-05T20:19:15Z

compiler/src/dotty/tools/dotc/typer/Namer.scala

@@ -1128,7 +1128,8 @@ class Namer { typer: Typer =>
      // it would be erased to BoxedUnit.
      def dealiasIfUnit(tp: Type) = if (tp.isRef(defn.UnitClass)) defn.UnitType else tp

-      val rhsCtx = ctx.addMode(Mode.InferringReturnType)
+      var rhsCtx = ctx.addMode(Mode.InferringReturnType)
+      if (sym.isTransparentMethod) rhsCtx = rhsCtx.addMode(Mode.TransparentBody)


I'm not sure TransparentBody should be a Mode. If it is, shouldn't it also be set in the other tree traversing helpers besides typer, i.e., TreeAccumulator and MegaPhase#transformTree? All of those have localContext helpers (in case of typer its in NamerContextOps, the other two are local defs) which currently set the owner, among other things.

As an alternative, we could deduce whether we are in a transparent context by walking the context's owner chain and checking for Transparent. Ideally that query would be cached by the context (and its descendants).

Good point. Since it's a single bit we cache, it could still go into the Mode structure (context fields are a precious resource). But it could be maintained fully automatically. I'll do that change.

gsps · 2018-06-05T20:36:54Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

@@ -104,14 +94,14 @@ object Inliner {
   *                     to have the inlined method as owner.
   */
  def registerInlineInfo(
-      sym: SymDenotation, treeExpr: Context => Tree)(implicit ctx: Context): Unit = {
-    sym.unforcedAnnotation(defn.BodyAnnot) match {
+      inlined: Symbol, treeExpr: Context => Tree)(implicit ctx: Context): Unit = {


Docs should be updated. Maybe call it inlineSym, since it's not inlined at this point?

gsps · 2018-06-05T20:40:20Z

tests/run/typelevel.scala

@@ -0,0 +1,31 @@
+object Test extends App {


Maybe give this file and its '.check' companion a more descriptive name like transparent.scala or TransparentToNat.scala?

I planned to extend this test with more typelevel operations as we go along.

gsps · 2018-06-05T20:47:03Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala

-          super.transform(accessorIfNeeded(tree))
-      }
-
+      override def transform(tree: Tree)(implicit ctx: Context): Tree =


(I have no useful comment, but I will say that I don't understand Inliner well enough yet to really judge this change.)

odersky · 2018-06-06T06:48:23Z

@gsps Regarding the next step, I was wondering whether TypeOf(tree, underlying) can be modelled as as an annotated type underlying @TypeOf(tree) or AnnotatedType(underyling, tpd.Apply(tpd.ref(defn.ToTypeAnnot), tree)) in internal representation? This would avoid having to add a new case to all typemaps etc.

The downside is that currently annotations on types are forgotten in a large number of cases, which would not be correct in this case. On the other hand, there are a number of other possible annotations that conceptually should behave the same as TypeOf, i.e. they are generally seen as
unknown subtypes of their underlying types. @olhotak worked on mutable/immutable classifiers that would fall into that category. Or units of measure. So maybe it's worthwhile to create a new trait of, say, SubTypeAnnotation which annotation classes can inherit. Types annotated with annotations deriving from SubTypeAnnotation would keep their annotation in most contexts, and they would be treated as subtypes of their underlying type. We'd have to go with a fine comb through all the occurrences of stripAnnotation to make this work.

Note: This would not affect surface syntax, where we'd still keep { tree }.

OlivierBlanvillain · 2018-06-06T09:00:31Z

@odersky Regarding your last comment, I think it would be more work to change the handling annotated type rather than simply adding a new type. It should maybe be done as a separate generalization step? I feel than TypeOf is going to have a large impact on typer than other potential annotations like mutable/immutable...

odersky · 2018-06-06T11:56:54Z

@OlivierBlanvillain I agree it would be easier to get started with a new form of type. But in the end, we will want to refactor to avoid the duplication.

Currently we have both inline and transparent. It is expected that we will drop inline in the future.

- avoid computing owner of NoSymbol

This was the enclosing class instead of the method containing the annotation. Fixing this is surprisingly hard.

- premature optimization, we can just inspect the owner chain, - if we need it, it should be an automatically updated cache

odersky · 2018-06-06T12:15:10Z

#4620 is in now. Rebased on top of current master.

odersky · 2018-06-06T12:19:46Z

I believe if we can do it, it would be ultimately better to base this on AnnotatedType. The reason is that any work we do along that path benefits the whole. The potential downside is that it might detract us from the immediate purpose. I'll take a look how hard it would be to change annotations.

OlivierBlanvillain

LGTM!

dottybot reviewed Jun 5, 2018

View reviewed changes

odersky requested review from gsps and OlivierBlanvillain June 5, 2018 18:13

odersky assigned gsps Jun 5, 2018

gsps suggested changes Jun 5, 2018

View reviewed changes

odersky mentioned this pull request Jun 6, 2018

Add transparent methods - untyped trees version #4616

Merged

odersky force-pushed the add-transparent-2 branch from 405ace0 to 2cd1590 Compare June 6, 2018 08:26

odersky force-pushed the add-transparent-2 branch from 2cd1590 to 101fe52 Compare June 6, 2018 09:14

odersky added 7 commits June 6, 2018 14:13

Add transparent modifier

4c4b403

Currently we have both inline and transparent. It is expected that we will drop inline in the future.

Fix "supercalls not allowed in inlined code" error logic

6331cd8

Some polishings in RefinedPrinter

2afb505

- avoid computing owner of NoSymbol

Improve error message

41c19c3

More polishings and tests

62b97b8

Fix wrong owner when reading annotations.

97f2eec

This was the enclosing class instead of the method containing the annotation. Fixing this is surprisingly hard.

Drop BodyOfTransparent mode bit

b359c7d

- premature optimization, we can just inspect the owner chain, - if we need it, it should be an automatically updated cache

odersky force-pushed the add-transparent-2 branch from 2bae1f6 to b359c7d Compare June 6, 2018 12:14

gsps approved these changes Jun 6, 2018

View reviewed changes

OlivierBlanvillain approved these changes Jun 6, 2018

View reviewed changes

odersky mentioned this pull request Jun 6, 2018

Add subtype annotations #4625

Closed

odersky merged commit c3469da into scala:master Jun 7, 2018

allanrenucci deleted the add-transparent-2 branch June 7, 2018 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transparent methods #4622

Add transparent methods #4622

odersky commented Jun 5, 2018

dottybot left a comment

odersky commented Jun 5, 2018 •

edited

Loading

gsps commented Jun 5, 2018

odersky commented Jun 5, 2018

gsps Jun 5, 2018

odersky Jun 6, 2018

gsps Jun 5, 2018

odersky Jun 6, 2018

gsps Jun 5, 2018

gsps Jun 5, 2018

odersky Jun 6, 2018

gsps Jun 5, 2018

odersky Jun 6, 2018

gsps Jun 5, 2018

gsps Jun 5, 2018

odersky Jun 6, 2018

gsps Jun 5, 2018

odersky commented Jun 6, 2018 •

edited

Loading

OlivierBlanvillain commented Jun 6, 2018

odersky commented Jun 6, 2018

odersky commented Jun 6, 2018

odersky commented Jun 6, 2018

OlivierBlanvillain left a comment

Add transparent methods #4622

Add transparent methods #4622

Conversation

odersky commented Jun 5, 2018

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

odersky commented Jun 5, 2018 • edited Loading

gsps commented Jun 5, 2018

odersky commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Jun 6, 2018 • edited Loading

OlivierBlanvillain commented Jun 6, 2018

odersky commented Jun 6, 2018

odersky commented Jun 6, 2018

odersky commented Jun 6, 2018

OlivierBlanvillain left a comment

Choose a reason for hiding this comment

odersky commented Jun 5, 2018 •

edited

Loading

odersky commented Jun 6, 2018 •

edited

Loading