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.

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

Scalac crashes when I use @elidable for trait #5215

Closed
scabug opened this Issue Nov 23, 2011 · 23 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Nov 23, 2011

Compile following code with -Xelide-below OFF and then Scalac crashed.

import scala.annotation.elidable

trait A {
  @elidable(elidable.FINEST) def xx()
}

class B extends A {
  override def xx() {
    println("xx")
  }
}

object Main {

  def main(args: Array[String]): Unit = {
    val a:A = new B
    a.xx()
  }

}
error: java.lang.Error: Illegal tree in gen: ()
	at scala.tools.nsc.symtab.SymbolTable.abort(SymbolTable.scala:34)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:139)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:59)
	at scala.collection.immutable.List.foreach(List.scala:45)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:136)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:88)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:59)
	at scala.collection.immutable.List.foreach(List.scala:45)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:79)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:65)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.apply(GenICode.scala:61)
	at scala.tools.nsc.Global$GlobalPhase.applyPhase(Global.scala:329)
	at scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:297)
	at scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:297)
	at scala.collection.Iterator$class.foreach(Iterator.scala:660)
	at scala.collection.mutable.ListBuffer$$anon$1.foreach(ListBuffer.scala:316)
	at scala.tools.nsc.Global$GlobalPhase.run(Global.scala:297)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.run(GenICode.scala:54)
	at scala.tools.nsc.Global$Run.compileSources(Global.scala:953)
	at scala.tools.nsc.Global$Run.compile(Global.scala:1038)
	at scala.tools.nsc.Main$.process(Main.scala:106)
	at scala.tools.nsc.Main$.main(Main.scala:123)
	at scala.tools.nsc.Main.main(Main.scala)

Exception in thread "main" java.lang.Error: Illegal tree in gen: ()
	at scala.tools.nsc.symtab.SymbolTable.abort(SymbolTable.scala:34)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:139)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:59)
	at scala.collection.immutable.List.foreach(List.scala:45)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:136)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:88)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase$$anonfun$gen$1.apply(GenICode.scala:69)
	at scala.collection.LinearSeqOptimized$class.foreach(LinearSeqOptimized.scala:59)
	at scala.collection.immutable.List.foreach(List.scala:45)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:69)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:79)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.gen(GenICode.scala:65)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.apply(GenICode.scala:61)
	at scala.tools.nsc.Global$GlobalPhase.applyPhase(Global.scala:329)
	at scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:297)
	at scala.tools.nsc.Global$GlobalPhase$$anonfun$run$1.apply(Global.scala:297)
	at scala.collection.Iterator$class.foreach(Iterator.scala:660)
	at scala.collection.mutable.ListBuffer$$anon$1.foreach(ListBuffer.scala:316)
	at scala.tools.nsc.Global$GlobalPhase.run(Global.scala:297)
	at scala.tools.nsc.backend.icode.GenICode$ICodePhase.run(GenICode.scala:54)
	at scala.tools.nsc.Global$Run.compileSources(Global.scala:953)
	at scala.tools.nsc.Global$Run.compile(Global.scala:1038)
	at scala.tools.nsc.Main$.process(Main.scala:106)
	at scala.tools.nsc.Main$.main(Main.scala:123)
	at scala.tools.nsc.Main.main(Main.scala)

If I change trait A to abstract class A, it's OK.

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 23, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5215?orig=1
Reporter: @Atry
Affected Versions: 2.9.1
Other Milestones: 2.10.0

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 18, 2012

@khernyo said:
The crash is not nice for sure, but I don't think @elidable should be applicable to an abstract method. In fact, it should only be applicable to final methods, otherwise an inconsistent class hierarchy could be generated.

So, I think it should be fixed by reporting an error if a non-final method is annotated with @elidable. Or maybe a warning for now and turn it into an error in a later release.

Any thoughts?

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 18, 2012

@Atry said (edited on Feb 18, 2012 3:27:32 AM UTC):
I think @elidable should just be treated as a directive for the function invoking, instead of erasing that function definition.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 18, 2012

@khernyo said:
Okay, that would make sense, but that's not what I see. Anyway, if only the calls are removed, we still have a problem because annotations are not inherited, or are they? In the following code which is based on your original example code, only the second call could ever be elided:

val b = new B
b.xx()
(b:A).xx()

So, I'm still in favor of "@elidable should be applicable only to final methods". Well, this also has a problem when B.xx() has the annotation and A.xx() does not. I will look into it later.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 18, 2012

@paulp said:
It doesn't intentionally do anything but remove the calls to the methods - it isn't intended to touch the method definitions. I wrote it a long time ago so I can believe it has issues. Patches gladly received.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 20, 2012

@som-snytt said:
The patch that was committed doesn't interact well with the edge case in which the elided method call is the right-hand of an assignment. Now the compiler silently elides the call by emitting nothing, but the resulting class results in a VerifyError.

$ spalac -version
Scala compiler version v2.10.0-M1-378-g63d9ae6dc4 -- Copyright 2002-2011, LAMP/EPFL
$ spala elysian.ElysianFields
java.lang.VerifyError: (class: elysian/ElysianFields, method: greeting signature: ()Ljava/lang/String;) Unable to pop operand off an empty stack

