Closes #1731 by fixing error message for overloaded method without re… #2823

bbarker · 2017-07-01T00:53:21Z

This was achieved by adding a more specific conditional to note the specific error and by overloading apply for the already existing OverloadedOrRecursiveMethodNeedsResultType.

While this is an improvement, it is still not quite right, as the error message now behaves like:

scala> case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) } 
-- [E043] Syntax Error: <console>:4:55 -----------------------------------------
4 |case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
  |                                                       ^^^
  |                      overloaded or recursive method Foo needs return type

To improve beyond this, it seems we would need some way to walk up the tree to get to the LHS (not sure if possible), or, catch it higher up the tree in the first place.

dottybot

Hello, and thank you for opening this PR! 🎉

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

Have an awesome day! ☀️

felixmulder

Thanks for taking the time to submit this PR! We really appreciate the thought going into these contributions.

There are a couple of things I'm hesitant about, but I await your response :)

felixmulder · 2017-07-03T07:42:09Z

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

+      OverloadedOrRecursiveMethodNeedsResultType(tree.name.toString)(ctx)
+    def apply(symbol: Symbol)(implicit ctx: Context)
+    : OverloadedOrRecursiveMethodNeedsResultType =
+      OverloadedOrRecursiveMethodNeedsResultType(symbol.name.toString)(ctx)


There's a "Showable" type class that can be accessed for both trees and symbols via .show

felixmulder · 2017-07-03T07:42:49Z

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

