Poor interaction between ScalaRunTime.stringOf and Interpreter

[scabug]

Hi folks,

Since some time in the 2.8.0 branch, the interpreter started using ScalaRunTime.stringOf to show string values instead of calling objects' toString methods. It seems the motivation here is for stringOf to construct a python-repr-like string for an object at runtime, a noble goal.

Unfortunately, the current implementation of ScalaRunTime.stringOf falls short for custom collections. An elaborate instance check is performed to determine how to construct the string with several special cases that apply only to built in collections, with no hooks for external collections that extend Traversable to provide an alternative. The code block in question from the current head branch is attached, below, for reference.

More specifically, custom Traversable instances with definite size that are not part of scala.tools.nsc.io cannot provide a custom string. This interacts poorly with the interpreter for, e.g., all the numerical collections in scalala http://github.com/scalala/Scalala, for which appropriate toString methods exists. The custom toString methods on those collections are concise and readable, but the stringOf version are often useless and/or slow. The result is that the interpreter moves from being a useful matlab-like environment to something far less useful for many real interactions.

It seems like there are a few choices here:

1. Make toString behave correctly for all scala data types, so that stringOf just does the check for isArray (because java arrays' toStrings cannot be changed), but otherwise delegates to x.toString.

2. If there really were a use case for having stringOf and toString look different, instead define a HasStringRepr trait that defines a .toStringRepr method. Let Traversable extends this. Then stringOf can defer to x.toStringRepr if the class implements HasStringRepr, or x.toString otherwise unless it isArray.

3. Use a HasStringRepr view class, i.e. def stringOf[A:HasStringRepr](arg : A). This options seems the most flexible, as it will allow a custom string repr for Arrays, but requires more complexity in the Interpreter loop, because it can't easily be done by reflection.

1 and 2 seem easy enough work-arounds for the near term. I think 3 is the way to go, but there is some complexity to it. I've worked out the details for doing this more generally here: http://github.com/scalanlp/scalanlp-core/blob/master/data/src/main/scala/scalanlp/serialization/TextSerialization.scala

(scala.runtime.ScalaRuntime)
237	  def stringOf(arg: Any): String = {
238	    import collection.{SortedSet, SortedMap}
239	    def mapTraversable(x: Traversable[_], f: Any => String) = x match {
240	      case ss: SortedSet[_] => ss.map(f)
241	      case ss: SortedMap[_, _] => ss.map(f)
242	      case _ => x.map(f)
243	    }
244	    def inner(arg: Any): String = arg match {
245	      case null                     => "null"
246	      // Node extends NodeSeq extends Seq[Node] strikes again
247	      case x: Node                  => x toString
248	      // Not to mention MetaData extends Iterable[MetaData]
249	      case x: MetaData              => x toString
250	      // Range/NumericRange have a custom toString to avoid walking a gazillion elements
251	      case x: Range                 => x toString
252	      case x: NumericRange[_]       => x toString
253	      case x: AnyRef if isArray(x)  => WrappedArray make x map inner mkString ("Array(", ", ", ")")
254	      case x: TraversableView[_, _] => x.toString
255	      case x: Traversable[_] if !x.hasDefiniteSize => x.toString
256	      case x: Traversable[_]        => 
257	        // Some subclasses of AbstractFile implement Iterable, then throw an
258	        // exception if you call iterator.  What a world.
259	        // And they can't be infinite either.
260	        if (x.getClass.getName startsWith "scala.tools.nsc.io") x.toString
261	        else (mapTraversable(x, inner)) mkString (x.stringPrefix + "(", ", ", ")")
262	      case x                        => x toString
263	    }
264	    val s = inner(arg)
265	    val nl = if (s contains "\n") "\n" else ""
266	    nl + s + "\n"   
267	  }
268	}

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-3710?orig=1
Reporter: Daniel Ramage (dramage)
Attachments:

• DateRange.scala (created on Aug 13, 2010 8:42:19 AM UTC, 694 bytes)

[scabug]

@paulp said:
In case it has any bearing on your evaluation, you may want to know the actual motivation for the complicated logic in stringOf. It is summarized as:

