Change "recursive/cyclic definitions needs type" errors to Message #2049
Conversation
| @@ -52,6 +52,9 @@ | |||
| MixedLeftAndRightAssociativeOpsID, | |||
| CantInstantiateAbstractClassOrTraitID, | |||
| AnnotatedPrimaryConstructorRequiresModifierOrThisID, | |||
| OverloadedOrRecursiveMethodNeedsResultTypeID, | |||
| RecursiveValueNeedsResultTypeID, | |||
| CyclicReferenceInvolvingImplicitID, | |||
There was a problem hiding this comment.
I have a more general question: Why are we using a Java enumeration for this? Would it not be easier to use a scala.Enumeration?
There was a problem hiding this comment.
@felixmulder recently changed from directly specified int constants to this Java enum.
| val explanation = | ||
| hl"""|${tree} is overloaded and at least one definition of it calls another. | ||
| |You need to specify the calling method's return type. | ||
| """.stripMargin |
There was a problem hiding this comment.
In this case the method might be overloaded but it could also be simply recursive. So claiming that it is overloaded is false. We might try to find out whether it is overloaded to improve the error message. Essentially, it's overloaded if its owner is a class and taking the member of the name in the owner gives several alternatives.
|
And I in turn blame @nicolasstucki ;)
In all seriousness, we wanted something lightweight (read: contributor
friendly) that could then be forgotten as soon as we're bootstrapped good
and proper (hello new scala enums!). I agreed with Nicolas to change it
since he was having trouble rebasing the call-graph when new errors were
implemented. It was a mutual decision.
…On Sun, Mar 5, 2017, 19:59 Enno ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java
<#2049 (comment)>:
> @@ -52,6 +52,9 @@
MixedLeftAndRightAssociativeOpsID,
CantInstantiateAbstractClassOrTraitID,
AnnotatedPrimaryConstructorRequiresModifierOrThisID,
+ OverloadedOrRecursiveMethodNeedsResultTypeID,
+ RecursiveValueNeedsResultTypeID,
+ CyclicReferenceInvolvingImplicitID,
@felixmulder <https://github.com/felixmulder> recently changed from
directly specified int constants to this Java enum.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2049 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdYwWAadGeLLzEW5390jW25dXenHZgpks5riwYggaJpZM4MTHgy>
.
|
Split error messages for recursive method and overloaded method needs type into two (but did not solve the analysis which to show). Make CyclicReference type error construct corresponding error message.
|
@odersky The "cyclic reference involving" from the |
| if (cx.mode is Mode.InferringReturnType) { | ||
| cx.tree match { | ||
| case tree: untpd.DefDef if !tree.tpt.typeOpt.exists => | ||
| OverloadedOrRecursiveMethodNeedsResultType(tree.name) | ||
| // TODO: analysis if tree is an overloaded method (or directly recursive) |
There was a problem hiding this comment.
I think this will do the trick:
cycleSym.owner.isClass && cycleSym.owner.info.member(cycleSym.name).isOverloaded
There was a problem hiding this comment.
In that direction was what I tried, but member fails in Tree.withType as it encounters an ErrorType.
There was a problem hiding this comment.
Why do you need Tree.withtype?
There was a problem hiding this comment.
That is what I get upon calling member(cycleSym.name) - not sure how the code gets there.
[error] Test dotty.tools.dotc.reporting.ErrorMessagesTests.recursiveMethodNeedsReturnType failed: java.lang.AssertionError: assertion failed, took 0.01 sec
[error] at scala.Predef$.assert(Predef.scala:151)
[error] at dotty.tools.dotc.ast.Trees$Tree.withType(Trees.scala:116)
[error] at dotty.tools.dotc.typer.ErrorReporting$.errorTree(ErrorReporting.scala:22)
[error] at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1584)
[error] at dotty.tools.dotc.typer.Typer$$anonfun$typed$2.apply(Typer.scala:1582)
[error] at dotty.tools.dotc.reporting.Reporting$class.traceIndented(Reporter.scala:136)
[error] at dotty.tools.dotc.core.Contexts$Context.traceIndented(Contexts.scala:57)
[error] at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1582)
...
There was a problem hiding this comment.
It complains that we assign an error type into a tree, yet not errors were reported. Is that stacktrace reached from the call
cycleSym.owner.info.member(cycleSym.name).isOverloaded
? If yes, can you show me the full stacktrace up to the call?
There was a problem hiding this comment.
Here you go: stacktrace.txt
There was a problem hiding this comment.
According to that stacktrace the error is not caused by the call
cycleSym.owner.info.member(cycleSym.name).isOverloaded
That one shows up nowhere. So it most likely happens later. Could it be that an error message is not reported?
There was a problem hiding this comment.
The error is triggered by using member, even though it is not on the stack. The error is reported correctly if I don't call it and it fails when I try to, at the time of calling the error is not reported yet.
There was a problem hiding this comment.
I think the member call causes another CyclicReference exception. That's not easy to avoid. So I think we should give up. Don't try to distinguish the error message and just say "recursive or overloaded".
Fall-back to reporting "overloaded or recursive needs type".
|
Yes, that's indeed harder than expected. Thanks for hanging in there! |
@odersky
Here are my changes to move the recursive/cyclic definition type required errors to Message.
I hope the explanations make sense.