Skip to content

unhelpful error message when overloading apply without return type #1731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
smarter opened this issue Nov 21, 2016 · 7 comments
Closed

unhelpful error message when overloading apply without return type #1731

smarter opened this issue Nov 21, 2016 · 7 comments

Comments

@smarter
Copy link
Member

smarter commented Nov 21, 2016

dotty:

scala> case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) } 
-- Error: <console> ----------------------------------------------------------------------
6 |case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
  |                                                       ^^^
  |                                                       object Foo does not take parameters

The solution is to write def apply[T](): Foo[T] but good luck figuring that out from our error message, contrast with scalac:

scala> case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
<console>:15: error: overloaded method apply needs result type
       case class Foo[T](x: T); object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
                                                                 ^
@bbarker
Copy link
Contributor

bbarker commented Jun 30, 2017

I have a tentative fix for this, should have something up to review in the next couple of days.

@bbarker
Copy link
Contributor

bbarker commented Jun 30, 2017

Just trying to wrap this up by adding a new test like this:

    checkMessagesAfter("frontend") {
      """
        |case class Foo[T](x: T)
        |object Foo { def apply[T]() = Foo(null.asInstanceOf[T]) }
      """.stripMargin
    }.expect { (ictx, messages) =>
      implicit val ctx: Context = ictx

      assertMessageCount(1, messages)
      val OverloadedOrRecursiveMethodNeedsResultType(treeName2) :: Nil = messages
      assertEquals("Foo", treeName2)
    }

This results in:

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.024 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:12)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTest$class.checkMessagesAfter(ErrorMessagesTest.scala:60)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTests.checkMessagesAfter(ErrorMessagesTests.scala:12)
[error]     at dotty.tools.dotc.reporting.ErrorMessagesTests.overloadedMethodNeedsReturnType(ErrorMessagesTests.scala:219)

I assume I'm doing something incorrect with which phase I'm picking, but having tried a few with no change of results (other than the phase apparently not being supported), wanted to check here for any insight.

@smarter
Copy link
Member Author

smarter commented Jun 30, 2017

No clue, /cc @felixmulder who wrote checkMessagesAfter. You can submit your PR with the test disabled until this is fixed.

bbarker added a commit to bbarker/dotty that referenced this issue Jul 1, 2017
…turn type

idenitified possible solution

issue seems fixed - fixed existing tests; new test isn't working right

disabling new test for now
bbarker added a commit to bbarker/dotty that referenced this issue Jul 1, 2017
@felixmulder
Copy link
Contributor

Looking into it for @bbarker's PR, I think the assertion is run using the wrong context.

@felixmulder
Copy link
Contributor

Nope, it works fine to run the commented out checkMessagesAfter

@smarter
Copy link
Member Author

smarter commented Jul 5, 2017

The original example in #1731 now shows:

overloaded or recursive method Foo needs return type

But this is incorrect, Foo is a class, not a method, the method is called apply.

allanrenucci added a commit to dotty-staging/dotty that referenced this issue Jan 4, 2018
@allanrenucci allanrenucci mentioned this issue Jan 4, 2018
@allanrenucci
Copy link
Contributor

The PR was reverted

@allanrenucci allanrenucci reopened this Jan 18, 2018
nicolasstucki added a commit that referenced this issue Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants