Fix #4415: remove show from repl #4437

OlivierBlanvillain · 2018-05-02T09:42:17Z

This PR removes the infrastructure around the Show type class in the repl. Instead we simply call .toString as done in scalac. It fixes #4415 and should make things more stable.

The goal of Show was to add quotes around strings to make the output of repl copy pastable. I think using a type class with a default for Any is the wrong approach as it gives inconsistent results depending on the type:

Starting dotty REPL...
scala> List("foo", "bar") 
val res0: List[String] = List("foo", "bar")

scala> Option(res0)           // So far so good...
val res2: Option[List[String]] = Some(List("foo", "bar"))

scala> Some(res0)             // No instance of Some[String], fallback to showAny!
val res1: Some[List[String]] = Some(List(foo, bar))

allanrenucci

Great to see this part of the REPL simplified! 🎉

allanrenucci · 2018-05-02T09:46:42Z

compiler/src/dotty/tools/repl/Rendering.scala

+    val string = value match {
+      case null => "null" // Calling .toString on null => NPE
+      case ""   => "\"\"" // Sepcial cased for empty string, following scalac
+      case x    => x.toString


Another special case for Array?

allanrenucci · 2018-05-02T09:50:20Z

compiler/src/dotty/tools/repl/Rendering.scala

-        .getDeclaredMethods.find(_.getName == sym.name.toString + "Show").get
-        .invoke(null).toString
-
+        .getDeclaredMethods.find(_.getName == sym.name.encode.toString).get


Does this assume a getter? Do we generate a getter for case like this:

> private[this] val foo = 1

It looks like we're fine:

scala> private[this] val foo = 1 1 |private[this] val foo = 1 |^^^^^^^ |Illegal start of statement: no modifiers allowed here

But I guess it doesn't hurt to avoid the .get...

I was wondering if you could access the field instead

allanrenucci · 2018-05-02T09:53:41Z

compiler/src/dotty/tools/repl/Rendering.scala

-    else if (res.startsWith(str.REPL_SESSION_LINE))
-      Some(res.drop(str.REPL_SESSION_LINE.length).dropWhile(c => c.isDigit || c == '$'))
+    else if (string.startsWith(str.REPL_SESSION_LINE))
+      Some(string.drop(str.REPL_SESSION_LINE.length).dropWhile(c => c.isDigit || c == '$'))


I know the code above is not new, but I'm wondering why this is needed

Ok I get it. It is to pretty print a class. However it quickly leaks:

scala> class Foo // defined class Foo scala> new Foo val res0: Foo = Foo@7baf1f5a scala> println(res0.toString) rs$line$1$Foo@7baf1f5a

It is the same in scalac, so I guess that's fine. I imagine we could generate a default toString for classes in the REPL, but it is maybe not worth the trouble

I would say it's fine to show the truth in this case, messing up with the object's toString might lead to more confusion...

allanrenucci · 2018-05-02T09:55:53Z

compiler/src/dotty/tools/repl/Rendering.scala

+        .invoke(null)
+    val string = value match {
+      case null => "null" // Calling .toString on null => NPE
+      case ""   => "\"\"" // Sepcial cased for empty string, following scalac


typo: Special

allanrenucci · 2018-05-02T17:53:40Z

Since this fixes #4415, can you add a test for it:

scala> lazy val x = ???
val x: Nothing = <lazy>

smarter · 2018-05-02T20:25:41Z

I'm not convinced having a few hardcoded cases is better than a Show typeclass. I agree that the Show typeclass isn't ideal if it's only used by the REPL, but if we agree that having a standard Show typeclass in the standard library of Scala is useful, then it makes a lot of sense to use it for the REPL.

smarter · 2018-05-02T20:26:24Z

/cc @felixmulder Maybe you want to defend Show too :).

felixmulder · 2018-05-02T20:38:08Z

Obviously the repl should only be able to show things that have a Show instance 😜

But I'm not emotionally attached to it even though I agree with Guillaume's comment :)

OlivierBlanvillain · 2018-05-02T21:29:22Z

A proper Show type class would clearly be better, but what we have right now is simply a fancy way to hardcoded a few cases with a fallback to .toString. Using implicit resolution makes things more fragile and somewhat less consistent (would you have guessed the output of my code snippet?). Given that we've already had a couple of issues related to Show, the proposal is to burn it with fire :)

felixmulder · 2018-05-03T05:54:14Z

So to fix Show, you'd have to both fix implicit resolution and remove the default value (thus no Java show - how would we solve that anyway with a principled type class 😭)?

If that's correct then we'd still have to fix implicit resolution to accommodate contravariant typeclasses at some point?

smarter · 2018-05-03T07:35:47Z

we'd still have to fix implicit resolution to accommodate contravariant typeclasses at some point?

This is already "fixed" in Dotty compared to scalac, unless you have something else in mind than what Dotty does now? See #4329

felixmulder · 2018-05-03T07:38:07Z

