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

Conversation

ybiquitous
Copy link
Contributor

@ybiquitous ybiquitous commented Oct 1, 2020

This change aims to fix the error due to undefined methods of RuboCop::Cop::Offence::PseudoSourceRange.

The RuboCop::Cop::Offence::PseudoSourceRange class emulates the Parser::Source::Range class in the parser gem.
(see https://github.com/whitequark/parser/blob/v2.7.1.5/lib/parser/source/range.rb)

The RuboCop::Cop::Offence#location attribute can have an instance of both these classes.
So, these classes need to have the same interface and semantics for the usage in RuboCop.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

This change aims to fix the error due to undefined methods of `RuboCop::Cop::Offence::PseudoSourceRange`.

The `RuboCop::Cop::Offence::PseudoSourceRange` class emulates the `Parser::Source::Range` class in the `parser` gem.
(see <https://github.com/whitequark/parser/blob/v2.7.1.5/lib/parser/source/range.rb>)

The `RuboCop::Cop::Offence#location` attribute can have an instance of both these classes.
So, these classes need to have the same interface and semantics for the usage in RuboCop.
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.


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.

length: 1,
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.

let(:location) { described_class::NO_LOCATION }

it 'returns a location with valid size and length' do
expect(offense.location.size).to eq 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember at all why end_pos is 1. Shouldn't it be 0 (and size too)?

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 following PR seems the origin of end_pos=1. I cannot find the reason. 😓

https://github.com/rubocop-hq/rubocop/pull/2516/files#diff-4f6676361bec108acba5cfd5a6d979b3

image

Copy link
Contributor

Choose a reason for hiding this comment

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

If you could try with 0, I think it would make more sense (especially on an empty file!)

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 try it via 7e3b3da.

@ybiquitous
Copy link
Contributor Author

I will squash the commits to one commit finally.

length: 1,
line: 1,
column: 1
})
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

@rrosenblum
Copy link
Contributor

Thanks for implementing this fix

@bbatsov bbatsov merged commit 1a84099 into rubocop:master Oct 8, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 8, 2020

Thanks!

@ybiquitous ybiquitous deleted the fix-pseudo-location branch October 8, 2020 12:41
@ybiquitous
Copy link
Contributor Author

@bbatsov Thank you for merging it!

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

Successfully merging this pull request may close these issues.

None yet

4 participants