Merge pull request #5730 from adriaanm/userdefined-apply-211

Allow user-defined apply/unapply method in case class companion

6 files changed, 242 insertions, 23 deletions

diff --git a/spec/05-classes-and-objects.md b/spec/05-classes-and-objects.md
index 69828ec7fe..65666e31cb 100644
--- a/spec/05-classes-and-objects.md
+++ b/spec/05-classes-and-objects.md
@@ -851,9 +851,8 @@ already a `val` or `var` modifier. Hence, an accessor
 definition for the parameter is [generated](#class-definitions).
 
 A case class definition of `$c$[$\mathit{tps}\,$]($\mathit{ps}_1\,$)$\ldots$($\mathit{ps}_n$)` with type
-parameters $\mathit{tps}$ and value parameters $\mathit{ps}$ implicitly
-generates an [extractor object](08-pattern-matching.html#extractor-patterns) which is
-defined as follows:
+parameters $\mathit{tps}$ and value parameters $\mathit{ps}$ implies
+the definition of a companion object, which serves as an [extractor object](08-pattern-matching.html#extractor-patterns). It has the following shape:
 
 ```scala
 object $c$ {
@@ -870,11 +869,13 @@ each $\mathit{xs}\_i$ denotes the parameter names of the parameter
 section $\mathit{ps}\_i$, and
 $\mathit{xs}\_{11}, \ldots , \mathit{xs}\_{1k}$ denote the names of all parameters
 in the first parameter section $\mathit{xs}\_1$.
-If a type parameter section is missing in the
-class, it is also missing in the `apply` and
-`unapply` methods.
-The definition of `apply` is omitted if class $c$ is
-`abstract`.
+If a type parameter section is missing in the class, it is also missing in the `apply` and `unapply` methods.
+
+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 case class definition contains an empty value parameter list, the
 `unapply` method returns a `Boolean` instead of an `Option` type and
@@ -887,9 +888,6 @@ def unapply[$\mathit{tps}\,$]($x$: $c$[$\mathit{tps}\,$]) = x ne null
 The name of the `unapply` method is changed to `unapplySeq` if the first
 parameter section $\mathit{ps}_1$ of $c$ ends in a
 [repeated parameter](04-basic-declarations-and-definitions.html#repeated-parameters).
-If a companion object $c$ exists already, no new object is created,
-but the `apply` and `unapply` methods are added to the existing
-object instead.
 
 A method named `copy` is implicitly added to every case class unless the
 class already has a member (directly defined or inherited) with that name, or the
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index ee64a6646f..69b8cb12e6 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -610,7 +610,15 @@ trait Namers extends MethodSynthesis {
       noDuplicates(selectors map (_.rename), AppearsTwice)
     }
 
-    def enterCopyMethod(copyDef: DefDef): Symbol = {
+    class CompleterWrapper(completer: TypeCompleter) extends TypeCompleter {
+      val tree = completer.tree
+
+      override def complete(sym: Symbol): Unit = {
+        completer.complete(sym)
+      }
+    }
+
+    def copyMethodCompleter(copyDef: DefDef): TypeCompleter = {
       val sym      = copyDef.symbol
       val lazyType = completerOf(copyDef)
 
@@ -629,13 +637,72 @@ trait Namers extends MethodSynthesis {
         )
       }
 
-      sym setInfo {
-        mkTypeCompleter(copyDef) { sym =>
-          assignParamTypes()
-          lazyType complete sym
+      mkTypeCompleter(copyDef) { sym =>
+        assignParamTypes()
+        lazyType complete sym
+      }
+    }
+
+    // for apply/unapply, which may need to disappear when they clash with a user-defined method of matching signature
+    def applyUnapplyMethodCompleter(un_applyDef: DefDef, companionContext: Context): TypeCompleter =
+      new CompleterWrapper(completerOf(un_applyDef)) {
+        override def complete(sym: Symbol): Unit = {
+          assert(sym hasAllFlags CASE | SYNTHETIC, sym.defString)
+
+          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.
+          // 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()
+
+          // 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 || {
+            // can't exclude deferred members using DEFERRED flag here (TODO: why?)
+            val userDefined = ownerInfo.memberBasedOnName(sym.name, BridgeFlags | SYNTHETIC)
+
+            (userDefined != NoSymbol) && {
+              assert(userDefined != sym)
+              val alts = userDefined.alternatives // could be just the one, if this member isn't overloaded
+              // don't compute any further `memberInfo`s if there's an error somewhere
+              alts.exists(_.isErroneous) || {
+                val self = companionContext.owner.thisType
+                val memberInfo = self.memberInfo(sym)
+                alts.exists(alt => !alt.isDeferred && (self.memberInfo(alt) matches memberInfo))
+              }
+            }
+          }
+
+          if (suppress) {
+            sym setInfo ErrorType
+            sym setFlag IS_ERROR
+
+            // Don't unlink in an error situation to generate less confusing error messages.
+            // Ideally, our error reporting would distinguish overloaded from recursive user-defined apply methods without signature,
+            // but this would require some form of partial-completion of method signatures, so that we can
+            // know what the argument types were, even though we can't complete the result type, because
+            // we hit a cycle while trying to compute it (when we get here with locked user-defined symbols, we
+            // are in the complete for that symbol, and thus the locked symbol has not yet received enough info;
+            // I hesitate to provide more info, because it would involve a WildCard or something for its result type,
+            // which could upset other code paths)
+            if (!scopePartiallyCompleted)
+              companionContext.scope.unlink(sym)
+          }
         }
       }
-    }
 
     def completerOf(tree: Tree): TypeCompleter = {
       val mono = namerOf(tree.symbol) monoTypeCompleter tree
@@ -697,11 +764,15 @@ trait Namers extends MethodSynthesis {
         val bridgeFlag = if (mods hasAnnotationNamed tpnme.bridgeAnnot) BRIDGE | ARTIFACT else 0
         val sym = assignAndEnterSymbol(tree) setFlag bridgeFlag
 
-        if (name == nme.copy && sym.isSynthetic)
-          enterCopyMethod(tree)
-        else
-          sym setInfo completerOf(tree)
-    }
+        val completer =
+          if (sym hasFlag SYNTHETIC) {
+            if (name == nme.copy) copyMethodCompleter(tree)
+            else if (sym hasFlag CASE) applyUnapplyMethodCompleter(tree, context)
+            else completerOf(tree)
+          } else completerOf(tree)
+
+        sym setInfo completer
+      }
 
     def enterClassDef(tree: ClassDef) {
       val ClassDef(mods, _, _, impl) = tree
@@ -1351,7 +1422,7 @@ trait Namers extends MethodSynthesis {
 
             val defTpt =
               // don't mess with tpt's of case copy default getters, because assigning something other than TypeTree()
-              // will break the carefully orchestrated naming/typing logic that involves enterCopyMethod and caseClassCopyMeth
+              // will break the carefully orchestrated naming/typing logic that involves copyMethodCompleter and caseClassCopyMeth
               if (meth.isCaseCopy) TypeTree()
               else {
                 // If the parameter type mentions any type parameter of the method, let the compiler infer the
diff --git a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala b/src/reflect/scala/reflect/internal/tpe/FindMembers.scala
index 83a5d23e7c..1b00815bca 100644
--- a/src/reflect/scala/reflect/internal/tpe/FindMembers.scala
+++ b/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 = {}
+  }
+
 }
diff --git a/test/files/neg/userdefined_apply.check b/test/files/neg/userdefined_apply.check
new file mode 100644
index 0000000000..c8c8976f5f
--- /dev/null
+++ b/test/files/neg/userdefined_apply.check
@@ -0,0 +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:14: error: overloaded method apply needs result type
+  private def apply(x: Int) = if (x > 0) ClashRecNoSig(1) else ???
+                                         ^
+userdefined_apply.scala:21: error: overloaded method apply needs result type
+  private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ???
+                                         ^
+userdefined_apply.scala:28: error: overloaded method apply needs result type
+  private def apply(x: Boolean) = if (x) NoClashOverload(1) else apply("")
+                                                        ^
+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
diff --git a/test/files/neg/userdefined_apply.scala b/test/files/neg/userdefined_apply.scala
new file mode 100644
index 0000000000..0a0d960b39
--- /dev/null
+++ b/test/files/neg/userdefined_apply.scala
@@ -0,0 +1,57 @@
+object ClashOverloadNoSig {
+  // error: overloaded method apply needs result type
+  private def apply(x: Int) = if (x > 0) new ClashOverloadNoSig(x) else apply("")
+
+  def apply(x: String): 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 ???
+}
+
+case class ClashRecNoSig private(x: Int)
+
+object NoClashNoSig {
+  // error: overloaded method apply needs result type
+  private def apply(x: Boolean) = if (x) NoClashNoSig(1) else ???
+}
+
+case class NoClashNoSig private(x: Int)
+
+object NoClashOverload {
+  // error: overloaded method apply needs result type
+  private def apply(x: Boolean) = if (x) NoClashOverload(1) else apply("")
+
+  def apply(x: String): 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)
diff --git a/test/files/pos/userdefined_apply.scala b/test/files/pos/userdefined_apply.scala
new file mode 100644
index 0000000000..e29f9f5141
--- /dev/null
+++ b/test/files/pos/userdefined_apply.scala
@@ -0,0 +1,54 @@
+// NOTE: the companion inherits a public apply method from Function1!
+case class NeedsCompanion private (x: Int)
+
+object ClashNoSig { // ok
+  private def apply(x: Int) = if (x > 0) new ClashNoSig(x) else ???
+}
+case class ClashNoSig private (x: Int)
+
+
+object Clash {
+  private def apply(x: Int) = if (x > 0) new Clash(x) else ???
+}
+case class Clash private (x: Int)
+
+object ClashSig {
+  private def apply(x: Int): ClashSig = if (x > 0) new ClashSig(x) else ???
+}
+case class ClashSig private (x: Int)
+
+object ClashOverload {
+  private def apply(x: Int): ClashOverload = if (x > 0) new ClashOverload(x) else apply("")
+  def apply(x: String): ClashOverload = ???
+}
+case class ClashOverload private (x: Int)
+
+object NoClashSig {
+  private def apply(x: Boolean): NoClashSig = if (x) NoClashSig(1) else ???
+}
+case class NoClashSig private (x: Int)
+
+object NoClashOverload {
+  // needs full sig
+  private def apply(x: Boolean): NoClashOverload = if (x) NoClashOverload(1) else apply("")
+  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)