// Given:
List(Array(1))
// How to print as above and not as:
List([I@18f55759)

100% of the logic spins out of the inability to change Array's toString method. It does not arise from some high-minded attempt to improve everybody's toString method.

Your "HasStringRepr" idea is haskell's Show typeclass. It would be my preference to use Show (and Eq) in preference to universal equals and toString wherever possible, but there is not general agreement on this point and there are performance implications.

[scabug]

Daniel Ramage (dramage) said:
Replying to [comment:1 extempore]:

In case it has any bearing on your evaluation, you may want to know the actual motivation for the complicated logic in stringOf. It is summarized as:
{code}
// Given:
List(Array(1))
// How to print as above and not as:
List([I@18f55759)
}}
100% of the logic spins out of the inability to change Array's toString method. It does not arise from some high-minded attempt to improve everybody's toString method.

I appreciate the clarification. It did occur to me that this method would also be used recursively. So if the main motivation is really to fix java arrays' toString, why not go with proposal 1? Is there a reason that stringOf should avoid x.toString except on null and on java arrays? It seems like the only other branch that doesn't call x.toString is for Traversable with definite size not in scala.tools.nsc.io. Something's fishy ... why have this branch instead of just implementing a default, desirable toString on Traversable?

Your "HasStringRepr" idea is haskell's Show typeclass. It would be my preference to use Show (and Eq) in preference to universal equals and toString wherever possible, but there is not general agreement on this point and there are performance implications.

There's certainly complexity there, so I won't push for the typeclass way of doing things in the near term (but I'd love it in the long term). I'd imagine that one such scenario where a Show style typeclass isn't desirable is when the static type signature is a supertype of the runtime type, which has its own Show. Statically, the supertype's Show would be used instead of the subtype's, which seems like it might not be the desired behavior.

[scabug]

Daniel Ramage (dramage) said:
Replying to [comment:2 dramage]:

Replying to [comment:1 extempore]:

In case it has any bearing on your evaluation, you may want to know the actual motivation for the complicated logic in stringOf. It is summarized as:
{code}
// Given:
List(Array(1))
// How to print as above and not as:
List([I@18f55759)
}}
100% of the logic spins out of the inability to change Array's toString method. It does not arise from some high-minded attempt to improve everybody's toString method.

I appreciate the clarification. It did occur to me that this method would also be used recursively. So if the main motivation is really to fix java arrays' toString, why not go with proposal 1? Is there a reason that stringOf should avoid x.toString except on null and on java arrays? It seems like the only other branch that doesn't call x.toString is for Traversable with definite size not in scala.tools.nsc.io. Something's fishy ... why have this branch instead of just implementing a default, desirable toString on Traversable?

A bit more concretely, why not change Traversable's toString (which ultimately delegates to TraversableOnce.addString) to use ScalaRunTime.stringOf(x) instead of x.toString?

[scabug]

@paulp said:
Replying to [comment:3 dramage]:

A bit more concretely, why not change Traversable's toString (which ultimately delegates to TraversableOnce.addString) to use ScalaRunTime.stringOf(x) instead of x.toString?

That's up to martin. For better or worse the "official" way to stringify objects in scala is .toString. The repl is a piece of software we ship and we can reasonably improve on that behavior in that context. Changing how Traversable goes about it is a more far-reaching change. If we were going to do it I'd hope to do a lot better than catching NPEs and improving how Array is printed, not that those aren't worthwhile things.

A simple backward compatible approach would be to define a new method like toScalaString with a default implementation which calls toString, maybe even taking some useful implicit argument with an empty default. But so far martin hasn't been too enthusiastic about such proposals.

[scabug]

Daniel Ramage (dramage) said:
Replying to [comment:4 extempore]:

That's up to martin. For better or worse the "official" way to stringify objects in scala is .toString. The repl is a piece of software we ship and we can reasonably improve on that behavior in that context. Changing how Traversable goes about it is a more far-reaching change. If we were going to do it I'd hope to do a lot better than catching NPEs and improving how Array is printed, not that those aren't worthwhile things.

