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.

Sign up for GitHub

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

Jump to bottom

False positive on select {} #804

Closed
abhinav opened this issue Mar 15, 2023 · 2 comments · Fixed by #805
Closed

False positive on select {} #804

abhinav opened this issue Mar 15, 2023 · 2 comments · Fixed by #805

Comments

@abhinav
Copy link
Contributor

abhinav commented Mar 15, 2023

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

  1. I updated revive go install github.com/mgechev/revive@latest
  2. I run it with the following flags & configuration file:
$ cat > main.go << EOF
import "net/http"

func main () {
  go http.ListenAndServe()
  select {}
}
EOF

$ revive -set_exit_status ./...

Expected behavior
No errors.

Logs

empty-block: this block is empty, you can remove it (revive)

Desktop (please complete the following information):

  • OS: Linux
  • Version of Go: 1.20.2

Additional context
Previously reported in #698
@abhinav abhinav mentioned this issue Mar 15, 2023
Merged
abhinav added a commit to abhinav/revive that referenced this issue Mar 15, 2023
@abhinav
empty-block: Fix false positive on select {}
50bb844 
This fixes a false positive reported by revive on the following:

    select {}

This is a way to block the program indefinitely.
It is used in cases like:

- Running a long-running server in a background thread
  and blocking `main` from exiting until the application dies.
  This is something you might do if your process has
  multiple servers/listeners in the same process.

  ```go
  go grpcListenAndServe()
  go httpListenAndServe()
  go logFlusher()

  select {}
  ```

- In programs compiled to WASM to prevent the application from exiting,
  so that the Javascript components may interact with it.

  ```go
  func main() {
    js.Global().Set("foo", foo)
    select {}
  }
  ```

  Without this, one may see an error like,
  "Error: Go program has already exited"

As a workaround, these programs can block forever
by receiving from a throwaway channel (`<-make(chan struct{})`),
but `select {}` is still a completely valid way of doing this,
so supporting it makes sense.

The issue was previously reported in mgechev#698 but was closed
because the author was satisfied with a `//nolint` comment.

Now that this rule is part of the default rule set (mgechev#799)
the case for addressing the false positive is stronger.

Resolves mgechev#804
@chavacava
Copy link
Collaborator

Hi @abhinav, thanks for filling the issue (and providing a PR)
(IMO this is not a false positive, the block is actually empty.)

As I've mentioned in the clone of this issue (#698) the frequency of the construction select {} is low. Moreover, this construction blocks control forever thus warning on it seems to me a good idea.

@abhinav
Copy link
Contributor Author

abhinav commented Mar 15, 2023

Hey, @chavacava. Thanks for the response and for maintaining the tool.

RE: false positive:
I disagree that this is not a false positive.
Yes, the block is empty, but there's a material difference in behavior between the block being there and not.
Sorry, let me clarify: when the linter complains about the empty block in if foo {}, it makes sense because deleting the block is equivalent behavior. No-op to no-op.
However, in the case of select {}, deleting the block does not turn a no-op to no-op. It turns a line that blocks the goroutine into a no-op, which will then allow the goroutine to exit.
Note, for example, that the rule also ignores empty functions because deleting an empty method body can have a material difference on the program's behavior (e.g. if it was implementing an interface).

I agree that as of #698, the frequency of select {} was an adequate reason not to add this.
However, I've added legitimate use cases of select {} in my PR.
Given that this rule is now in the default rule set, avoiding false positives where possible is desirable.

mgechev pushed a commit that referenced this issue Mar 16, 2023
@abhinav
empty-block: Fix false positive on select {} (#805)
b03e54f 
This fixes a false positive reported by revive on the following:

    select {}

This is a way to block the program indefinitely.
It is used in cases like:

- Running a long-running server in a background thread
  and blocking `main` from exiting until the application dies.
  This is something you might do if your process has
  multiple servers/listeners in the same process.

  ```go
  go grpcListenAndServe()
  go httpListenAndServe()
  go logFlusher()

  select {}
  ```

- In programs compiled to WASM to prevent the application from exiting,
  so that the Javascript components may interact with it.

  ```go
  func main() {
    js.Global().Set("foo", foo)
    select {}
  }
  ```

  Without this, one may see an error like,
  "Error: Go program has already exited"

As a workaround, these programs can block forever
by receiving from a throwaway channel (`<-make(chan struct{})`),
but `select {}` is still a completely valid way of doing this,
so supporting it makes sense.

The issue was previously reported in #698 but was closed
because the author was satisfied with a `//nolint` comment.

Now that this rule is part of the default rule set (#799)
the case for addressing the false positive is stronger.

Resolves #804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

empty-block: Fix false positive on select {} abhinav/revive
2 participants
@abhinav @chavacava