From a5cd601dacb94d65ba035f5ca9fd3c2064668b70 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Mon, 24 Feb 2014 23:28:54 -0800 Subject: SI-8197 clarify overloading resolution with default args This commit was co-authored with Lukas. His analysis is below. If there are multiple applicable alternatives, drop those that use default arguments. This is done indirectly by checking applicability based on arity. TODO: this `namesMatch` business is not spec'ed, and is the wrong fix for SI-4592. We should instead clarify what the spec means by "typing each argument with an undefined expected type". What does typing a named argument entail when we don't know what the valid parameter names are? (Since we're doing overload resolution, there are multiple alternatives that can define different names.) Luckily, the next step checks applicability to the individual alternatives, so it knows whether an assignment is: - a valid named argument - a well-typed assignment (which doesn't necessarily have type `Unit`!) - an error (e.g., rhs does not refer to a variable) I propose the following solution (as a TODO): check whether a named argument (when typing it in `doTypedApply`) could be interpreted as an assign; `infer.checkNames` should use the type of the well-typed assignment instead of `Unit`. Lukas's analysis: 990fa04 misunderstood the spec of overloading resolution with defaults. it should not discard applicable alternatives that define defaults, but only those that use defaults in the given invocation. bugs were shadowed because the refactoring used `alt.hasDefault` to check whether the alternative has some parameters with defaults, but this never worked. d5bb19f fixed that part by checking the flags of parameters, which fixed some but but un-shadowed others: ``` object t { def f(x: String = "") = 1; def f(x: Object) = 2 } scala> t.f("") // should return 1, but returns 2 after d5bb19f ``` The commit message of d5bb19f also mentions that its test "should fail to compile according to SI-4728", which is another misunderstanding. ``` class A; class B extends A object t { def f(x: A = null) = 1; def f(x: B*) = 2 } t.f() ``` This should return `2`: both alternatives are applicable, so the one that uses a default is eliminated, and we're left with the vararg one. SI-4728 is different in that it uses explicit parameters, `t.f(new B)` is ambiguous. --- .../scala/tools/nsc/typechecker/Infer.scala | 44 ++++++++++++++++------ .../scala/tools/nsc/typechecker/Typers.scala | 19 ++++++++-- test/files/run/t8197.scala | 7 +++- 3 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/compiler/scala/tools/nsc/typechecker/Infer.scala b/src/compiler/scala/tools/nsc/typechecker/Infer.scala index b42113c84f..fc0e2c7c80 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Infer.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Infer.scala @@ -578,13 +578,21 @@ trait Infer extends Checkable { */ private[typechecker] def isApplicableBasedOnArity(tpe: Type, argsCount: Int, varargsStar: Boolean, tuplingAllowed: Boolean): Boolean = followApply(tpe) match { case OverloadedType(pre, alts) => + // followApply may return an OverloadedType (tpe is a value type with multiple `apply` methods) alts exists (alt => isApplicableBasedOnArity(pre memberType alt, argsCount, varargsStar, tuplingAllowed)) case _ => val paramsCount = tpe.params.length + // simpleMatch implies we're not using defaults val simpleMatch = paramsCount == argsCount val varargsTarget = isVarArgsList(tpe.params) + + // varargsMatch implies we're not using defaults, as varargs and defaults are mutually exclusive def varargsMatch = varargsTarget && (paramsCount - 1) <= argsCount + // another reason why auto-tupling is a bad idea: it can hide the use of defaults, so must rule those out explicitly def tuplingMatch = tuplingAllowed && eligibleForTupleConversion(paramsCount, argsCount, varargsTarget) + // varargs and defaults are mutually exclusive, so not using defaults if `varargsTarget` + // we're not using defaults if there are (at least as many) arguments as parameters (not using exact match to allow for tupling) + def notUsingDefaults = varargsTarget || paramsCount <= argsCount // A varargs star call, e.g. (x, y:_*) can only match a varargs method // with the same number of parameters. See SI-5859 for an example of what @@ -592,7 +600,7 @@ trait Infer extends Checkable { if (varargsStar) varargsTarget && simpleMatch else - simpleMatch || varargsMatch || tuplingMatch + simpleMatch || varargsMatch || (tuplingMatch && notUsingDefaults) } private[typechecker] def followApply(tp: Type): Type = tp match { @@ -633,7 +641,7 @@ trait Infer extends Checkable { if (pos == -1) { if (positionalAllowed) { // treat assignment as positional argument argPos(index) = index - res = UnitTpe + res = UnitTpe // TODO: this is a bit optimistic, the name may not refer to a mutable variable... } else // unknown parameter name namesOK = false } else if (argPos.contains(pos)) { // parameter specified twice @@ -1316,21 +1324,33 @@ trait Infer extends Checkable { * @param varargsStar true if the call site has a `: _*` attached to the last argument */ private def overloadsToConsiderBySpecificity(eligible: List[Symbol], argtpes: List[Type], varargsStar: Boolean): List[Symbol] = { + // TODO spec: this namesMatch business is not spec'ed, and is the wrong fix for SI-4592 + // we should instead clarify what the spec means by "typing each argument with an undefined expected type". + // What does typing a named argument entail when we don't know what the valid parameter names are? + // (Since we're doing overload resolution, there are multiple alternatives that can define different names.) + // Luckily, the next step checks applicability to the individual alternatives, so it knows whether an assignment is: + // 1) a valid named argument + // 2) a well-typed assignment + // 3) an error (e.g., rhs does not refer to a variable) + // + // For now, the logic is: // If there are any foo=bar style arguments, and any of the overloaded - // methods has a parameter named `foo`, then only those methods are considered. - val namesMatch = namesOfNamedArguments(argtpes) match { + // methods has a parameter named `foo`, then only those methods are considered when we must disambiguate. + def namesMatch = namesOfNamedArguments(argtpes) match { case Nil => Nil case names => eligible filter (m => names forall (name => m.info.params exists (p => paramMatchesName(p, name)))) } - if (namesMatch.nonEmpty) - namesMatch - else if (eligible.isEmpty || eligible.tail.isEmpty) - eligible + if (eligible.isEmpty || eligible.tail.isEmpty) eligible else - eligible filter (alt => - !alt.info.params.exists(_.hasDefault) // run/t8197b first parameter list only! - && isApplicableBasedOnArity(alt.tpe, argtpes.length, varargsStar, tuplingAllowed = true) - ) + namesMatch match { + case namesMatch if namesMatch.nonEmpty => namesMatch // TODO: this has no basis in the spec, remove! + case _ => + // If there are multiple applicable alternatives, drop those using default arguments. + // This is done indirectly by checking applicability based on arity in `isApplicableBasedOnArity`. + // If defaults are required in the application, the arities won't match up exactly. + // TODO: should we really allow tupling here?? (If we don't, this is the only call-site with `tuplingAllowed = true`) + eligible filter (alt => isApplicableBasedOnArity(alt.tpe, argtpes.length, varargsStar, tuplingAllowed = true)) + } } /** Assign `tree` the type of an alternative which is applicable diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 8a3ceb3aca..153e32f3a3 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -3214,10 +3214,23 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper args.map { case arg @ AssignOrNamedArg(Ident(name), rhs) => // named args: only type the righthand sides ("unknown identifier" errors otherwise) - val rhs1 = typedArg0(rhs) // the assign is untyped; that's ok because we call doTypedApply - val arg1 = treeCopy.AssignOrNamedArg(arg, arg.lhs, rhs1) - (arg1, NamedType(name, rhs1.tpe.deconst)) + val typedRhs = typedArg0(rhs) + val argWithTypedRhs = treeCopy.AssignOrNamedArg(arg, arg.lhs, typedRhs) + + // TODO: SI-8197/SI-4592: check whether this named argument could be interpreted as an assign + // infer.checkNames must not use UnitType: it may not be a valid assignment, or the setter may return another type from Unit + // + // var typedAsAssign = true + // val argTyped = silent(_.typedArg(argWithTypedRhs, amode, BYVALmode, WildcardType)) orElse { errors => + // typedAsAssign = false + // argWithTypedRhs + // } + // + // TODO: add an assignmentType field to NamedType, equal to: + // assignmentType = if (typedAsAssign) argTyped.tpe else NoType + + (argWithTypedRhs, NamedType(name, typedRhs.tpe.deconst)) case arg @ treeInfo.WildcardStarArg(repeated) => val arg1 = typedArg0(arg) (arg1, RepeatedType(arg1.tpe.deconst)) diff --git a/test/files/run/t8197.scala b/test/files/run/t8197.scala index 5ca67088de..910a3ebc83 100644 --- a/test/files/run/t8197.scala +++ b/test/files/run/t8197.scala @@ -1,7 +1,7 @@ -// NOTE: according to SI-4728, this shouldn't even compile... +// SI-8197, see also SI-4592 and SI-4728 class A class B -// default arguments do not participate in overload resolution + class Foo(val x: A = null) { def this(bla: B*) { this(new A) @@ -9,5 +9,8 @@ class Foo(val x: A = null) { } object Test extends App { + // both constructors of `Foo` are applicable. Overloading resolution + // will eliminate the alternative that uses a default argument, therefore + // the vararg constructor is chosen. assert((new Foo).x != null) } -- cgit v1.2.3