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.

Sign up for GitHub

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

Fix gemspec sanitization bug when heredoc has methods chained onto it #3220

Merged
merged 4 commits into from
Mar 6, 2021
Merged

Fix gemspec sanitization bug when heredoc has methods chained onto it #3220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e25a21c
Add a failing test to demonstrate gemspec sanitization bug
jasonrudolph Mar 2, 2021
8101b77
Fix gemspec sanitization bug when heredoc has methods chained onto it
jasonrudolph Mar 4, 2021
c1a9fed
Merge main into gracefully-handle-gemspecs-with-heredocs-with-method-…
jasonrudolph Mar 4, 2021
d54e24e
Merge remote-tracking branch 'origin/main' into gracefully-handle-gem…
xlgmokha Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions bundler/lib/dependabot/bundler/file_updater/gemspec_sanitizer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,8 @@ def node_calls_find_dot_find?(node)
def remove_unnecessary_assignments(node)
return unless node.is_a?(Parser::AST::Node)

if unnecessary_assignment?(node) &&
node.children.last&.location.respond_to?(:heredoc_end)
range_to_remove = node.loc.expression.join(
node.children.last.location.heredoc_end
)
if unnecessary_assignment?(node) && node_includes_heredoc?(node)
range_to_remove = node.loc.expression.join(find_heredoc_end_range(node))
return replace(range_to_remove, '"sanitized"')
elsif unnecessary_assignment?(node)
return replace(node.loc.expression, '"sanitized"')
Expand All @@ -249,6 +246,30 @@ def remove_unnecessary_assignments(node)
end
end

def node_includes_heredoc?(node)
find_heredoc_end_range(node)
end

# Performs a depth-first search for the first heredoc in the given
# Parser::AST::Node.
#
# Returns a Parser::Source::Range identifying the location of the end
# of the heredoc, or nil if no heredoc was found.
def find_heredoc_end_range(node)
return unless node.is_a?(Parser::AST::Node)

node.children.each do |child|
next unless child.is_a?(Parser::AST::Node)

return child.location.heredoc_end if child.location.respond_to?(:heredoc_end)

range = find_heredoc_end_range(child)
return range if range
end

nil
end
Comment on lines +258 to +271
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We (@feelepxyz and I) suspect that there's a more readable way to implement this. We explored a few options, but the other options were a bit harder to follow in our opinion. So, if you have ideas for an alternative implementation that would be easier to read/maintain, definitely let us know. 🙇


def unnecessary_assignment?(node)
return false unless node.is_a?(Parser::AST::Node)
return false unless node.children.first.is_a?(Parser::AST::Node)
Expand Down
17 changes: 17 additions & 0 deletions bundler/spec/dependabot/bundler/file_updater/gemspec_sanitizer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,23 @@
)
end
end

context "that uses a heredoc with methods chained onto it" do
let(:content) do
%(Spec.new do |s|
s.version = "0.1.0"
s.post_install_message = <<~DESCRIPTION.strip.downcase
My description
DESCRIPTION
end)
end
it "removes the whole heredoc" do
expect(rewrite).to eq(
"Spec.new do |s|\n s.version = \"0.1.0\""\
"\n \"sanitized\"\n end"
)
end
end
end

describe "version assignment" do
Expand Down