So it seems like there are really two things going on.

One is a bug in the REPL: the "official" way to stringify an object in scala is not used by the REPL for types that extend Traversable. Perhaps this ticket should be re-filed as a defect rather than enhancement, accordingly?

At the same time, we're starting a discussing on something that's more of a feature or enhancement, which is how to fix toString in some more general way. While that would be great, I don't think we should have to do it before we fix the REPL.

A simple backward compatible approach would be to define a new method like toScalaString with a default implementation which calls toString, maybe even taking some useful implicit argument with an empty default. But so far martin hasn't been too enthusiastic about such proposals.

This would be a method on Traversable? Seems like adding a new method (and thereby introducing more API that might change) might be a bigger deal than changing Traversable's toString to properly handle nulls and arrays ...

dan

[scabug]

@paulp said:
Replying to [comment:5 dramage]:

While that would be great, I don't think we should have to do it before we fix the REPL.

We don't. That's why the ticket is still open.

This would be a method on Traversable? Seems like adding a new method (and thereby introducing more API that might change) might be a bigger deal than changing Traversable's toString to properly handle nulls and arrays ...

The question is bigger than Traversable. This is like doing null checks in some subset of dereferences. You do a little of it and it becomes an endless series of why here and not there. Whatever the mechanism for stringifying an object is to be, we should adhere to it consistently.

[scabug]

@lrytz said:
we discussed this in the meeting and decided not to change Traversable

[scabug]

Daniel Ramage (dramage) said:

Thanks for discussing this at the meeting. I'm confused by the wontfix resolution, though, because this ticket has two parts, one is a enhancement and the other is a defect.

The enhancement under discussion involves changing Traversable's toString, and it's fine that the answer there is wontfix, because it will be tricky for all the reasons outlined above and presumably for other reasons discussed at the meeting.

But the main reason for this ticket is that the way the REPL deals with Traversables is broken. Currently, the REPL uses ScalaRunTime.stringOf, which selectively avoids calling toString for anything that extends Traversable. So scala's interpreter loop ignores scala's official mechanism for turning objects into strings. This just has to be considered a defect, no?

[scabug]

@paulp said:
You're right -- I was going to reopen it anyway. When lukas says "the meeting" that includes most of the key developers except for me, and I'm the guy who would have said "we still have to fix the repl printing."

[scabug]

@lrytz said:
that means that the repl won't print arrays nicely anymore? i guess that's up to you to decide, paul, but from what we discussed in the meeting we would agree that using runtime.stringOf in the repl is not a bad thing.

[scabug]

@paulp said:
Replying to [comment:12 rytz]:

that means that the repl won't print arrays nicely anymore? i guess that's up to you to decide, paul, but from what we discussed in the meeting we would agree that using runtime.stringOf in the repl is not a bad thing.

No, I wasn't suggesting we stop using stringOf, only that we take a harm minimization strategy in so doing.

[scabug]

Alex McGuire (cage433) said:
A simple Day implementation that breaks in the interpreter

[scabug]

Alex McGuire (cage433) said:
This method is causing me a problem in the repl that I think you may have already met with Node and MetaData. We have various date range classes (Day, Month, Year etc) that implement Iterable[Day]. When I create a Day object in the repl I get a stack overflow due to its being an instance of Iterable[Day].

I think choice 2 would fix this. For now I think I can work around the problem by getting my DateRange class to extend IterableView.

I've attached a minimal case. attachment:ticket:3710:DateRange.scala

Running 'scala test.Day' causes the stack overflow

[scabug]

@paulp said:
(In r22766) Overhaul of ScalaRunTime.stringOf. Simplified the logic, but the
material changes are a) deferring to the target object's toString method
on custom collections, so now only built-in collections receive special
treatment, and b) guarding against runaways and inappropriate
exceptions, falling back on toString. Closes #3710, Review by prokopec.

[scabug]

@paulp said:
Hopefully everyone will find r22766 to their satisfaction.

[scabug]

Daniel Ramage (dramage) said:
Excellent! Thanks, extempore. This does the job.