SI-7740 Trim stack trace before printing in REPL #2824

Merged
merged 1 commit into from Aug 20, 2013

5 participants

@qerub

review @paulp

@scala-jenkins

(kitty-note-to-self: ignore 22522496)
🐱 Roger! Rebuilding pr-scala for 06b5d56f. 🚨

@som-snytt

My comment disappeared, you must have pushed the fix already... I was saying, the new stack string method could go with the other util methods, suitably named and parameterized.

@qerub

My comment disappeared, you must have pushed the fix already...

Yeah, I replaced the broken commit. GitHub still had a notification for your comment though.

the new stack string method could go with the other util methods, suitably named and parameterized.

Do you find def filteredStackTraceString(exception: Throwable, filter: Seq[StackTraceElement] => Seq[StackTraceElement]) suitable?

@qerub

PLS REBUILD/pr-scala@af4f9fe

I renamed the function to filteredStackTraceString and moved it to scala.tools.nsc.util.

@scala-jenkins

(kitty-note-to-self: ignore 22592536)
🐱 Roger! Rebuilding pr-scala for af4f9fe. 🚨

@scala-jenkins

(kitty-note-to-self: ignore 22626094)
🐱 Roger! Rebuilding pr-scala for af4f9fe. 🚨

@som-snytt

Here is what I was thinking: https://github.com/som-snytt/scala/compare/review;7740

That is, stackTracePrefixString takes a predicate for the takeWhile. I made it look a little more like stackTraceHeadString, and in handling empty message.

I threw some more names where it's used, like isWrapperInit, after I figured out what is tested for.

I think using regex in a match is the Scala way.

Lastly, I converted the test to SessionTest, where you can just paste a REPL session output into the string.

However, beware of the tabs in the stack trace. So my open question is, just because java uses tabs, do we have to use tabs to indent the stack trace? Other indent code in the codebase that I've seen uses spaces.

@som-snytt

I did figure out how to make this url:

som-snytt@f6d4040...e2402ba

@som-snytt

Is that kitten alive, or is it like when a cat crawls under the house to die?

@adriaanm
The Scala Programming Language member

It's more like when your dependents give you a surprise visit where the sole goal is extracting cash. (Broke dependents?)

PLS REBUILD ALL

@scala-jenkins

(kitty-note-to-self: ignore 22653974)
🐱 Roger! Rebuilding pr-scala for af4f9fe. 🚨

@qerub

I added some comments to som-snytt@e2402ba. Thanks for reviewing! <3

@qerub

Incorporated your changes, but decided that I prefer to not hide empty exception messages (to be consistent with Java and not confuse matters). Started using two spaces instead of a tab for indentation of the stack trace.

@som-snytt

Thanks, I was going to ask how to proceed.

I think getSimpleName crashes on some encodings in Scala because of the dollars. Do I remember correctly?

I guess I do:

scala> class Foo
defined class Foo

scala> classOf[Foo].getSimpleName
java.lang.InternalError: Malformed class name
    at java.lang.Class.getSimpleName(Class.java:1180)
@paulp

The commit message for that last doesn't properly communicate my sense of futility at ever seeing that fix go in. I'm not sure why I kept working on that sort of thing. Anyway do anything you want with that branch, because it will not be going anywhere with me.

@som-snytt

@paulp either filters on the word REPL or his ears burn when someone mentions something lying in a branch.

So we'll leave in getSimpleName and then they'll have to take the fix first. Or we'll leave out the fix and they'll have to take the Scala VM that people keep agitating for. That's the vm supporting scala names and tail recursion and has no bugs.

@paulp

In case anyone wonders where issue/2034 went, I'm cleaning up my branches. I attached the commit in the issue database: https://issues.scala-lang.org/browse/SI-2034

