SI-10068 Only permit elidable methods #5539

som-snytt · 2016-11-17T21:25:47Z

In uncurry, check that symbol with @elidable is a method.

This fails to notice the annotation on lazy vals, which seems
to be a product of synthesis at namer/typer.

JIRA: https://issues.scala-lang.org/browse/SI-10068

som-snytt · 2016-11-18T01:16:00Z

Somebody clever:

// annotations on abstract types
abstract class C1[@annotation.elidable(0) +T, U, V[_]]

pos/annotations.scala
pos/spec-annotations.scala

Edit: it used to read @cloneable.

Atry · 2016-11-18T01:46:18Z

This PR is a regression, as the current compiler allows @elidable(INFO) val i: Int = 42 .

som-snytt · 2016-11-18T06:16:14Z

@Atry I think that was just a bug.

The doc says:

An annotation for methods whose bodies may be excluded from compiler-generated bytecode.

It's kind of too bad Scala doesn't have the crazy Java ways to control where an annotation will stick.

It worked by accident for vals by converting the RHS of the accessor method and replacing the field definition itself with an anomalous null. (Everything that isn't a method becomes null.asInstanceOf[A].)

My first thought was to make it work for variables (see comment in test), but it doesn't work for constant value defs and lazy vals. It couldn't work for constants, though probably could be made to work for lazy vals. The shortcoming in this PR is that it still just silently ignores lazy vals.

$ scala -Xelide-below WARNING
Welcome to Scala 2.12.0 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_111).
Type in expressions for evaluation. Or try :help.

scala> import annotation._, elidable._
import annotation._
import elidable._

scala> @elidable(INFO) final val x = 42
x: Int(42) = 42

scala> def f = x
f: Int

scala> f
res0: Int = 42

scala> @elidable(INFO) def g() = 42
g: ()Int

scala> def f = g
f: Int

scala> f
res1: Int = 0

scala> @elidable(INFO) lazy val x = 42
x: Int = <lazy>

scala> def f = x
f: Int

scala> f
res2: Int = 42

scala>

dragos · 2016-11-25T10:43:48Z

I understand that the intent of this annotation is indeed to be for methods. I am not sure if there is code out there relying on this, but it's source-incompatible. Should this target 2.13 then?

Regardless of that, I don't think uncurry is the right place to emit these errors. Doing this in typer may actually allow you to catch lazy vals as well.

som-snytt · 2016-11-25T10:55:20Z

@dragos I tried what I thought was the obvious spot in typer (typedValDefImpl) and missed, so I did what worked.

I don't have a strong opinion about 2.13, except that it's just a bug and not a de facto standard. I'd be generous if it weren't documented, or if the community build breaks.

dragos · 2016-11-25T16:56:58Z

I don't feel strongly about 2.13, as you are right that it's forbidding a corner-case in a fairly esoteric option.

However, I feel a bit stronger about where this check should be done. Besides the design intent of Typer and Uncurry, the latter is not run by the presentation compiler, so Ensime/Eclipse users won't be seeing these errors before building.

If you are stuck, I'm sure there are people on scala-internals who could help.

som-snytt · 2016-11-25T23:52:17Z

@dragos How about refchecks?

There some annotation checks in Typers but it doesn't try too hard:

scala> class C[@specialized A](a: A)
defined class C

scala> @specialized class D[A](a: A)
defined class D

scala> class E[@specialized A](val a: A) extends AnyVal
<console>:11: error: type parameter of value class may not be specialized
       class E[@specialized A](val a: A) extends AnyVal
                            ^

scala> import annotation._, elidable._
import annotation._
import elidable._

scala> @elidable(INFO) def f = 42
f: Int

scala> @elidable(INFO) class F
<console>:17: error: F: Only methods can be marked @elidable.
       @elidable(INFO) class F
                             ^

dragos · 2016-11-28T10:03:17Z

Much better! If you push a version in RefCheck I could have a look and see if it's easy to move one notch to the left, in Typer. :)

lrytz · 2016-12-12T14:50:12Z

Agree for preferring Typers (or RefChecks). You could put in 2.12 under -Xsource:2.13 as a compromise :)

som-snytt · 2016-12-13T09:42:41Z

[warn] Unable to reparse org.scala-lang#scala-library;2.12.2-737427d-SNAPSHOT from private-repo, using Tue Dec 13 08:43:46 UTC 2016
[warn] Unable to reparse org.scala-lang#scala-compiler;2.12.2-737427d-SNAPSHOT from private-repo, using Tue Dec 13 08:54:28 UTC 2016
[warn] Unable to reparse org.scala-lang#scala-reflect;2.12.2-737427d-SNAPSHOT from private-repo, using Tue Dec 13 08:46:40 UTC 2016
[info] Compiling 582 Scala sources and 120 Java sources to /home/jenkins/workspace/scala-2.12.x-validate-test@2/build/quick/classes/library...
[info] 'compiler-interface' not yet compiled for Scala 2.12.2-20161213-003525-737427d. Compiling...
error: scala.MatchError: null (of class scala.reflect.internal.Trees$Literal)
	at scala.tools.nsc.backend.jvm.GenBCode$BCodePhase.gen$1(GenBCode.scala:449)
	at scala.tools.nsc.backend.jvm.GenBCode$BCodePhase.apply(GenBCode.scala:453)

som-snytt · 2016-12-13T19:07:26Z

Passes modulo mima.

lrytz · 2016-12-14T11:25:20Z

Looks good, thanks! The MiMa failure will go away after a rebase. @dragos, do you think the check should move to Typers?

In refchecks, check that symbol with `@elidable` is a method. When eliding in uncurry, doublecheck. The check is enabled under `-Xsource:2.13`.

adriaanm · 2016-12-19T23:30:51Z

