scala 3 repl does not show if a message is an error or a warning (regression from scala 2)

[bjornregnell]

Compiler version

3.0.0

Minimized code and output

scala> val x = if true then 42
1 |val x = if true then 42
  |                     ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

scala> val x = val
1 |val x = val
  |        ^^^
  |        expression expected but val found

Expectation

The first message above should include warning and the the second should include error. It is conceptually important to understand if the code has run or not, esp. for learners but also for professionals - I don't know all messages by hart if they are warnings or errors. Now it all looks the same in the Scala 3 REPL.

This is a regression from the Scala 2 REPL, in which you can always tell if a message is an error or a warning.

[som-snytt]

Warning text could be in yellow, error text in red and blinking!

But maybe manage that annoying behavior with -color:no-blink?

[bjornregnell]

I'm copy-pasting things around in various teaching material (also for B&W print) and also, (even) more importantly, some people are color blind so I think warning: and error: is still necessary. Although a colorful life is always nice 🌈
(Sorry if I took potential irony too serious :))

[bjornregnell]

@som-snytt Is this not too hard to fix for a REPL-hacker-noob like me, do you think? I mean, I managed to find out how to prohibit mal-truncation last time with your help ⛑️

[SethTisue]

for reference/comparison, Scala 2:

[dwijnand]

I don't know for sure but I would hazard a guess that this is very noob-friendly :)

[bjornregnell]

Thanks @dwijnand for encouragement. I'll hopefully find a coffee-break slot for looking into this soon.

[bjornregnell]

I have started to browse the code and the REPL has ParseResult and the compiler dotc itself has StoreReporter but I'm uncertain if this should just be a tweak to the printout at the outer rim of the REPL or it should go deep down into the rendering of error messages inside the compiler?

[dwijnand]

I think it's a REPL thing, as the batch compiler already displays it its own way:

$ scalac3 test/files/neg/t8597.1.scala
-- Warning: test/files/neg/t8597.1.scala:5:43 ----------------------------------
5 |  def warn1[T]  = (null: Any) match { case _: T            => } // warn
  |                                           ^^^^
  |                          the type test for T cannot be checked at runtime
1 warning found

[bishabosha]

you can look in compiler/src/dotty/tools/dotc/reporting/MessageRendering.scala for the way it is printed in the standard compiler

[bjornregnell]

Thanks for pointers! Will try to grok it. It seems to me that the REPL should reuse all the reporting stuff from dotc sans the file name + line pos marker, but apparently the REPL just utilizes the vertical bar stuff.

[bjornregnell]

My plan is to try to fix this before or during rendering.formatError which is called from displayErrors and I'm starting to investigate it with some good old println instrumentation but it seems as the error level does not differentiate between warnings and errors; check the level output here:

sbt:scala3> repl
[info] compiling 1 Scala source to /home/bjornr/git/hub/bjornregnell/dotty/compiler/target/scala-3.0.0/classes ...
[info] compiling 1 Scala source to /home/bjornr/git/hub/bjornregnell/dotty/out/bootstrap/scala3-compiler-bootstrapped/scala-3.0.2-RC1-bin-SNAPSHOT-nonbootstrapped/classes ...

scala> val x = if true then 42  // should be a warning but level == 2 not 1
*** DEBUG: start of displayErrors(List(class dotty.tools.dotc.reporting.Diagnostic$Error at rs$line$1:[21..23]: A pure expression does nothing in statement position; you may be omitting necessary parentheses))
*** DEBUG: errs.size = 1
*** DEBUG: errs.head.level = 2
*** DEBUG: rendering.formatError(errs.head) = class dotty.tools.dotc.reporting.Diagnostic at rs$line$1:[21..23]: 1 |val x = if true then 42
  |                     ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses
1 |val x = if true then 42
  |                     ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

scala> val x: String = 42
*** DEBUG: start of displayErrors(List(class dotty.tools.dotc.reporting.Diagnostic$Error at rs$line$1:[16..18]: Found:    (42 : Int)
Required: String))
*** DEBUG: errs.size = 1
*** DEBUG: errs.head.level = 2
*** DEBUG: rendering.formatError(errs.head) = class dotty.tools.dotc.reporting.Diagnostic at rs$line$1:[16..18]: 1 |val x: String = 42
  |                ^^
  |                Found:    (42 : Int)
  |                Required: String
