Fix #4986: Support implicitNotFound on parameters #9957
Conversation
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
from
d95fff7 to
2ae6999
Compare
| val paramsNames = ap.typeSymbol.typeParams.map(_.name.unexpandedName.toString) | ||
| val paramsValues = ap.widenExpr.dropDependentRefinement.argInfos | ||
| paramsNames.zip(paramsValues) ++ extractTypeParams(tycon) | ||
| case RefinedType(parent, _, refinedInfo) => extractTypeParams(parent) ++ extractTypeParams(refinedInfo) |
There was a problem hiding this comment.
Are there any test case for refinement types?
There was a problem hiding this comment.
While trying to test that deeper I realized that there are potentially some more cases but I'm not sure how some of them should be handled. E.g. the piece of code below
@annotation.implicitNotFound("There's no Foo[${A}]") trait Foo[A]
implicitly[Foo]
- when compiled with scala 2.13 simply returns the error
error: trait Foo takes type parameters
- when compiled with dotty from latest master, throws an exception inside the compiler and kills the REPL
- when compiled from this branch, prints the user's custom error message but substituting an empty string for the variable
There's no Foo[]
Should the behaviour in such a case be exactly the same as for scala 2.13? This is not clear for me because if no message is defined by the user, the behaviour is not the same. E.g. for this code
trait Bar[A]
implicitly[Bar]
the results are as follows:
- scala 2.13:
error: trait Bar takes type parameters
- dotty's latest master and this PR:
no implicit argument of type Bar was found for parameter ev of method implicitly in object DottyPredef
Should the same error be reported as in scala 2.13 then or does implicitly[Foo] without a type parameter for Foo actually make sense in dotty (I assume by treating a type constructor as a type lambda)? If yes then how should the type variable in Foo be substituted while printing the message?
There was a problem hiding this comment.
@smarter any thoughts on this?
There was a problem hiding this comment.
implicitly takes a type parameter T upper-bounded by Any, so type constructors like Foo are not valid arguments for that method (in Dotty we can use an upper-bound of AnyKind to allow that sort of things), so Scala 2.13 is correct to complain (and in fact Dotty also complains if I try to write identity[List](???), it's only when the parameter list is implicit that it does something weird). So we should open an issue to fix Dotty's behavior here but this can be done independently from this PR
There was a problem hiding this comment.
Issue reported, missing tests added.
There was a problem hiding this comment.
@smarter Is it an expected behaviour that a method defined inside a refinement does not have a symbol assigned to it? A comment in Denotations.scala claims that
* A denotation might refer to `NoSymbol`. This is the case if the denotation
* was produced from a disjunction of two denotations with different symbols
* and there was no common symbol in a superclass that could substitute for
* both symbols.
but it's not clear for me if this should be the only case.
E.g. in the code below
trait Foo[A]
type Fooable[A] = {
def foo(implicit @annotation.implicitNotFound("There's no Foo[${A}]") ev: Foo[A]): Any
}
val x: Fooable[Long] = null
x.foo
the tree corresponding to x.foo has no symbol so the solution using tree.symbol.paramSymss to extract the annotation on an argument from the method's definition doesn't work.
When the code above is compiled with scala 2.13, the error message gets resolved properly but the variable is not substituted
cmd3.sc:1: There's no Foo[]
val res3 = x.foo
^
Compilation Failed
There was a problem hiding this comment.
Is it an expected behaviour that a method defined inside a refinement does not have a symbol assigned to it?
Yes, the comment in Denotations is incomplete, structural members (members of refinement types) don't have a symbol (a symbol is supposed to have an owner, but a structural member doesn't have a parent which could be used as an owner, and we can't create a new symbol with a dummy owner every time we see a structural member, because two types with the same refinements are equal so it doesn't make sense that they would have members with different symbols)
There was a problem hiding this comment.
I think the simplest solution is to disallow implicitNotFound annotations on structural members.
There was a problem hiding this comment.
In terms of compatibility with scala 2.13 then: should we raise an error in such a case or rather simply ignore the annotation and raise a warning?
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
2 times, most recently
from
869b03c to
6c45f5c
Compare
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
2 times, most recently
from
2b541fe to
dec6e61
Compare
|
|
||
| def msg(shortForm: String)(headline: String = shortForm) = arg match { | ||
| def missingArgMsg(arg: Tree, pt: Type, where: String, paramSymWithMethodTree: Option[(Symbol, Tree)] = None)(using Context): String = { |
There was a problem hiding this comment.
This method is getting quite complex with many arguments, I think it's worth adding a documentation comment explaining what this does and what each parameter is for.
| msg(userDefined.getOrElse( | ||
| em"no implicit argument of type $pt was found${location("for")}"))() ++ | ||
| hiddenImplicitsAddendum | ||
| val userDefinedParamMessage = paramSymWithMethodTree.flatMap{ (sym, tree) => |
There was a problem hiding this comment.
Here I would add a comment with an example of an implicitNotFound on a parameter so the reader knows what we're looking for.
| } | ||
| } | ||
|
|
||
| val userDefinedTypeMessage = userDefinedMsg(pt.typeSymbol, defn.ImplicitNotFoundAnnot).map{ rawMsg => |
There was a problem hiding this comment.
Same here, I would add a comment with an example of an implicitNotFound on a type definition
| ) | ||
| } | ||
|
|
||
| val shortMessage = userDefinedParamMessage.orElse(userDefinedTypeMessage).getOrElse( |
There was a problem hiding this comment.
It looks like we either use the userDefinedParamMessage or the userDefinedTypeMessage but never both, so they could be lazy vals or defs instead of vals.
| wtp.paramNames.lazyZip(wtp.paramInfos).lazyZip(args).foreach { (paramName, formal, arg) => | ||
| arg.tpe match { | ||
| case failure: SearchFailureType => | ||
| val paramSymWithMethodTree = if tree.symbol.exists then Some((paramSyms(paramName), tree)) else None | ||
|
|
||
| report.error( |
There was a problem hiding this comment.
In many situation we will recover from an error without ever showing it to the user (e.g., in implicit search we might find another candidate), so it's important to delay computing anything we want to show to the user until we know we need it. Here things are a bit subtle but missingArgMsg returns a String and report.error takes as input a Message, this works because there is an implicit conversion from String to Message: https://github.com/lampepfl/dotty/blob/22c23a5cc648650bb6380dec0c11a5fdc34e1d73/compiler/src/dotty/tools/dotc/reporting/Message.scala#L15
Note that this conversion takes a => String as input and which means the call to missingArgMsg will be delayed, which is what we want. But it would be even better if paramSyms and paramSymWithMethodTree were also delayed, which can be achieved by computing them inside missingArgMsg (or making them both lazy vals)
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
from
dec6e61 to
8cea91a
Compare
| /** | ||
| * Extracting the message from a method parameter, e.g. in |
There was a problem hiding this comment.
The documentation style in the compiler is to start at the first line, and subsequent lines start with two spaces after the * to align the message:
/** Extracting the message from a method parameter, e.g. in
* ....
* ....
*/
| def userDefinedParamMessage = paramSymWithMethodTree.flatMap{ (sym, tree) => | ||
| userDefinedMsg(sym, defn.ImplicitNotFoundAnnot).map{ rawMsg => |
There was a problem hiding this comment.
Space before braces (same in a few other places):
| def userDefinedParamMessage = paramSymWithMethodTree.flatMap{ (sym, tree) => | |
| userDefinedMsg(sym, defn.ImplicitNotFoundAnnot).map{ rawMsg => | |
| def userDefinedParamMessage = paramSymWithMethodTree.flatMap { (sym, tree) => | |
| userDefinedMsg(sym, defn.ImplicitNotFoundAnnot).map { rawMsg => |
| lazy val paramSyms = tree.symbol.paramSymss.flatten.map(sym => sym.name -> sym).toMap | ||
| def paramSymWithMethodTree(paramName: TermName) = if tree.symbol.exists then Some((paramSyms(paramName), tree)) else None |
There was a problem hiding this comment.
paramSyms could be a non-lazy local val defined inside paramSymWithMethodTree now.
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
from
8cea91a to
6052353
Compare
|
@smarter reworks done |
|
|
||
| /** Extracting the message from a type, e.g. in | ||
| * | ||
| * implicit @annotation.implicitNotFound |
| */ | ||
| def userDefinedParamMessage = paramSymWithMethodTree.flatMap { (sym, tree) => | ||
| userDefinedMsg(sym, defn.ImplicitNotFoundAnnot).map { rawMsg => | ||
| val paramSyms = typeVariablesInScope(sym) |
There was a problem hiding this comment.
"type variables" in the compiler usually refer the variables the compiler internally creates when performing type inference so it's better to not use that term, typeParamsInScope would be fine, and since this method is only used once you could inline it here actually: val typeParamsInScope = ...
| * | ||
| * trait Foo | ||
| * | ||
| * def foo(implicit @annotation.implicitNotFound foo: Foo) |
There was a problem hiding this comment.
I presume this should be@implicitNotFound("some message")
| def userDefinedParamMessage = paramSymWithMethodTree.flatMap { (sym, tree) => | ||
| userDefinedMsg(sym, defn.ImplicitNotFoundAnnot).map { rawMsg => | ||
| val paramSyms = typeVariablesInScope(sym) | ||
| val paramTypeRefs = paramSyms.map(_.typeRef.typeConstructor) |
There was a problem hiding this comment.
I don't think the use of typeConstructor here is needed.
| val methodTypeParams = tree.symbol.paramSymss.flatten.filter(_.isType) | ||
| val methodTypeArgs = targs.map(_.tpe) | ||
| paramTypeRefs.map(_.asSeenFrom(methodOwnerType, methodOwner).subst(methodTypeParams, methodTypeArgs)) | ||
| case _ => paramTypeRefs |
There was a problem hiding this comment.
asSeenFrom is also needed when the tree isn't a polymorphic method:
import scala.annotation.implicitNotFound
class Foo[A] {
def foo(implicit @implicitNotFound("A = ${A}") x: String): Unit = ???
}
object Test {
val x: Foo[Int] = ???
x.foo
}Currently outputs A = A instead of A = Int
| tpe match { | ||
| case ap @ AppliedType(tycon, _) => | ||
| val paramsNames = ap.typeSymbol.typeParams.map(_.name.unexpandedName.toString) | ||
| val paramsValues = ap.widenExpr.dropDependentRefinement.argInfos |
There was a problem hiding this comment.
I don't think widenExpr.dropDependentRefinement is useful here.
| * def foo(foo: Foo) | ||
| */ | ||
| def userDefinedTypeMessage = userDefinedMsg(pt.typeSymbol, defn.ImplicitNotFoundAnnot).map { rawMsg => | ||
| def extractTypeParams(tpe: Type): List[(String, Type)] = |
There was a problem hiding this comment.
I realize this was broken before so doesn't really need to be addressed in this PR, but just looking at the type parameters of the type isn't enough, e.g.:
import scala.annotation.implicitNotFound
class Outer[A] {
@implicitNotFound("A = ${A} -- B = ${B}")
class Inner[B]
}
object Test {
val x: Outer[Int] = new Outer[Int]
implicitly[x.Inner[Double]]
}I'd expect A = Int -- B = Double but instead I get A = -- B = Double, Scala 2 has the same issue! I think a solution based on asSeenFrom like what is done in userDefinedParamMessage would make more sense, but I haven't tried it.
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
from
6052353 to
8fc8aaa
Compare
|
@smarter reworks done. Using |
|
Nice!
Since it's only used in error messages, perhaps in ErrorReporting.scala ? In fact, maybe other parts of the code like missingArgMsg could be moved to that file too. |
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
2 times, most recently
from
2c8dedd to
391719d
Compare
|
@smarter refactor done and tests fixed |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9957/ to see the changes. Benchmarks is based on merging with master (7eac235) |
| val methodOwnerType: Type = applTree match { | ||
| case Select(qual, _) => qual.tpe | ||
| case TypeApply(Select(qual, _), _) => qual.tpe | ||
| case _ => methodOwner.typeRef | ||
| } |
There was a problem hiding this comment.
You can use tpd.decomposeCall to extract the method that is applied (as well as the type arguments which are used below), and then tpd.qualifier to get the prefix.
| * def foo(implicit foo: Foo): Any = ??? | ||
| */ | ||
| private def userDefinedImplicitNotFoundTypeMessage = userDefinedMsg(pt.typeSymbol, defn.ImplicitNotFoundAnnot).map { rawMsg => | ||
| val substituteType = (_: Type).asSeenFrom(pt, pt.typeSymbol) |
There was a problem hiding this comment.
The second argument of asSeenFrom is supposed to be a class so pt.classSymbol is more appropriate (if pt is a class type it's the same as typeSymbol, it'll be different for a type alias for example).
Also fixes some complex cases of resolving type variables in `implicitNotFound` message for annotations put on type definitions
prolativ
force-pushed
the
implicit-not-found-on-parameters
branch
from
391719d to
1760da3
Compare
|
@smarted reworks done. Is there a way to rerun only a part of the CI workflow? The normal tests seem not to have even started properly but all the other parts succeeded |
|
If the tests haven't started it means all our runners are busy running other jobs, the only thing to do is wait. |
|
Now test_bootstrapped randomly failed with |
|
Looks like the sbt cache was corrupted, I've deleted it on the server, let's merge since all the tests passed at least once. |
Porting the feature from Scala 2 with a slight improvement on handling nested parameterized types.
Partially based on #6703