[Fix #6331] Fix a false positive for Style/RedundantFreeze
#6333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I also found that there is a false negative for the case using a range object. I'm going to open another PR afterwards. |
8 tasks
bbatsov
approved these changes
Sep 26, 2018
Fixes rubocop#6331. This PR fixes a false positive for `Style/RedundantFreeze` when assigning a regexp object to a constant. The following is a reproduction procedure. ```console % cat example.rb CONSTANT = /regexp/.freeze % rubocop example.rb --only Style/RedundantFreeze Inspecting 1 file C Offenses: example.rb:1:12: C: Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect. CONSTANT = /regexp/.freeze ^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` A regexp object is a mutable object. ```console % ruby -ve 'p(/regexp/.frozen?)' ruby 2.2.10p489 (2018-03-28 revision 63023) [x86_64-darwin17] false % ruby -ve 'p(/regexp/.frozen?)' ruby 2.3.7p456 (2018-03-28 revision 63024) [x86_64-darwin17] false % ruby -ve 'p(/regexp/.frozen?)' ruby 2.4.4p296 (2018-03-28 revision 63013) [x86_64-darwin17] false % ruby -ve 'p(/regexp/.frozen?)' ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17] false ``` However, this false positive was caused by not including regexp object as a mutable object in `RuboCop::AST::Node::MUTABLE_LITERALS`. This PR fixes this problem and applies auto-correct for the following new offenses. ```console lib/rubocop/string_interpreter.rb:12:27: C: Style/MutableConstant: Freeze mutable objects assigned to constants. STRING_ESCAPE_REGEX = /\\(?: ... ^^^^^^ ``` This was a false negative of `Style/MutableConstant`.
koic
force-pushed
the
fix_false_positive_for_style_redundant_freeze
branch
from
77d8282 to
5ab7f74
Compare
koic
added a commit
to koic/rubocop
that referenced
this pull request
Sep 27, 2018
Follow up of rubocop#6333 (comment). This PR fixes a false negative for `Style/RedundantFreeze` when assigning a range object to a constant. A range object is a mutable object. ```console % ruby -ve 'p (1..99).frozen?' ruby 2.2.10p489 (2018-03-28 revision 63023) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.3.7p456 (2018-03-28 revision 63024) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.4.4p296 (2018-03-28 revision 63013) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17] false ``` However, this false positive was caused by not including range object as a mutable object in `RuboCop::AST::Node::MUTABLE_LITERALS`. And there was an mistake in the test case that no parenthesis for range object. It was the test case that applied `.freeze` to integer object rather than range object. ```ruby foo = 1..5.freeze foo.frozen? #=> false bar = (1..5).freeze bar.frozen? #=> true ```
bbatsov
pushed a commit
that referenced
this pull request
Sep 27, 2018
Follow up of #6333 (comment). This PR fixes a false negative for `Style/RedundantFreeze` when assigning a range object to a constant. A range object is a mutable object. ```console % ruby -ve 'p (1..99).frozen?' ruby 2.2.10p489 (2018-03-28 revision 63023) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.3.7p456 (2018-03-28 revision 63024) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.4.4p296 (2018-03-28 revision 63013) [x86_64-darwin17] false % ruby -ve 'p (1..99).frozen?' ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin17] false ``` However, this false positive was caused by not including range object as a mutable object in `RuboCop::AST::Node::MUTABLE_LITERALS`. And there was an mistake in the test case that no parenthesis for range object. It was the test case that applied `.freeze` to integer object rather than range object. ```ruby foo = 1..5.freeze foo.frozen? #=> false bar = (1..5).freeze bar.frozen? #=> true ```
yahonda
added a commit
to yahonda/rails
that referenced
this pull request
Sep 29, 2018
Since Rails 6.0 will support Ruby 2.4.1 or higher `# frozen_string_literal: true` magic comment is enough to make string object frozen. This magic comment is enabled by `Style/FrozenStringLiteralComment` cop. * Exclude these files not to auto correct false positive `Regexp#freeze` - 'actionpack/lib/action_dispatch/journey/router/utils.rb' - 'activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb' It has been fixed by rubocop/rubocop#6333 Once the newer version of RuboCop released and available at Code Climate these exclude entries should be removed. * Replace `String#freeze` with `String#-@` manually if explicit frozen string objects are required - 'actionpack/test/controller/test_case_test.rb' - 'activemodel/test/cases/type/string_test.rb' - 'activesupport/lib/active_support/core_ext/string/strip.rb' - 'activesupport/test/core_ext/string_ext_test.rb' - 'railties/test/generators/actions_test.rb'
yahonda
added a commit
to yahonda/rails
that referenced
this pull request
Nov 8, 2018
Since rubocop/rubocop#6333 has been included into RuboCop 0.60.0.
4 tasks
ghost
pushed a commit
to rubygems/rubygems
that referenced
this pull request
Nov 11, 2018
2465: Fix and lock rubocop r=segiddins a=deivid-rodriguez # Description: I was getting rubocop offenses against latest master. This is because I had `0.60.0` installed and that version was allowed by the previous requirement (`~> 0.58`), so the rake task was picking that version. `0.60.0` added [a bug fix](rubocop/rubocop#6333) around `Style/MutableConstant` that detects new offenses, so that's why it was failing. This PR bumps the version of rubocop in use to `0.60.0`, and consistently sets the requirement to include the patch version (`~> 0.60.0`). I would actually go as far as fully locking the dependency, because right now if the same thing happens with a patch level release (for example, `0.60.1`), the same problem will be introduced and get in the middle for contributors. But I haven't done it in this PR. ______________ # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #6331.
This PR fixes a false positive for
Style/RedundantFreezewhen assigning a regexp object to a constant.The following is a reproduction procedure.
A regexp object is a mutable object.
However, this false positive was caused by not including regexp object as a mutable object in
RuboCop::AST::Node::MUTABLE_LITERALS.This PR fixes this problem and applies auto-correct for the following new offenses.
This was a false negative of
Style/MutableConstant.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake default. It executes all tests and RuboCop for itself, and generates the documentation.