Improvements based on reviews by Lukas & Jason

spec/05-classes-and-objects.md

@@ -873,10 +873,9 @@ If a type parameter section is missing in the class, it is also missing in the `
 
 If the companion object $c$ is already defined,
 the  `apply` and `unapply` methods are added to the existing object.
+If the object $c$ already has a [matching](#definition-matching)
+`apply` (or `unapply`) member, no new definition is added.
 The definition of `apply` is omitted if class $c$ is `abstract`.
-If the object $c$ already defines a [matching](#definition-matching) member of the
-same name as the synthetic member to be added, the synthetic member
-is not added (overloading or mutual recursion is allowed, however).
 
 If the case class definition contains an empty value parameter list, the
 `unapply` method returns a `Boolean` instead of an `Option` type and

src/compiler/scala/tools/nsc/typechecker/Namers.scala

@@ -649,23 +649,39 @@ trait Namers extends MethodSynthesis {
         override def complete(sym: Symbol): Unit = {
           super.complete(sym)
 
+          // owner won't be locked
+          val ownerInfo = companionContext.owner.info
+
           // If there's a same-named locked symbol, we're currently completing its signature.
-          // This means it (may) refer to us, and is thus either overloaded or recursive without a signature.
-          // rule out locked symbols from the owner.info.member call
-          val scopePartiallyCompleted =
-            companionContext.scope.lookupAll(sym.name).exists(existing => existing != sym && existing.hasFlag(LOCKED))
-
-          val suppress =
-            scopePartiallyCompleted || {
-            val userDefined = companionContext.owner.info.member(sym.name).filter(_ != sym)
-            (userDefined != NoSymbol) && {
-              userDefined.info match {
+          // If `scopePartiallyCompleted`, the program is known to have a type error, since
+          // this means a user-defined method is missing a result type while its rhs refers to `sym` or an overload.
+          // This is an error because overloaded/recursive methods must have a result type.
+          // The method would be overloaded if its signature, once completed, would not match the synthetic method's,
+          // or recursive if it turned out we should unlink our synthetic method (matching sig).
+          // In any case, error out. We don't unlink the symbol so that `symWasOverloaded` says yes,
+          // which would be wrong if the method is in fact recursive, but it seems less confusing.
+          val scopePartiallyCompleted = new HasMember(ownerInfo, sym.name, BridgeFlags | SYNTHETIC, LOCKED).apply()
+
+          assert(sym hasAllFlags CASE | SYNTHETIC, sym.defString)
+
+          // Check `scopePartiallyCompleted` first to rule out locked symbols from the owner.info.member call,
+          // as FindMember will call info on a locked symbol (while checking type matching to assemble an overloaded type),
+          // and throw a TypeError, so that we are aborted.
+          // Do not consider deferred symbols, as suppressing our concrete implementation would be an error regardless
+          // of whether the signature matches (if it matches, we omitted a valid implementation, if it doesn't,
+          // we would get an error for the missing implementation it isn't implemented by some overload other than our synthetic one)
+          val suppress = scopePartiallyCompleted || {
+            val userDefined = ownerInfo.memberBasedOnName(sym.name, BridgeFlags | SYNTHETIC) // can't exclude deferred members using DEFERRED flag here (TODO: why?)
+            (userDefined != NoSymbol) && !userDefined.isDeferred && {
+              assert(userDefined != sym)
+              val self = companionContext.owner.thisType
+              self.memberInfo(userDefined) match {
                 // TODO: do we have something for this already? the synthetic symbol can't be overloaded, right?
                 case OverloadedType(pre, alternatives) =>
                   // pre probably relevant because of inherited overloads?
-                  alternatives.exists(_.isErroneous) || alternatives.exists(alt => pre.memberInfo(alt) matches pre.memberInfo(sym))
+                  alternatives.exists(_.isErroneous) || alternatives.exists(alt => pre.memberInfo(alt) matches self.memberInfo(sym))
                 case tp =>
-                  (tp eq ErrorType) || tp.matches(sym.info)
+                  (tp eq ErrorType) || tp.matches(self.memberInfo(sym))
               }
             }
           }
@@ -748,8 +764,6 @@ trait Namers extends MethodSynthesis {
         val bridgeFlag = if (mods hasAnnotationNamed tpnme.bridgeAnnot) BRIDGE | ARTIFACT else 0
         val sym = assignAndEnterSymbol(tree) setFlag bridgeFlag
 
-        // copy/apply/unapply synthetics are added using the addIfMissing mechanism,
-        // which ensures the owner has its preliminary info (we may add another decl here)
         val completer =
           if (sym hasFlag SYNTHETIC) {
             if (name == nme.copy) copyMethodCompleter(tree)

src/reflect/scala/reflect/internal/tpe/FindMembers.scala

@@ -285,4 +285,18 @@ trait FindMembers {
       initBaseClasses.head.newOverloaded(tpe, members)
     }
   }
+
+  private[scala] final class HasMember(tpe: Type, name: Name, excludedFlags: Long, requiredFlags: Long) extends FindMemberBase[Boolean](tpe, name, excludedFlags, requiredFlags) {
+    private[this] var _result = false
+    override protected def result: Boolean = _result
+
+    protected def shortCircuit(sym: Symbol): Boolean = {
+      _result = true
+      true // prevents call to addMemberIfNew
+    }
+
+    // Not used
+    protected def addMemberIfNew(sym: Symbol): Unit = {}
+  }
+
 }

test/files/neg/userdefined_apply.check

@@ -1,13 +1,25 @@
 userdefined_apply.scala:3: error: overloaded method apply needs result type
   private def apply(x: Int) = if (x > 0) new ClashOverloadNoSig(x) else apply("")
                                                                              ^
-userdefined_apply.scala:12: error: overloaded method apply needs result type
+userdefined_apply.scala:14: error: overloaded method apply needs result type
   private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ???
                                          ^
-userdefined_apply.scala:19: error: overloaded method apply needs result type
+userdefined_apply.scala:21: error: overloaded method apply needs result type
   private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ???
                                          ^
-userdefined_apply.scala:26: error: overloaded method apply needs result type
+userdefined_apply.scala:28: error: overloaded method apply needs result type
   private def apply(x: Boolean) = if (x) NoClashOverload(1) else apply("")
                                                         ^
-four errors found
+userdefined_apply.scala:45: error: recursive method apply needs result type
+case class NoClashNoSigPoly private(x: Int)
+           ^
+userdefined_apply.scala:39: error: NoClashNoSigPoly.type does not take parameters
+  def apply(x: T) = if (???) NoClashNoSigPoly(1) else ???
+                                             ^
+userdefined_apply.scala:57: error: recursive method apply needs result type
+case class ClashNoSigPoly private(x: Int)
+           ^
+userdefined_apply.scala:51: error: ClashNoSigPoly.type does not take parameters
+  def apply(x: T) = if (???) ClashNoSigPoly(1) else ???
+                                           ^
+8 errors found

test/files/neg/userdefined_apply.scala

@@ -8,6 +8,8 @@ object ClashOverloadNoSig {
 case class ClashOverloadNoSig private(x: Int)
 
 object ClashRecNoSig {
+  // TODO: status quo is that the error refers to an overloaded method, which is actually recursive
+  // (we should have unlinked the symbol in the `if(suppress)` part of `applyUnapplyMethodCompleter`)
   // error: recursive method apply needs result type
   private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ???
 }
@@ -29,3 +31,27 @@ object NoClashOverload {
 }
 
 case class NoClashOverload private(x: Int)
+
+
+class BaseNCNSP[T] {
+  // TODO: suppress the following error
+  // error: NoClashNoSigPoly.type does not take parameters
+  def apply(x: T) = if (???) NoClashNoSigPoly(1) else ???
+}
+
+object NoClashNoSigPoly extends BaseNCNSP[Boolean]
+// TODO: position error at definition of apply in superclass instead of on case clss
+// error: recursive method apply needs result type
+case class NoClashNoSigPoly private(x: Int)
+
+
+class BaseCNSP[T] {
+  // TODO: suppress the following error
+  // error: ClashNoSigPoly.type does not take parameters
+  def apply(x: T) = if (???) ClashNoSigPoly(1) else ???
+}
+
+object ClashNoSigPoly extends BaseCNSP[Int]
+// TODO: position error at definition of apply in superclass instead of on case clss
+// error: recursive method apply needs result type
+case class ClashNoSigPoly private(x: Int)

test/files/pos/userdefined_apply.scala

@@ -34,3 +34,21 @@ object NoClashOverload {
   def apply(x: String): NoClashOverload = ???
 }
 case class NoClashOverload private (x: Int)
+
+
+
+class BaseNCP[T] {
+  // error: overloaded method apply needs result type
+  def apply(x: T): NoClashPoly = if (???) NoClashPoly(1) else ???
+}
+
+object NoClashPoly extends BaseNCP[Boolean]
+case class NoClashPoly private(x: Int)
+
+
+class BaseCP[T] {
+  // error: overloaded method apply needs result type
+  def apply(x: T): ClashPoly = if (???) ClashPoly(1) else ???
+}
+object ClashPoly extends BaseCP[Int]
+case class ClashPoly private(x: Int)