Skip to content

Commit

Permalink
[Fix rubocop#13437] Add new cop Lint/DefinedWithFixedResult
Browse files Browse the repository at this point in the history
See the cop documentation for the rationale.

Interpolation is included. Ruby does not check individual parts, for example `defined?("foo#{bar}")` is truthy even if `bar` doesn't exist.

It will find code such as the following:
/~https://github.com/search?q=lang%3Aruby+%2F+defined%5C%3F%5C%28%5B%3A%22%27%5D%5Ba-zA-Z%5D%2F&type=code

Extra reading: /~https://github.com/ruby/spec/blob/bbcd077ac3ff6da3730f388c61d4fcbbf085f4ab/language/defined_spec.rb
  • Loading branch information
Earlopain committed Nov 10, 2024
1 parent 72d944d commit abceba7
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_lint_useless_defined.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#13437](/~https://github.com/rubocop/rubocop/issues/13437): Add a new cop `Lint/UselessDefined` to detect cases such as `defined?('Foo')` when `defined?(Foo)` was intended. ([@earlopain][])
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2561,6 +2561,11 @@ Lint/UselessAssignment:
VersionAdded: '0.11'
VersionChanged: '1.66'

Lint/UselessDefined:
Description: 'Checks for calls to `defined?` with strings and symbols. The result of such a call will always be truthy.'
Enabled: pending
VersionAdded: '<<next>>'

Lint/UselessElseWithoutRescue:
Description: 'Checks for useless `else` in `begin..end` without `rescue`.'
Enabled: true
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@
require_relative 'rubocop/cop/lint/uri_regexp'
require_relative 'rubocop/cop/lint/useless_access_modifier'
require_relative 'rubocop/cop/lint/useless_assignment'
require_relative 'rubocop/cop/lint/useless_defined'
require_relative 'rubocop/cop/lint/useless_else_without_rescue'
require_relative 'rubocop/cop/lint/useless_method_definition'
require_relative 'rubocop/cop/lint/useless_numeric_operation'
Expand Down
56 changes: 56 additions & 0 deletions lib/rubocop/cop/lint/useless_defined.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Lint
# Checks for calls to `defined?` with strings or symbols as the argument.
# Such calls will always return `'expression'`, you probably meant to
# check for the existence of a constant, method, or variable instead.
#
# `defined?` is part of the Ruby syntax and doesn't behave like normal methods.
# You can safely pass in what you are checking for directly, without encountering
# a `NameError`.
#
# When interpolation is used, oftentimes it is not possible to write the
# code with `defined?`. In these cases, switch to one of the more specific methods:
# * `class_variable_defined?`
# * `const_defined?`
# * `method_defined?`
# * `instance_variable_defined?`
# * `binding.local_variable_defined?`
#
# # bad
# defined?('FooBar')
# defined?(:FooBar)
# defined?(:foo_bar)
# defined?('foo_bar')
#
# # good
# defined?(FooBar)
# defined?(foo_bar)
#
# # bad - interpolation
# bar = 'Bar'
# defined?("Foo::#{bar}::Baz")
#
# # good
# bar = 'Bar'
# defined?(Foo) && Foo.const_defined?(bar) && Foo.const_get(bar).const_defined?(:Baz)
class UselessDefined < Base
MSG = 'Calling `defined?` with a %<type>s argument will always return a truthy value.'
TYPES = { str: 'string', dstr: 'string', sym: 'symbol', dsym: 'symbol' }.freeze

# @!method offense?(node)
def_node_matcher :offense?, <<~PATTERN
(defined (${#{TYPES.keys.join(' ')}} ...))
PATTERN

def on_defined?(node)
offense?(node) do |type|
add_offense(node, message: format(MSG, type: TYPES[type]))
end
end
end
end
end
end
49 changes: 49 additions & 0 deletions spec/rubocop/cop/lint/useless_defined_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Lint::UselessDefined, :config do
it 'registers an offense when using `defined?` with a string argument' do
expect_offense(<<~RUBY)
defined?("FooBar")
^^^^^^^^^^^^^^^^^^ Calling `defined?` with a string argument will always return a truthy value.
RUBY
end

it 'registers an offense when using `defined?` with interpolation' do
expect_offense(<<~'RUBY')
defined?("Foo#{bar}")
^^^^^^^^^^^^^^^^^^^^^ Calling `defined?` with a string argument will always return a truthy value.
RUBY
end

it 'registers an offense when using `defined?` with a symbol' do
expect_offense(<<~RUBY)
defined?(:FooBar)
^^^^^^^^^^^^^^^^^ Calling `defined?` with a symbol argument will always return a truthy value.
RUBY
end

it 'registers an offense when using `defined?` an interpolated symbol' do
expect_offense(<<~'RUBY')
defined?(:"Foo#{bar}")
^^^^^^^^^^^^^^^^^^^^^^ Calling `defined?` with a symbol argument will always return a truthy value.
RUBY
end

it 'does not register an offense when using `defined?` with a constant' do
expect_no_offenses(<<~RUBY)
defined?(FooBar)
RUBY
end

it 'does not register an offense when using `defined?` with a method' do
expect_no_offenses(<<~RUBY)
defined?(foo_bar)
RUBY
end

it 'does not register an offense when calling a method on a symbol' do
expect_no_offenses(<<~RUBY)
defined?(:foo.to_proc)
RUBY
end
end

0 comments on commit abceba7

Please sign in to comment.