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 #6305] Fix infinite loop for `Layout/EmptyLinesAroundAccessModifier` #6307

Conversation

Projects
None yet
2 participants
@koic
Copy link
Member

commented Sep 19, 2018

Fixes #6305.

This is a problem that occurs after #6236.

This PR fixes infinite loop for Layout/EmptyLinesAroundAccessModifier and Layout/EmptyLinesAroundClassBody when specifying a superclass that breaks the line.

class AVerryLongClassNameDoYouLikeIt <
  ItEvenInheritsFromALongClassNameSOINeedToAddALineBrake

  protected

  def foobar
  end
end

With this PR, Layout/EmptyLinesAroundAccessModifier cop makes aware of the above case that specifying a superclass that breaks the line. This is the same behavior as writing a class definition on a single line.

As a result, Layout/EmptyLinesAroundAccessModifier and Layout/EmptyLinesAroundClassBody will not conflict with the handling of empty line under the class definition.

This problem was caused by using the regular expression in EmptyLinesAroundAccessModifier#class_def? method. This PR will change it to use AST.


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 rake default. It executes all tests and RuboCop for itself, and generates the documentation.
[Fix #6305] Fix infinite loop for `Layout/EmptyLinesAroundAccessModif…
…ier`

Fixes #6305.

This is a problem that occurs after #6236.

This PR fixes infinite loop for `Layout/EmptyLinesAroundAccessModifier`
and `Layout/EmptyLinesAroundClassBody` when specifying a superclass
that breaks the line.

```ruby
class AVerryLongClassNameDoYouLikeIt <
  ItEvenInheritsFromALongClassNameSOINeedToAddALineBrake

  protected

  def foobar
  end
end
```

With this PR, `Layout/EmptyLinesAroundAccessModifier` cop makes
aware of the above case that specifying a superclass that
breaks the line. This is the same behavior as writing a class
definition on a single line.

As a result, `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody` will not conflict with
the handling of empty line under the class definition.

This problem was caused by using the regular expression
in `EmptyLinesAroundAccessModifier#class_def?` method.
This PR will change it to use AST.

@bbatsov bbatsov merged commit fce51ea into rubocop-hq:master Sep 19, 2018

23 checks passed

ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: cc-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-rc-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-rc-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-rc-spec Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

This problem was caused by using the regular expression in EmptyLinesAroundAccessModifier#class_def? method. This PR will change it to use AST.

Ah, regular expressions bite us again. :-)

Great work, Koichi!

@koic koic deleted the koic:fix_infinite_loop_empty_lines_around_access_modifier branch Sep 19, 2018

@koic

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2018

Thank you, Bozhidar!
This cop still has places that use regular expression. I'd like to refactor it when the opportunity comes 😃

koic added a commit to koic/rubocop that referenced this pull request Oct 5, 2018

Refactor `Layout/EmptyLinesAroundAccessModifier` to using AST instrea…
…d of regexp

Follow up of rubocop-hq#6307 (comment).

This PR refactors `Layout/EmptyLinesAroundAccessModifier` to using AST instread of regexp.

koic added a commit to koic/rubocop that referenced this pull request Oct 5, 2018

Refactor `Layout/EmptyLinesAroundAccessModifier` to use AST instread …
…of regexp

Follow up of rubocop-hq#6307 (comment).

This PR refactors `Layout/EmptyLinesAroundAccessModifier` to use AST instread of regexp.

koic added a commit to koic/rubocop that referenced this pull request Oct 5, 2018

Refactor `Layout/EmptyLinesAroundAccessModifier` to use AST instread …
…of regexp

Follow up of rubocop-hq#6307 (comment).

This PR refactors `Layout/EmptyLinesAroundAccessModifier` to use AST instread of regexp.

bbatsov added a commit that referenced this pull request Oct 6, 2018

Refactor `Layout/EmptyLinesAroundAccessModifier` to use AST instread …
…of regexp

Follow up of #6307 (comment).

This PR refactors `Layout/EmptyLinesAroundAccessModifier` to use AST instread of regexp.

koic added a commit to koic/rubocop that referenced this pull request Dec 12, 2018

Fix a false positive for `Layout/EmptyLinesAroundAccessModifier`
This PR fixes a false positive for `Layout/EmptyLinesAroundAccessModifier`
when at the end of specifying a superclass is missing blank line.

