Skip to content
This repository

SI-4134: abstract override crasher if lacking super impl #359

Closed
wants to merge 1 commit into from

2 participants

som-snytt Paul Phillips
som-snytt

This commit just changes the assert on a bad abstract super.m
to a compiler error. Probably it's the not that simple, right?
In this case, review is easy; but if it weren't, should I ask
on scala-internals or on the jira ticket? By way of not
wasting people's time? I was just looking for small bugs as
an excuse to look at code.

The example from the ticket is committed as a neg test.
The problem is that a super.m on an abstract override
member m has no concrete implementation, that is, the
trait T is not mixed in after a class C with a concrete m.

The error is noticed at phase mixin when the super accessor
is added to the concrete mixer. (Pun alert?) When super.m
is rebound, no concrete matching symbol is found up the
linearization.

Previously, it was asserted that such a symbol should
be found, but since this is our first opportunity to
detect that there is none, an error should be emitted
instead. The new message is of the form:

Member method f of mixin trait T2 is missing a concrete super implementation.

Additionally, a couple of flag tests were changed to use isAbstractOverride.

som-snytt SI-4134: abstract override crasher if lacking super impl
The example from the ticket is committed as a neg test.
The problem is that a super.m on an abstract override
member m has no concrete implementation, that is, the
trait T is not mixed in after a class C with a concrete m.

The error is noticed at phase mixin when the super accessor
is added to the concrete mixer. (Pun alert?) When super.m
is rebound, no concrete matching symbol is found up the
linearization.

Previously, it was asserted that such a symbol should
be found, but since this is our first opportunity to
detect that there is none, an error should be emitted
instead. The new message is of the form:

Member method f of mixin trait T2 is missing a concrete super implementation.

Additionally, a couple of flag tests were changed to use isAbstractOverride.
b28ece2
Paul Phillips
paulp commented April 06, 2012

I had to rebase unfortunately, but this is committed.

Paul Phillips paulp closed this April 06, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Apr 05, 2012
som-snytt SI-4134: abstract override crasher if lacking super impl
The example from the ticket is committed as a neg test.
The problem is that a super.m on an abstract override
member m has no concrete implementation, that is, the
trait T is not mixed in after a class C with a concrete m.

The error is noticed at phase mixin when the super accessor
is added to the concrete mixer. (Pun alert?) When super.m
is rebound, no concrete matching symbol is found up the
linearization.

Previously, it was asserted that such a symbol should
be found, but since this is our first opportunity to
detect that there is none, an error should be emitted
instead. The new message is of the form:

Member method f of mixin trait T2 is missing a concrete super implementation.

Additionally, a couple of flag tests were changed to use isAbstractOverride.
b28ece2
This page is out of date. Refresh to see the latest.
11  src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -121,7 +121,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
121 121
    *  @param member     The symbol statically referred to by the superaccessor in the trait
122 122
    *  @param mixinClass The mixin class that produced the superaccessor
123 123
    */
124  
-  private def rebindSuper(base: Symbol, member: Symbol, mixinClass: Symbol): Symbol =
  124
+  private def rebindSuper(base: Symbol, member: Symbol, mixinClass: Symbol): Option[Symbol] =
125 125
     afterPickler {
126 126
       var bcs = base.info.baseClasses.dropWhile(mixinClass !=).tail
127 127
       var sym: Symbol = NoSymbol
@@ -136,8 +136,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
136 136
         sym = member.matchingSymbol(bcs.head, base.thisType).suchThat(sym => !sym.hasFlag(DEFERRED | BRIDGE))
137 137
         bcs = bcs.tail
138 138
       }
139  
-      assert(sym != NoSymbol, member)
140  
-      sym
  139
+      if (sym != NoSymbol) Some(sym) else None
141 140
     }
142 141
 
143 142
 // --------- type transformation -----------------------------------------------
