Add printStackTrace to Console

[kubukoz]

Closes #1895.

[kubukoz]

I'm not sure if we can reliably test this that way: I suppose the representation of a stack trace on the JVM could vary between versions... maybe it's a good idea to keep it while it works, and remove if there are any issues in the future? Or should we just drop it now?

[kubukoz]

I guess we'll first find out whether it breaks between JDK 8 and 11.

[vasilmkd]

IMO, I think it's fine to just check that the custom message that we do control is present on the err stream.

[kubukoz]

uh oh.

[error] cats-effect-std: Failed binary compatibility check against org.typelevel:cats-effect-std_2.13:3.0.0! Found 1 potential problems
[error]  * abstract method printStackTrace(java.lang.Throwable)java.lang.Object in interface cats.effect.std.Console is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.std.Console.printStackTrace")

😢

A workaround I see would be manually printing in the trait (calling errorln with an imaginary string, like the one we have in tests right now) and overriding it in the standard implementation as an optimization.

[djspiewak]

Yeah I think the only way we can introduce this is by giving it a default implementation. Something like this might work:

  def printStackTrace(t: Throwable): F[Unit] = {
    val bos = new ByteArrayOutputStream()
    val ps = new PrintStream(bos)
    t.printStackTrace(ps)
    errorln(bos.toString)
  }

A real question though is whether or not this is entirely safe. I think that, in nearly all cases, printStackTrace will just be accessing memory and is safe to call in pure code (which is where this is), but it might be worth sanity checking that.

[rossabaker]

printStackTrace is not final and can launch the proverbial nukes, but the same argument applies to toString.

The default implementation is impure because exceptions are mutable:

scala> import java.io._
import java.io._

scala> val oops = new Exception("oops")
val oops: Exception = java.lang.Exception: oops

scala> val sw0 = new StringWriter
val sw0: java.io.StringWriter =

scala> oops.printStackTrace(new PrintWriter(sw0))

scala> oops.addSuppressed(new Exception("lulz"))

scala> val sw1 = new StringWriter
val sw1: java.io.StringWriter =

scala> oops.printStackTrace(new PrintWriter(sw1))

scala> sw0.toString == sw1.toString
val res8: Boolean = false

Adding suppressed exceptions is already controversial in our corner of the JVM. Printing the trace before suppressing the exception would be weird. We're not returning a String of the trace. It's impure, but only pedantically so.

[djspiewak]

Ugggghhhhh. Okay, @rossabaker, you have a lot more knowledge of horrible things that one can do with exceptions than I do (my heart is still pure!)… Is there a sane default we can throw on here?

[rossabaker]

I think the proposed default is fine. It matters when you:

1. mutate a Throwable
2. between constructing and evaluating the printStackTrace effect
3. when not using a console derived from the Sync instance
4. when you want to reason algebraically about what happens in this Unit return

It's impure and that sucks, but there are four stacked reasons not to worry. Scaladoc that "pure only if t is not mutated" and move on, I'd say.

[vasilmkd]

In the interest of trying to resolve this issue, I pushed out a couple of commits.

The idea is to copy all of the details from the original throwable to a throwable controlled by the method. The only difference is that the type of the Throwable (the class) is Console$anon because it is an anonymous class, which may not match the original throwable class.

Do we think this is acceptable? On the other hand, we get a bit more safety with regards to mutation.

@rossabaker @djspiewak @kubukoz Interested to hear your thoughts. Thanks.

[vasilmkd]

I managed to work around the changed type of the Throwable using some horrible hacking.

[kubukoz]

I'll give it a closer look later today 👀

[kubukoz]

actually no, I did it now.

The idea is to copy all of the details from the original throwable to a throwable controlled by the method.

@vasilmkd what's the purpose of this? Handling the purity problem? If so, I'd just go with the simplest thing that works (Daniel's idea here #1897 (comment)) and add the note about possible impurity

[vasilmkd]

The idea was to not rely on the printStackTrace of a Throwable that we do not control. Feel free to nuke these commits and use Daniel's solution.

[rossabaker]

I don't know if there are ever overrides to printStackTrace that are pure, but this would circumvent any of those.

This is quite clever, but I'm worried about unintended consequences when the flaws in the simpler method are both rare and easy to accept.

[vasilmkd]

I agree, the risk is not worth it ultimately. I was having too much fun yesterday.