Skip to content

Bugfix false positive for "unused" variable for nested defs #1376

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.

Sign up for GitHub

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

Merged
oreflow merged 1 commit into from
Nov 7, 2025
Merged

Bugfix false positive for "unused" variable for nested defs #1376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions warn/warn_control_flow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,9 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
// Symbols for which the warning should be suppressed
suppressedWarnings := make(map[string]bool)

// Symbols from outer scopes that are used in the current scope
usedSymbolsFromOuterScope := make(map[string]bool)

build.WalkStatements(root, func(expr build.Expr, stack []build.Expr) (err error) {
switch expr := expr.(type) {
case *build.File:
Expand Down Expand Up @@ -463,7 +466,8 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
// a function call with a keyword parameter.
_, used := extractIdentsFromStmt(assign.RHS)
for ident := range used {
usedSymbols[ident.Name] = true
// RHS idents in the def statement contains direct references to the outer scope.
usedSymbolsFromOuterScope[ident.Name] = true
}
}

Expand Down Expand Up @@ -497,7 +501,6 @@ func unusedVariableCheck(f *build.File, root build.Expr) (map[string]bool, []*Li
// make the variable with the same name from an outer scope also used.
// Collect variables that are used in the current or inner scopes but are not
// defined in the current scope.
usedSymbolsFromOuterScope := make(map[string]bool)
for symbol := range usedSymbols {
if _, ok := definedSymbols[symbol]; ok {
continue
Expand Down
29 changes: 29 additions & 0 deletions warn/warn_control_flow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,35 @@ bar()
":9: Variable \"y\" is unused.",
},
scopeEverywhere)

// foo is unused in the outer macro, since the symbol is overloaded, and only
// the inner_def "foo" is actually used.
checkFindings(t, "unused-variable", `
Copy link
Collaborator

@AnnaSvalova AnnaSvalova Nov 6, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@oreflow oreflow Nov 7, 2025

Choose a reason for hiding this comment

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

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

def sample_macro_with_unused_foo(name, foo = "foo"):
def inner_def(foo = "bar"):
print(foo)

inner_def()

sample_macro_with_unused_foo()
`,
[]string{
":1: Variable \"foo\" is unused.",
},
scopeEverywhere)

// Foo is referenced as the default value for foo in inner_def, hence it is not unused.
checkFindings(t, "unused-variable", `
def sample_macro_with_used_foo(name, foo = "foo"):
def inner_def(bar = foo):
print(bar)

inner_def()

sample_macro_with_used_foo()
`,
[]string{},
scopeEverywhere)
}

func TestRedefinedVariable(t *testing.T) {
Expand Down
Loading