From ff0263534f9ff4a8dea60e355deb7ef10314a9f4 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Wed, 31 Jan 2024 21:40:46 -0500 Subject: [PATCH 1/3] update --- annotation/consume_trigger.go | 39 ------------------- annotation/consume_trigger_test.go | 1 - assertion/function/assertiontree/backprop.go | 14 +++---- .../assertiontree/root_assertion_node.go | 17 ++++---- inference/engine.go | 1 - testdata/src/go.uber.org/channels/channels.go | 32 +++++++-------- .../go.uber.org/errormessage/errormessage.go | 22 ++++------- 7 files changed, 37 insertions(+), 89 deletions(-) diff --git a/annotation/consume_trigger.go b/annotation/consume_trigger.go index 3898616e..421b01e8 100644 --- a/annotation/consume_trigger.go +++ b/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 ©Consumer -} - -// 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 diff --git a/annotation/consume_trigger_test.go b/annotation/consume_trigger_test.go index b2fc2405..3b2dec7c 100644 --- a/annotation/consume_trigger_test.go +++ b/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()}}, 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/inference/engine.go b/inference/engine.go index 43f25f5a..e20cbba5 100644 --- a/inference/engine.go +++ b/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{}) 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 From 676012c94fb1f9cce1d6a50cde635b6397bc7491 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Fri, 2 Feb 2024 13:53:57 -0500 Subject: [PATCH 2/3] Update assertion/function/assertiontree/backprop.go Co-authored-by: Sonal Mahajan <101232472+sonalmahajan15@users.noreply.github.com> --- assertion/function/assertiontree/backprop.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assertion/function/assertiontree/backprop.go b/assertion/function/assertiontree/backprop.go index bc6435d9..a64718b0 100644 --- a/assertion/function/assertiontree/backprop.go +++ b/assertion/function/assertiontree/backprop.go @@ -121,7 +121,7 @@ func backpropAcrossSend(rootNode *RootAssertionNode, node *ast.SendStmt) error { // (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. + // cannot support it. // TODO: rethink our strategy of handling channels (#192). consumer, err := exprAsAssignmentConsumer(rootNode, node, nil) if err != nil { From fe6b551edc51076918c30152d4c181bfada0e29a Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Fri, 2 Feb 2024 13:54:01 -0500 Subject: [PATCH 3/3] Update assertion/function/assertiontree/root_assertion_node.go Co-authored-by: Sonal Mahajan <101232472+sonalmahajan15@users.noreply.github.com> --- assertion/function/assertiontree/root_assertion_node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assertion/function/assertiontree/root_assertion_node.go b/assertion/function/assertiontree/root_assertion_node.go index 9463be7f..ba8bafd6 100644 --- a/assertion/function/assertiontree/root_assertion_node.go +++ b/assertion/function/assertiontree/root_assertion_node.go @@ -813,7 +813,7 @@ func (r *RootAssertionNode) AddComputation(expr ast.Expr) { // (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. + // state of channels, we currently cannot support it either. // TODO: rethink our strategy of handling channels (#192). r.AddComputation(expr.X) case *ast.FuncLit: