Implement unused (renamed to ghost terms) #3342

nicolasstucki · 2017-10-17T15:27:09Z

Decide or propose keyword that should be used for unused: https://goo.gl/forms/UD44AfLUD3dkfnxs2

TODO list:

liufengyun · 2017-10-19T16:13:06Z

test performance please

dottybot · 2017-10-19T16:13:31Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-10-19T17:11:34Z

performance test failed:

Error line number: 16

[check /data/workspace/bench/logs/pull-3342-10-19-18.14.out for more information]

nicolasstucki · 2017-10-20T14:11:55Z

test performance please

dottybot · 2017-10-20T14:12:54Z

performance test scheduled: 2 job(s) in queue, 1 running.

dottybot · 2017-10-20T17:47:00Z

performance test failed:

Error line number: 16

[check /data/workspace/bench/logs/pull-3342-10-20-18.49.out for more information]

liufengyun · 2017-10-21T20:05:39Z

I just checked the logs, this PR failed scalapb.

nicolasstucki · 2017-10-27T12:11:36Z

Rebased and changed all MiniPhaseTransforms with MegaPhase.MiniPhase.

odersky · 2017-10-29T17:27:21Z

I would add to the TODO list:

remove phantom types, and migrate tests over where they make sense

odersky

More to come...

odersky · 2017-11-13T08:49:48Z

compiler/src/dotty/tools/dotc/transform/UnusedChecks.scala