@smarter - that's what I thought as well. So I'm sort of curious what part is broken about our Show when it comes to "fickle implicits". 🙂

Maybe it's the base case for Any that needs to disappear?

OlivierBlanvillain · 2018-05-03T11:33:43Z

There isn't anything to fix about implicits, they are just the wrong tool for the job. I think people are used to have the REPL produce Strings for everything:

scala> class Foo
defined class Foo

scala> val a = new Foo
a: Foo = Foo@377008df

Unless we plan to change the above (and update the entire Scala ecosystem to provide instances of Show), we shouldn't use a typeclass.

For reference, here is what you get in ghci if you forget the deriving Show:

Prelude> data Shape = Circle Float Float Float | Square Float -- deriving Show
Prelude> Circle 10 20 5 

<interactive>:2:1: error:
    • No instance for (Show Shape) arising from a use of ‘print’
    • In a stmt of an interactive GHCi command: print it

felixmulder · 2018-05-03T11:46:40Z

If you just want feature parity with Scala 2 and to close some minor bugs - then yes, you're absolutely right.

I think we can do better with Dotty, but I leave the how up to you 😀

Blaisorblade · 2018-05-03T16:18:47Z

I think the best implementation of Show that we have is http://www.lihaoyi.com/PPrint/, unsurprisingly, but it relies on current macros.

Re variance, it seems Show should be made contravariant, and that would fix the issue with Show[Some[T]]. Testing just the implicit search in Dotty:

trait Show[-T] { def show(x: T): String }
implicit def showOption[T]: Show[Option[T]] = new Show { def show(x: Option[T]) = "showOption" } // XXX
implicitly[Show[Some[String]]].show(Some(""))

Of course, that wouldn't fix the issue with showing values upcast to Any.

Unless we plan to change the above (and update the entire Scala ecosystem to provide instances of Show), we shouldn't use a typeclass.

Yes. The de-facto policy seems that we're trying to keep the same library as Scala2.

allanrenucci · 2018-05-04T18:33:45Z

@Blaisorblade Show used to be contravariant but was changed in cee2b50#diff-4c776b5d5db3fc261f0abe59b7a1a803

We got ambiguous implicit errors when Nothing was inferred:

val x = List() // error ambiguous implicit. found charShow and floatShow...

Blaisorblade · 2018-05-04T19:23:30Z

We got ambiguous implicit errors when Nothing was inferred:

I see why, but adding a more specific Show[Nothing] instance should fix this. Turns out it doesn't, but I'd wonder if that's either a specification or implementation bug — wouldn't it affect any contravariant typeclass? I'd certainly like to define class Ord[-T], Ord[List[T]] and Ord[Set[T]] (either in some extra library, or in the future in the standard library).

(EDIT: in the example below instName is just a cheap way to test the found instance).

trait Show[-T] { def show(x: T): String; def instName: String }
implicit def showOption[T]: Show[Option[T]] = new Show { def show(x: Option[T]) = "showOption"; def instName = "showOption" } // XXX
scala> implicitly[Show[Nothing]].instName
val res5: String = "showOption"
scala> implicit def showNothing: Show[Nothing] = new Show[Nothing] { def show(x: Nothing) = ""; def instName = "showNothing" }
implicit val showNothing: Show[Nothing]
scala> implicitly[Show[Nothing]].instName
val res6: String = "showNothing"

scala> implicit def showList[T]: Show[List[T]] = new Show { def show(x: List[T]) = "showList"; def instName = "showList" } // XXX
implicit val showList[T] => Show[List[T]]
scala> implicitly[Show[Nothing]].instName
1 |implicitly[Show[Nothing]].instName
  |                         ^
  |ambiguous implicit arguments: both method showList and method showNothing match type Show[Nothing] of parameter ev of method implicitly in object DottyPredef

OlivierBlanvillain · 2018-05-05T05:11:19Z

wouldn't it affect any contravariant typeclass?

The pattern is called implicit spaghetti, people try to avoid it ;)

Seriously, if you need contravariance + an instance for Nothing, it's really not a typeclass...

sir-wabbit · 2018-05-05T06:41:25Z

Why not? There is nothing wrong with instances for Nothing.

new Show[Nothing] {
  def show(n: Nothing): String = n
}

is a perfectly valid Show.

I would not add Show[Any] though, that is evil 😉

The main reason people don't put variance on typeclasses in Scala2 is because implicit resolution w/ variance simply doesn't work there in most cases.

OlivierBlanvillain · 2018-05-05T07:03:35Z

Right, I mixed the two, the non sense is a Nothing instance for a covariant typeclass and an Any instance for a contravarant one 😄

Blaisorblade · 2018-05-05T10:48:00Z

@alexknvl Opened a separate issue on typeclass/implicit resolution.

lihaoyi · 2018-05-06T01:11:27Z

