#1589: Improve error message for WrongNumberOfArgs #1623

ShaneDelmore · 2016-10-25T00:40:21Z

No description provided.

ShaneDelmore · 2016-10-25T00:42:27Z

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

+        hl"""|You have not supplied enough type parameters
+             |  If you specify one type parameter then you need to specify every type parameter.""".stripMargin
+    }
+  }


Example message for too many type args

-- [E018] Syntax Error: test.scala ------------------------------------------------------------------------------------- 16 | var bad = List[Int, String] | ^^^^^^^^^^^^^^^^^ |Too many type arguments for ([scala$collection$immutable$List$apply$$A] => (xs: A*)scala.collection.immutable.List[A])(List.apply) |expected: [A] |actual: [Int, String] Explanation =========== You have supplied too many type parameters For example List takes a single type parameter (List[A]) If you need to hold more types in a list then you need to combine them into another data type that can contain the number of types you need, In this example one solution would be to use a Tuple: val tuple2: Tuple2[Int, String = (1, "one) List[(Int, String)] = List(tuple2)

Example message for not enough type args

-- [E018] Syntax Error: test.scala ------------------------------------------------------------------------------------- 17 | var bad2 = Map[(Int, String)] | ^^^^^^^^^^^^^^^^^^ |Not enough type arguments for ([scala$collection$generic$GenMapFactory$apply$$A, scala$collection$generic$GenMapFactory$apply$$B] => | (elems: (A, B)*)scala.collection.immutable.Map[A, B] |)(Map.apply) |expected: [A, B] |actual: [Tuple2[Int, String]] Explanation =========== You have not supplied enough type parameters If you specify one type parameter then you need to specify every type parameter.

ShaneDelmore · 2016-10-25T01:23:01Z

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

+           |      case _ => 0
+           |    }
+           |
+           |""".stripMargin


Due to #1612 I am unable to actually reach this block of code right now so I kept the message simple. Once #1612 is fixed I would like to revisit this message and generate the additional case statements for the user from their alternatives.

That's fine :) Also, if you want - you can take a stab at fixing #1612 as a more challenging task!

I was hoping I could fix this but so far don't have a good enough idea of the flow through the compiler to know where the right fix is. I will spend some time on it this weekend most likely though.

Just ping us in the gitter channel and we'll try to help :)

felixmulder

Great explanations all around! I'm not too much of a fan of the left whitespace padding - but maybe you have some idea with this that I'm not getting?

Some idiomatic things to change and we're good to go :)

felixmulder · 2016-10-25T12:06:14Z

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

+    val kind = "Syntax"
+    val expectedCount = expectedArgs.length
+    val actualCount = actual.length
+    val msgPrefix = if (actualCount > expectedCount) "Too many" else "Not enough"


I would wrap this in NoColor so that it's not highlighted in the msg.

felixmulder · 2016-10-25T12:06:48Z

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

+    val expectedArgString = (fntpe.widen match {
+      case pt: MethodOrPoly => pt //ensure we return a type that will have useful typeParms
+      case _ => fntpe
+    }).typeParams.map(_.paramName.show.split("\\$").last).mkString("[", ", ", "]")


this match expression is the same as just taking: fntpe.widen.typeParams

Nice simplification

felixmulder · 2016-10-25T12:14:04Z

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

+    val actualArgString = actual.map(_.show.split("\\.").last).mkString("[", ", ", "]")
+
+    val msg =
+      hl"""|$msgPrefix ${argKind} arguments for $fntpe


Instead of $fntpe, I would do: ${fntpe.termSymbol.showFullName} which will give you something like: scala.collection.immutable.List.apply which you can then add the correct parameter list to (if you want).

That is great, I was hunting around for something like it but didn't find it.

No luck, it doesn't return the same result. on the below code

object Test { case class Bar[D, E](baz: D, qux: E) var bad3: Bar[Int] = null }

My original solution returned "Test.Bar" where fntpe.termSymbol returns nonSensicalType

felixmulder · 2016-10-25T12:17:33Z

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

+             |  into another data type that can contain the number of types you need,
+             |  In this example one solution would be to use a Tuple:
+             |  val tuple2: Tuple2[Int, String = (1, "one)
+             |    List[(Int, String)] = List(tuple2)""".stripMargin


Maybe kill the left padding on these lines and add a newline before the code?

No problem. I was trading off indentation for vertical space as I worried we didn't want the messages too long. Newlines work just as well for me though.

felixmulder · 2016-10-25T12:17:44Z

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

+             |  If you need to hold more types in a list then you need to combine them
+             |  into another data type that can contain the number of types you need,
+             |  In this example one solution would be to use a Tuple:
+             |  val tuple2: Tuple2[Int, String = (1, "one)


missing ] and "?

Also perhaps add ${"..."} around the code to get highlighting, or just move it to variable and interpolate that :)

felixmulder · 2016-10-25T12:20:16Z

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

@@ -512,4 +513,64 @@ object messages {
        |}""".stripMargin
  }

+  case class WrongNumberOfArgs(fntpe: Type, argKind: String, expectedArgs: List[TypeParamInfo], actual: List[untpd.Tree])(implicit ctx: Context)
+    extends Message(18) {


This will be number 19 if merged first :)

felixmulder · 2016-10-25T12:21:51Z

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

+           |      case _ => 0
+           |    }
+           |
+           |""".stripMargin


That's fine :) Also, if you want - you can take a stab at fixing #1612 as a more challenging task!

felixmulder · 2016-10-25T12:22:31Z

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

+           |      case (1, n) => n
+           |      case (n, 1) => n
+           |      case _ => 0
+           |    }


Interpolate the code in this guy too? We love syntax highlighting 🦄 🌈

smarter · 2016-10-25T17:10:00Z

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

+             |  If you need to hold more types in a list then you need to combine them
+             |  into another data type that can contain the number of types you need,
+             |  In this example one solution would be to use a Tuple:
+             |  val tuple2: Tuple2[Int, String = (1, "one)


I would just write val tuple2 = (1, "one"), even if you wanted to write the type explicitly you would write (Int, String), not Tuple2[Int, String]

| val tuple2: Tuple2[Int, String = (1, "one)
Shouldnt it be

| val tuple2: Tuple2[Int, String] = (1, "one)
?

I was unsure on this one, was trying to place myself in the shoes of a beginner who would even need -explain for this and wasn't sure if they would understand that (1 -> "one) is tuple.

smarter · 2016-10-25T17:21:33Z

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

+    val expectedArgString = (fntpe.widen match {
+      case pt: MethodOrPoly => pt //ensure we return a type that will have useful typeParms
+      case _ => fntpe
+    }).typeParams.map(_.paramName.show.split("\\$").last).mkString("[", ", ", "]")


As much as possible, we should avoid playing with regexps on names in error messages, this way lies madness. If there are issues with the way we print names, they should be fixed in a common place, not worked around in every error message.
A slightly nicer way:

import NameOps._ //... }).typeParams.map(_.paramName.unexpandedName.show).....

Technically this is still incorrect, unexpandedName should only be called on a name of a symbol with the flag ExpandedName but we have no way of checking that here, I think the correct solution would be to add a def simpleParamName method to TypeParamInfo, but this could be mentioned in a TODO comment and done later.

I second the TODO approach - and perhaps an issue?

Any reason for me not to just add def simpleParamName in this PR?

You can if you figure out how (you need an implementation in Symbol and one in LambdaParam, for Symbol you check if this.is(ExpandedName), for LambdaParam you just use the regular paramName which should never be expanded (I think?)

smarter · 2016-10-25T17:24:51Z

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

+      case _ => fntpe
+    }).typeParams.map(_.paramName.show.split("\\$").last).mkString("[", ", ", "]")
+
+    val actualArgString = actual.map(_.show.split("\\.").last).mkString("[", ", ", "]")


This is also a dubious usage of a regexp, what if the user wrote List[Foo.A, Bar.A]? You just rewrote this to List[A, A] which is bound to be more confusing. We don't have a good solution to prefix-eliding right now but this isn't it.

I can't think of a great solution for prefix eliding without knowing what is in scope which feels way beyond me right now. Any other ideas, or do you prefer the full verbose including $$ name? To handle the case you mentioned maybe I could find some way to get distinct types and then only keep enough of the prefix to disambiguate them? If I just had Foo.A and Bar.B I would just show A, B but if I had Foo.A and Bar.A I would show Foo.A and Bar.A?

I'm wondering how much of the functionality from asSeenFrom can be used here...I say, show the full path (without $$$$$) otherwise.

I remember why I had no luck with asSeenFrom, one of the four callers of this function has an untyped arg tree. Should I split the message into two, one for typed callers and one for untyped callers, or just leave the full path?

If it doesn't have a type (there's a typeOpt function available to trees as well as hasType) - go for the full type - otherwise asSeenFrom?

smarter · 2016-10-25T17:25:39Z

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

+             |  into another data type that can contain the number of types you need,
+             |  In this example one solution would be to use a Tuple:
+             |  val tuple2: Tuple2[Int, String = (1, "one)
+             |    List[(Int, String)] = List(tuple2)""".stripMargin


Extra indentation of two spaces compared to the previous line

Oops. I was using indentation to try to group related or nested lines of code, but is the concensus that I should just left align everything?

Yes please :) That's been the policy thus far.

