Fix NilAway FPs on channel sends/receives

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