Filter backtrace before formatting in #handle_exception

[joshbroughton]

Why is this change needed?

The current implementation of handle_exception formats the exception backtrace using Exception#full_message prior to applying filter_backtrace on split lines of the full_message. Other projects that use irb intuitively expect that backtrace filtering will act on objects that conform to the Exception#backtrace API, and this discrepancy has introduced a bug in Rails where backtraces in the Rails console are entirely silenced (rails/rails#49259)

Closes #913

Why this solution?

We considered working around this in Rails, but the backtrace_cleaner class in Rails is not only used for the console; it still needs to work in other contexts where backtrace cleaning is actually being applied to Exception#backtrace.

This proposed change is the least amount of change to fix the behaviour - the filter now acts on Exception#backtrace, but the rest of the logic for formatting and order remains unchanged and relies on Exception#full_message. This seems to be a reasonable use case for Exception#full_message and avoids needing to rewrite the formatting and ordering logic that is already working.

The filtering previously did not apply to irb bugs (it was within unless irb_bug), so it was added to the else branch of the irb_bug conditional to preserve that behaviour.

Testing

This branch passes the tests run with bundle exec rake. It looks like handle_exception is tested by the tests eval_input within test_context.rb, which I briefly looked over and the test coverage seems good to me. This is my first contribution to irb, please let me know if there are other testing requirements.

With respect to fixing the bug in Rails, here is a screenshot showing the correct/expected backtrace in the Rails console in a fresh Rails test app pointing to the version of irb proposed in this PR.

[tompng]

I think IRB should not modify the original exception's backtrace.
How about exc = exc.dup before set_backtrace?

[joshbroughton]

That's a great point, I'll push up that change

[joshbroughton]

Hmm, this results in some test failures, whether added directly above exc.set_backtrace(backtrace) or at the very top of the method (before the conditional). I wonder if its because exc.dup creates a shallow copy?

I will investigate those failures more to see if exc.dup can work, but if not, should I look for some other way to avoid modifying the original exception's backtrace?

[tompng]

should I look for some other way to avoid modifying the original exception's backtrace?

Yes, please.

One way could be
lines = lines.map { |l| @context.workspace.filter_backtrace(l[/REGEXP_TO_EXTACT_BACKTRACE_PART/]) }.compact
Another way is to modify the backtrace and restore it in ensure statement, but I'd like to avoid doing this if possible.

[joshbroughton]

Sounds good. I appreciate the feedback, I'll explore the options when I have some time.

Should I close this PR and open a new one when I have a solution that doesn't modify the original exception's backtrace? Or should I just push that solution here when it is ready?

[joshbroughton]

Actually, I tried using exc.clone rather than exc.dup and that appears to resolve whatever part of the state wasn't getting copied using dup.

[st0012]

I think it's worth having this small fix to immediately improve Rails users' dev experience.

[st0012]

I think we can add some comments to make the intention and context clearer:

Suggested change

	backtrace = exc.backtrace.map { |l| @context.workspace.filter_backtrace(l) }.compact
	exc = exc.clone
	exc.set_backtrace(backtrace)
	# This is mostly to make IRB work nicely with Rails console's backtrace filtering, which patches WorkSpace#filter_backtrace
	# In such use case, we want to filter the exception's backtrace before its displayed through Exception#full_message
	# And we clone the exception object in order to avoid mutating the original exception
	# TODO: introduce better API to expose exception backtrace externally
	backtrace = exc.backtrace.map { |l| @context.workspace.filter_backtrace(l) }.compact
	exc = exc.clone
	exc.set_backtrace(backtrace)

[joshbroughton]

Thanks Stan!

What will release for this look like? Will it be patched onto older versions of irb or available on the next release? I have no idea how these things work. I'm wondering when we can mark the rails issue as fixed

[st0012]

This will be included in the next IRB release (1.13 or 1.12.1). If Rails apps bump their IRB version, they'll be running Rails console with this patch.

[joshbroughton]

Awesome, thanks!

[tompng]

Thank you! looks good 👍