Skip to content

Improved error messages in Desugar.scala #1611

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

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Improved error messages in Desugar.scala #1611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions src/dotty/tools/dotc/ast/Desugar.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,11 +433,11 @@ object desugar {
if (!mods.is(Implicit))
Nil
else if (ctx.owner is Package) {
ctx.error("implicit classes may not be toplevel", cdef.pos)
ctx.error(TopLevelImplicitClass(cdef), cdef.pos)
Nil
}
else if (isCaseClass) {
ctx.error("implicit classes may not be case classes", cdef.pos)
ctx.error(ImplicitCaseClass(cdef), cdef.pos)
Nil
}
else
Expand Down Expand Up @@ -497,7 +497,7 @@ object desugar {
.withPos(mdef.pos)
val ValDef(selfName, selfTpt, _) = tmpl.self
val selfMods = tmpl.self.mods
if (!selfTpt.isEmpty) ctx.error("object definition may not have a self type", tmpl.self.pos)
if (!selfTpt.isEmpty) ctx.error(ObjectMayNotHaveSelfType(mdef), tmpl.self.pos)
val clsSelf = ValDef(selfName, SingletonTypeTree(Ident(name)), tmpl.self.rhs)
.withMods(selfMods)
.withPos(tmpl.self.pos orElse tmpl.pos.startPos)
Expand Down Expand Up @@ -931,7 +931,7 @@ object desugar {
val arity = ts.length
def tupleTypeRef = defn.TupleType(arity)
if (arity > Definitions.MaxTupleArity) {
ctx.error(s"tuple too long (max allowed: ${Definitions.MaxTupleArity})", tree.pos)
ctx.error(TupleTooLong(ts), tree.pos)
unitLiteral
} else if (arity == 1) ts.head
else if (ctx.mode is Mode.Type) AppliedTypeTree(ref(tupleTypeRef), ts)
Expand Down
84 changes: 83 additions & 1 deletion src/dotty/tools/dotc/reporting/diagnostic/messages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ package diagnostic

import dotc.core._
import Contexts.Context, Decorators._, Symbols._, Names._, Types._
import ast.untpd.{Modifiers, ModuleDef}
import util.{SourceFile, NoSource}
import util.{SourcePosition, NoSourcePosition}
import config.Settings.Setting
import interfaces.Diagnostic.{ERROR, WARNING, INFO}
import printing.SyntaxHighlighting._
import printing.Highlighting._
import printing.Formatting

object messages {
Expand Down Expand Up @@ -318,4 +319,85 @@ object messages {
|$code2
|""".stripMargin
}

def implicitClassRestrictionsText(implicit ctx: Context) =
hl"""${NoColor("For a full list of restrictions on implicit classes visit")}
| ${Blue("http://docs.scala-lang.org/overviews/core/implicit-classes.html")}""".stripMargin

case class TopLevelImplicitClass(cdef: untpd.TypeDef)(implicit ctx: Context)
extends Message(10) {
val kind = "Syntax"

val msg = hl"""|An ${"implicit class"} may not be top-level"""

val explanation = {
val TypeDef(name, impl @ Template(constr0, parents, self, _)) = cdef
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is duplicated from Desugar.scala. Is it preferred to duplicate it here to keep the coupling looser or should I instead add more parameters to the case class and pass along explicitely what I need, name, impl, constr0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, I think there's a risk that more people will need to print the class def and as such perhaps we should factor this out into a method call that takes the cdef - a small bit of duplication here is fine in order for it to be easy to use for new contributors IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that the explanation body has been simplified there isn't really much here worth extracting.

val exampleArgs = constr0.vparamss(0).map(_.withMods(Modifiers()).show).mkString(", ")
def defHasBody[T] = impl.body.exists(!_.isEmpty)
val exampleBody = if (defHasBody) "{\n ...\n }" else ""
hl"""|There may not be any method, member or object in scope with the same name as the
|implicit class and a case class automatically gets a companion object with the same name
|created by the compiler which would cause a naming conflict if it were allowed.
|
|""".stripMargin + implicitClassRestrictionsText + hl"""|
|
|To resolve the conflict declare ${cdef.name} inside of an ${"object"} then import the class
|from the object at the use site if needed, for example:
|
|object Implicits {
| implicit class ${cdef.name}($exampleArgs)$exampleBody
|}
|
|// At the use site:
|import Implicits.${cdef.name}""".stripMargin
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- [E010] Syntax Error: test.scala -------------------------------------------------------------------------------------
4 |implicit class Foo(bar: Int, baz: String) {
  |^
  |An implicit class may not be top-level
5 |  def baz = bar.toString
6 |
7 |  def qux = baz.take(5)
8 |}

Explanation
===========
There may not be any method, member or object in scope with the same name as the
implicit class and a case class automatically gets a companion object with the same name
created by the compiler which would cause a naming conflict if it were allowed.

For a full list of restrictions on implicit classes visit
  http://docs.scala-lang.org/overviews/core/implicit-classes.html

To resolve the conflict declare Foo inside of an object then import the class
from the object at the use site if needed, for example:

object Implicits {
  implicit class Foo(val bar: Int, val baz: String){
 ...
 }
}

// At the use site:
import Implicits.Foo

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example output:

-- [E010] Syntax Error: test.scala -------------------------------------------------------------------------------------
2 |implicit class Foo(n: Int)
  |^^^^^^^^^^^^^^^^^^^^^^^^^^
  |An implicit class may not be top-level

Explanation
===========
There may not be any method, member or object in scope with the same name as the
implicit class and a case class automatically gets a companion object with the same name
created by the compiler which would cause a naming conflict if it were allowed.

For a full list of restrictions on implicit classes visit
http://docs.scala-lang.org/overviews/core/implicit-classes.html

To resolve the conflict declare Foo inside of an object then import the class
from the object at the use site if needed, for example:

object Implicits {
  implicit class Foo(val n: Int)
}

import Implicits.Foo

-- [E010] Syntax Error: test.scala -------------------------------------------------------------------------------------
4 |implicit class Foo(bar: Int, baz: String) {
  |^
  |An implicit class may not be top-level
5 |  def baz = bar.toString
6 |
7 |  def qux = baz.take(5)
8 |}

Explanation
===========
There may not be any method, member or object in scope with the same name as the
implicit class and a case class automatically gets a companion object with the same name
created by the compiler which would cause a naming conflict if it were allowed.

For a full list of restrictions on implicit classes visit
http://docs.scala-lang.org/overviews/core/implicit-classes.html

To resolve the conflict declare Foo inside of an object then import the class
from the object at the use site if needed, for example:

object Implicits {
  implicit class Foo(val bar: Int, val baz: String) {
    def baz = bar.toString

    def qux = baz.take(5)
  }
}

import Implicits.Foo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated error on def with body to prevent overly long error explanation:

-- [E010] Syntax Error: test.scala -------------------------------------------------------------------------------------
4 |implicit class Foo(bar: Int, baz: String) {
  |^
  |An implicit class may not be top-level
5 |  def baz = bar.toString
6 |
7 |  def qux = baz.take(5)
8 |}

Explanation
===========
There may not be any method, member or object in scope with the same name as the
implicit class and a case class automatically gets a companion object with the same name
created by the compiler which would cause a naming conflict if it were allowed.

For a full list of restrictions on implicit classes visit
  http://docs.scala-lang.org/overviews/core/implicit-classes.html

To resolve the conflict declare Foo inside of an object then import the class
from the object at the use site if needed, for example:

object Implicits {
  implicit class Foo(val bar: Int, val baz: String){
 ...
 }
}

import Implicits.Foo

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to make it more explicit that the import Implicits.Foo is a at the use site, for example by writing:

object Implicits {
  implicit class Foo(val bar: Int, val baz: String){
 ...
 }
}

...

// At the use site:
import Implicits.Foo


case class ImplicitCaseClass(cdef: untpd.TypeDef)(implicit ctx: Context)
extends Message(11) {
val kind = "Syntax"

val msg = hl"""|A ${"case class"} may not be defined as ${"implicit"}"""

val explanation =
hl"""|implicit classes may not be case classes. Instead use a plain class:
| example: implicit class ${cdef.name}...
|
|""".stripMargin + implicitClassRestrictionsText
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- [E011] Syntax Error: test.scala -------------------------------------------------------------------------------------
14 |  implicit case class Foo(bar: Int, baz: String)
   |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |  A case class may not be defined as implicit

Explanation
===========
implicit classes may not be case classes. Instead use a plain class:
  example: implicit class Foo...

For a full list of restrictions on implicit classes visit
http://docs.scala-lang.org/overviews/core/implicit-classes.html


case class ObjectMayNotHaveSelfType(mdef: untpd.ModuleDef)(implicit ctx: Context)
extends Message(12) {
val kind = "Syntax"

val msg = hl"""|${"objects"} must not have a ${"self type"}"""

val explanation = {
val ModuleDef(name, tmpl) = mdef
val ValDef(_, selfTpt, _) = tmpl.self
hl"""|objects must not have a ${"self type"}:
|
|Consider these alternative solutions:
| - Create a trait or a class instead of an object
| - Let the object extend a trait containing the self type:
| example: object $name extends ${selfTpt.show}""".stripMargin
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- [E012] Syntax Error: test.scala -------------------------------------------------------------------------------------
13 |  self: Foo =>
   |  ^^^^^^^^^
   |  objects must not have a self type

Explanation
===========
objects must not have a self type:

Consider these alternative solutions:
  - Create a trait or a class instead of an object
  - Let the object extend a trait containing the self type:
      example: object Test extends Foo


case class TupleTooLong(ts: List[untpd.Tree])(implicit ctx: Context)
extends Message(13) {
import Definitions.MaxTupleArity
val kind = "Syntax"

val msg = hl"""|A ${"tuple"} cannot have more than ${MaxTupleArity} members"""

val explanation = {
val members = ts.map(_.showSummary).grouped(MaxTupleArity)
val nestedRepresentation = members.map(_.mkString(", ")).mkString(")(")
hl"""|This restriction will be removed in the future.
|Currently it is possible to use nested tuples when more than ${MaxTupleArity} are needed, for example:
|
| ((${nestedRepresentation}))""".stripMargin
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-- [E013] Syntax Error: test.scala -------------------------------------------------------------------------------------
24 |  val largeTuple = (1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                   A tuple cannot have more than 22 members

Explanation
===========
This restriction will be removed in the future.
Currently it is possible to use nested tuples when more than 22 are needed, for example:

  ((1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22)(23))

}