Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty array instead of nil for source_extract #36532

Merged
merged 1 commit into from Jul 15, 2019

Conversation

itsWill
Copy link
Contributor

@itsWill itsWill commented Jun 21, 2019

Summary

The source_extract method will return nil when it can't find the file name in
the backtrace, methods that consume this method expect an array and the nil ends
up causing type errors down the road like it happened here: #36341. This patch
introduces a to_a call that converts the nil into an empty array this makes it so
client classes can correctly handle a empty list of source code lines and not
trigger a type error.

Other Information

Note this is a bit defensive and preventative imo, this occured for the edge case described here: #36341 (comment) and potentially other unknown code paths could still cause this behaviour to occur, but I don't know of any.

At any rate I do like having the method return a [] instead of a nil and thing that is a better interface for this class in general since the method will be returning consistent types.

discussed this with @rafaelfranca and @gmcgibbon

@rails-bot rails-bot bot added the actionview label Jun 21, 2019
@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch from 90cd90d to 73fc006 Compare June 21, 2019 20:00
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good to merge this, but I want a second opinion as this could be considered overly defensive programming.

actionview/test/template/template_error_test.rb Outdated Show resolved Hide resolved
actionview/CHANGELOG.md Outdated Show resolved Hide resolved
@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch from 73fc006 to 8cf47a4 Compare June 21, 2019 21:06
@kaspth
Copy link
Contributor

kaspth commented Jun 21, 2019

I've been looking into the surrounding code and I don't source_extract is right. It uses formatted_code_for which could return a Hash if output was :html. However it never seems to be anything but the default :console from source_extract (because source_extract is only called from annotated_source_code.

My take is that we should simplify formatted_code_for to this while we're here:

def formatted_code_for(source_code, line_counter, indent)
  indent_template = "%#{indent}s: %s"

  source_code.map.with_index(line_counter) do |line, counter|
    indent_template % [counter, line]
  end
end

Otherwise calling to_a on the Hash would also break the code. And if not, the Hash would this when concat(annotated_source_code) is hit:

>> [].concat({})
Traceback (most recent call last):
        5: from /Users/kaspth/.rbenv/versions/2.6.3/bin/irb:23:in `<main>'
        4: from /Users/kaspth/.rbenv/versions/2.6.3/bin/irb:23:in `load'
        3: from /Users/kaspth/.rbenv/versions/2.6.3/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        2: from (irb):2
        1: from (irb):2:in `concat'
TypeError (no implicit conversion of Hash into Array)

Finally source_extract should have it's output argument removed. In fact, maybe we should just inline source_extract into annotated_source_code. I think we need to dig deeper here than just going for to_a and calling it a day — I'm very happy you're looking into this! 😊

@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch 3 times, most recently from f230f18 to ce2bd04 Compare June 24, 2019 21:28
@itsWill
Copy link
Contributor Author

itsWill commented Jun 24, 2019

@kaspth good catch 👍

I've removed the output parameter and cleaned up the code some, so we return [] instead of nil so we no longer need the to_a.

This is a much cleaner solution ❤️

@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch from ce2bd04 to 48eb4d8 Compare June 24, 2019 21:55
@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch from 48eb4d8 to f33dd81 Compare June 28, 2019 19:18
actionview/CHANGELOG.md Outdated Show resolved Hide resolved
The source_extract method will return nil when it can't find the file name in
the backtrace, methods that consume this method expect an array and the nil ends
up causing type errors down the road like it happened here: rails#36341. This
patch refactors the source_extract method so that it returns an empty
array instead of nil when it can't find the source code.

Co-authored-by: Kasper Timm Hansen <kaspth@gmail.com>
@itsWill itsWill force-pushed the add_to_a_to_annotated_source_code branch from f33dd81 to 526a5eb Compare July 14, 2019 19:04
@gmcgibbon gmcgibbon merged commit 88b9129 into rails:master Jul 15, 2019
@kaspth
Copy link
Contributor

kaspth commented Jul 15, 2019

❤️💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants