Fix #4709: report if local implicits need type annotation #4729

Blaisorblade · 2018-06-27T11:39:51Z

Fix #4709 and #3253, and revisit fixes to #2001.

allanrenucci

I would do a more generic solution that also handles #3253

allanrenucci · 2018-06-27T12:03:24Z

tests/neg/i4709.scala

+  def test(ctx0: Context) = {
+    implicit val ctx = { ctx0.settings; ??? } // error
+  }
+}


I would make it an error message test as well

Blaisorblade · 2018-06-27T20:04:19Z

Memo: both here and in #3253, implicit search fails because it tries to force the type of the member we're typechecking — whether the method is actually implicit or not. In #3253, we produce OverloadedOrRecursiveMethodNeedsResultType without checking if there's actually any overload/recursion. Ditto for RecursiveValueNeedsResultType.

It seems we should check, instead, if the failure is in implicit resolution or not (not sure how, ctx.searchHistory.searchDepth > 0 only finds nested implicit searches; I might have to record more info), maybe what tree we're looking at. If we're not doing implicit search, then we can use the current errors.

Concerns addressed

Blaisorblade · 2018-07-24T16:34:35Z

Memo for future explorers: if you really really want, it might be possible to find all the symbols involved in a cycle, but at least for now we decided not to try.

This would work following the idea of #4385: we'd recording more information on the CyclicReference as it traverses the stack outward — one would have to catch it and rethrow it in SymDenotation.completeFrom.

allanrenucci · 2018-07-24T16:35:51Z

compiler/src/dotty/tools/dotc/core/NameOps.scala

@@ -49,7 +49,8 @@ object NameOps {
    }
  }

