Skip to content

Commit

Permalink
[Fix #13556] Fix false positives for Style/FileNull
Browse files Browse the repository at this point in the history
Fixes #13556.

This PR fixes false positives for `Style/FileNull` when using `'nul'` string.

This PR ensures only files that use the string `'/dev/null'` are targeted for detection.
This is because the string `'nul'` is not limited to the null device.
This behavior results in false negatives when the `'/dev/null'` string is not used,
but it is a trade-off to avoid false positives.
  • Loading branch information
koic authored and bbatsov committed Dec 12, 2024
1 parent fd13dab commit 19ee909
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 6 deletions.
1 change: 1 addition & 0 deletions changelog/fix_false_positives_for_style_file_null.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#13556](https://github.com/rubocop/rubocop/issues/13556): Fix false positives for `Style/FileNull` when using `'nul'` string. ([@koic][])
24 changes: 20 additions & 4 deletions lib/rubocop/cop/style/file_null.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ module Style
# Only looks for full string matches, substrings within a longer string are not
# considered.
#
# However, only files that use the string `'/dev/null'` are targeted for detection.
# This is because the string `'NUL'` is not limited to the null device.
# This behavior results in false negatives when the `'/dev/null'` string is not used,
# but it is a trade-off to avoid false positives. `NULL:`
# Unlike `'NUL'`, `'NUL:'` is regarded as something like `C:` and is always detected.
#
# NOTE: Uses inside arrays and hashes are ignored.
#
# @safety
Expand Down Expand Up @@ -42,11 +48,21 @@ class FileNull < Base
REGEXP = %r{\A(/dev/null|NUL:?)\z}i.freeze
MSG = 'Use `File::NULL` instead of `%<source>s`.'

def on_new_investigation
return unless (ast = processed_source.ast)

@contain_dev_null_string_in_file = ast.each_descendant(:str).any? do |str|
content = str.str_content

valid_string?(content) && content.downcase == '/dev/null' # rubocop:disable Style/FileNull
end
end

def on_str(node)
value = node.value

return if invalid_string?(value)
return unless valid_string?(value)
return if acceptable?(node)
return if value.downcase == 'nul' && !@contain_dev_null_string_in_file # rubocop:disable Style/FileNull
return unless REGEXP.match?(value)

add_offense(node, message: format(MSG, source: value)) do |corrector|
Expand All @@ -56,8 +72,8 @@ def on_str(node)

private

def invalid_string?(value)
value.empty? || !value.valid_encoding?
def valid_string?(value)
!value.empty? && value.valid_encoding?
end

def acceptable?(node)
Expand Down
26 changes: 24 additions & 2 deletions spec/rubocop/cop/style/file_null_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,40 @@
RUBY
end

it 'registers an offense and corrects when the entire string is `NUL`' do
it "registers an offense and corrects when the entire string is `NUL` or '/dev/null'" do
expect_offense(<<~RUBY)
CONST = '/dev/null'
^^^^^^^^^^^ Use `File::NULL` instead of `/dev/null`.
path = 'NUL'
^^^^^ Use `File::NULL` instead of `NUL`.
RUBY

expect_correction(<<~RUBY)
CONST = File::NULL
path = File::NULL
RUBY
end

it 'registers an offense and corrects when the entire string is `NUL:`' do
it "does not register an offense when the entire string is `NUL` without '/dev/null'" do
expect_no_offenses(<<~RUBY)
path = 'NUL'
RUBY
end

it "registers an offense and corrects when the entire string is `NUL:` or '/dev/null'" do
expect_offense(<<~RUBY)
path = cond ? '/dev/null' : 'NUL:'
^^^^^^^^^^^ Use `File::NULL` instead of `/dev/null`.
^^^^^^ Use `File::NULL` instead of `NUL:`.
RUBY

# Different cops will detect duplication of the branch bodies.
expect_correction(<<~RUBY)
path = cond ? File::NULL : File::NULL
RUBY
end

it "registers an offense when the entire string is `NUL:` without '/dev/null'" do
expect_offense(<<~RUBY)
path = 'NUL:'
^^^^^^ Use `File::NULL` instead of `NUL:`.
Expand Down

0 comments on commit 19ee909

Please sign in to comment.