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 #7842] Add AcceptImplicitNamespaces to Lint/RaiseException #7846

Merged
Show file tree
Hide file tree
Changes from all 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 @@ -7,6 +7,7 @@
* [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when raising Exception with explicit namespace. ([@koic][])
* [#7834](https://github.com/rubocop-hq/rubocop/issues/7834): Fix `Lint/UriRegexp` to register offense with array arguments. ([@tejasbubane][])
* [#7841](https://github.com/rubocop-hq/rubocop/issues/7841): Fix an error for `Style/TrailingCommaInBlockArgs` when lambda literal (`->`) has multiple arguments. ([@koic][])
* [#7842](https://github.com/rubocop-hq/rubocop/issues/7842): Fix a false positive for `Lint/RaiseException` when Exception without cbase specified under the namespace `Gem` by adding `AllowedImplicitNamespaces` option. ([@koic][])

## 0.81.0 (2020-04-01)

Expand Down
2 changes: 2 additions & 0 deletions config/default.yml
Expand Up @@ -1567,6 +1567,8 @@ Lint/RaiseException:
StyleGuide: '#raise-exception'
Enabled: pending
VersionAdded: '0.81'
AllowedImplicitNamespaces:
- 'Gem'

Lint/RandOne:
Description: >-
Expand Down
48 changes: 42 additions & 6 deletions lib/rubocop/cop/lint/raise_exception.rb
Expand Up @@ -6,32 +6,68 @@ module Lint
# This cop checks for `raise` or `fail` statements which are
# raising `Exception` class.
#
# You can specify a module name that will be an implicit namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably the explanation should be extended to mention the cop normally ignores namespaced "Exception", but might cause false positives if some module has its own Exception class. This would make it easier for people to understand the need for the extra config. That being said - I think it's a very bad idea to name base classes "Exception" and I'd rather name them "(BaseError)" or something along those lines. Hiding built-in constants is a recipe for trouble IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for reviewing. I extended the explanation.

# using `AllowedImplicitNamespaces` option. The cop cause a false positive
# for namespaced `Exception` when a namespace is omitted. This option can
# prevent the false positive by specifying a namespace to be omitted for
# `Exception`. Alternatively, make `Exception` a fully qualified class
# name with an explicit namespace.
#
# @example
# # bad
# raise Exception, 'Error message here'
#
# # good
# raise StandardError, 'Error message here'
#
# @example AllowedImplicitNamespaces: ['Gem']
# # good
# module Gem
# def self.foo
# raise Exception # This exception means `Gem::Exception`.
# end
# end
class RaiseException < Cop
MSG = 'Use `StandardError` over `Exception`.'

def_node_matcher :exception?, <<~PATTERN
(send nil? ${:raise :fail} (const {cbase nil?} :Exception) ... )
(send nil? {:raise :fail} (const ${cbase nil?} :Exception) ... )
PATTERN

def_node_matcher :exception_new_with_message?, <<~PATTERN
(send nil? ${:raise :fail}
(send (const {cbase nil?} :Exception) :new ... ))
(send nil? {:raise :fail}
(send (const ${cbase nil?} :Exception) :new ... ))
PATTERN

def on_send(node)
add_offense(node) if raise_exception?(node)
exception?(node, &check(node)) ||
exception_new_with_message?(node, &check(node))
end

private

def raise_exception?(node)
exception?(node) || exception_new_with_message?(node)
def check(node)
lambda do |cbase|
return if cbase.nil? && implicit_namespace?(node)

add_offense(node)
end
end

def implicit_namespace?(node)
return false unless (parent = node.parent)

if parent.module_type?
namespace = parent.identifier.source

return allow_implicit_namespaces.include?(namespace)
end

implicit_namespace?(parent)
end

def allow_implicit_namespaces
cop_config['AllowedImplicitNamespaces'] || []
end
end
end
Expand Down
23 changes: 23 additions & 0 deletions manual/cops_lint.md
Expand Up @@ -1496,6 +1496,13 @@ Pending | Yes | No | 0.81 | -
This cop checks for `raise` or `fail` statements which are
raising `Exception` class.

You can specify a module name that will be an implicit namespace
using `AllowedImplicitNamespaces` option. The cop cause a false positive
for namespaced `Exception` when a namespace is omitted. This option can
prevent the false positive by specifying a namespace to be omitted for
`Exception`. Alternatively, make `Exception` a fully qualified class
name with an explicit namespace.

### Examples

```ruby
Expand All @@ -1505,6 +1512,22 @@ raise Exception, 'Error message here'
# good
raise StandardError, 'Error message here'
```
#### AllowedImplicitNamespaces: ['Gem']

```ruby
# good
module Gem
def self.foo
raise Exception # This exception means `Gem::Exception`.
end
end
```

### Configurable attributes

Name | Default value | Configurable values
--- | --- | ---
AllowedImplicitNamespaces | `Gem` | Array

### References

Expand Down
29 changes: 27 additions & 2 deletions spec/rubocop/cop/lint/raise_exception_spec.rb
@@ -1,7 +1,9 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Lint::RaiseException do
subject(:cop) { described_class.new }
RSpec.describe RuboCop::Cop::Lint::RaiseException, :config do
subject(:cop) { described_class.new(config) }

let(:cop_config) { { 'AllowedImplicitNamespaces' => ['Gem'] } }

it 'registers an offense for `raise` with `::Exception`' do
expect_offense(<<~RUBY)
Expand Down Expand Up @@ -87,4 +89,27 @@
raise Foo::Exception
RUBY
end

context 'when under namespace' do
it 'does not register an offense when Exception without cbase specified' do
expect_no_offenses(<<~RUBY)
module Gem
def self.foo
raise Exception
end
end
RUBY
end

it 'does not register an offense when Exception with cbase specified' do
expect_offense(<<~RUBY)
module Gem
def self.foo
raise ::Exception
^^^^^^^^^^^^^^^^^ Use `StandardError` over `Exception`.
end
end
RUBY
end
end
end