+  /* Tree transform */
+
+  override def transformTypeDef(tree: TypeDef)(implicit ctx: Context): tree.type = {
+    if (tree.symbol.is(UnusedCommon))


is(Unused). UnusedCommon should never be used in an is check.

odersky · 2017-11-13T08:50:50Z

compiler/src/dotty/tools/dotc/transform/UnusedChecks.scala

+
+  override def transformTypeDef(tree: TypeDef)(implicit ctx: Context): tree.type = {
+    if (tree.symbol.is(UnusedCommon))
+      ctx.error(tree.symbol.showKind + " cannot be unused", tree.pos)


I think this belongs not in this phase, but where we check the other flags whether they make sense.

odersky · 2017-11-13T08:52:03Z

compiler/src/dotty/tools/dotc/transform/UnusedChecks.scala

+  }
+
+  private def isUnusedContext(implicit ctx: Context): Boolean =
+    ctx.owner.ownersIterator.exists(_.is(Unused)) // TODO make context mode?


Or put it in the store, yes.

odersky · 2017-11-13T08:53:46Z

compiler/src/dotty/tools/dotc/transform/UnusedChecks.scala

@@ -0,0 +1,102 @@
+package dotty.tools.dotc.transform
+


I think it's better to implement these checks as a top-down traversal in PostTyper. It's inherently a top-down algorithm not a bottom-up one.

I will try this in PostTyper. My first approach was in typer and I quickly realized it was a mistake because these failures influenced typing desitions. Hence decided to do it in an isolated miniphase.

odersky · 2017-11-13T08:59:56Z

compiler/src/dotty/tools/dotc/transform/UnusedArgLift.scala

@@ -2,17 +2,17 @@ package dotty.tools.dotc.transform

 import dotty.tools.dotc.ast.tpd
 import dotty.tools.dotc.core.Contexts._
-import dotty.tools.dotc.core.NameKinds._
+import dotty.tools.dotc.core.Symbols._
 import dotty.tools.dotc.core.Types._
 import dotty.tools.dotc.transform.MegaPhase.MiniPhase
 import dotty.tools.dotc.typer.EtaExpansion

 import scala.collection.mutable.ListBuffer



We need to have a discussion whether we want to do this. The whole point of unused is to avoid generating code. So if we lift most arguments we miss that point. If we assume a precise effect analysis we could lift arguments if the argument is pure only. But we do not have such an effect analysis.

An alternative is to always drop unused arguments. In a sense unused arguments are then treated similarly to pattern guards - the compiler is allowed to assume they are pure. In the future, if we have a good effect analysis we could also require that unused arguments are pure (and total?).

Now I'm convinced that we should not evaluate any of the arguments in an unused method. This implies that unused implicits only check for the existence of such implicit and then disregard its computation. For unused arguments that are explicitly stated we should drop them as well but if they are not pure we will emit a warning.
We should not require them to be pure or total, but rather use those as hints in the warnings to point that proof might be unsafe.

def f(unused x: A): B = ??? def g()(unused implicit x: A): B = ??? f(pureA) // pureA is not evaluated g() // the implicit A for x is not evaluated g()(pureA) // pureA is not evaluated f(impureA) // warn that impureA will not be evaluated g()(impureA) // the implicit A for x is not evaluated

odersky · 2017-11-13T09:03:26Z

compiler/src/dotty/tools/dotc/transform/UnusedRefs.scala

+/** This phase removes all references and calls to unused methods or vals
+ *
+ *   if `unused def f(x1,...,xn): T = ...`
+ *   then `f(y1,...,yn)` --> `y1; ...; yn; (default value for T)`


Why not use ??? instead of default value? If something goes wrong later you will fail loudly instead of silently, which is better.

These default values are intended to stay there, it is the simplest tree the can be both used when f(...) is a statement or an expression. These are trivially eliminated by the optimizer.
This way UnusedRefs does not need to know and special case unused function applications in different positions. The same applies to member selection and identifiers.

nicolasstucki · 2017-11-17T16:33:58Z

@odersky all changes have been made. The PR is ready for review.

nicolasstucki · 2018-02-15T11:04:28Z

Rebase

Blaisorblade · 2018-02-15T20:05:04Z

Just voted in the poll (is it advertised more widely?).

FWIW, to me unused suggests not that the parameter is erased, but that it is simply not used (but the compiler shouldn't warn me please).

Also, we might want to erase them even though they seem to be used. The current semantics prevent from making =:= erased (as soon as you call its apply method); yesterday I talked with Martin about how that could be fixed by marking as inline methods like apply.

odersky · 2018-02-21T10:13:52Z

docs/docs/reference/unused-terms.md

-present at runtime to be able to do separate compilation and retain binary compatiblity. Unused parameters are contractually
-obligated to not be used at runtime, enforcing the essence of evidences on types and allows them to always be optimized away.
-
+present at runtime to be able to do separate compilation and retain binary compatiblity.


present at runtime --> present in some form in the generated code

nicolasstucki · 2018-02-21T13:21:15Z

test performance please

dottybot · 2018-02-21T13:22:16Z

performance test scheduled: 3 job(s) in queue, 1 running.

dottybot · 2018-02-21T15:20:50Z

performance test failed:

Error line number: 24

[check /data/workspace/bench/logs/pull-3342-02-21-14.37.out for more information]

joan38 · 2018-03-01T16:18:51Z

Really weired name to call this unused, for me unused == useless == bin it
It also clashes with -Ywarn-unused.
I would have prefered something like erased or phantom.

nicolasstucki · 2018-03-01T16:49:07Z

I also want to change it. erased is not the best name as there is the concept of erasure on types that is quite different. The best contenders are phantom or ghost.

Blaisorblade · 2018-03-01T17:10:30Z

Any chance to pick another name, maybe even before the release, since apparently nobody likes unused?

nicolasstucki · 2018-03-01T17:32:57Z

I could make the change tomorrow morning.

nicolasstucki · 2018-03-02T11:00:03Z

Renamed to ghost in #4061

Blaisorblade · 2018-03-05T00:34:56Z

tests/run/unused-24.scala

+    println(fun(new Bar))
+  }
+
+  def fun(unused foo: Foo): foo.X = {


This tests is unsound because it treats ghost values as stable (#4060) /cc @nicolasstucki

nicolasstucki force-pushed the implement-unused branch 4 times, most recently from ab32456 to 35b7541 Compare October 19, 2017 06:54

nicolasstucki requested a review from odersky October 19, 2017 12:30

nicolasstucki added the stat:needs review label Oct 19, 2017

nicolasstucki force-pushed the implement-unused branch from 331f06f to 975b36f Compare October 27, 2017 12:10

nicolasstucki assigned odersky Oct 27, 2017

odersky mentioned this pull request Oct 29, 2017

Add an error message class for phantom types #3399

Closed

nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Oct 31, 2017

Squased scala#3342

9cd1e27

nicolasstucki force-pushed the implement-unused branch 2 times, most recently from ea541a6 to bd4b6dd Compare October 31, 2017 14:16

nicolasstucki mentioned this pull request Oct 31, 2017

Implement simple phantom functions #2405

Closed

nicolasstucki force-pushed the implement-unused branch from d6b4c56 to a1354a8 Compare November 6, 2017 09:34

odersky reviewed Nov 13, 2017

View reviewed changes

nicolasstucki force-pushed the implement-unused branch 5 times, most recently from f7247a2 to a06d1a1 Compare November 17, 2017 15:38

nicolasstucki added 9 commits February 15, 2018 11:59

Use set for funArgMods

7c4662d

Add missing syntax for FunArgMods

ce2d805

Improve variable names

544c8d5

Improve comment

f10f2d0

Fix synthetic methods of value class with unused params

ddf5b2c

Make Unused flag a term flag only

7f81bc3

Add rules

c877473

Fix documentation

ed50958

Move methods from UnusedUtil to Applications

c040798

nicolasstucki force-pushed the implement-unused branch from a50f046 to c040798 Compare February 15, 2018 11:04

odersky approved these changes Feb 21, 2018

View reviewed changes

odersky assigned nicolasstucki and unassigned odersky Feb 21, 2018

Update unused parameter doc

68afbcd

nicolasstucki merged commit 8d07271 into scala:master Feb 21, 2018

allanrenucci deleted the implement-unused branch February 21, 2018 17:31

allanrenucci mentioned this pull request Feb 22, 2018

Regression in Community Build: ScalaPB #4033

Closed

nicolasstucki changed the title ~~Implement unused~~ Implement unused (renamed to ghost terms) Mar 2, 2018

Blaisorblade reviewed Mar 5, 2018

View reviewed changes

karolchmist mentioned this pull request Jun 7, 2018

dotty: 0.6.0-RC1 -> 0.8.0-RC1 NixOS/nixpkgs#41626

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unused (renamed to ghost terms) #3342

Implement unused (renamed to ghost terms) #3342

nicolasstucki commented Oct 17, 2017 •

edited by liufengyun

liufengyun commented Oct 19, 2017

dottybot commented Oct 19, 2017

dottybot commented Oct 19, 2017

nicolasstucki commented Oct 20, 2017

dottybot commented Oct 20, 2017

dottybot commented Oct 20, 2017

liufengyun commented Oct 21, 2017

nicolasstucki commented Oct 27, 2017

odersky commented Oct 29, 2017

odersky left a comment

odersky Nov 13, 2017

nicolasstucki Nov 14, 2017

odersky Nov 13, 2017

nicolasstucki Nov 14, 2017

odersky Nov 13, 2017

odersky Nov 13, 2017

nicolasstucki Nov 14, 2017

odersky Nov 13, 2017

nicolasstucki Nov 15, 2017

odersky Nov 13, 2017

nicolasstucki Nov 14, 2017

nicolasstucki commented Nov 17, 2017

nicolasstucki commented Feb 15, 2018

Blaisorblade commented Feb 15, 2018

odersky Feb 21, 2018

nicolasstucki commented Feb 21, 2018

dottybot commented Feb 21, 2018

dottybot commented Feb 21, 2018

joan38 commented Mar 1, 2018 •

edited

nicolasstucki commented Mar 1, 2018

Blaisorblade commented Mar 1, 2018

nicolasstucki commented Mar 1, 2018

nicolasstucki commented Mar 2, 2018

Blaisorblade Mar 5, 2018

Implement unused (renamed to ghost terms) #3342

Implement unused (renamed to ghost terms) #3342

Conversation

nicolasstucki commented Oct 17, 2017 • edited by liufengyun

liufengyun commented Oct 19, 2017

dottybot commented Oct 19, 2017

dottybot commented Oct 19, 2017

nicolasstucki commented Oct 20, 2017

dottybot commented Oct 20, 2017

dottybot commented Oct 20, 2017

liufengyun commented Oct 21, 2017

nicolasstucki commented Oct 27, 2017

odersky commented Oct 29, 2017

odersky left a 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

nicolasstucki commented Nov 17, 2017

nicolasstucki commented Feb 15, 2018

Blaisorblade commented Feb 15, 2018

Choose a reason for hiding this comment

nicolasstucki commented Feb 21, 2018

dottybot commented Feb 21, 2018

dottybot commented Feb 21, 2018

joan38 commented Mar 1, 2018 • edited

nicolasstucki commented Mar 1, 2018

Blaisorblade commented Mar 1, 2018

nicolasstucki commented Mar 1, 2018

nicolasstucki commented Mar 2, 2018

Choose a reason for hiding this comment

nicolasstucki commented Oct 17, 2017 •

edited by liufengyun

joan38 commented Mar 1, 2018 •

edited