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 #8948] Fix autocorrection for Style/RedundantRegexpCharacterClass with %r #8954

Conversation

ysakasin
Copy link
Contributor

@ysakasin ysakasin commented Oct 27, 2020

This commit uses RegexpParser::Map#body instead of Parser::Source::Range#adjust in Style/RedundantRegexpCharacterClass.

When regular expression are written by %r literal, RedundantRegexpCharacterClass made incorrect loc as following.

%r{abc[\d]+}
    ^^^^^^

This bug are caused by callingnode.loc.begin.adjust to get range of character class with %r regexp.
Parser::Source::Range#adjust adjust based on Parser::Source::Range#begin_pos but it with %r literal doesn't indicate start of regular expression source, point to location of '%'.
So, RedundantRegexpCharacterClass makes incorrect loc if %r literal are used in code.

r = %r{abc}
    ^      # `node.loc.begin.begin_pos`
r = %r{abs}
       ^   # `node.loc.begin.end_pos`

Ref. #8948


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.

@ysakasin ysakasin force-pushed the fix_style_redundant_regexp_character_class_with_percent_string branch from a187221 to e397e29 Compare October 27, 2020 10:19
@owst
Copy link
Contributor

owst commented Oct 27, 2020

Thanks for fixing this @ysakasin!

I recently added RegexpNode#parsed_tree_expr_loc: https://github.com/rubocop-hq/rubocop/blob/0259bed3df30e7d0e18645e651a683b890d03db4/lib/rubocop/ext/regexp_node.rb#L42-L44 which correctly uses loc.begin.end. I wonder if we could alter that method so that it accepts an optional explicit length argument that you could use in this PR?

What about something like:

def parsed_tree_expr_loc(expr, loc_size: expr.full_length)
  loc.begin.end.adjust(begin_pos: expr.ts, end_pos: expr.ts).resize(loc_size)
end

@ysakasin ysakasin force-pushed the fix_style_redundant_regexp_character_class_with_percent_string branch from e397e29 to 074bc8c Compare October 27, 2020 13:57
@ysakasin
Copy link
Contributor Author

@owst Thanks reviewing!

I modify to use RegexpNode#parsed_tree_expr_loc and add keyword argument loc_size to it.

@marcandre
Copy link
Contributor

It's very clucky to have to provide parsed_tree_expr_loc with the right answer.
At minimum, that method should check the type of the node and adjust if need be (like here).
Probably better would be to monkey patch Regexp's parser Nodes and add a loc method that would provide us with the right information.
For a char class like here, you could use loc.expression and that's it.

@ysakasin
Copy link
Contributor Author

Like following?

        def each_redundant_character_class(node)
          each_single_element_character_class(node) do |char_class|
            next unless redundant_single_element_character_class?(node, char_class)

            # This means that: '['.length + len_char_class_source + ']'.length
            len_char_class = 1 + char_class.expressions.first.text.length + 1

            yield node.loc.expression.adjust(begin_pos: char_class.ts).resize(len_char_class)
          end
        end

@marcandre
Copy link
Contributor

Not what I meant. Let me prepare a PR

@marcandre
Copy link
Contributor

I created #8960. I believe that the correction would then simply be to remove the loc.begin and loc.end.

@ysakasin ysakasin force-pushed the fix_style_redundant_regexp_character_class_with_percent_string branch from 074bc8c to 20ec0fb Compare October 28, 2020 10:56
…terClass with %r

This commit uses `RegexpNode::RegexpParser#body` (rubocop#8960) instead of `Parser::Source::Range#adjust` in Style/RedundantRegexpCharacterClass.

When regular expression are written by %r literal, RedundantRegexpCharacterClass made incorrect loc as following.
```
%r{abc[\d]+}
    ^^^^^^
```

This bug are caused by calling`node.loc.begin.adjust` to get range of character class with %r regexp.
`Parser::Source::Range#adjust` adjust based on `Parser::Source::Range#begin_pos` but it with %r literal doesn't indicate start of regular expression source, point to location of '%'.
So, RedundantRegexpCharacterClass makes incorrect loc if %r literal are used in code.

```
r = %r{abc}
    ^      # `node.loc.begin.begin_pos`
```

```
r = %r{abs}
       ^   # `node.loc.begin.end_pos`
```
@ysakasin ysakasin force-pushed the fix_style_redundant_regexp_character_class_with_percent_string branch from 20ec0fb to 0e74823 Compare October 28, 2020 11:01
@ysakasin
Copy link
Contributor Author

@marcandre Thanks adding awesome method!
I replaced RegexpNode#parsed_tree_expr_loc with RegexpParser::Map#body.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 29, 2020

Looks great! Thanks!

@bbatsov bbatsov merged commit 5905396 into rubocop:master Oct 29, 2020
@ysakasin ysakasin deleted the fix_style_redundant_regexp_character_class_with_percent_string branch October 29, 2020 12:21
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