This is related to #5051. The old transform would turn the expr into a BoxedUnit, which is something.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 20, 2012

@khernyo said:
I can fix it, but probably not today.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 20, 2012

@paulp said:
Right, that was the kind of scenario I had in mind when I dropped a unit in there. (Could be a null/0/false/() actually and would probably be less likely to cause trouble.)

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 20, 2012

@khernyo said:
I just opened a pull request: scala/scala#216
This should fix the problem with missing return values.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@som-snytt said:
I would expect a char to default to the normal default, namely zero and not '?'. I.e., same as

var c: Char = _

This is not a use case, but should this print null or complain?

    val junk: Nothing = elided()
    println("Well, "+ junk)

If it doesn't complain, should the inserted code throw an exception? ElidedToNothingException?

As an aside (or stage whisper), I had expected elision in these contexts to be an error ("or at least a warning," as we say), but now I agree that the "default value" behavior could be useful, perhaps to supply special default values during development.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@paulp said:
If you see a '?' it's because your terminal doesn't know how to print \0, not because it's anything other than 0.

scala> var c: Char = _
c: Char = ?

scala> c == '\0'
res0: Boolean = true
@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@khernyo said:
Well, Marki is right, that's a question mark.

I simply don't have a use case in mind where you would want to use the result of an elidable call other than printing it. If you are printing it, then a '\0' somewhere along the way will get treated as the terminator in a null terminated string. So, you might cut off some more important information. Also, it could be harder to find the reason for a shorter-than-usual string than for a '?' (e.g. if the @elidable annotation was left there unintentionally).

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@paulp said:
Oh. Well, it "was" a question mark. I see your point, but being unsurprising is more important.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@khernyo said:
OK, fine by me

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@som-snytt said:
Ha, I hadn't tried it in the REPL.

I think that assigning in the context of Nothing has to result in an error. In particular, eliding the call (and continuing execution) breaks the guarantee that you don't return.

Adding this check

if (expectedType.isNothingType) unit.error(tree.pos, "Cannot elide where Nothing is required.")

results in

$ spalac -Xelide-below 1000 elysian.scala
elysian.scala:18: error: Cannot elide where Nothing is required.
    val junk: Nothing = elided()
                              ^

Also, I think the isInstanceOf test is covered by the method:

//expectedType.isInstanceOf[ValueTypeKind]
generatedType = if (expectedType.isValueType) expectedType else NullReference

For a use case for using the result of an elidable: in development, there's often a "mode" flag for testing, that might affect not only logging but feature behavior, perhaps stubbing or mocking expensive operations, or features that aren't implemented yet.

var icon: Char = TestDefaults.icon
if (icon == 0) icon = '@'

object TestDefaults {
  @elidable(-100) def icon: Char = 't'
}

In this scenario, elision level < 0 means features may be stubbed or disabled. A milestone release might require elision at 1000, with logging disabled.

I don't work this way, but maybe I would if I could.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@khernyo said:
Yeah, I missed the case with Nothing, too. Your fix looks good to me.

Paul has already cleaned up the code in commit fbb7865e137e83660257fdc79d19d29ff39c775b. This includes changing expectedType.isInstanceOf[ValueTypeKind] into expectedType.isValueType

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 21, 2012

@som-snytt said:
As an exercise, I created a pull request for Nothing (expectedType.isNothingType)
scala/scala#218
with the quick fix to raise an error.

I don't have any knowledge of the code base, so I don't know whether more work is needed for the error case.

(I'll admit to second thoughts whether the elision should just result in ElidedToNothingException. Or, analogous to NotImplementedError, call it NoLongerImplementedError...)

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@paulp said:
In case anyone is looking for something to figure out, I had to revert all the recent elidable commits in a983f2b30c as noted in the commit message. See red balls at https://scala-webapps.epfl.ch/jenkins/job/scala-checkin/ .

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@paulp said:
Ah, epfl builds under -optimise. I don't have time to look, but that is almost certainly the extra ingredient in its failure.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@paulp said:
I think things work sensibly and robustly as of 98cf4014a3 . All expressions are elidable, and replacements have the same type. Feedback welcome.

@scabug scabug closed this Feb 23, 2012

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@som-snytt said:
+1 on eliding for Nothing. I guess it would have been a high bar to introduce

def ??! : Nothing = throw new NoLongerImplementedError

into Predef.

And yet there is a certain symmetry:

def yagni = ???
def yagni = feature()
@deprecated("Useless","1.0") def yagni = feature()
@deprecated("Useless","1.0") def yagni = ??!
@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@paulp said:
Then, as the impact of the misfeature continues to be felt, it can be transitioned to ?!! and finally !!!, at which point no argument will be brooked.

@scabug

This comment has been minimized.

Copy link

scabug commented Feb 23, 2012

@khernyo said:
The tests do pass, so all is well.
I like this solution because it will do the right thing in case an annotated method is called through reflection or the method is in a jar (compiled separately from the code calling the annotated method).

I opened a pull request (scala/scala#233) to update the docs for @elided to better match the reality. I consider this issue closable when this commit is merged.

@scabug scabug added this to the 2.10.0-M1 milestone Apr 7, 2017

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