ShaneDelmore · 2016-10-25T20:40:19Z

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

+    val prettyName = fntpe.termSymbol match {
+      case NoSymbol => fntpe.show
+      case symbol   => symbol.showFullName
+    }


I would like to make another PR to add def prettyName (or something like it) to a few types. I will leave it as a follow up unless you prefer it as part of this PR.

With these changes

object Test { var bad = List[Int, String] case class Bar[D, E](baz: D, qux: E) var bad3: Bar[Int] = null }

results in

-- [E019] Syntax Error: test.scala ------------------------------------------------------------------------------------- 16 | var bad = List[Int, String] | ^^^^^^^^^^^^^^^^^ | Too many type arguments for scala.collection.immutable.List.apply[A] | expected: [A] | actual: [Int, String] Explanation =========== You have supplied too many type parameters For example List takes a single type parameter (List[A]) If you need to hold more types in a list then you need to combine them into another data type that can contain the number of types you need, In this example one solution would be to use a Tuple: val tuple2: (Int, String) = (1, "one") val list: List[(Int, String)] = List(tuple2) -- [E019] Syntax Error: test.scala ------------------------------------------------------------------------------------- 19 | var bad3: Bar[Int] = null | ^^^^^^^^ | Not enough type arguments for Test.Bar[D, E] | expected: [D, E] | actual: [Int] Explanation =========== You have not supplied enough type parameters If you specify one type parameter then you need to specify every type parameter.

