SI-9834 Improve error on failed op= by som-snytt · Pull Request #5454 · scala/scala

som-snytt · 2016-10-12T05:36:36Z

If rewriting x += y fails to typecheck, emit error messages
for both the original tree and the assignment.

If rewrite is not attempted because x is a val, then say so.

Supersedes #5254 from last June. I must've forgotten it was against 2.11. What fashions did we wear and what music did we listen to back then? Who was president?

adriaanm

Looks good overall, but I have a few questions after a first pass.

adriaanm · 2016-10-18T22:13:29Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

              if (Statistics.canEnable) Statistics.stopTimer(failedApplyNanos, appStart)
-                reportError
+              val t = reportError
+              val Apply(Select(qual2, _), args2) = t


val t@Apply(Select(qual2, _), args2) = reportError?

Check! People think newlines grow on trees. Wait was that a compiler joke? Next time someone asks for a feature, People think syntax grows on trees!

adriaanm · 2016-10-18T22:26:22Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                  case SilentResultValue(t) => t
+                  case err: SilentTypeError =>
+                    val t = reportError
+                    context.reporter.echo("Expression does not convert to assignment because:")


What's the reasoning behind echoing? I'd expect to see context.issue/context.warning?

Because there's no context.reporter.sendTextAlert? I don't remember, so you're probably right as rain.

I remember now, we're just appending to the reported error; we don't want to bump an error or warning count.

The reportError function should take the optional text to append.

Also, I don't know what 'start a review' and 'review changes' buttons do yet.

I think this may be missed by tools, like Ensime/Eclipse and maybe even Sbt. It might play better if the additional text was part of the original error.

I thought about it briefly last night and agree, but not sure about code yet. I haven't looked at dotty "explain" messages yet, and whether they include an "append advice" feature. I like the idea of decoupled, "something happened, and this additional text may help."

som-snytt · 2016-11-01T06:24:46Z

More retro history is that the crasher fix on 2.12 is #5179

som-snytt · 2016-11-01T18:32:51Z

Before issuing errors, update the error(s) at the tree.pos with the additional text. It would be nicer to err.copy(errMsg = "more") to preserve the actual TypeError in case a reporter cares.

som-snytt · 2016-11-01T18:42:32Z

@SethTisue "Hello, Seth, can you tell me how I broke the build?" (If I could talk to Seth's Build Tool like Siri.)

adriaanm · 2016-11-01T18:45:59Z

scala/scala-dev#251 is looking less and less spurious.

som-snytt · 2016-11-01T22:43:18Z

“Spuriouser and spuriouser!” Cried Alice (she was so much surprised, that for the moment she quite forgot how to speak good English).

som-snytt · 2016-11-25T21:16:44Z

How about showing the expansion as well:

scala> val arr1 = new Array[Byte](10)
arr1: Array[Byte] = Array(0, 0, 0, 0, 0, 0, 0, 0, 0, 0)

scala> val arr2 = new Array[Byte](10)
arr2: Array[Byte] = Array(0, 0, 0, 0, 0, 0, 0, 0, 0, 0)

scala> arr1(4) ^= arr2(5)
<console>:14: error: value ^= is not a member of Byte
  Expression does not convert to assignment because:
    type mismatch;
     found   : Int
     required: Byte
    expansion: arr1.update(4, arr1.apply(4).^(arr2(5)))
       arr1(4) ^= arr2(5)
               ^

scala> val j = collection.mutable.Set("a")
j: scala.collection.mutable.Set[String] = Set(a)

scala> j("a") += "b"
<console>:13: error: value += is not a member of Boolean
  Expression does not convert to assignment because:
    type mismatch;
     found   : String
     required: Boolean
    expansion: j.update("a", j.apply("a").$plus("b"))
       j("a") += "b"
              ^

I see the caret operator is not shown as $up.

If rewriting `x += y` fails to typecheck, emit error messages for both the original tree and the assignment. If rewrite is not attempted because `x` is a val, then say so. The error message at `tree.pos` is updated with the additional advice. SI-8763 Crash in update conversion When there are already errors, don't attempt mechanical rewrites.

som-snytt · 2016-11-26T08:48:26Z

/rebuild

lrytz · 2016-12-14T13:41:20Z

Looks very nice. Could we even include the receiver expression? Expression does not convert to assignment because x is not assignable.

retronym · 2016-12-14T22:37:28Z

Could we even include the receiver expression

This can lead to poor messages in cases when the expression is long, or contained desugarings. Usually we just identify it with the error position and only refer to the expressions type in the message.

som-snytt · 2016-12-14T23:05:01Z

That was my original reason, although the word receiver could be poorly received. But maybe use a simple ident when you've got one.

I also forgot my comment that the expansion string shows $plus but not $up. What's $up with that? Or more colloquially, Wass^?

The message will be perfect in dotty...

scala-jenkins added this to the 2.11.9 milestone Oct 12, 2016

adriaanm self-assigned this Oct 18, 2016

adriaanm suggested changes Oct 20, 2016

View reviewed changes

som-snytt force-pushed the issue/9834-2.11 branch from db9290a to 1854742 Compare November 1, 2016 18:28

som-snytt added 2 commits November 25, 2016 13:24

SI-9834 Show expansion of update on error

76598e8

som-snytt force-pushed the issue/9834-2.11 branch from 1854742 to 76598e8 Compare November 25, 2016 21:25

adriaanm approved these changes Dec 15, 2016

View reviewed changes

adriaanm merged commit e231c09 into scala:2.11.x Dec 15, 2016

SethTisue mentioned this pull request Feb 18, 2017

[ci: last-only] merge 2.12.x onto 2.13.x (Feb 17, 2017) #5718

Merged

This was referenced Apr 7, 2017

Crash in compiler on incorrect code scala/bug#8763

Closed

Compiler crash on undefined symbol in update assignment, symbol value <error> does not exist in scala/bug#9235

Closed

Error message with val and += is confusing scala/bug#9834

Closed

SethTisue modified the milestones: 2.11.11, 2.11.9 May 1, 2017

som-snytt deleted the issue/9834-2.11 branch June 19, 2017 00:44

Conversation

som-snytt commented Oct 12, 2016

Uh oh!

adriaanm left a comment

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!

som-snytt commented Nov 1, 2016

Uh oh!

som-snytt commented Nov 1, 2016

Uh oh!

som-snytt commented Nov 1, 2016

Uh oh!

adriaanm commented Nov 1, 2016

Uh oh!

som-snytt commented Nov 1, 2016

Uh oh!

som-snytt commented Nov 25, 2016

Uh oh!

som-snytt commented Nov 26, 2016

Uh oh!

lrytz commented Dec 14, 2016

Uh oh!

retronym commented Dec 14, 2016

Uh oh!

som-snytt commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants