Bugfix false positive for "unused" variable for nested defs#1376
Merged
Bugfix false positive for "unused" variable for nested defs#1376
defs#1376Conversation
AnnaSvalova
requested review from
AnnaSvalova
and removed request for
pmbethe09
AnnaSvalova
requested changes
Nov 6, 2025
| }, | ||
| scopeEverywhere) | ||
|
|
||
| checkFindings(t, "unused-variable", ` |
Collaborator
There was a problem hiding this comment.
Could you split this into 2 different test cases please? Current ":1: Variable "foo" is unused." is error prone, as it depends on the order. Also it's not clear without enough context what this :1 is.
Collaborator
Author
There was a problem hiding this comment.
Oh, actually the first and second test case here seem to have nothing to do with each other 🤔 added comments to describe more detail on what is tested, and renamed the second inner param to "bar" since the name overlap had nothing to do with the test (if I understand this correctly). PTAL
oreflow
force-pushed
the
nested-defs
branch
2 times, most recently
from
fdb700f to
05dd2f8
Compare
When detecting whether symbols are used or not, variables referenced as argument defaults of the inner def would count as "use" in the inner scope, and hence falsely trigger "unused" warning.
Example:
```
def outer_def(name, foo = "foo"):
def inner_def(foo = foo):
print(foo) # buildifier: disable=print
inner_def()
```
Unused variable check for the above function would
1. Check outer_def for variable usage
2. Recursively check inner_def for variable usage
- Detect that `foo` is used
- Omit `foo` from returned usedSymbolsFromOuterScope since `foo` is a local variable
3. Receive no "usedSymbol" for `foo`
4. Output "unused-variable" warning
If there are RHS idents in a def statement, these idents are always from an outer scope, so these can check the later inner-scope definedSymbols check.
AnnaSvalova
approved these changes
Nov 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When detecting whether symbols are used or not, variables referenced as argument defaults of the inner def would count as "use" in the inner scope, and hence falsely trigger "unused" warning.
Example:
Unused variable check for the above function would
foois usedfoofrom returned usedSymbolsFromOuterScope sincefoois a local variablefooIf there are RHS idents in a def statement, these idents are always from an outer scope, so these can check the later inner-scope definedSymbols check.