From 25652b04ee948596ec8ecb85dc44486c80331841 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Wed, 31 Jan 2024 21:40:46 -0500 Subject: [PATCH] update --- assertion/function/assertiontree/backprop.go | 14 ++++---- .../assertiontree/root_assertion_node.go | 17 +++++----- testdata/src/go.uber.org/channels/channels.go | 32 ++++++++----------- .../go.uber.org/errormessage/errormessage.go | 22 +++++-------- 4 files changed, 37 insertions(+), 48 deletions(-) diff --git a/assertion/function/assertiontree/backprop.go b/assertion/function/assertiontree/backprop.go index 1ef8b5b9..bc6435d9 100644 --- a/assertion/function/assertiontree/backprop.go +++ b/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 + // can not support it. + // TODO: rethink our strategy of handling channels (#192). consumer, err := exprAsAssignmentConsumer(rootNode, node, nil) if err != nil { return err diff --git a/assertion/function/assertiontree/root_assertion_node.go b/assertion/function/assertiontree/root_assertion_node.go index a3fdcde2..9463be7f 100644 --- a/assertion/function/assertiontree/root_assertion_node.go +++ b/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 can not 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 diff --git a/testdata/src/go.uber.org/channels/channels.go b/testdata/src/go.uber.org/channels/channels.go index 7583921c..e313da53 100644 --- a/testdata/src/go.uber.org/channels/channels.go +++ b/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 - - -*/ +// Package channels tests that nilaway properly handles channels. +// 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) diff --git a/testdata/src/go.uber.org/errormessage/errormessage.go b/testdata/src/go.uber.org/errormessage/errormessage.go index 47cfcc91..2d81fe58 100644 --- a/testdata/src/go.uber.org/errormessage/errormessage.go +++ b/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