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

Jump to bottom

Fix bug in RangeHelp where range_by_whole_lines goes past end of buffer #5908

Merged
merged 3 commits into from
May 26, 2018
Merged

Fix bug in RangeHelp where range_by_whole_lines goes past end of buffer #5908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a66faa8
Fix bug in RangeHelp where range_by_whole_lines could return a range …
May 22, 2018
9da4343
Add additional context to CHANGELOG
May 24, 2018
74f0fd4
Merge branch 'master' into fix-range-bug
bbatsov May 26, 2018
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* [#5894](https://github.com/bbatsov/rubocop/pull/5894): Fix `Rails/AssertNot` to allow it to have failure message. ([@koic][])
* [#5888](https://github.com/bbatsov/rubocop/issues/5888): Do not register an offense for `headers` or `env` keyword arguments in `Rails/HttpPositionalArguments`. ([@rrosenblum][])
* Fix the indentation of autocorrected closing squiggly heredocs. ([@garettarrowood][])
* [#5908](https://github.com/bbatsov/rubocop/pull/5908): Fix `Style/BracesAroundHashParameters` auto-correct going past the end of the file when the closing curly brace is on the last line of a file. ([@EiNSTeiN-][])
* Fix a bug where `Style/FrozenStringLiteralComment` would be added to the second line if the first line is empty. ([@rrosenblum][])

### Changes
Expand Down Expand Up @@ -3382,3 +3383,4 @@
[@balbesina]: https://github.com/balbesina
[@cupakromer]: https://github.com/cupakromer
[@TikiTDO]: https://github.com/TikiTDO
[@EiNSTeiN-]: https://github.com/EiNSTeiN-
10 changes: 3 additions & 7 deletions lib/rubocop/cop/mixin/range_help.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,13 @@ def range_with_surrounding_space(range:, side: :both,
def range_by_whole_lines(range, include_final_newline: false)
buffer = @processed_source.buffer

begin_pos = range.begin_pos
begin_offset = range.column
begin_of_first_line = begin_pos - begin_offset

last_line = buffer.source_line(range.last_line)
end_pos = range.end_pos
end_offset = last_line.length - range.last_column
end_offset += 1 if include_final_newline
end_of_last_line = end_pos + end_offset

Parser::Source::Range.new(buffer, begin_of_first_line, end_of_last_line)
range
.adjust(begin_pos: -range.column, end_pos: end_offset)
.intersect(buffer.source_range)
end

## Helpers for above range methods. Do not use inside Cops.
Expand Down
1 change: 1 addition & 0 deletions spec/rubocop/cop/range_help_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ class TestRangeHelp
let(:include_final_newline) { true }

it { is_expected.to eq('newline_at_end') }
it { expect(output_range.end_pos).to eq(source.size) }
end
end
end
Expand Down
8 changes: 8 additions & 0 deletions spec/rubocop/cop/style/braces_around_hash_parameters_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,14 @@
expect(corrected).to eq('get :i, x: 1')
end
end

context 'in a method call with multi line arguments without parentheses' do
it 'removes hash braces' do
src = "render 'foo', {\n foo: bar\n}"
corrected = autocorrect_source(src)
expect(corrected).to eq("render 'foo', \n foo: bar\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this wasn't working before the change. It seems like this is what should be in the CHANGELOG. We normally only add CHANGELOG references for changes that could impact users. The average user doesn't know what RangeHelp or range_by_whole_lines is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, do you have a preferred wording for the changelog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with

Fix `Style/BracesAroundHashParameters` auto-correct sometimes going past the end of the file.

Does that work better?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds fine to me. Thanks for adding some context. Do you know what in particular about this line of code caused it go past the end of the file? Maybe something about the spacing or new lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the example broken source is on line 267

src = "render 'foo', {\n  foo: bar\n}"

the problem is that this cop wants to remove the braces around hash argument, and to do that it will try to select the entire line where } is located including a possible trailing newline, to do so range_by_whole_lines will add 1 character to the end of the line here:
https://github.com/bbatsov/rubocop/blob/a66faa88ce004f8717aff136230d196e8be3ee2c/lib/rubocop/cop/mixin/range_help.rb#L68
and this makes the range go past the end of the buffer because } in this example is the last character in the buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like the issue comes from a closing curly bracket, } living on a separate line than the contents of the hash. If that is the case, it would be useful to add to the CHANGELOG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some clarification to changelog 👍

end
end
end

context 'when EnforcedStyle is no_braces' do
Expand Down