Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive for a nil channel select case #98

Closed
sonalmahajan15 opened this issue Nov 15, 2023 · 4 comments · Fixed by #193
Closed

False positive for a nil channel select case #98

sonalmahajan15 opened this issue Nov 15, 2023 · 4 comments · Fixed by #193
Labels
false positive Requires more analysis and support

Comments

@sonalmahajan15
Copy link
Contributor

Although a direct send/receive on a nil channel can cause a panic, the following code with use of select is safe. However, NilAway still reports an error.

func foo(aChan, bChan chan string) {
	aChan <- "abc"
	bChan <- "xyz"

	if true {
		aChan = nil
	}

	select {
	case a := <-aChan:   // error: "literal `nil` uninitialized; nil channel accessed"
		print(a)
	case b := <-bChan:
		print(b)
	}
}

func test() {
	aChan := make(chan string)
	bChan := make(chan string)

	go foo(aChan, bChan)
}
@bokwoon95
Copy link

Although a direct send/receive on a nil channel can cause a panic

Actually, both writing and reading on a nil channel never panics 🙂. Not just on select. Here is some code to replicate:

package main

import (
    "fmt"
    "time"
)

func main() {
    var aChan chan string = nil
    // Write to a nil channel; does not panic, blocks forever.
    go func() {
        aChan <- "sendValue"
    }()
    // Read from a nil channel; does not panic, blocks forever.
    go func() {
        receiveValue := <-aChan
        fmt.Println(receiveValue)
    }()
    // Print an empty string to console every 5 seconds to prevent deadlock.
    for {
        time.Sleep(5 * time.Second)
        fmt.Print("")
    }
}

@orsinium
Copy link

Here is a convenient table from "Concurrency in Go" of how each channel operation behaves on each channel type. As you can see, only closing a nil channel may cause panic.

image

@roma-glushko
Copy link

Is there a way to ignore a specific line where the error happens? I have tried nolint:nilaway but that did not help 😞

@sonalmahajan15
Copy link
Contributor Author

Is there a way to ignore a specific line where the error happens? I have tried nolint:nilaway but that did not help 😞

@roma-glushko: If you are using the standalone go/analysis driver, then unfortunately //nolint:nilaway is not currently supported. In principle, we believe that enabling this support falls under the responsibility of the driver, as demonstrated by the support provided by nogo. Nevertheless, we have it on the roadmap to implement this support within NilAway itself to make it driver independent. (You can refer to #143 (comment) for more context.)

yuxincs added a commit that referenced this issue Feb 2, 2024
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 <101232472+sonalmahajan15@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false positive Requires more analysis and support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants