Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 18 additions & 2 deletions assertion/function/assertiontree/parse_expr_producer.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,20 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool)
}

// Check if the method is a function value, e.g., `f := func() {}` and then `f()`.
// TODO: this is a temporary fix to suppress false positives caused by function values.
// Remove this once we have have implemented the function value support.
// For function variables, attempt to resolve to the underlying function for proper nil tracking.
// If resolution fails (dynamic assignment, etc.), fall back to trusted behavior.
if r.isVariable(fun) {
// Try to resolve the function variable to its underlying function
if resolvedFunc := r.resolveFuncVariable(fun); resolvedFunc != nil {
// Successfully resolved - use the resolved function for analysis
if !doNotTrack && litArgs() {
return TrackableExpr{&funcAssertionNode{
decl: resolvedFunc, args: expr.Args}}, nil
}
// function call has non-literal args, so is not literal, use its return annotation
return nil, r.getFuncReturnProducersForFunc(resolvedFunc, expr)
}
// Could not resolve - fall back to trusted behavior to avoid false positives
return nil, []producer.ParsedProducer{producer.ShallowParsedProducer{Producer: &annotation.ProduceTrigger{
Annotation: &annotation.TrustedFuncNonnil{ProduceTriggerNever: &annotation.ProduceTriggerNever{}},
Expr: expr,
Expand Down Expand Up @@ -533,7 +544,12 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool)
// getFuncReturnProducers returns a list of producers that are triggered at the call expression
func (r *RootAssertionNode) getFuncReturnProducers(ident *ast.Ident, expr *ast.CallExpr) []producer.ParsedProducer {
funcObj := r.ObjectOf(ident).(*types.Func)
return r.getFuncReturnProducersForFunc(funcObj, expr)
}

// getFuncReturnProducersForFunc returns a list of producers for a given function object and call expression.
// This is the core implementation used by getFuncReturnProducers and also for resolved function variables.
func (r *RootAssertionNode) getFuncReturnProducersForFunc(funcObj *types.Func, expr *ast.CallExpr) []producer.ParsedProducer {
numResults := util.FuncNumResults(funcObj)
isErrReturning := util.FuncIsErrReturning(funcObj.Signature())
isOkReturning := util.FuncIsOkReturning(funcObj.Signature())
Expand Down
61 changes: 61 additions & 0 deletions assertion/function/assertiontree/root_assertion_node.go
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,15 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) {
// Add Consumptions for struct field params
r.addConsumptionsForArgAndReceiverFields(expr, fun)
}
} else if fun := getFuncIdent(expr, &r.functionContext); fun != nil && r.isVariable(fun) {
// Check if this is a function variable that can be resolved to an underlying function.
// This enables nil tracking through function variables like `var ProcessFunc = process`.
if resolvedFunc := r.resolveFuncVariable(fun); resolvedFunc != nil {
consumeArg = consumeArgTrigger(resolvedFunc)
} else {
// Could not resolve the function variable - fall back to no-op
consumeArg = consumeArgNoop
}
} else {
// here we have found either a builtin function like make or new,
// or a typecast like int(x) - in either case (at least for now), do nothing to try
Expand Down Expand Up @@ -941,6 +950,58 @@ func getFuncLitFromAssignment(ident *ast.Ident) *ast.FuncLit {
return nil
}

// resolveFuncVariable attempts to resolve a function variable to the underlying function it points to.
// This handles the common pattern of package-level function variables like `var ProcessFunc = process`
// or local assignments like `f := someFunc`. Returns the resolved *types.Func if successful, nil otherwise.
// This enables nilaway to track nil flows through function variables instead of treating them as trusted.
func (r *RootAssertionNode) resolveFuncVariable(ident *ast.Ident) *types.Func {
if ident == nil || ident.Obj == nil || ident.Obj.Decl == nil {
return nil
}

// Case 1: Package-level variable declaration (var ProcessFunc = process)
if valSpec, ok := ident.Obj.Decl.(*ast.ValueSpec); ok {
// Only handle 1-1 assignments for now
if len(valSpec.Names) != len(valSpec.Values) {
return nil
}

for i, name := range valSpec.Names {
if name.Obj != ident.Obj {
continue
}
// Check if RHS is an identifier pointing to a function
if rhsIdent, ok := valSpec.Values[i].(*ast.Ident); ok {
if funcObj, ok := r.Pass().TypesInfo.ObjectOf(rhsIdent).(*types.Func); ok {
return funcObj
}
}
}
}

// Case 2: Local assignment statement (f := someFunc or f = someFunc)
if assign, ok := ident.Obj.Decl.(*ast.AssignStmt); ok {
if len(assign.Lhs) != len(assign.Rhs) {
return nil
}

for i := range assign.Lhs {
lhsIdent, ok := assign.Lhs[i].(*ast.Ident)
if !ok || lhsIdent.Obj != ident.Obj {
continue
}
// Check if RHS is an identifier pointing to a function
if rhsIdent, ok := assign.Rhs[i].(*ast.Ident); ok {
if funcObj, ok := r.Pass().TypesInfo.ObjectOf(rhsIdent).(*types.Func); ok {
return funcObj
}
}
}
}

return nil
}

// LiftFromPath takes a `path` of assertion nodes, and searches for it in the assertion tree rooted
// at `rootNode`. If found, it removes that tree and returns its root as `node`, with `ok` = true.
// If not found, it returns `node`, `ok` = nil, false
Expand Down
1 change: 1 addition & 0 deletions nilaway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func TestNilAway(t *testing.T) {
{name: "AbnormalFlow", patterns: []string{"go.uber.org/abnormalflow"}},
{name: "NoLint", patterns: []string{"go.uber.org/nolint"}},
{name: "Templ", patterns: []string{"go.uber.org/templ"}},
{name: "FuncVariable", patterns: []string{"go.uber.org/funcvariable"}},
}

for _, tt := range tests {
Expand Down
103 changes: 103 additions & 0 deletions testdata/src/go.uber.org/funcvariable/funcvariable.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) 2023 Uber Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/*
These tests check nil tracking through function variables.

<nilaway no inference>
*/
package funcvariable

type Config struct {
Value string
}

// nilable(result 0)
func nilableFunc() *Config {
return nil
}

func nonNilFunc() *Config {
return &Config{Value: "test"}
}

// Package-level function variable pointing to a nilable function
var NilableFuncVar = nilableFunc

// Package-level function variable pointing to a non-nil function
var NonNilFuncVar = nonNilFunc

// Test 1: Direct call to nilable function - should be detected
func testDirectNilableCall() {
cfg := nilableFunc()
_ = cfg.Value //want "accessed field"
}

// Test 2: Call through function variable pointing to nilable function - should be detected
func testFuncVarNilableCall() {
cfg := NilableFuncVar()
_ = cfg.Value //want "accessed field"
}

// Test 3: Direct call to non-nil function - should NOT be detected
func testDirectNonNilCall() {
cfg := nonNilFunc()
_ = cfg.Value // OK - nonNilFunc always returns non-nil
}

// Test 4: Call through function variable pointing to non-nil function - should NOT be detected
func testFuncVarNonNilCall() {
cfg := NonNilFuncVar()
_ = cfg.Value // OK - NonNilFuncVar points to nonNilFunc which always returns non-nil
}

// Test 5: Local function variable assigned from nilable function
func testLocalFuncVarNilable() {
f := nilableFunc
cfg := f()
_ = cfg.Value //want "accessed field"
}

// Test 6: Local function variable assigned from non-nil function
func testLocalFuncVarNonNil() {
f := nonNilFunc
cfg := f()
_ = cfg.Value // OK - f points to nonNilFunc
}

// Test 7: Function that takes a pointer and dereferences it
func process(cfg *Config) {
// The nil flow is detected at the call site, not here
println(cfg.Value)
}

// Test 8: Package-level function variable for process
var ProcessFunc = process

// Test 9: Calling process through function variable with nil - should be detected
func testProcessFuncVarNil() {
ProcessFunc(nil) //want "passed"
}

// Test 10: Calling process directly with nil - should be detected
func testProcessDirectNil() {
process(nil) //want "passed"
}

// Test 11: Calling process with non-nil - should NOT be detected
func testProcessNonNil() {
cfg := &Config{Value: "test"}
process(cfg) // OK
ProcessFunc(cfg) // OK
}
Loading