The following is originally no offense code.
(It is probably a regression by rubocop-hq#6307)

```console
% cat example.rb
class SomeController < SomeOtherController
  def index; end

  private
end

% rubocop example.rb --only Layout/EmptyLinesAroundAccessModifier -a
Inspecting 1 file
C

Offenses:

example.rb:4:3: C: [Corrected] Layout/EmptyLinesAroundAccessModifier:
Keep a blank line before and after private.
  private
  ^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct adds a blank line after `private`. This is due to false positives.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
+
 end
```

This caused the infinite loop in `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody`.

```console
% rubocop example.rb --only Layout/EmptyLinesAroundClassBody -a
Inspecting 1 file
C

Offenses:

example.rb:5:1: C: [Corrected] Layout/EmptyLinesAroundClassBody: Extra
empty line detected at class body end.

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct removes the blank line from after of `private`.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
-
 end
```

It loops to the first code. This PR fixes the infinite loop.

koic added a commit to koic/rubocop that referenced this pull request Dec 12, 2018

[Fix rubocop-hq#6566] Fix a false positive for `Layout/EmptyLinesArou…
…ndAccessModifier`

Fixes rubocop-hq#6566.

This PR fixes a false positive for `Layout/EmptyLinesAroundAccessModifier`
when at the end of specifying a superclass is missing blank line.

The following is originally no offense code.
(It is probably a regression by rubocop-hq#6307)

```console
% cat example.rb
class SomeController < SomeOtherController
  def index; end

  private
end

% rubocop example.rb --only Layout/EmptyLinesAroundAccessModifier -a
Inspecting 1 file
C

Offenses:

example.rb:4:3: C: [Corrected] Layout/EmptyLinesAroundAccessModifier:
Keep a blank line before and after private.
  private
  ^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct adds a blank line after `private`. This is due to false positives.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
+
 end
```

This caused the infinite loop in `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody`.

```console
% rubocop example.rb --only Layout/EmptyLinesAroundClassBody -a
Inspecting 1 file
C

Offenses:

example.rb:5:1: C: [Corrected] Layout/EmptyLinesAroundClassBody: Extra
empty line detected at class body end.

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct removes the blank line from after of `private`.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
-
 end
```

It loops to the first code. This PR fixes the infinite loop.

koic added a commit to koic/rubocop that referenced this pull request Dec 14, 2018

[Fix rubocop-hq#6566] Fix a false positive for `Layout/EmptyLinesArou…
…ndAccessModifier`

Fixes rubocop-hq#6566.

This PR fixes a false positive for `Layout/EmptyLinesAroundAccessModifier`
when at the end of specifying a superclass is missing blank line.

The following is originally no offense code.
(It is probably a regression by rubocop-hq#6307)

```console
% cat example.rb
class SomeController < SomeOtherController
  def index; end

  private
end

% rubocop example.rb --only Layout/EmptyLinesAroundAccessModifier -a
Inspecting 1 file
C

Offenses:

example.rb:4:3: C: [Corrected] Layout/EmptyLinesAroundAccessModifier:
Keep a blank line before and after private.
  private
  ^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct adds a blank line after `private`. This is due to false positives.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
+
 end
```

This caused the infinite loop in `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody`.

```console
% rubocop example.rb --only Layout/EmptyLinesAroundClassBody -a
Inspecting 1 file
C

Offenses:

example.rb:5:1: C: [Corrected] Layout/EmptyLinesAroundClassBody: Extra
empty line detected at class body end.

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct removes the blank line from after of `private`.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
-
 end
```

It loops to the first code. This PR fixes the infinite loop.

bbatsov added a commit that referenced this pull request Dec 14, 2018

[Fix #6566] Fix a false positive for `Layout/EmptyLinesAroundAccessMo…
…difier`

Fixes #6566.

This PR fixes a false positive for `Layout/EmptyLinesAroundAccessModifier`
when at the end of specifying a superclass is missing blank line.

The following is originally no offense code.
(It is probably a regression by #6307)

```console
% cat example.rb
class SomeController < SomeOtherController
  def index; end

  private
end

% rubocop example.rb --only Layout/EmptyLinesAroundAccessModifier -a
Inspecting 1 file
C

Offenses:

example.rb:4:3: C: [Corrected] Layout/EmptyLinesAroundAccessModifier:
Keep a blank line before and after private.
  private
  ^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct adds a blank line after `private`. This is due to false positives.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
+
 end
```

This caused the infinite loop in `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody`.

```console
% rubocop example.rb --only Layout/EmptyLinesAroundClassBody -a
Inspecting 1 file
C

Offenses:

example.rb:5:1: C: [Corrected] Layout/EmptyLinesAroundClassBody: Extra
empty line detected at class body end.

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct removes the blank line from after of `private`.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
-
 end
```

It loops to the first code. This PR fixes the infinite loop.

Nyangawa pushed a commit to Nyangawa/rubocop that referenced this pull request Jan 24, 2019

[Fix rubocop-hq#6566] Fix a false positive for `Layout/EmptyLinesArou…
…ndAccessModifier`

Fixes rubocop-hq#6566.

This PR fixes a false positive for `Layout/EmptyLinesAroundAccessModifier`
when at the end of specifying a superclass is missing blank line.

The following is originally no offense code.
(It is probably a regression by rubocop-hq#6307)

```console
% cat example.rb
class SomeController < SomeOtherController
  def index; end

  private
end

% rubocop example.rb --only Layout/EmptyLinesAroundAccessModifier -a
Inspecting 1 file
C

Offenses:

example.rb:4:3: C: [Corrected] Layout/EmptyLinesAroundAccessModifier:
Keep a blank line before and after private.
  private
  ^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct adds a blank line after `private`. This is due to false positives.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
+
 end
```

This caused the infinite loop in `Layout/EmptyLinesAroundAccessModifier` and
`Layout/EmptyLinesAroundClassBody`.

```console
% rubocop example.rb --only Layout/EmptyLinesAroundClassBody -a
Inspecting 1 file
C

Offenses:

example.rb:5:1: C: [Corrected] Layout/EmptyLinesAroundClassBody: Extra
empty line detected at class body end.

1 file inspected, 1 offense detected, 1 offense corrected
```

Auto-correct removes the blank line from after of `private`.

```diff
 % cat example.rb
 class SomeController < SomeOtherController
   def index; end

   private
-
 end
```

It loops to the first code. This PR fixes the infinite loop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.