Fix Lint/UselessAssignment false positive
#11510
Conversation
sambostock
force-pushed
the
useless-assignment-bug
branch
from
716af99
to
ef779ad
Compare
| @@ -113,14 +113,14 @@ def variable_exist?(name) | |||
| def accessible_variables | |||
| scope_stack.reverse_each.with_object([]) do |scope, variables| | |||
| variables.concat(scope.variables.values) | |||
| break variables unless scope.node.block_type? | |||
| break variables unless scope.node.block_type? || scope.node.numblock_type? | |||
There was a problem hiding this comment.
As mentioned in the commit message, I tentatively added this to match the other block_type? call sites in this file:
- [Fix #11485] Fix a false positive for
Lint/UselessAssignment#11488 added it infind_variableabove, to resolve a false positive (Lint/UselessAssignmentfalse positive when variables are used in blocks with numbered parameters #11485) - I am adding it in
mark_variable_as_captured_by_block_if_sobelow, to resolve a false positive
I don't know what the false positive is here, but I'm assuming there is one. If someone with more context knows what it might be, I'm can add a corresponding test and update the commit message and changelog.
| it 'registers an offense when the variable is assigned and but unused' do | ||
| expect_offense(<<~RUBY) | ||
| var = nil | ||
| ^^^ Useless assignment to variable - `var`. | ||
|
|
||
| do_something { var = _1 } | ||
| ^^^ Useless assignment to variable - `var`. | ||
| RUBY | ||
| end |
There was a problem hiding this comment.
Hmm... I just added this test as the inverse of the one above, but it looks like it fails. I think it's because
var = nilis considered fine, because a block later uses itvar = _1is considered fine, because it's assigning to a shadowed variable
I don't think there's anything checking that the variable is used in neither the block, nor the surrounding scope.
I'm not sure how to go about implementing that, so I may just remove the test and accept the unlikely false negative. 🤷
There was a problem hiding this comment.
Yep. It's an existing issue that also happens with normal block (not numbered block). So, can you remove this test?
|
@sambostock The CI is not passing currently. |
sambostock
force-pushed
the
useless-assignment-bug
branch
from
ef779ad
to
ef725e6
Compare
| @@ -0,0 +1 @@ | |||
| * [#11510](https://github.com/rubocop/rubocop/pull/11510): Tentatively check against `numblock_type?` too. ([@sambostock][]) | |||
There was a problem hiding this comment.
Can you remove this changelog entry?
|
I left a comment about removing 2 places. Please squash your commits into one after the remove. |
sambostock
force-pushed
the
useless-assignment-bug
branch
from
ef725e6
to
12c374e
Compare
|
@koic Sorry for the delay; updated. 👍 |
|
@sambostock Finally noticed about the changelog entry. Can you take a look? |
A variable assigned in a block is not unused, if it was captured from
the surrounding scope, where it is later used.
var = nil
do_something { |arg| var = arg }
use(var)
We already cover this case for normal blocks, but missed the edge case
where the block uses numbered variables (i.e. `numblock` vs `block`).
var = nil
do_something { var = _1 }
use(var)
sambostock
force-pushed
the
useless-assignment-bug
branch
from
da20990
to
d5e2e60
Compare
|
Thanks! |
A variable assigned in a block is not unused, if it was captured from the surrounding scope, where it is later used.
We already cover this case for normal blocks, but missed the edge case where the block uses numbered variables (i.e.
numblockvsblock).This fixes the false positive.
Before submitting the PR make sure the following are checked:
Commit message starts with[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.