-  implicit class NameDecorator[N <: Name](val name: N) extends AnyVal {
+  implicit class NameDecorator[N <: Name](val name_ : N) extends AnyVal {


I think you can make it private:

- implicit class NameDecorator[N <: Name](val name: N) extends AnyVal { + implicit class NameDecorator[N <: Name](private val name: N) extends AnyVal { import nme._ def testSimple(f: SimpleName => Boolean): Boolean = name match { @@ -233,12 +233,12 @@ object NameOps { case nme.clone_ => nme.clone_ } - def specializedFor(classTargs: List[Types.Type], classTargsNames: List[Name], methodTargs: List[Types.Type], methodTarsNames: List[Name])(implicit ctx: Context): name.ThisName = { + def specializedFor(classTargs: List[Types.Type], classTargsNames: List[Name], methodTargs: List[Types.Type], methodTarsNames: List[Name])(implicit ctx: Context): N = { val methodTags: Seq[Name] = (methodTargs zip methodTarsNames).sortBy(_._2).map(x => defn.typeTag(x._1)) val classTags: Seq[Name] = (classTargs zip classTargsNames).sortBy(_._2).map(x => defn.typeTag(x._1)) - name.likeSpaced(name ++ nme.specializedTypeNames.prefix ++ + likeSpaced(name ++ nme.specializedTypeNames.prefix ++ methodTags.fold(nme.EMPTY)(_ ++ _) ++ nme.specializedTypeNames.separator ++ classTags.fold(nme.EMPTY)(_ ++ _) ++ nme.specializedTypeNames.suffix)

allanrenucci · 2018-07-24T16:41:44Z

compiler/src/dotty/tools/dotc/core/TypeErrors.scala

 class CyclicReference private (val denot: SymDenotation) extends TypeError {
+  var inImplicitSearch: Boolean = false


Can you make this a parameter of the class and then do something like:

def withinImplicitSearch = if (inImplicitSearch) this else new CyclicReference(denot, inImplicitSearch = true) ... } catch { case ce: CyclicReference => throw ce.withinImplicitSearch }

I'm not the best to come up with method name

The new Exception has a different stack trace and other state. I could try to copy the various relevant fields (I spent some time investigating), but that seems more fragile than having one mutable field.

allanrenucci · 2018-07-24T16:45:53Z

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

      }
+    } catch {


Does this need to wrap the whole method? Can't you do:

val result = try new ImplicitSearch(pt, argument, pos).bestImplicit(contextual = true) catch { case ce: CyclicReference => ... }

allanrenucci · 2018-07-24T16:47:34Z

compiler/src/dotty/tools/dotc/core/TypeErrors.scala

+    val unsafeFlags = cycleSym.flagsUNSAFE
+    val isMethod = unsafeFlags.is(Method)
+    val isVal = !isMethod && cycleSym.isTerm
+    val isImplicit = unsafeFlags.is(Implicit)


allanrenucci

Otherwise LGTM. Sorry for the late review @Blaisorblade

allanrenucci · 2018-08-03T09:09:52Z

compiler/src/dotty/tools/dotc/core/TypeErrors.scala

+            else if (isVal)
+              RecursiveValueNeedsResultType(tree.name, cycleSym)
+            else
+              errorMsg(cx.outer)


Yes I think we should remove all of this if tree.name is unused and we only use the cycleSym

allanrenucci · 2018-08-03T09:12:53Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

    val explanation =
-      hl"""The definition of `${value}` is recursive and you need to specify its type.
+      hl"""The definition of `${cycleSym}` is recursive and you need to specify its type.


Nitpick: here and in some of the error messages above s/${cycleSym}/$cycleSym

allanrenucci · 2018-08-04T13:51:07Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -1260,26 +1260,27 @@ object messages {
           |""".stripMargin
  }

-  case class OverloadedOrRecursiveMethodNeedsResultType(method: Names.TermName)(implicit ctx: Context)
+  case class OverloadedOrRecursiveMethodNeedsResultType(methodName: Names.TermName, cycleSym: Symbol)(implicit ctx: Context)
  extends Message(OverloadedOrRecursiveMethodNeedsResultTypeID) {
    val kind = "Syntax"


Oh, and we should probably classify OverloadedOrRecursiveMethodNeedsResultType, RecursiveValueNeedsResultType, TermMemberNeedsResultTypeForImplicitSearch, CyclicReferenceInvolvingImplicit and CyclicReferenceInvolving as Cyclic errors not Syntax

Tentative advice copied from `CyclicReferenceInvolvingImplicit`.

…better - Record in CyclicReference if it's emitted during implicit search. - Before checking if the error is on an implicit, check if we have an error during implicit search that happens while inferring a return type; that can happen whether the member we're typechecking is implicit or not, and needs the same cure. - Add ErrorMessagesTests as requested This improves the error message, and also puts more effort into diagnosing the cycle: we now use similar logic as for recursive/overloaded members to check we're indeed dealing with a (potentially) recursive implicit. Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve bugs, so make the error message and explanation more tentative, following `CyclicReferenceInvolving`. *However*, this does not yet handle mutually recursive methods in a reasonable way.

Make reports about cycleSym not tree.name. We need then to use flagsUNSAFE, see description. * Store cycleSym in all relevant errors * Mention cycleSym instead of tree.name in errors Tests pass, and have the correct expectations both for `tree.name` and `cycleSym.name`, tho we just show `cycleSym.name` and `tree.name` is less accurate. But at this point, I might be able to stop using `tree.name` altogether.

As suggested in review.

Blaisorblade · 2018-08-11T20:42:36Z

All comments addressed, so merging.

Blaisorblade requested a review from allanrenucci June 27, 2018 11:39

allanrenucci previously requested changes Jun 27, 2018

View reviewed changes

Blaisorblade self-assigned this Jun 27, 2018

Blaisorblade force-pushed the fix-4709 branch 3 times, most recently from 1be9a81 to 7c51b08 Compare July 5, 2018 19:46

Blaisorblade added the stat:wip label Jul 5, 2018

This comment has been minimized.

Sign in to view

Blaisorblade force-pushed the fix-4709 branch from 7c51b08 to 3de76ea Compare July 7, 2018 07:56

Blaisorblade force-pushed the fix-4709 branch from b0d9b74 to dd11b11 Compare July 24, 2018 09:44

Blaisorblade added stat:needs review and removed stat:wip labels Jul 24, 2018

Blaisorblade force-pushed the fix-4709 branch from dd11b11 to 75f3c50 Compare July 24, 2018 11:00

Blaisorblade assigned allanrenucci and unassigned Blaisorblade Jul 24, 2018

allanrenucci reviewed Jul 24, 2018

View reviewed changes

Blaisorblade mentioned this pull request Jul 24, 2018

Refactorings on Names: forbid .name on Name and cleanups on the way #4834

Merged

This comment has been minimized.

Sign in to view

Blaisorblade added stat:needs rebase and removed stat:needs review labels Jul 24, 2018

Blaisorblade force-pushed the fix-4709 branch from b38b932 to e19b972 Compare July 25, 2018 12:00

Blaisorblade added stat:needs review and removed stat:needs rebase labels Jul 25, 2018

allanrenucci approved these changes Aug 3, 2018

View reviewed changes

allanrenucci removed their assignment Aug 3, 2018

allanrenucci removed the stat:needs review label Aug 3, 2018

Blaisorblade self-assigned this Aug 3, 2018

allanrenucci reviewed Aug 4, 2018

View reviewed changes

Blaisorblade added 8 commits August 9, 2018 15:43

Refine type of ValOrDefDef.name

be1b0fb

Fix unrelated typo

0bd5dbe

Make error explanation more correct

feef97e

CyclicReferenceInvolving: improve explanation for consistency

68c5d57

Tentative advice copied from `CyclicReferenceInvolvingImplicit`.

Review comment: ${cycleSym} -> $cycleSym

ba06519

Drop {method,val}Name from cyclic errors

e75ee84

Blaisorblade force-pushed the fix-4709 branch from e19b972 to e75ee84 Compare August 9, 2018 13:51

Reclassify error messages due to CyclicReference as Cyclic

d8e6d3e

As suggested in review.

Blaisorblade merged commit a91d82a into scala:master Aug 11, 2018

Blaisorblade deleted the fix-4709 branch August 11, 2018 20:42

Blaisorblade mentioned this pull request Aug 15, 2018

Overloaded or recursive method test needs return type #3253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4709: report if local implicits need type annotation #4729

Fix #4709: report if local implicits need type annotation #4729

Blaisorblade commented Jun 27, 2018 •

edited

Loading

allanrenucci left a comment

allanrenucci Jun 27, 2018

Blaisorblade commented Jun 27, 2018

This comment has been minimized.

Blaisorblade commented Jul 24, 2018

allanrenucci Jul 24, 2018

allanrenucci Jul 24, 2018

Blaisorblade Jul 24, 2018

allanrenucci Jul 24, 2018

allanrenucci Jul 24, 2018

This comment has been minimized.

allanrenucci left a comment

allanrenucci Aug 3, 2018

allanrenucci Aug 3, 2018

allanrenucci Aug 4, 2018

Blaisorblade commented Aug 11, 2018

		class CyclicReference private (val denot: SymDenotation) extends TypeError {
		var inImplicitSearch: Boolean = false

Fix #4709: report if local implicits need type annotation #4729

Fix #4709: report if local implicits need type annotation #4729

Conversation

Blaisorblade commented Jun 27, 2018 • edited Loading

allanrenucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented Jun 27, 2018

This comment has been minimized.

Blaisorblade commented Jul 24, 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

This comment has been minimized.

allanrenucci left a 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

Blaisorblade commented Aug 11, 2018

Blaisorblade commented Jun 27, 2018 •

edited

Loading