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

[Fix #8718] Fix undefined methods of pseudo location #8823

Merged
merged 5 commits into from Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -13,6 +13,7 @@
* [#8801](https://github.com/rubocop-hq/rubocop/issues/8801): Fix `Layout/SpaceAroundEqualsInParameterDefault` only registered once in a line. ([@rdunlop][])
* [#8514](https://github.com/rubocop-hq/rubocop/issues/8514): Correct multiple `Style/MethodDefParentheses` per file. ([@rdunlop][])
* [#8825](https://github.com/rubocop-hq/rubocop/issues/8825): Fix crash in `Style/ExplicitBlockArgument` when code is called outside of a method. ([@ghiculescu][])
* [#8718](https://github.com/rubocop-hq/rubocop/issues/8718): Fix undefined methods of pseudo location. ([@ybiquitous][])

### Changes

Expand Down
17 changes: 15 additions & 2 deletions lib/rubocop/cop/offense.rb
Expand Up @@ -63,10 +63,23 @@ class Offense
attr_reader :corrector

PseudoSourceRange = Struct.new(:line, :column, :source_line, :begin_pos,
:end_pos)
:end_pos) do
alias_method :first_line, :line
alias_method :last_line, :line
alias_method :last_column, :column

def column_range
column...last_column
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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


def size
end_pos - begin_pos
end
alias_method :length, :size
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] This redefines Struct#size and Struct#length.

See https://ruby-doc.org/core-2.7.1/Struct.html#size-method

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's ok.

end
private_constant :PseudoSourceRange

NO_LOCATION = PseudoSourceRange.new(1, 0, '', 0, 1).freeze
NO_LOCATION = PseudoSourceRange.new(1, 0, '', 0, 0).freeze

# @api private
def initialize(severity, location, message, cop_name, # rubocop:disable Metrics/ParameterLists
Expand Down
45 changes: 45 additions & 0 deletions spec/rubocop/cop/offense_spec.rb
Expand Up @@ -180,4 +180,49 @@ def Foo
expect(offense.highlighted_area.source).to eq('Foo')
end
end

context 'when the location is pseudo' do
let(:location) { described_class::NO_LOCATION }

it 'returns a location with valid size and length' do
expect(offense.location.size).to eq 0
expect(offense.location.length).to eq 0
end

it 'returns a line' do
expect(offense.line).to eq 1
end

it 'returns a column' do
expect(offense.column).to eq 0
end

it 'returns a source line' do
expect(offense.source_line).to eq ''
end

it 'returns a column length' do
expect(offense.column_length).to eq 0
end

it 'returns the first line' do
expect(offense.first_line).to eq 1
end

it 'returns the last line' do
expect(offense.last_line).to eq 1
end

it 'returns the last column' do
expect(offense.last_column).to eq 0
end

it 'returns a column range' do
expect(offense.column_range).to eq 0...0
end

it 'returns a real column' do
expect(offense.real_column).to eq 1
end
end
end
16 changes: 16 additions & 0 deletions spec/rubocop/formatter/json_formatter_spec.rb
Expand Up @@ -157,5 +157,21 @@
it 'sets length value for :length key' do
expect(hash[:length]).to eq(8)
end

context 'when the location is pseudo' do
let(:location) { RuboCop::Cop::Offense::NO_LOCATION }

it 'returns a valid hash' do
expect(hash).to eq({
start_line: 1,
start_column: 1,
last_line: 1,
last_column: 0,
length: 0,
line: 1,
column: 1
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] start_column is 1, but last_column is 0. Because this method uses #real_column and #last_column as follows.
But I feel a bit weird for the difference. Is there no problem?

https://github.com/rubocop-hq/rubocop/blob/751edc7bf3df93d7ec5d59f5ac5b501627a6b723/lib/rubocop/formatter/json_formatter.rb#L68-L76

https://github.com/rubocop-hq/rubocop/blob/751edc7bf3df93d7ec5d59f5ac5b501627a6b723/lib/rubocop/cop/offense.rb#L193-L195

Copy link
Contributor

Choose a reason for hiding this comment

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

This does seem a little odd, however the pseudo source range is also not exactly accurate compared to other offenses since it is used to identify offenses for a file rather than an actual source range. I think it is OK for there to be a bit of non perfect behavior at the moment. It does kind of seem like there might be reason to do a larger refactor to better handle offenses in a file vs offenses in source code.

Maybe the offense itself shouldn't be responsible for the column information, and this kind of info should be handled in the location. I'm not sure how large of a refactor this would take.

Similarly, does it make sense to be setting the line number of the offense to 1 for NO_LOCATION? I think it's possible to have a file offense for a file with no code in it. NO_LOCATION = PseudoSourceRange.new(1, 0, '', 0, 0).freeze

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rrosenblum I agree with your idea, thank you. Surely, in this PR, it seems good that we avoid the big change of the current behavior.

But for the benefit of others, here is an example of comparison:

Current (line=1, col=1):

empty_file.rb:1:1: W: Lint/EmptyFile: Empty file detected.

Maybe (line=0, col=0):

empty_file.rb:0:0: W: Lint/EmptyFile: Empty file detected.

I will follow the decision of the community.

end
end
end
end