Fix #3797: Add support for @implicitAmbiguous #4007

Jasper-M · 2018-02-16T12:17:08Z

There are no tests yet. Can I add a test to https://github.com/lampepfl/dotty/blob/master/compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala if this error doesn't have a dedicated case class yet?

Dotc will not warn like scalac when the user defined error message references a non-existent type parameter, but that's consistent with @implicitNotFound in dotty.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2018-02-16T15:01:41Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

-              raw,
-              pt.typeSymbol.typeParams.map(_.name.unexpandedName.toString),
-              pt.argInfos)
+          alt.tstate.commit()


We should try avoiding committing a TyperState with errors if possible. Instead you might be able to make a new Context with the correct TyperState to report the error: val altCtx = ctx.fresh.setTyperState(alt.tstate), and then move the code below to be in a method that takes an implicit ctx: Context and give it altCtx as argument.

Sounds good.

smarter · 2018-02-16T15:07:06Z

f this error doesn't have a dedicated case class yet?

I suggest just making a case class for the error, it doesn't have to be very fancy, e.g. it's okay if def explanation is left empty.

smarter · 2018-02-16T15:08:38Z

Dotc will not error out like scalac when the user defined error message references a non-existent type parameter, but that's consistent with @implicitnotfound in dotty.

Could you open an issue for that?

Jasper-M · 2018-02-16T15:47:48Z

I suggest just making a case class for the error, it doesn't have to be very fancy, e.g. it's okay if def explanation is left empty.

That would be a general ImplicitNotFound message then? Untangling the different subtypes of errors in missingArgMsg looks difficult to me, especially as I don't really understand the distinction between case arg: Trees.SearchFailureIdent[_] and case _ underneath it.

Jasper-M · 2018-02-16T15:52:29Z

Could you open an issue for that?

It's actually a warning apparently. But I'll open an issue.

smarter · 2018-02-16T15:55:29Z

That would be a general ImplicitNotFound message then?

Sure, but you could have an AmbiguousImplicit subclass or something.

smarter · 2018-02-16T16:35:43Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -696,22 +697,46 @@ trait Implicits { self: Typer =>
        }
    }
    def location(preposition: String) = if (where.isEmpty) "" else s" $preposition $where"
