Implement phantom types part 1 #2136

nicolasstucki · 2017-03-22T13:12:59Z

230 LOC of code without error messages and tests.

In this PR phantom types are defined and implemented. Only a minimal version of phantom term erasure is included.

Phantom types are in latices outside of the normal Any/Nothing type lattice.
A new phantom lattice can be defined by extending an object with scala.Phantom.
This trait defines synthetic members

protected final trait Any where this Any does not extends scala.Any
protected final abstract class Nothing extends this.Any
protected final def assume: this.Nothing

A phantom lattice object can expose any of the phantom members (Any, Nothing, assume)
using an alias but it is not required.

Restriction on lattices:

Types from different lattice can not be mixed using & or | types.
Type parameters must be bounded by types in a single lattice.

Phantom type erasure (minimalistic):

Type references Phantom.Any and Phantom.Nothing are erased to BoxedUnit
Phantom.assume is erased to BoxedUnit.UNIT
Phantom is removed from the parents of the object inheriting it.

Extra tests for Phantoms 1 and Extra tests for Phantoms 2 are tests intended to check the correct erasure of phantom arguments and fields (not in this PR). These tests also pass on this PR as the semantics and runtime result is equivalent with or without that additional erasure.

nicolasstucki · 2017-03-22T13:16:37Z

This PR is the first part of #1408.

odersky · 2017-04-04T20:30:50Z

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

          newScopeEntry(newFunctionNTrait(name.asTypeName))
-        else res
+        else null


Isn't that exactly the same logic as before? If yes, I don't see it as an improvement.

odersky · 2017-04-05T15:53:08Z

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

+  }
+
+  def isPhantomAnyClass(sym: Symbol)(implicit ctx: Context): Boolean =
+    sym.exists && (sym.owner eq PhantomClass) && sym.name == tpnme.Any


If you swap the name and owner tests, you can avoid the additional exists test. NoSymbol has a valid name: <none>.

Same observation applies to the following methods.

odersky · 2017-04-05T16:29:58Z

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

    }

+    private def isPhantomClass(sym: Symbol)(implicit ctx: Context): Boolean =
+      sym.isClass && (sym.owner eq defn.PhantomClass) && (sym.name == tpnme.Any || sym.name == tpnme.Nothing)


Isn't the last part, (sym.name == tpnme.Any || sym.name == tpnme.Nothing), redundant?

odersky · 2017-04-05T16:39:17Z

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

+          if (tp1.symbol eq NothingClass) tp2.isValueTypeOrLambda && !isPhantom(tp2)
+          else if (tp1.symbol eq NullClass) isNullable(tp2) && !isPhantom(tp2)
+          else if (defn.isPhantomNothingClass(tp1.symbol)) tp2.isValueTypeOrLambda && (tp1.phantomTopClass == tp2.phantomTopClass)
+          else false
      }


@i think isPhantom is not strong enough. Concretely, the test would return true for

scala.Nothing <: phantom.X

if X is an abstract type in phantom. So I believe you need to follow supertypes to determine isPhantom, e.g. isPhantomClass(tp2.phantomTopClass).

However, we should carefully benchmark the runtime cost of this. Type comparison takes the lion's share of typechecking time. We should get some data, like: how much time takes the dotty test or the compileStdLib test, or the whole junit test suite? If we see a slowdown, we can try to mitigate by either finding a faster way to test or caching the result of the test in a type.

odersky · 2017-04-05T16:43:05Z

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

@@ -199,7 +201,7 @@ trait TypeOps { this: Context => // TODO: Make standalone object.
    }

    /** The minimal set of classes in `cs` which derive all other classes in `cs` */