1 |val x: String = 42
  |                ^^
  |                Found:    (42 : Int)
  |                Required: String

scala> def warn[T] = (null: Any) match { case _: T => }  // should also be a warning
*** DEBUG: start of displayErrors(List(class dotty.tools.dotc.reporting.Diagnostic$Error at rs$line$1:<39..39>: the type test for T cannot be checked at runtime))
*** DEBUG: errs.size = 1
*** DEBUG: errs.head.level = 2
*** DEBUG: rendering.formatError(errs.head) = class dotty.tools.dotc.reporting.Diagnostic at rs$line$1:<39..39>: 1 |def warn[T] = (null: Any) match { case _: T => }
  |                                       ^
  |                          the type test for T cannot be checked at runtime
1 |def warn[T] = (null: Any) match { case _: T => }
  |                                       ^
  |                          the type test for T cannot be checked at runtime

scala>                                                                                                                                                                                   

Above is the output of this tweaked displayErrors during the above sbt repl session:

  private def displayErrors(errs: Seq[Diagnostic])(implicit state: State): State = {
    println(s"*** DEBUG: start of displayErrors($errs)")
    println(s"*** DEBUG: errs.size = ${errs.size}")
    println(s"*** DEBUG: errs.head.level = ${errs.head.level}")
    println(s"*** DEBUG: rendering.formatError(errs.head) = ${rendering.formatError(errs.head)}")
    errs.map(rendering.formatError).map(_.msg).foreach(out.println)
    //println(s"*** DEBUG: end of displayErrors: State = $state")
    state
  }

Seems strange that the level attribute is not at level 1... but .... *Edit: see comments below

[bjornregnell]

Perhaps the Build's fatal warnings propagate? I'm checking currently with all fatal warnings turned off in Build.sbt.

[bjornregnell]

Yes! Turning off fatal warnings helped! Now trying to find ways to tweak the rendering... Hang on.

[bjornregnell]

SUCCESS: 😄

scala> val x = if true then 42                                                                                                                                                           
-- Warning:
1 |val x = if true then 42
  |                     ^^
  |A pure expression does nothing in statement position; you may be omitting necessary parentheses

scala> val x: String = 42                                                                                                                                                 
-- Error:
1 |val x: String = 42
  |                ^^
  |                Found:    (42 : Int)
  |                Required: String

I have found a minimal tweak of the REPL's messageRenderer.
I have no idea how to add tests for this; esp since all fatal warnings needs to be cleansed from Build.sbt for this to happen... But I guess I should not commit that as dotty devs want fatal warnings?

[bjornregnell]

@dwijnand Should I make a PR now?

[bishabosha]

you will probably have to change the sources in compiler/test-resources/repl, which are used to "replay" repl sessions and check the output (these are ran by compiler/test/dotty/tools/repl/ScriptedTests.scala)

in sbt shell
> testOnly dotty.tools.repl.ScriptedTests

[bjornregnell]

@bishabosha thanks; will testOnly dotty.tools.repl.ScriptedTests not have fatal warnings on?

[bjornregnell]

Aha I get it. Sorry. Checking errors now and fixing output.

[bjornregnell]

... a lot of manual work. Have just managed to do a tiny percentage of the tests so far. I'm considering writing a script or something.

[bjornregnell]

I've made a simple utility in repl-test-update to insert "Error:" and then tweaked the few "Warning:" cases manually.

[bjornregnell]

Now here: #13000 A nice and even PR number 😄
Happy for review input @bishabosha @dwijnand

[bjornregnell]

@bishabosha The build fails. Are there more sbt tasks than dotty.tools.repl.ScriptedTests that I should run? (I guess my little util needs to dig deeper into the guts of the repl test dir...)

[bjornregnell]

The CI build fails here: Test dotty.tools.repl.ShadowingTests.i7635 failed: java.lang.AssertionError: Error in script ,
But I can't find it even if I search for the file name. @bishabosha Any clue where it is?

[bjornregnell]

@bishabosha Found it: compiler/test/dotty/tools/repl/ShadowingTests.scala

[bjornregnell]

I'm running testOnly dotty.tools.repl.* now to be sure.

[bjornregnell]

All tests pass in CI build. 🥳

[bjornregnell]

@som-snytt Your colorful suggestion has gotten red alert attention with hope of a green light soon. (but no epi(lepti)c blink)