felixmulder

Otherwise LGTM, after the minor changes I think we should merge this and let you move on to new adventures :)

felixmulder · 2016-10-25T21:00:53Z

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

+    val actualCount = actual.length
+    val msgPrefix = if (actualCount > expectedCount) "Too many" else "Not enough"
+
+    val expectedArgString = fntpe.widen.typeParams.map(_.paramName.unexpandedName.show).mkString("[", ", ", "]")


This was supposed to have a //TODO right?

I added it now.

felixmulder · 2016-10-25T21:02:04Z

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

@@ -512,4 +513,75 @@ object messages {
        |}""".stripMargin
  }

+  case class WrongNumberOfArgs(fntpe: Type, argKind: String, expectedArgs: List[TypeParamInfo], actual: List[untpd.Tree])(implicit ctx: Context)
+    extends Message(19) {


The errorId has to be updated - to 21 I think - so 22 for the other one

Sorry, merged @AndrewZurn's, so 22 and 23 - you're next I promise :)

220 and 230 it is 👍

felixmulder · 2016-10-25T21:32:32Z

@ShaneDelmore - seems there is still a conflict in messages.scala, rebase? (no need to squash)

ShaneDelmore · 2016-10-25T21:37:40Z

I rebased and resolved the merge conflict.

ShaneDelmore commented Oct 25, 2016

View reviewed changes

ShaneDelmore force-pushed the 1589_Missing_error_messages branch from 179a0fe to 3790e93 Compare October 25, 2016 02:01

felixmulder suggested changes Oct 25, 2016

View reviewed changes

smarter reviewed Oct 25, 2016

View reviewed changes

ShaneDelmore commented Oct 25, 2016

View reviewed changes

felixmulder reviewed Oct 25, 2016

View reviewed changes

ShaneDelmore force-pushed the 1589_Missing_error_messages branch from 8f0dc96 to a83725a Compare October 25, 2016 21:22

felixmulder approved these changes Oct 25, 2016

View reviewed changes

ShaneDelmore added 3 commits October 25, 2016 14:37

Improve error message for WrongNumberOfArgs

63f1a3c

Update IllegalVariableInPatternAlternative error message

a2245dd

Improve error message formatting

aaae563

ShaneDelmore force-pushed the 1589_Missing_error_messages branch from a83725a to aaae563 Compare October 25, 2016 21:37

felixmulder merged commit aae4019 into scala:master Oct 25, 2016

#1589: Improve error message for WrongNumberOfArgs #1623

#1589: Improve error message for WrongNumberOfArgs #1623

Uh oh!

Conversation

ShaneDelmore commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixmulder left a comment

Choose a reason for hiding this comment

felixmulder left a comment •

edited

Loading

felixmulder commented Oct 25, 2016 •

edited

Loading