diff --git a/inference/engine.go b/inference/engine.go index d1b63a73..31940506 100644 --- a/inference/engine.go +++ b/inference/engine.go @@ -262,22 +262,24 @@ func (e *Engine) ObservePackage(pkgFullTriggers []annotation.FullTrigger) { isDeep := kind == annotation.DeepConditional primitive := e.primitive.site(site, isDeep) if val, ok := e.inferredMap.Load(primitive); ok { - switch vType := val.(type) { - case *DeterminedVal: + if vType, ok := val.(*DeterminedVal); ok { if !vType.Bool.Val() { return assertiontree.ProducerIsNonNil } - case *UndeterminedVal: - // Consider the producer site as non-nil, if the determined value is non-nil, i.e., - // `!vType.Bool.Val()`, or the site is undetermined. Undetermined sites are assumed "non-nil" here based on the following: - // (a) the inference algorithm does not propagate non-nil values forward, and - // (b) the processing of the sites under question, error return sites, are allowed to be processed first in step 1 above - // - // This above assumption works favorably in most of the cases, except the one demonstrated in - // `testdata/errorreturn/inference/downstream.go`, for instance, where it leads to a false negative. - return assertiontree.ProducerIsNonNil + return assertiontree.ProducerIsNil } } + // We reach here if `primitive` site is + // - present in `inferredMap` but UndeterminedVal, or + // - not present in `inferredMap`, implying undetermined. + // + // At this point we consider undetermined sites producer site as non-nil, based on the following: + // (a) the inference algorithm does not propagate non-nil values forward + // (b) the processing of the sites under question (i.e., error return sites) are allowed to be processed first in step 1 above + // + // This above assumption works favorably in most of the cases, except the one demonstrated in + // `testdata/errorreturn/inference/downstream.go`, for instance, where it leads to a false negative. + return assertiontree.ProducerIsNonNil } // In all other cases, return ProducerNilabilityUnknown to indicate that all we diff --git a/testdata/src/go.uber.org/errorreturn/inference/errorreturn-with-inference.go b/testdata/src/go.uber.org/errorreturn/inference/errorreturn-with-inference.go index a5792ac1..1442f058 100644 --- a/testdata/src/go.uber.org/errorreturn/inference/errorreturn-with-inference.go +++ b/testdata/src/go.uber.org/errorreturn/inference/errorreturn-with-inference.go @@ -38,7 +38,7 @@ func retNonNilErr2() error { return &myErr2{} } -// ***** the below test case checks error return via a function and assigned to a vairable ***** +// ***** the below test case checks error return via a function and assigned to a variable ***** func retPtrAndErr2(i int) (*int, error) { if dummy2 { return nil, retNonNilErr2() @@ -46,8 +46,21 @@ func retPtrAndErr2(i int) (*int, error) { return &i, retNilErr2() } -func testFuncRet2(i int) (*int, error) { +// same as retPtrAndErr2 but with the return statements swapped. This is to check that the order of return statements +// does not affect the error return analysis +func retPtrAndErr3() (*int, error) { + if dummy2 { + return new(int), retNilErr2() + } + return nil, retNonNilErr3() +} +// duplicated from retNonNilErr2 to make a fresh instance of the function for supporting the testing of retPtrAndErr3 +func retNonNilErr3() error { + return &myErr2{} +} + +func testFuncRet2(i int) (*int, error) { var errNil = retNilErr2() var errNonNil = retNonNilErr2() switch i { @@ -69,6 +82,8 @@ func testFuncRet2(i int) (*int, error) { return &i, errNonNil case 8: return &i, retNonNilErr2() + case 9: + return retPtrAndErr3() } return &i, nil }