-    def dominators(cs: List[ClassSymbol], accu: List[ClassSymbol]): List[ClassSymbol] = (cs: @unchecked) match {
+    @tailrec def dominators(cs: List[ClassSymbol], accu: List[ClassSymbol]): List[ClassSymbol] = cs match {


I don't think we need the @tailrec here. @tailrec is intended to be normative. We should only use it if it would be a problem if the method was not tail recursive. I don't see this being the case here. dominators will not be called on huge lists of classes, and neither is it very performance sensitive.

odersky · 2017-04-05T18:30:05Z

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

+    // todo: retract mode between Type and Pattern?
+
+    /** Check that the are not mixed Any/Phantom.Any types in `&`, `|` and type bounds,
+     *  this includes Phantom.Any of different universes.


typo: the -> there

odersky · 2017-04-05T18:31:52Z

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

+        case TypeBoundsTree(lo, hi) =>
+          checkedTops2(lo, hi, PhantomCrossedMixedBounds(lo, hi), tree.pos)
+        case untpd.InfixOp(left, op, right) =>
+          checkedTops2(left, right, PhantomCrossedMixedBounds(left, right), tree.pos)


Wrong error message. (In general I prefer if one does not immediately generate error messages in new code. It's a premature optimization. Much clearer to write the error directly as a string, then one can check whether it makes sense or not).

odersky · 2017-04-05T18:41:27Z

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

+        case _ => Set(defn.topOf(tree.typeOpt))
+      }
+    }
+    checkedTops(tree)


I think it's better to reorganize type checking as follows: In TypeAssigner, insert check code in the assignType methods for AndType, OrType, TypeBounds, If, and Match. Here's an example for OrType:

def assignType(tree: untpd.OrTypeTree, left: Tree, right: Tree)(implicit ctx: Context) = { checkSameUniverse(left, right, "be combined in `|`) tree.withType(left.tpe | right.tpe) }

where

checkSameUniverse(tree1: Tree, tree2: Tree, relationship: => String) = if (topClass(tree1.tpe) != topClass(tree2.tpe)) ctx.error(ex"${tree1.tpe} and ${tree2.tpe} are in different universes. They cannot $relationship")

If we do this, Typer can be left completely alone.

odersky · 2017-04-05T18:53:00Z

compiler/src/dotty/tools/dotc/transform/phantom/PhantomTypeErasure.scala

+import dotty.tools.dotc.core.Types._
+import dotty.tools.dotc.transform.TreeTransforms.{MiniPhaseTransform, TransformerInfo}
+
+class PhantomTypeErasure extends MiniPhaseTransform with InfoTransformer {


We need a doc comment here that describes in detail what the phase does.

Aren't we supposed to also erase values and parameters of phantom types here? Without a leading doc comment, impossible to tell.

odersky · 2017-04-05T18:53:25Z

compiler/src/dotty/tools/dotc/transform/phantom/PhantomTypeErasure.scala

+  }
+
+  override def transformTypeApply(tree: TypeApply)(implicit ctx: Context, info: TransformerInfo): Tree =
+    if (defn.isPhantomAssume(tree.fun.symbol)) Literal(Constant(null)).withType(defn.ErasedPhantomType) else tree


Why null? Looks like a code smell to me.

nicolasstucki · 2017-04-13T07:41:17Z

The following table shows the result of a macro benchmark done on my laptop. The time is the time taken to run the standard lib test in the test suite under vulpix. The columns contain the number of runs that finished in the given time.

sec     before phantoms           after phantoms
68           0                         0
69           0                         5
70           4                         6
71           3                         2
72           4                         1
73           3                         0
74           1                         0
75           0                         0
76           1                         1
77           0                         1
78           0                         0
79           0                         0

Surprisingly the execution times are are slightly faster after the phantom types where added. We would need to run this on a stabler machine if we would wish to have more precise numbers. But at least there is no evident time degradation.

nicolasstucki · 2017-04-13T07:50:01Z

@odersky all requested changes where taken care of.

Additionally, the PhantomTypeErasure erasure phase was removed and integrated into Erasure as it should have been from the start.

odersky · 2017-04-13T11:23:19Z

I think CI is a bad indicator of times. We need to get better benchmarks. Until then, we need to test locally. It's better to do specific tests than rely on aggregates.

…

On Thu, Apr 13, 2017 at 9:50 AM, Nicolas Stucki ***@***.***> wrote: @odersky <https://github.com/odersky> all requested changes where taken care of. Additionally, the PhantomTypeErasure erasure phase was removed and integrated into Erasure as it should have been from the start. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwlVtQ5hTxwtK9wsklFIeA8NCLXIDbMks5rvdOqgaJpZM4MlLH0> . {"api_version":"1.0","publisher":{"api_key":" 05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity": {"external_key":"github/lampepfl/dotty","title":" lampepfl/dotty","subtitle":"GitHub repository","main_image_url":" https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88- 11e6-95fc-7290892c7bb5.png","avatar_image_url":"https:// cloud.githubusercontent.com/assets/143418/15842166/ 7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/lampepfl/dotty"}}," ***@***.*** in #2136: @odersky all requested changes where taken care of. \r\n\r\nAdditionally, the `PhantomTypeErasure` erasure phase was removed and integrated into `Erasure` as it should have been from the start."}],"action":{"name":"View Pull Request","url":"https:// github.com/lampepfl/dotty/pull/2136#issuecomment-293818404"}}}

-- Prof. Martin Odersky LAMP/IC, EPFL

nicolasstucki · 2017-04-13T11:26:18Z

I ran the benchmarks locally on my machine, not on the CI.

This removes an unnecessary additional phase and fixes siggnatures of erased methods.

odersky · 2017-05-08T15:32:10Z

@nicolasstucki I did a pass to polish. Can you review my last commit? If you are happy with it, we can get the PR in.

nicolasstucki · 2017-05-08T15:54:37Z

Last commit LGTM :)

nicolasstucki · 2017-05-08T16:51:38Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+
+    val explanation =
+      hl"""|""".stripMargin
+  }  


All changes in this file should be reverted.

nicolasstucki · 2017-05-08T17:17:20Z

@odersky now it is ready to be merged.

odersky · 2017-05-08T17:17:25Z

So, let's merge this at last!

nicolasstucki · 2017-05-08T17:18:18Z

Finally :)

nicolasstucki assigned odersky Mar 22, 2017

felixmulder requested a review from odersky April 4, 2017 09:59

odersky requested changes Apr 5, 2017

View reviewed changes

nicolasstucki force-pushed the implement-phantom-types-part-1 branch 6 times, most recently from d4da0b1 to 069bc38 Compare April 11, 2017 19:08

nicolasstucki mentioned this pull request Apr 11, 2017

Overrides with abstract types crash complier #2230

Closed

nicolasstucki force-pushed the implement-phantom-types-part-1 branch 5 times, most recently from 74a643a to 8b27beb Compare April 12, 2017 12:09

nicolasstucki force-pushed the implement-phantom-types-part-1 branch 3 times, most recently from 0555a33 to 5ee0a9a Compare April 20, 2017 10:46

nicolasstucki force-pushed the implement-phantom-types-part-1 branch 7 times, most recently from 36eaf5e to 12da43d Compare April 27, 2017 11:47

nicolasstucki and others added 17 commits May 8, 2017 16:52

Move phantom universe checks from Typer to TypeAssigner.

341cdf1

Remove PhantomTypeErasure and integrate into Erasure.

a50328f

This removes an unnecessary additional phase and fixes siggnatures of erased methods.

Slighly faster phantom erasure checks.

9502cb2

Simplify the definition of Phantom.assume.

6f03972

Cache PhantomNothing, PhantomAny and PhantomAssume symbols.

621610f

Shortcircuit slow path typing Nothing <:< XYZ for non phantoms.

ef62884

Faster bottomType and phantomness checks.

49e88fe

Improve names of phantom classes in defn.

e062df3

Remove unnecessary changes.

3ff903e

Remove ErasedPhantom class and use Unit.

f0383fe

Set flags of scala.Phantom to be NoInitsTrait.

91558c3

Rename Phantoms in defn and add Type.isPhantom.

ec13270

Add documentation.

62b7732

Fix neg/cycles.scala infinite loop on phantomLatticeType.

4202b98

Code cleanup.

c6fe778

Fix after rebase, if/match missmatch on phantoms.

07c02c8

Polishings of Typer and TypeAssigner

40ab53a

odersky force-pushed the implement-phantom-types-part-1 branch from ddbce6d to 40ab53a Compare May 8, 2017 15:12

odersky approved these changes May 8, 2017

View reviewed changes

nicolasstucki commented May 8, 2017

View reviewed changes

Remove unused error messages.

a21404a

odersky merged commit a391a58 into scala:master May 8, 2017

nicolasstucki mentioned this pull request Jul 20, 2017

Phantom Types #2040

Closed

allanrenucci deleted the implement-phantom-types-part-1 branch December 14, 2017 19:23

odersky mentioned this pull request May 12, 2022

Fix documentation regressions #15170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement phantom types part 1 #2136

Implement phantom types part 1 #2136

nicolasstucki commented Mar 22, 2017 •

edited

Loading

nicolasstucki commented Mar 22, 2017

odersky Apr 4, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

nicolasstucki Apr 6, 2017

odersky Apr 5, 2017 •

edited

Loading

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

odersky Apr 5, 2017

nicolasstucki commented Apr 13, 2017

nicolasstucki commented Apr 13, 2017

odersky commented Apr 13, 2017 via email

nicolasstucki commented Apr 13, 2017

odersky commented May 8, 2017

nicolasstucki commented May 8, 2017

nicolasstucki May 8, 2017

nicolasstucki commented May 8, 2017

odersky commented May 8, 2017

nicolasstucki commented May 8, 2017

Implement phantom types part 1 #2136

Implement phantom types part 1 #2136

Conversation

nicolasstucki commented Mar 22, 2017 • edited Loading

nicolasstucki commented Mar 22, 2017

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 Apr 5, 2017 • edited Loading

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

nicolasstucki commented Apr 13, 2017

nicolasstucki commented Apr 13, 2017

odersky commented Apr 13, 2017 via email

nicolasstucki commented Apr 13, 2017

odersky commented May 8, 2017

nicolasstucki commented May 8, 2017

Choose a reason for hiding this comment

nicolasstucki commented May 8, 2017

odersky commented May 8, 2017

nicolasstucki commented May 8, 2017

nicolasstucki commented Mar 22, 2017 •

edited

Loading

odersky Apr 5, 2017 •

edited

Loading