Fix NilAway FPs on channel sends/receives (#193)

See issue #192 for a more detailed discussion on handling channels in
NilAway. To make this PR more self-contained, below is a partial copy of
the discussion there:

See the following behaviors regarding nil / closed channels (taken from
[this SO
post](https://stackoverflow.com/questions/43616434/closed-channel-vs-nil-channel),
slightly modified):
> 1. A send to a nil channel blocks forever
> 2. A receive from a nil channel blocks forever
> 3. A send to a closed channel panics
> 4. A receive from a closed channel returns the zero value immediately

Only case 3 (sending to a closed channel) will result in a panic. The
other cases arguably may not be within scope of NilAway's analysis.
Moreover, NilAway currently only tracks nilabilities of channels, but
not the states of them, making it unable to really handle case 2 and 4.

Since this introduces a lot more FPs than TPs (if any), this PR removes
logic to require sending to/receiving from nonnil channels. We leave
properly handling channels as future work (especially around tracking
states of channels).

The test cases have been updated to reflect this. Specifically, we kept
the test cases there to now serve as negative cases in case for future
development of such a support.

Fixes #98 
Fixes #167

---------

Co-authored-by: Sonal Mahajan <[email protected]>

annotation/consume_trigger.go

@@ -1615,45 +1615,6 @@ func (g GlobalVarAssignDeepPrestring) String() string {
 	return sb.String()
 }
 
-// ChanAccess is when a channel is accessed for sending, and thus must be non-nil
-type ChanAccess struct {
-	*ConsumeTriggerTautology
-}
-
-// equals returns true if the passed ConsumingAnnotationTrigger is equal to this one
-func (c *ChanAccess) equals(other ConsumingAnnotationTrigger) bool {
-	if other, ok := other.(*ChanAccess); ok {
-		return c.ConsumeTriggerTautology.equals(other.ConsumeTriggerTautology)
-	}
-	return false
-}
-
-// Copy returns a deep copy of this ConsumingAnnotationTrigger
-func (c *ChanAccess) Copy() ConsumingAnnotationTrigger {
-	copyConsumer := *c
-	copyConsumer.ConsumeTriggerTautology = c.ConsumeTriggerTautology.Copy().(*ConsumeTriggerTautology)
-	return &copyConsumer
-}
-
-// Prestring returns this MapWrittenTo as a Prestring
-func (c *ChanAccess) Prestring() Prestring {
-	return ChanAccessPrestring{
-		AssignmentStr: c.assignmentFlow.String(),
-	}
-}
-
-// ChanAccessPrestring is a Prestring storing the needed information to compactly encode a ChanAccess
-type ChanAccessPrestring struct {
-	AssignmentStr string
-}
-
-func (c ChanAccessPrestring) String() string {
-	var sb strings.Builder
-	sb.WriteString("uninitialized; nil channel accessed")
-	sb.WriteString(c.AssignmentStr)
-	return sb.String()
-}
-
 // LocalVarAssignDeep is when a value flows to a point where it is assigned deeply into a local variable of deeply nonnil type
 type LocalVarAssignDeep struct {
 	*TriggerIfDeepNonNil

annotation/consume_trigger_test.go

@@ -52,7 +52,6 @@ var initStructsConsumingAnnotationTrigger = []any{
 	&VariadicParamAssignDeep{TriggerIfNonNil: &TriggerIfNonNil{Ann: newMockKey()}},
 	&FieldAssignDeep{TriggerIfDeepNonNil: &TriggerIfDeepNonNil{Ann: newMockKey()}},
 	&GlobalVarAssignDeep{TriggerIfDeepNonNil: &TriggerIfDeepNonNil{Ann: newMockKey()}},
-	&ChanAccess{ConsumeTriggerTautology: &ConsumeTriggerTautology{}},
 	&LocalVarAssignDeep{TriggerIfDeepNonNil: &TriggerIfDeepNonNil{Ann: newMockKey()}},
 	&ChanSend{TriggerIfDeepNonNil: &TriggerIfDeepNonNil{Ann: newMockKey()}},
 	&FldEscape{TriggerIfNonNil: &TriggerIfNonNil{Ann: newMockKey()}},

assertion/function/assertiontree/backprop.go

@@ -116,13 +116,13 @@ func backpropAcrossNode(rootNode *RootAssertionNode, node ast.Node) error {
 // backpropAcrossSend handles backpropagation for send statements. It is designed to be called from
 // backpropAcrossNode as a special handler.
 func backpropAcrossSend(rootNode *RootAssertionNode, node *ast.SendStmt) error {
-	// Added this consumer since sending over a nil channel can cause panic
-	rootNode.AddConsumption(&annotation.ConsumeTrigger{
-		Annotation: &annotation.ChanAccess{ConsumeTriggerTautology: &annotation.ConsumeTriggerTautology{}},
-		Expr:       node.Chan,
-		Guards:     util.NoGuards(),
-	})
-
+	// Note that for channel sends, we have:
+	// (1) A send to a nil channel blocks forever;
+	// (2) A send to a closed channel panics.
+	// (1) falls out of scope for NilAway and hence we do not create a consumer here for the
+	// channel variable. For (2), since we do not track the state of the channels, we currently
+	// cannot support it.
+	// TODO: rethink our strategy of handling channels (#192).
 	consumer, err := exprAsAssignmentConsumer(rootNode, node, nil)
 	if err != nil {
 		return err

assertion/function/assertiontree/root_assertion_node.go

@@ -807,15 +807,14 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) {
 		// doesn't need to be non-nil, but really should be
 		r.AddComputation(expr.X)
 	case *ast.UnaryExpr:
-		// channel receive case
-		if expr.Op == token.ARROW {
-			// added this consumer since receiving over a nil channel can cause panic
-			r.AddConsumption(&annotation.ConsumeTrigger{
-				Annotation: &annotation.ChanAccess{ConsumeTriggerTautology: &annotation.ConsumeTriggerTautology{}},
-				Expr:       expr.X,
-				Guards:     util.NoGuards(),
-			})
-		}
+		// Note if expr.Op == token.ARROW it represents a channel receive (<-X), and we have:
+		// (1) A receive from a nil channel blocks forever;
+		// (2) A receive from a closed channel returns the zero value immediately.
+		// (1) falls out of scope of NilAway, and we have a lot of valid Go code that receives
+		// from nil channels (e.g., select statements with nilable channels). So we do not create
+		// consumer for the channel variable here. For (2), since we currently do not track the
+		// state of channels, we currently cannot support it either.
+		// TODO: rethink our strategy of handling channels (#192).
 		r.AddComputation(expr.X)
 	case *ast.FuncLit:
 		// TODO: analyze the bodies of anonymous functions

inference/engine.go

@@ -505,7 +505,6 @@ func GobRegister() {
 	gob.RegisterName(nextStr(), annotation.MapAccessPrestring{})
 	gob.RegisterName(nextStr(), annotation.MapWrittenToPrestring{})
 	gob.RegisterName(nextStr(), annotation.SliceAccessPrestring{})
-	gob.RegisterName(nextStr(), annotation.ChanAccessPrestring{})
 	gob.RegisterName(nextStr(), annotation.FldAccessPrestring{})
 	gob.RegisterName(nextStr(), annotation.UseAsErrorResultPrestring{})
 	gob.RegisterName(nextStr(), annotation.FldAssignPrestring{})

testdata/src/go.uber.org/channels/channels.go

@@ -12,14 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-/*
-These tests aim to ensure that nilaway properly handles channels
-
-<nilaway no inference>
-*/
+// Package channels tests that nilaway properly handles channels.
+// <nilaway no inference>
 package channels
 
-// BELOW TESTS CHECK DEEP NILABILITY OF CHANNELS
 
 // nilable(<-nilableChan)
 var nilableChan = make(chan *int)
@@ -528,7 +524,7 @@ func testRangeOverChans(a, b, c, d chan *int) *int {
 func takesNonnil(interface{}) {}
 
 func singleKeysEstablishNonnil(ch chan *int) {
-	v, ok := <-ch //want "uninitialized"
+	v, ok := <-ch
 
 	// here, ch and v should be nilable
 	takesNonnil(v)  //want "passed"
@@ -581,7 +577,7 @@ func plainReflCheck(ch chan any) any {
 		return ch //want "returned"
 	}
 
-	_, ok := <-ch //want "uninitialized"
+	_, ok := <-ch
 
 	if ok {
 		return ch
@@ -595,39 +591,39 @@ var nilChanGlobal chan string
 var nonnilChanGlobal = make(chan string)
 
 func testSendToGlobalChan() {
-	nilChanGlobal <- "xyz" //want "uninitialized"
+	nilChanGlobal <- "xyz"
 	nonnilChanGlobal <- "xyz"
 }
 
 // nonnil(nonnilChanParam)
 func testSendToParamChan(nilChanParam chan string, nonnilChanParam chan string) {
-	nilChanParam <- "xyz" //want "uninitialized"
+	nilChanParam <- "xyz"
 	nonnilChanParam <- "xyz"
 }
 
 func testSendToLocalChan() {
 	var nilChanLocal chan string
-	nilChanLocal <- "xyz" //want "uninitialized"
+	nilChanLocal <- "xyz"
 
 	var nonnilChanLocal = make(chan string)
 	nonnilChanLocal <- "xyz"
 }
 
 func testRecvFromGlobalChan() (string, string) {
-	return <-nilChanGlobal, <-nonnilChanGlobal //want "uninitialized"
+	return <-nilChanGlobal, <-nonnilChanGlobal
 }
 
 // nonnil(nonnilChanParam)
 func testRecvFromParamChan(nilChanParam chan string, nonnilChanParam chan string) {
-	v1 := <-nilChanParam //want "uninitialized"
+	v1 := <-nilChanParam
 	v2 := <-nonnilChanParam
 	func(...any) {}(v1, v2)
 }
 
 func testRecvFromLocalChan() {
 	var nilChanLocal chan string
-	nilChanLocal <- "xyz" //want "uninitialized"
-	v1 := <-nilChanLocal  //want "uninitialized"
+	nilChanLocal <- "xyz"
+	v1 := <-nilChanLocal
 
 	var nonnilChanLocal = make(chan string)
 	nonnilChanLocal <- "xyz"
@@ -648,14 +644,14 @@ func retNonNilChan() chan string {
 
 func testSendRecvFuncRet() {
 	nilChanLocal := retNilChan()
-	nilChanLocal <- "xyz" //want "uninitialized"
-	v1 := <-nilChanLocal  //want "uninitialized"
+	nilChanLocal <- "xyz"
+	v1 := <-nilChanLocal
 
 	nonnilChanLocal := retNonNilChan()
 	nonnilChanLocal <- "xyz"
 	v2 := <-nonnilChanLocal
 
-	nilChanLocal <- <-nonnilChanGlobal //want "uninitialized"
+	nilChanLocal <- <-nonnilChanGlobal
 	nonnilChanLocal <- <-nonnilChanGlobal
 
 	func(...any) {}(v1, v2)

testdata/src/go.uber.org/errormessage/errormessage.go

@@ -101,28 +101,22 @@ func test9(m map[int]*int) {
 	print(*y) //want "`m\\[0\\]` to `x`"
 }
 
-func test10(ch chan *int) {
-	x := <-ch //want "nil channel accessed"
-	y := x
-	print(*y)
-}
-
-func callTest10() {
-	var ch chan *int
-	test10(ch)
+// nilable(nilableChan) nonnil(nonnilDeeplyNonnilChan, <-nonnilDeeplyNonnilChan)
+func test10(nilableChan chan *int, nonnilDeeplyNonnilChan chan *int) {
+	x := 1
+	nilableChan <- &x
+	// Sending nilable values to nonnil and deeply nonnil channels is not OK.
+	var y *int
+	nonnilDeeplyNonnilChan <- y //want "`y` assigned deeply into parameter arg `nonnilDeeplyNonnilChan`"
 }
 
+// nilable(s)
 func test11(s []*int) {
 	x := s[0] //want "`s` sliced into"
 	y := x
 	print(*y)
 }
 
-func callTest11() {
-	var s []*int
-	test11(s)
-}
-
 func test12(mp map[int]S, i int) {
 	x := mp[i] // unrelated assignment, should not be printed in the error message
 	_ = x