+    def userDefinedMessage(annot: Annotation, params: List[String], args: List[Type]): Option[String] =
+      for(Trees.Literal(Constant(raw: String)) <- annot.argument(0)) yield {


space after for

smarter · 2018-02-16T16:36:40Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+            case TypeApply(_, targs) if params.nonEmpty =>
+              def resolveTypes(targs: List[tpd.Tree])(implicit ctx: Context) = {
+                targs.map(a => fullyDefinedType(a.tpe, "type parameter", a.pos))
+              }


No need for braces since the body is an expression.

I thought it was better for readability in this case, but your call.

Jasper-M · 2018-02-19T16:30:05Z

I added tests, but I think it's better if I don't add a half assed custom Message case class in this pull request. I think it's ready for (final) review.

Blaisorblade · 2018-02-19T16:41:25Z

compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala

@@ -159,11 +159,11 @@ object ErrorReporting {
      TypeMismatch(found2, expected2, whyNoMatchStr(found, expected), postScript)
    }

-    /** Format `raw` implicitNotFound argument, replacing all
-     *  occurrences of `${X}` where `X` is in `paramNames` with the
+    /** Format `raw` implicitNotFound or implicitAmbiguous argument, replacing


Looks like this function might be useful for more annotations (@smarter mentioned a couple more ones on Gitter), as suggested by the new name you picked, but not by the comment. What do you think?

I sort of agree. I'm open to suggestions 😅

I think it's not worth overgeneralizing, we can change this when we need to.

I may have misinterpreted your question a little bit. It is useful for all annotations whose messages can reference type parameters, which could potentially be more than the 2 listed here, but not all annotations carrying a custom message. I'm open to suggestions for a better name.

Hm, I suspect the name is fine and would write "Format raw user-defined error message" in the doc. WDYT? Anyway, this isn't critical, just nice to have. (Warning, I've been told I'm a perfectionist).

Blaisorblade · 2018-02-19T16:45:56Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -696,22 +697,45 @@ trait Implicits { self: Typer =>
        }
    }
    def location(preposition: String) = if (where.isEmpty) "" else s" $preposition $where"
+    def userDefinedMessage(annot: Annotation, params: List[String], args: List[Type]): Option[String] =
+      for (Trees.Literal(Constant(raw: String)) <- annot.argument(0)) yield {


Wondering about the magic constant 0, I guess because the string is the 0th argument for all annotations currently supported? But if you try to make userDefinedMessage a good abstraction, maybe annot.argument(0) should move to the call site (even if, indeed, it gets duplicated).

Blaisorblade · 2018-02-19T17:34:02Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+          ambi.alt2.ref.symbol.getAnnotation(defn.ImplicitAmbiguousAnnot).map(
+            (_, ambi.alt2)
+          )
+        )


Hm, can this be abstracted with for { alt <- List(ambi.alt1, ambi.alt2) } .... alt.ref.symbol...? Or better, in terms of a new ambi.alts?

(Except if people feel this is too slow and we're in a performance-critical path).

Actually surprised the error only lists two implicits, apparently since this class' birth (in ea640a3) only the first two candidate implicits are reported.

Potential bug (not blocking the PR): if you have three equally good candidates that are ambiguous, the @implicitAmbiguous annotation will be ignored on the third candidate, which is against the specification in http://www.scala-lang.org/api/2.12.3/scala/annotation/implicitAmbiguous.html. If that bug can indeed be triggered (haven't tried, but it's not clear why that shouldn't happen), fixing it seems to require a refactoring of the search algorithm to return all the ambiguous candidates — which is, again, out of scope for the PR.

Yes I also thought about this. I think the Scalac implementation has the same limitation. I suspect the most realistic fix would probably be in the documentation of @implicitAmbiguous.

Ah, fair enough — this wasn't a well-known issue around here. Not sure how to specify it,
so for now I've opened scala/bug#10739?

For the PR, please go ahead!

Blaisorblade · 2018-02-20T13:29:23Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        val userDefined = maybeAnnot.flatMap { case (annot, alt) =>
+          val params = alt.ref.underlying match {
+            case lambda: TypeLambda => lambda.typeParams.map(_.paramName.unexpandedName.toString)
+            case _                  => Nil


@allanrenucci Couldn't quite review code from here on, please take a look. Otherwise LGTM.

allanrenucci · 2018-02-20T17:14:07Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        )
+        val userDefined = maybeAnnot.flatMap { case (annot, alt) =>
+          val params = alt.ref.underlying match {
+            case lambda: TypeLambda => lambda.typeParams.map(_.paramName.unexpandedName.toString)


I think you can do:

case p: PolyType => p.paramNames.map(_.toString)

allanrenucci · 2018-02-20T17:48:57Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

-              pt.argInfos)
+          val args = alt.tree match {
+            case TypeApply(_, targs) if params.nonEmpty =>
+              def resolveTypes(targs: List[tpd.Tree])(implicit ctx: Context) =


You can remove the tpd prefix

allanrenucci · 2018-02-20T17:54:15Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

          }
+          userDefinedMessage(annot, params, args)
+        }
+        userDefined.map(msg(_)()).getOrElse(


I think here .map(msg(_)()) does nothing and can be removed

That's very likely. Is the same true for the ImplicitNotFound case?

Actually I am not sure anymore. I revert what I said before. Let's be conservative and also wrap the user defined message in msg(_)()

allanrenucci · 2018-02-20T17:55:14Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

-              pt.typeSymbol.typeParams.map(_.name.unexpandedName.toString),
-              pt.argInfos)
+          val args = alt.tree match {
+            case TypeApply(_, targs) if params.nonEmpty =>


Not sure it is enough. See https://github.com/scala/scala/blob/8ad55a4ed8424aa6da372d3b6e41750f3d098523/src/compiler/scala/tools/nsc/typechecker/ContextErrors.scala#L1311 in scalac

allanrenucci · 2018-02-20T18:36:49Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+          ambi.alt2.ref.symbol.getAnnotation(defn.ImplicitAmbiguousAnnot).map(
+            (_, ambi.alt2)
+          )
+        )


I would do it the other way around. First look for a user defined annotation message and then try try to recover type params only if needed:

def userDefinedMsg(sym: Symbol, cls: Symbol) = for { ann <- symbol.getAnnotation(cls) Trees.Literal(Constant(msg: String)) <- ann.argument(0) } yield msg object AmbiguousImplicitMsg { def unapply(search: SearchSuccess) = userDefinedMsg(search.ref.symbol, defn.ImplicitAmbiguousAnnot) } (ambi.alt1, ambi.alt2) match { case (AmbiguousImplicitMsg(msg), _) => ... // recover type params from alt1 case (_, AmbiguousImplicitMsg(msg)) => ... // recover type params from alt2 case _ => msg(s"ambiguous implicit arguments: ${ambi.explanation}${location("of")}")( s"ambiguous implicit arguments of type ${pt.show} found${location("for")}") }

Jasper-M · 2018-02-21T10:37:52Z

How are you supposed to handle LazyTrees? If I assign them a type ascription in a pattern match (rhs: Tree) I get an error because of an unchecked pattern. If I try to match it with a pattern (Apply(TypeApply(_, targs), _)) and then do targs.map(_.tpe) I get a ClassCastException: TypeVar cannot be cast to Null$, nor can I use targs where a List[tpd.Tree] is expected (while it obviously is, hence the exception).

Am I just supposed to asInstanceOf it? Is this a bug that I should file? (it sure looks like it... but I can't be the first to bump into this?)

Jasper-M · 2018-03-06T10:46:19Z

As far as I'm concerned this is pretty much ready (except for the cast maybe). Please let me know if there's still something that is blocking. Or if there's a better way to handle the LazyTree thing.

Blaisorblade · 2018-03-06T11:05:22Z

@Jasper-M what about the CI failure?
(EDIT: we might also be late, and sorry about that.)

Jasper-M · 2018-03-06T11:10:20Z

what about the CI failure?

Not a clue. There should be no difference between this commit and the one that previously succeeded.
My guess is either it's spurious or a rebase or somesuch is required. Someone who understands the CI job and test framework better will have to point me in the right direction.

Blaisorblade · 2018-03-07T03:46:56Z

OK, we’ll take a look on our side. Delays are possible, but we’ll do our best.

Blaisorblade · 2018-03-07T03:52:05Z

(Build restarted, it was weird that only test failed.)

smarter · 2018-03-21T13:44:48Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

-              pt.typeSymbol.typeParams.map(_.name.unexpandedName.toString),
-              pt.argInfos)
+        object AmbiguousImplicitMsg {
+          def unapply(search: SearchSuccess) =


Write down the result type of this method for clarity.

smarter · 2018-03-21T13:45:49Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+            userDefinedMsg(search.ref.symbol, defn.ImplicitAmbiguousAnnot)
+        }
+
+        def userDefinedAmbiguousImplicitMsg(alt: SearchSuccess, raw: String) = {


Add a documentation comment for this method.

smarter · 2018-03-21T13:46:09Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -685,22 +686,54 @@ trait Implicits { self: Typer =>
        }
    }
    def location(preposition: String) = if (where.isEmpty) "" else s" $preposition $where"
+
+    def userDefinedMsg(sym: Symbol, cls: Symbol) = for {


Add a documentation comment for this method.

smarter · 2018-03-21T13:47:32Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        }
+
+        (ambi.alt1, ambi.alt2) match {
+          case (AmbiguousImplicitMsg(msg), _) =>


If you write alt @ AmbiguousImplicitMsg(msg), you can use alt below instead of ambi.alt1, same in the next case.

allanrenucci · 2018-03-21T14:37:10Z

Merging! Thanks @Jasper-M 🎉

dottybot reviewed Feb 16, 2018

View reviewed changes

smarter reviewed Feb 16, 2018

View reviewed changes

Blaisorblade assigned Blaisorblade and allanrenucci Feb 16, 2018

Blaisorblade mentioned this pull request Feb 16, 2018

Warn when a user defined error message references undefined names #4008

Closed

Blaisorblade reviewed Feb 19, 2018

View reviewed changes

Blaisorblade mentioned this pull request Feb 20, 2018

@implicitAmbiguous is ignored on third ambiguous implicit scala/bug#10739

Open

Blaisorblade removed their assignment Feb 20, 2018

Blaisorblade reviewed Feb 20, 2018

View reviewed changes

allanrenucci reviewed Feb 20, 2018

View reviewed changes

Jasper-M force-pushed the topic/implicit-ambiguous branch from 93e7013 to 4261c6a Compare February 21, 2018 12:34

Jasper-M force-pushed the topic/implicit-ambiguous branch from 4261c6a to a550d09 Compare March 6, 2018 10:40

fix scala#3797 Add support for @implicitAmbiguous

d656ff2

allanrenucci force-pushed the topic/implicit-ambiguous branch from a550d09 to d656ff2 Compare March 20, 2018 16:53

Move code around

a1732a7

allanrenucci force-pushed the topic/implicit-ambiguous branch from cbe5d9c to a1732a7 Compare March 20, 2018 17:00

allanrenucci added stat:wip and removed stat:wip labels Mar 20, 2018

allanrenucci force-pushed the topic/implicit-ambiguous branch from e0ffb5e to 9498dee Compare March 21, 2018 13:48

smarter approved these changes Mar 21, 2018

View reviewed changes

Use existing helper functions to deconstruct closures and function calls

86ecede

allanrenucci force-pushed the topic/implicit-ambiguous branch from 9498dee to 86ecede Compare March 21, 2018 14:00

Address review comments

5a55c33

allanrenucci merged commit 0c5e39d into scala:master Mar 21, 2018

Fix #3797: Add support for @implicitAmbiguous #4007

Fix #3797: Add support for @implicitAmbiguous #4007

Conversation

Jasper-M commented Feb 16, 2018 • edited Loading

dottybot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter commented Feb 16, 2018

smarter commented Feb 16, 2018

Jasper-M commented Feb 16, 2018

Jasper-M commented Feb 16, 2018

smarter commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasper-M commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jasper-M commented Feb 21, 2018 • edited Loading

Jasper-M commented Mar 6, 2018 • edited Loading

Blaisorblade commented Mar 6, 2018 • edited Loading

Jasper-M commented Mar 6, 2018 • edited Loading

Blaisorblade commented Mar 7, 2018

Blaisorblade commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci commented Mar 21, 2018

Jasper-M commented Feb 16, 2018 •

edited

Loading

Jasper-M commented Feb 21, 2018 •

edited

Loading

Jasper-M commented Mar 6, 2018 •

edited

Loading

Blaisorblade commented Mar 6, 2018 •

edited

Loading

Jasper-M commented Mar 6, 2018 •

edited

Loading