@@ -48,7 +48,7 @@ object ProtoTypes {

    private def disregardProto(pt: Type)(implicit ctx: Context): Boolean = pt.dealias match {
      case _: OrType => true
-      case pt => pt.isRef(defn.UnitClass)
+      case ptd => ptd.isRef(defn.UnitClass)


Maybe an unnecessary diff?

the 'case' pt shadows the outer pt, which may not be an issue here, but could crop up later if the function became more complex, I suppose. Happy to take it out though.

I think the point here is to specifically shadow it, we do this all over the compiler :)

felixmulder · 2017-07-03T07:42:59Z

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

@@ -1872,8 +1872,6 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
   */
  def adaptInterpolated(tree: Tree, pt: Type, original: untpd.Tree)(implicit ctx: Context): Tree = {

-    assert(pt.exists)
-


Why was this assertion removed?

Wow, I don't know - it shouldn't have been, sorry that typo slipped through (I really do not remember making this change, but still should have spotted it, hmm).

felixmulder · 2017-07-03T07:43:54Z

compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala

+//      val OverloadedOrRecursiveMethodNeedsResultType(treeName2) :: Nil = messages
+//      assertEquals("Foo", treeName2)
+//    }
+  }


This should be fine as it is reported after the front end (of which typer is a part). Why can't this test be enabled?

I cloned your branch and tried out the commented out test - and it runs fine for me. Did you do git submodule update --init?

Hmm, I thought I did but can't quite remember exactly which submodule command I originally ran. I tried this but it didn't make a difference. I'll uncomment for now and try to figure out my issue later.

Submit it uncommented and we'll see what the CI says 😄

Will do, I'm getting a number of failures from sbt test so will see how this compares.

felixmulder · 2017-07-03T07:44:23Z

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

@@ -1320,7 +1329,8 @@ object messages {
           |"""
  }

-  case class MethodDoesNotTakeParameters(tree: tpd.Tree, methPartType: Types.Type)(err: typer.ErrorReporting.Errors)(implicit ctx: Context)
+  case class MethodDoesNotTakeParameters(tree: tpd.Tree, methPartType: Types.Type)
+  (err: typer.ErrorReporting.Errors)(implicit ctx: Context)


felixmulder · 2017-07-03T07:45:23Z

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

          |""".stripMargin
  }

+  object OverloadedOrRecursiveMethodNeedsResultType {
+    @specialized def apply[T >: Trees.Untyped](tree: NameTree[T])(implicit ctx: Context)


No need for the @specialized annotation, Dotty will have its own form of specialization that does not require the annotation. Also, the bootstrapped compiler currently doesn't do anything for @specialized.

felixmulder · 2017-07-03T07:45:59Z

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

@@ -1209,20 +1209,29 @@ object messages {
           |""".stripMargin
  }

-  case class OverloadedOrRecursiveMethodNeedsResultType(tree: Names.TermName)(implicit ctx: Context)
+  case class OverloadedOrRecursiveMethodNeedsResultType private (termName: String)(implicit ctx: Context)


Why make the name private? If you're an IDE, wouldn't you want to be able to extract the name from the message?

My thought was to prevent accidental use that might happen by passing in a string directly; I may not understand all the implications, but even with private it seems I can still extract the string in ErrorMessagesTests.scala:

val OverloadedOrRecursiveMethodNeedsResultType(treeName) :: Nil = messages assertEquals("i", treeName)

Seems to work

Hmm, I did correct this before having coffee. Ignore this comment, it's just the constructor marked private, not the members ☕️

felixmulder · 2017-07-03T07:46:06Z

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

@@ -126,7 +126,7 @@ object Contexts {
    protected def sstate_=(sstate: SettingsState) = _sstate = sstate
    def sstate: SettingsState = _sstate

-    /** The current tree */
+    /** The current compilation unit */


bbarker · 2017-07-03T12:09:26Z

I'm thinking about revising the commit message to "fixes for #1731" instead of "closes", and just update the issue. I'll plan to come back to it soon for a full fix.

…hod without return type

felixmulder · 2017-07-03T13:39:35Z

Looks like it ran just fine - except that the test fails with:

recursiveMethodNeedsReturnType failed:
org.junit.ComparisonFailure: expected:<[i]> but was:<[def i = i + 5]>

// and

overloadedMethodNeedsReturnType failed:
org.junit.ComparisonFailure: expected:<[foo]> but was:<[def foo(i: Int) = foo(i.toString)]>

hint:

tree.show      // string of tree as "scala-like" code
tree.name.show // string of name without encodings

bbarker · 2017-07-03T16:05:21Z

Thanks again for the review ,and the help. I reviewed the dotty build instructions and started afresh on another system; I think the --recursive option may have been needed for git submodule update --init; also I don't think I ran sbt managedSources previously, which (may) have been causing other tests to fail.

Unfortunately, I still get the error for this test as originally reported in #1731, and now I seem to have managed (by means unknown) to have gotten the CI build system to replicate the issue:

[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.traitMayNotBeFinal started
[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.overloadedMethodNeedsReturnType started
[error] Test dotty.tools.dotc.reporting.ErrorMessagesTests.overloadedMethodNeedsReturnType failed: java.lang.AssertionError: assertion failed: denotation module _root_ invalid in run 2. ValidFor: Period(1..2, run = 3), took 0.023 sec
[error]     at scala.Predef$.assert(Predef.scala:170)
[error]     at dotty.tools.dotc.core.Denotations$SingleDenotation.bringForward(Denotations.scala:727)
[error]     at dotty.tools.dotc.core.Denotations$SingleDenotation.current(Denotations.scala:783)
[error]     at dotty.tools.dotc.core.Symbols$Symbol.denot(Symbols.scala:406)
[error]     at dotty.tools.dotc.core.Symbols$Symbol.isType(Symbols.scala:429)
[error]     at dotty.tools.dotc.core.Scopes$MutableScope.enter(Scopes.scala:270)
[error]     at dotty.tools.dotc.Compiler.rootContext(Compiler.scala:134)
[error]     at dotty.tools.DottyTest$class.checkCompile(DottyTest.scala:60)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTests.checkCompile(ErrorMessagesTests.scala:14)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTest$class.checkMessagesAfter(ErrorMessagesTest.scala:60)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTests.checkMessagesAfter(ErrorMessagesTests.scala:14)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTests.overloadedMethodNeedsReturnType(ErrorMessagesTests.scala:222)
[error]     ...
[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.expectedClassOrObjectDef started
[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.methodDoesNotTakeMorePrameters started
[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.missingTypeParameter started
[info] Test dotty.tools.dotc.reporting.ErrorMessagesTests.typeDoesNotTakeParameters started

felixmulder · 2017-07-04T15:02:12Z

That's great, I should be able to reproduce it now!

felixmulder · 2017-07-05T10:58:08Z

#2832 should have fixed your issue! :)

bbarker · 2017-07-05T15:03:28Z

Thanks @felixmulder - looks like CI is happy now

felixmulder · 2017-08-22T13:45:32Z

@bbarker it does indeed! I'm sorry this fell through the cracks, I went on vacation and simply haven't gotten around to merging this until today.

So, again, sorry for the belated merge, and thanks a bunch for contributing! 🎉

…od without re… (scala#2823)" This reverts commit 91d3ec1.

dottybot reviewed Jul 1, 2017

View reviewed changes

felixmulder suggested changes Jul 3, 2017

View reviewed changes

Partial fix for scala#1731 by fixing error message for overloaded met…

30ac9e2

…hod without return type

bbarker force-pushed the 1731_overload_unhelpful_error_message branch from 3bc3c31 to 30ac9e2 Compare July 3, 2017 12:19

fixes for review in PR scala#2823

6c334e1

fixing 'show' usage to only show name

0d8d1ca

Merge branch 'master' into 1731_overload_unhelpful_error_message

16be5c5

felixmulder approved these changes Aug 22, 2017

View reviewed changes

felixmulder merged commit 91d3ec1 into scala:master Aug 22, 2017

allanrenucci added a commit to dotty-staging/dotty that referenced this pull request Jan 4, 2018

Revert "Closes scala#1731 by fixing error message for overloaded meth…

614e5ca

…od without re… (scala#2823)" This reverts commit 91d3ec1.

allanrenucci mentioned this pull request Jan 4, 2018

Fix #2994 #3750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #1731 by fixing error message for overloaded method without re… #2823

Closes #1731 by fixing error message for overloaded method without re… #2823

bbarker commented Jul 1, 2017

dottybot left a comment

felixmulder left a comment

felixmulder Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017 •

edited

Loading

felixmulder Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

bbarker Jul 3, 2017

felixmulder Jul 3, 2017

felixmulder Jul 3, 2017

bbarker commented Jul 3, 2017

felixmulder commented Jul 3, 2017 •

edited

Loading

bbarker commented Jul 3, 2017 •

edited

Loading

felixmulder commented Jul 4, 2017

felixmulder commented Jul 5, 2017 •

edited

Loading

bbarker commented Jul 5, 2017

felixmulder commented Aug 22, 2017

Closes #1731 by fixing error message for overloaded method without re… #2823

Closes #1731 by fixing error message for overloaded method without re… #2823

Conversation

bbarker commented Jul 1, 2017

dottybot left a comment

Choose a reason for hiding this comment

felixmulder 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbarker Jul 3, 2017 • edited Loading

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

bbarker commented Jul 3, 2017

felixmulder commented Jul 3, 2017 • edited Loading

bbarker commented Jul 3, 2017 • edited Loading

felixmulder commented Jul 4, 2017

felixmulder commented Jul 5, 2017 • edited Loading

bbarker commented Jul 5, 2017

felixmulder commented Aug 22, 2017

bbarker Jul 3, 2017 •

edited

Loading

felixmulder commented Jul 3, 2017 •

edited

Loading

bbarker commented Jul 3, 2017 •

edited

Loading

felixmulder commented Jul 5, 2017 •

edited

Loading