Note that Ammonite made the same decision and moved it's pretty-printing (https://github.com/lihaoyi/PPrint) to pprintln(x: Any) and based on asInstanceOf rather than an typeclass. You can configure it by manually registering Class[_] => pprint.Tree handlers, rather than relying on implicit resolution.

While it would be nice to "fix" implicit resolution to make Show always work, I highly doubt you'll be able to get implicit resolution stable enough to work 100% of the time in the presence of all the various type-system tricks that people and libraries use. Getting it working 100% of the time is necessary for something as simple as showing the result of a value at the REPL; people do not like it when things do not compile for a feature they never asked for!

In practice, manually recursing over Any using asInstanceOf checks works well enough, is 100% stable (falling back to .toString for un-handled cases), and have the advantage of printing Seq(1, 2, 3) and Seq(1, 2, 3): Any the same way

lihaoyi · 2018-05-06T01:19:22Z

@Blaisorblade your information is out of date, PPrint has been macro-free for almost exactly a year now. In fact it could probably be rewritten in Java if necessary; it doesn't require fancy Scala stuff at all. I would be highly surprised if you couldn't copy-paste the 10 files of the pprint codebase into dotc and have it compile out of the box

Blaisorblade · 2018-05-06T11:00:45Z

@lihaoyi Ah thanks for the correction, I didn't find that in http://www.lihaoyi.com/PPrint/#TPrint, and I might have been misled by https://github.com/lihaoyi/PPrint/blob/005a51a509bae088dc69c4427a27280249ce8cf6/pprint/shared/src/main/scala-2.10%2B/TPrintImpl.scala. Is that dead code?

have the advantage of printing Seq(1, 2, 3) and Seq(1, 2, 3): Any the same way

👍 yes, I think that's crucial for a REPL, the typeclass issues are probably more relevant for other typeclasses.

lihaoyi · 2018-05-06T13:38:37Z

@Blaisorblade oh TPrint is used just for printing the type signature, not the actual value. For some reason that hasn't caused any stability issues like the typeclass-based value pretty-printer has

If you want to pretty-print values in dotty, just delete the TPrint source files when you copy-paste

allanrenucci · 2018-05-07T13:58:34Z

Having something similar to PPrint for the Dotty REPL would be nice. This can be implemented in a separate PR.

dwijnand · 2018-05-13T22:43:24Z

I have a dead PR against scala/scala to "Introduce ReplPrinter as a hook for alternatives to replStringOf" that might be useful inspiration: scala/scala#5222.

OlivierBlanvillain added 3 commits May 2, 2018 11:28

Remove show in repl; use .toString instead

9793424

Actually call .toString instead going thought generated *Show method

88b62e8

Special case .toString for null and empty String

cfcc624

OlivierBlanvillain assigned allanrenucci May 2, 2018

OlivierBlanvillain requested a review from allanrenucci May 2, 2018 09:42

allanrenucci approved these changes May 2, 2018

View reviewed changes

Special case .toString on Arrays

ec958ac

allanrenucci removed their assignment May 3, 2018

Blaisorblade mentioned this pull request May 5, 2018

Contravariant typeclasses and specificity for Nothing were not yet good enough for ourselves? #4465

Closed

1 task

allanrenucci merged commit 9a30e20 into scala:master May 7, 2018

allanrenucci deleted the remove-show branch May 7, 2018 13:55

Fix #4415: remove show from repl #4437

Fix #4415: remove show from repl #4437

Uh oh!

Conversation

OlivierBlanvillain commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci 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!

allanrenucci commented May 2, 2018

Uh oh!

smarter commented May 2, 2018

Uh oh!

smarter commented May 2, 2018

Uh oh!

felixmulder commented May 2, 2018

Uh oh!

OlivierBlanvillain commented May 2, 2018

Uh oh!

felixmulder commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented May 3, 2018

Uh oh!

felixmulder commented May 3, 2018

Uh oh!

OlivierBlanvillain commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixmulder commented May 3, 2018

Uh oh!

Blaisorblade commented May 3, 2018

Uh oh!

allanrenucci commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain commented May 5, 2018

Uh oh!

sir-wabbit commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lihaoyi commented May 6, 2018

Uh oh!

lihaoyi commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented May 6, 2018

Uh oh!

lihaoyi commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci commented May 7, 2018

OlivierBlanvillain commented May 2, 2018 •

edited

Loading

felixmulder commented May 3, 2018 •

edited

Loading

OlivierBlanvillain commented May 3, 2018 •

edited

Loading

allanrenucci commented May 4, 2018 •

edited

Loading

Blaisorblade commented May 4, 2018 •

edited

Loading

sir-wabbit commented May 5, 2018 •

edited

Loading

OlivierBlanvillain commented May 5, 2018 •

edited

Loading

Blaisorblade commented May 5, 2018 •

edited

Loading

lihaoyi commented May 6, 2018 •

edited

Loading

lihaoyi commented May 6, 2018 •

edited

Loading