Fix for making rich check effect handling unbiased for return stateme…

…nt ordering (#254)

This PR fixes the bug which made rich check effect handling sensitive to
return statement ordering, resulting in false positives for certain
cases. The effect of ordering of statements is illustrated in the
example below.

```
// Problematic
func retPtrErr() (*int, error) {
	if cond {
		return new(int), retNil()
	}
	return nil, retErr()
}

// Works fine
func retPtrErr() (*int, error) {
	if cond {
	       return nil, retErr()	
	}	
        return new(int), retNil()
}
```

The problem is with no entry being created in `inferredMap` for non-nil
error return from `retErr()` in `StoreImplication`, since we don't
propagate non-nilness forward. This creates a problem when the anonymous
function parameter in `FilterTriggersForErrorReturn` analyzes for
nilability of return sites, where it incorrectly assumes that absence in
`inferredMap` implies unknown nilability. This PR corrects that logic.

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

testdata/src/go.uber.org/errorreturn/inference/errorreturn-with-inference.go

@@ -38,16 +38,29 @@ 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()
 	}
 	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
 }