Skip to content

Better error message for ifs that miss an else branch #8672

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

Merged
merged 3 commits into from
Apr 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 10 additions & 9 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ object messages {
}
}

class TypeMismatch(found: Type, expected: Type, addendum: => String = "")(implicit ctx: Context)
class TypeMismatch(found: Type, expected: Type, addenda: => String*)(implicit ctx: Context)
extends TypeMismatchMsg(TypeMismatchID):

// replace constrained TypeParamRefs and their typevars by their bounds where possible
Expand Down Expand Up @@ -269,14 +269,15 @@ object messages {
val expected1 = reported(expected)
val (found2, expected2) =
if (found1 frozen_<:< expected1) (found, expected) else (found1, expected1)
val postScript =
if !addendum.isEmpty
|| expected.isAny
|| expected.isAnyRef
|| expected.isRef(defn.AnyValClass)
|| defn.isBottomType(found)
then addendum
else ctx.typer.importSuggestionAddendum(ViewProto(found.widen, expected))
val postScript = addenda.find(!_.isEmpty) match
case Some(p) => p
case None =>
if expected.isAny
|| expected.isAnyRef
|| expected.isRef(defn.AnyValClass)
|| defn.isBottomType(found)
then ""
else ctx.typer.importSuggestionAddendum(ViewProto(found.widen, expected))
val (where, printCtx) = Formatting.disambiguateTypes(found2, expected2)
val whereSuffix = if (where.isEmpty) where else s"\n\n$where"
val (foundStr, expectedStr) = Formatting.typeDiff(found2, expected2)(printCtx)
Expand Down
8 changes: 6 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package typer
import ast._
import core._
import Types._, ProtoTypes._, Contexts._, Decorators._, Denotations._, Symbols._
import Implicits._, Flags._
import Implicits._, Flags._, Constants.Constant
import util.Spans._
import util.SourcePosition
import java.util.regex.Matcher.quoteReplacement
Expand Down Expand Up @@ -98,7 +98,11 @@ object ErrorReporting {
val normTp = normalize(tree.tpe, pt)
val treeTp = if (normTp <:< pt) tree.tpe else normTp
// use normalized type if that also shows an error, original type otherwise
errorTree(tree, TypeMismatch(treeTp, pt, implicitFailure.whyNoConversion))
def missingElse = tree match
case If(_, _, elsep @ Literal(Constant(()))) if elsep.span.isSynthetic =>
"\nMaybe you are missing an else part for the conditional?"
Copy link
Member

Choose a reason for hiding this comment

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

If the Unit in the error message is the confusing part, maybe we could explain that:

Suggested change
"\nMaybe you are missing an else part for the conditional?"
"\nNote: an `if` without a corresponding `else` will always have type `Unit`"

Not sure if that's better.

Copy link
Contributor Author

@odersky odersky Apr 6, 2020

Choose a reason for hiding this comment

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

I think the single line is enough of a hint. An intelligent reader will connect the dots.

case _ => ""
errorTree(tree, TypeMismatch(treeTp, pt, implicitFailure.whyNoConversion, missingElse))
}

/** A subtype log explaining why `found` does not conform to `expected` */
Expand Down
6 changes: 6 additions & 0 deletions tests/neg/if-error.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- [E007] Type Mismatch Error: tests/neg/if-error.scala:2:2 ------------------------------------------------------------
2 | if ??? then System.in.read() // error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| Found: Unit
| Required: Int
| Maybe you are missing an else part for the conditional?
2 changes: 2 additions & 0 deletions tests/neg/if-error.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
val x: Int =
if ??? then System.in.read() // error
5 changes: 5 additions & 0 deletions tests/run/byname-varargs.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
a
b
a
b
result = (ArraySeq(1, 2),ArraySeq(1, 2))
12 changes: 12 additions & 0 deletions tests/run/byname-varargs.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
def f[T](xs: => T*): (Seq[T], Seq[T]) = (xs, xs)

def a =
println("a")
1

def b =
println("b")
2

@main def Test =
println(s"result = ${f(a, b)}")