@@ -339,8 +338,10 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {
339 338
         else if (mixinMember.isSuperAccessor) { // mixin super accessors
340 339
           val superAccessor = addMember(clazz, mixinMember.cloneSymbol(clazz)) setPos clazz.pos
341 340
           assert(superAccessor.alias != NoSymbol, superAccessor)
342  
-          val alias1 = rebindSuper(clazz, mixinMember.alias, mixinClass)
343  
-          superAccessor.asInstanceOf[TermSymbol] setAlias alias1
  341
+          rebindSuper(clazz, mixinMember.alias, mixinClass) match {
  342
+            case Some(alias1) => superAccessor.asInstanceOf[TermSymbol] setAlias alias1
  343
+            case None => unit.error(clazz.pos, "Member "+ mixinMember.alias +" of mixin "+ mixinClass +" is missing a concrete super implementation.")
  344
+          }
344 345
         }
345 346
         else if (mixinMember.isMethod && mixinMember.isModule && mixinMember.hasNoFlags(LIFTED | BRIDGE)) {
346 347
           // mixin objects: todo what happens with abstract objects?
7  src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -662,7 +662,7 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R
662 662
           }
663 663
 
664 664
           // Check the remainder for invalid absoverride.
665  
-          for (member <- rest ; if ((member hasFlag ABSOVERRIDE) && member.isIncompleteIn(clazz))) {
  665
+          for (member <- rest ; if (member.isAbstractOverride && member.isIncompleteIn(clazz))) {
666 666
             val other = member.superSymbol(clazz)
667 667
             val explanation =
668 668
               if (other != NoSymbol) " and overrides incomplete superclass member " + infoString(other)
@@ -756,11 +756,10 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R
756 756
 
757 757
       // 4. Check that every defined member with an `override` modifier overrides some other member.
758 758
       for (member <- clazz.info.decls)
759  
-        if ((member hasFlag (OVERRIDE | ABSOVERRIDE)) &&
760  
-            !(clazz.thisType.baseClasses exists (hasMatchingSym(_, member)))) {
  759
+        if (member.isAnyOverride && !(clazz.thisType.baseClasses exists (hasMatchingSym(_, member)))) {
761 760
           // for (bc <- clazz.info.baseClasses.tail) Console.println("" + bc + " has " + bc.info.decl(member.name) + ":" + bc.info.decl(member.name).tpe);//DEBUG
762 761
           unit.error(member.pos, member.toString() + " overrides nothing");
763  
-          member resetFlag OVERRIDE
  762
+          member resetFlag (OVERRIDE | ABSOVERRIDE)  // Any Override
764 763
         }
765 764
     }
766 765
 
2  src/compiler/scala/tools/nsc/typechecker/SuperAccessors.scala
@@ -121,7 +121,7 @@ abstract class SuperAccessors extends transform.Transform with transform.TypingT
121 121
       if (sym.isDeferred) {
122 122
         val member = sym.overridingSymbol(clazz);
123 123
         if (mix != tpnme.EMPTY || member == NoSymbol ||
124  
-            !((member hasFlag ABSOVERRIDE) && member.isIncompleteIn(clazz)))
  124
+            !(member.isAbstractOverride && member.isIncompleteIn(clazz)))
125 125
           unit.error(sel.pos, ""+sym.fullLocationString+" is accessed from super. It may not be abstract "+
126 126
                                "unless it is overridden by a member declared `abstract' and `override'");
127 127
       }
4  test/files/neg/t4134.check
... ...
@@ -0,0 +1,4 @@
  1
+t4134.scala:22: error: Member method f of mixin trait T2 is missing a concrete super implementation.
  2
+class Konkret extends T3
  3
+      ^
  4
+one error found
30  test/files/neg/t4134.scala
... ...
@@ -0,0 +1,30 @@
  1
+
  2
+
  3
+
  4
+trait T1 {
  5
+  def f: String
  6
+}                                                                               
  7
+
  8
+trait T2 extends T1 {
  9
+  abstract override def f: String = "goo"
  10
+  def something = super.f  // So the "abstract override" is needed
  11
+}                                                                               
  12
+
  13
+trait Q1 {
  14
+  def f: String = "bippy"
  15
+}                                                                               
  16
+
  17
+//trait T3 extends Q1 with T2 {
  18
+trait T3 extends T2 with Q1 {
  19
+  abstract override def f: String = super[Q1].f + " " + super[T2].f + " hoo"
  20
+}
  21
+
  22
+class Konkret extends T3
  23
+
  24
+object Test {
  25
+  def main(args: Array[String]): Unit = {
  26
+    val k = new Konkret
  27
+    println(k.f)
  28
+    println(k.something)
  29
+  }
  30
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.