Skip to content

Commit

Permalink
Optimistic default for nil receivers of out-of-scope (e.g., generated…
Browse files Browse the repository at this point in the history
… code) methods (#146)

This PR adds optimistic default for nil receivers of out-of-scope (e.g.,
generated code) methods. Without this support, NilAway was observed to
report false positives for nil-safe methods that were outside of
NilAway's analysis scope.
  • Loading branch information
sonalmahajan15 authored Dec 4, 2023
1 parent 0ef3071 commit 2f6a74d
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 6 deletions.
16 changes: 12 additions & 4 deletions assertion/function/assertiontree/root_assertion_node.go
Original file line number Diff line number Diff line change
Expand Up @@ -730,9 +730,12 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) {
// with so far the only known case being of method invocations for supporting nilable receivers. Our support
// is currently limited to enabling this analysis only if the below criteria is satisfied.
// - Check 1: selector expression is a method invocation (e.g., `s.foo()`)
// - Check 2: the invoked method is in scope
// - Check 3: the invoking expression (caller) is of struct type. (We are restricting support only for structs
// due to the challenges documented in .)
// - In-scope flow:
// - Check 2: the invoked method is in scope
// - Check 3: the invoking expression (caller) is of struct type. (We are restricting support only for structs
// due to the challenges of secret nil for interfaces.)
// - Out-of-scope flow:
// - Check 4: consider the criteria satisfied to support optimistic default
//
// - (2) Don't allow the expression X to be nilable by creating a FldAccess (ConsumeTriggerTautology) consumer for it.
// This is default behavior which gets triggered if the above special case is not satisfied.
Expand All @@ -742,7 +745,7 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) {
conf := r.Pass().ResultOf[config.Analyzer].(*config.Config)
if conf.IsPkgInScope(funcObj.Pkg()) { // Check 2: invoked method is in scope
t := util.TypeOf(r.Pass(), expr.X)
// Here, `t` can only be of type struct or interface, of which we only support for structs (see .
// Here, `t` can only be of type struct or interface, of which we only support for structs.
if util.TypeAsDeeplyStruct(t) != nil { // Check 3: invoking expression (caller) is of struct type
allowNilable = true
// We are in the special case of supporting nilable receivers! Can be nilable depending on declaration annotation/inferred nilability.
Expand All @@ -757,6 +760,11 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) {
Guards: util.NoGuards(),
})
}
} else { // Check 4: invoked method is out of scope
// We are setting an optimistic default here for methods out of scope, specifically to avoid
// false positives being reported for methods in generated code. It means that such external
// methods are assumed to be safely handling nil receivers
allowNilable = true
}
}
if !allowNilable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@

package inference

import (
"os"
)

var dummy bool

type A struct {
Expand Down Expand Up @@ -53,16 +57,21 @@ func testRecv() {

// -----------------------------------
// the below test checks for in-scope analysis of receivers. If a receiver-based call is made to an external method,
// such as `err.Error()`, then it is treated as a normal field access of `err`, reporting an error if `err == nil`.
// such as `err.Error()`, then it is treated with optimistic default, assuming the external method to be handling
// nil receivers. This can potentially result in false negatives, as shown below in the example of `err.Error()`.
// However, this is a trade-off made to avoid false positives.

func (a *A) retErr() error {
return nil
}

func testInScope() {
var file *os.File
_, _ = file.Stat() // true negative, since `Stat()` is nil-safe

var a *A
err := a.retErr()
print(err.Error()) //want "returned as error result 0 of `retErr.*`"
print(err.Error()) // false negative, since `Error()` is nil-unsafe
}

// -----------------------------------
Expand Down

0 comments on commit 2f6a74d

Please sign in to comment.