@gourlaysama gourlaysama and 3 others commented on an outdated diff Aug 15, 2013
src/repl/scala/tools/nsc/interpreter/IMain.scala
@@ -736,10 +737,18 @@ class IMain(@BeanProperty val factory: ScriptEngineFactory, initialSettings: Set
throw t
val unwrapped = unwrap(t)
+
+ // Example input: $line3.$read$$iw$$iw$
+ val importWrapper = """\$line\d+\.\$read\$(?:\$iw\$)*""".r
@gourlaysama
The Scala Programming Language member
gourlaysama added a line comment Aug 15, 2013

shoudln't $line and $read be the values of sessionNames.line and sessionNames.read rather than be hardcoded? Although I don't know if anyone has ever used the fact that they can be changed...

@paulp
paulp added a line comment Aug 15, 2013

That attempt to lift the abstraction bar was probably a failure; I'd be surprised if changing the names worked.

@gourlaysama
The Scala Programming Language member
gourlaysama added a line comment Aug 15, 2013

Ok, apparently $line is already hardcoded in a couple of places. It (accidentally) sill works fine though.

@som-snytt
som-snytt added a line comment Aug 15, 2013

When I looked at the imports code, I thought it was high time to abstract it; I didn't notice sessionNames.

But the next round is the Spark REPL issue, where clients can muck up the templating, so it will have to work by then.

@qerub
qerub added a line comment Aug 19, 2013

shoudln't $line and $read be the values of sessionNames.line and sessionNames.read rather than be hardcoded?

Good call. My latest commit uses the names from SessionNames via Naming.lineRegex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@som-snytt

Actually, I already fetched it and pushed it to the github.com/paulpatorium so posterity can study it in detail.

It was weird, granted, when I took a quick nap, refreshed the github page and got "not the page you want."

There's a John Donne sonnet, which I am unable to quote, in which he says how painful it must be for God to condemn a sinner, after all that work, to dig a pit, toss him in, and cover it with a stone.

I'm pretty sure a JIRA attachment is the equivalent of patch hell.

@adriaanm
The Scala Programming Language member

Welcome, @qerub! Please sign the CLA and address the merge failure.
Sorry, but the M5 merge window is closing tomorrow for non-blocker PRs. M6 is coming next month, though.

PS: as you push new commits to the PR, the bot automatically rebuilds them -- the rebuild command is only necessary when failure was external

@som-snytt

Recently saw a StackOverflow question with the needlessly long stack trace, so this change will have many benefits.

@qerub

The patch now uses lineRegex from Naming to keep working even if the user configures SessionNames.

I removed the extra commit for getSimpleName. If anyone wants to add a note about it in the code, I've created 77453815179ca4d3366b725753e39adde6cfae5e.

Please sign the CLA [...]

Done!

@adriaanm
The Scala Programming Language member

Awesome -- thanks!

@som-snytt

Dumb question, but can this work? Have we tried it? lineRegex doesn't match the $iw$s, so the case fails, right? without the unanchored.

Yes, the test passes.

lineRegex doesn't match the $iw$s

That is why I append ".*" to it.

This should be classNameRegex(_*) after all.

Did you remove this remark deliberately?

Yes, there's no capturing groups. Actually, I left to read a bedtime story, came back and was still distracted. I didn't even see the .*, sorry about that. As an aside, I do regex(_*) now routinely so I never worry about groups.

@qerub

@paulp, are you up for a review?

@paulp

I'm not a great reviewer these days. It looks fine to me for what that's worth.

@adriaanm
The Scala Programming Language member

LGTM by proxy, then

@adriaanm adriaanm merged commit 738441c into scala:master Aug 20, 2013

1 check passed

Details default pr-scala Took 78 min.
@som-snytt

Too late to matter, but I had marked this on my repo -- i imported the func but didn't remove the util prefix. Some people like to use prefixes, but I find util isn't helpful as a prefix because the util package is legion. Just a footnote.

OK, noted for possible future tweaks! Thanks.

@qerub qerub deleted the qerub:ticket/7740 branch Aug 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment