Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix:better error msg for cyclic error for constructors #17131

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix:better error msg for cyclic error for constructors #17131

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6a725e
fix:better error msg for cyclic error for constructors
doofin Mar 15, 2023
7bca6b3
test:add test for error msg for cyclic error
doofin Apr 29, 2023
7aad7ec
Merge branch 'lampepfl:main' into cyclicErrMsg
doofin Apr 29, 2023
037be3f
rm comments in TypeErrors.scala
doofin Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion compiler/src/dotty/tools/dotc/core/TypeErrors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,11 @@ class CyclicReference private (val denot: SymDenotation)(using Context) extends
// cycleSym.flags would try completing denot and would fail, but here we can use flagsUNSAFE to detect flags
// set by the parser.
val unsafeFlags = cycleSym.flagsUNSAFE
val isMethod = unsafeFlags.is(Method)
val isMethod = unsafeFlags.is(Method) // sometimes,isMethod and isConstructor can both be true!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that a symbol can both be isMethod and isConstructor ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is a method because it is represented with a def.

Copy link
Author

@doofin doofin Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I remember correctly, isMethod is true(which shouldn't be) for case class as in #17076 so I add this warning, maybe I should indicate in a different way? (or maybe open a different issue for isMethod)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we print in PostTyper

        case tree: DefDef =>
          println("---")
          println("DefDef: " + tree.show)
          println("flags: " + tree.symbol.flagsString)
          println("isConstructor: " + tree.symbol.isConstructor)

we can see that this class

class F(a: Int):
  def this() = this(0)
  def f = 1

has the the Method flag and isConstructor to true for both the primary and secondary constructors.

---
DefDef: def <init>(a: Int): Unit
flags: <method> <stable> <touched>
isConstructor: true
---
DefDef: def <init>(): Unit =
  {
    this(0)
    ()
  }
flags: <method> <touched> <no-default-params>
isConstructor: true
---
DefDef: def f: Int = 1
flags: <method> <touched>
isConstructor: false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What might happen is that the Method was not in flagsUNSAFE because it would be computed and added to flags while typing. But the failure may have happened before that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks i got it, removed the comment

val isVal = !isMethod && cycleSym.isTerm
val isConstructor = cycleSym.isConstructor

// println("isMethod?"+isMethod+",isConstr:"+isConstructor)

/* This CyclicReference might have arisen from asking for `m`'s type while trying to infer it.
* To try to diagnose this, walk the context chain searching for context in
Expand All @@ -154,6 +157,8 @@ class CyclicReference private (val denot: SymDenotation)(using Context) extends
case tree: untpd.ValOrDefDef if !tree.tpt.typeOpt.exists =>
if (inImplicitSearch)
TermMemberNeedsResultTypeForImplicitSearch(cycleSym)
else if (isConstructor)
CyclicMsgUnknownBug(cycleSym)
else if (isMethod)
OverloadedOrRecursiveMethodNeedsResultType(cycleSym)
else if (isVal)
Expand Down
8 changes: 8 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,14 @@ extends CyclicMsg(OverloadedOrRecursiveMethodNeedsResultTypeID) {
|"""
}

class CyclicMsgUnknownBug(cycleSym: Symbol)(using Context)
extends CyclicMsg(OverloadedOrRecursiveMethodNeedsResultTypeID) {
def msg(using Context) = i"""Compiler bug: unknown cyclic error for $cycleSym. please report"""
def explain(using Context) =
i"""|For temporary fix, add type annotations to areas involving this constructor
"""
}

class RecursiveValueNeedsResultType(cycleSym: Symbol)(using Context)
extends CyclicMsg(RecursiveValueNeedsResultTypeID) {
def msg(using Context) = i"""Recursive $cycleSym needs type"""
Expand Down
38 changes: 38 additions & 0 deletions compiler/test/dotty/tools/dotc/reporting/CyclicErrorMessages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package dotty.tools
package dotc
package reporting

import dotty.tools.dotc.core.Contexts.Context
import org.junit.Assert._
import org.junit.Test

/*better error message for cyclic errors when using exports as in #17076 */
class CyclicErrorMessages extends ErrorMessagesTest {
@Test def cyclicErrMsg =
checkMessagesAfter("typer") {
"""object A {
| def bar(x: B.Foo[Int]) = x
|}
|
|object B {
| import C.*
| case class Foo[T <: Int](x: Any)
| def foo = Foo(0)
|}
|
|object C extends D.Foo[Int](0)
|
|object D {
| export B.foo
| type Foo[T <: Int] = B.Foo[T]
|}
""".stripMargin
}.expect { (itcx, messages) =>
given Context = itcx

assertMessageCount(2, messages)
val msg = messages.head.message

assertTrue(msg.contains("Compiler bug: unknown cyclic error"))
}
}