LGTM -- excellent! Also much pleased to see we're allocating fewer ScalaVersion objects :-)

retronym · 2016-12-19T23:34:51Z

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

@@ -84,7 +84,10 @@ trait ScalaSettings extends AbsScalaSettings
   * though this helper.
   */
  def isScala211: Boolean = source.value >= ScalaVersion("2.11.0")
-  def isScala212: Boolean = source.value >= ScalaVersion("2.12.0")
+  private[this] val version212 = ScalaVersion("2.12.0")


Forgot ScalaVersion("2.11.0") ?

I didn't want to push it.

lrytz · 2017-09-22T07:50:22Z

@som-snytt I stumbled over this PR while enabling Xsource:2.13 by default in the 2.13 branch. I wonder why there's code to report an error both in RefChecks and UnCurry. Are there cases that slip the first check?

Also, the error is reported if the annotation is on an abstract method. This makes sense, as the annotation is not "inherited", we don't check if an overridden member has the annotation. I assume this is intentional? I'll update the error message, because it can be confusing:

Test.scala:5: error: f1: Only methods can be marked @elidable.
  @elidable(MINIMUM) def f1(): Unit
                         ^

lrytz · 2017-09-22T08:42:02Z

Ah, the static type is relevant:

import annotation._
import elidable._

abstract class A {
  @elidable(10) def f(): Unit
}

class C extends A {
  def f(): Unit = println("f")
}

object Test {
  def main(args: Array[String]): Unit = {
    val c = new C
    c.f()
    (c: A).f()
  }
}

In 2.12, the above prints f once. With -Xsource:2.13 there's a compiler error. Is this a reason to allow the annotation on abstract methods?

som-snytt · 2017-09-22T15:36:07Z

@lrytz I don't remember directly, but from the comments it looks like I had trouble detecting a lazy val when I was trying to support elidable lazy? I don't know what paranoia made me leave the later check. The message could have been augmented under debug, and then tests could document where the error was issued. I should have written more tests. Your clever example rings a bell, but it seems to me a reason not to allow it. Now we need a new lint rule to warn about overriding an elidable with a non-elidable. Which is the officially blessed lint project now?

In the inverse case, the call to A.f is not elided, but the RHS of C.f is. OK, so maybe it ought to be allowed. The story for overriding is very fragile.

Not supporting vals is too bad. An elidable debug flag is an obvious use case. (It's a value with a useful natural default.)

lrytz · 2017-09-23T08:06:47Z

Lazy vals are caught by the first test, so probably it's just an oversight. Let's remove the report in UnCurry.

About @elidable on an abstract method, I think it makes sense to leave it as-is (report as error). I don't think anyone should rely on the receiver type / having a different annotation on an overriden method.

Which is the officially blessed lint project now?

Time, adoption, contributors will tell :-) You were the primary contributor to -Xlint in the recent past, so you do has an impact. Scalafix is young, it has sbt integration but needs to be called explicitly, and no IDE integration..

som-snytt · 2017-09-23T09:40:13Z

A use case would be a logger interface with implementations. My app code could then elide logger.log() entirely. Oh, in fact under separate compilation, my logger library needs the elidable abstract, since loggerImpl.log() won't be recompiled.

Yeah, -Xlint was a nice hole I dug.

lrytz · 2017-09-24T05:30:14Z

I'm happy to have it changed back to allow it on abstract members. Here's my clarifications for the status quo: d5bf1d7, part of #6092 for 2.13.x

som-snytt · 2017-09-24T07:34:31Z

@lrytz thanks for the info. I'd wait for someone to ask for the feature, as there is no right path. I was only speaking hypothetically about a logger interface.

My pet use case for elidable was for API prioritization, where methods marked "must have" would be enabled earlier in a development cycle. The elide level in the build would gradually fall until all features goals are met. That's also a hypothetical workflow.

SethTisue added in progress and removed in progress labels Nov 17, 2016

SethTisue mentioned this pull request Nov 17, 2016

Audit webhooks scala/scala-swing#55

Closed

scala-jenkins added this to the 2.12.2 milestone Nov 17, 2016

som-snytt force-pushed the issue/10068 branch from 4b5837c to 557bf29 Compare November 18, 2016 01:28

som-snytt force-pushed the issue/10068 branch 3 times, most recently from 0d9cc0d to 737427d Compare December 13, 2016 08:35

som-snytt force-pushed the issue/10068 branch from 737427d to 194a3bf Compare December 13, 2016 16:54

SI-10068 Only permit elidable methods

582c8a2

In refchecks, check that symbol with `@elidable` is a method. When eliding in uncurry, doublecheck. The check is enabled under `-Xsource:2.13`.

som-snytt force-pushed the issue/10068 branch from 194a3bf to 582c8a2 Compare December 14, 2016 18:05

scala-jenkins added the reviewed label Dec 19, 2016

adriaanm merged commit 56fb917 into scala:2.12.x Dec 19, 2016

retronym reviewed Dec 19, 2016

View reviewed changes

som-snytt deleted the issue/10068 branch December 20, 2016 00:20

SethTisue mentioned this pull request Feb 18, 2017

[ci: last-only] merge 2.12.x onto 2.13.x (Feb 17, 2017) #5718

Merged

scabug mentioned this pull request Apr 7, 2017

@elidable on var cause scalac crash scala/bug#10068

Closed

som-snytt mentioned this pull request May 5, 2017

Precompute Setting.isScala211 etc #5883

Closed

adriaanm mentioned this pull request Feb 1, 2018

Document features conditional on source and target versions scala/scala-dev#470

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SI-10068 Only permit elidable methods #5539

SI-10068 Only permit elidable methods #5539

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SI-10068 Only permit elidable methods #5539

SI-10068 Only permit elidable methods #5539

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!