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

all: redundant or missing nil checks #30208

Closed
marat-rkh opened this issue Feb 13, 2019 · 10 comments
Closed

all: redundant or missing nil checks #30208

marat-rkh opened this issue Feb 13, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.13

Comments

@marat-rkh
Copy link

Hi! I am writing a static analysis tool to detect problems with nil values in Go programs. I have executed it on the source code of the Go itself and found some minor issues.

  1. Redundant err == nil check:
    https://github.com/golang/go/blob/master/src/cmd/go/internal/work/exec.go#L106
    err is a local variable created with nil value and it is not updated before the check (making it redundant).

  2. Redundant err != nil check:
    https://github.com/golang/go/blob/master/src/crypto/x509/name_constraints_test.go#L2223
    err is always not nil because of the continue statement a couple of lines above.

  3. Potential 'nil pointer dereference':
    https://github.com/golang/go/blob/master/src/net/textproto/reader_test.go#L335
    https://github.com/golang/go/blob/master/src/html/template/escape_test.go#L1873
    In both cases, err can be nil making err.Error() call dangerous. err == nil check above doesn't stop the execution of the test.

I already have a fork with these issues fixed, will be happy to submit a pull request.
@agnivade
Copy link
Contributor

Please go ahead ! Thanks.

@mvdan mvdan added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 13, 2019
marat-rkh added a commit to marat-rkh/go that referenced this issue Feb 13, 2019
@marat-rkh
go/cmd: remove redundant check for nil
05ea5a8 
Local variable err is defined with nil value and almost immediately
compared with nil. This comparison is redundant as always evaluates
to true.

Updates golang#30208
marat-rkh added a commit to marat-rkh/go that referenced this issue Feb 13, 2019
@marat-rkh
crypto/x509: remove redundant check for nil in tests
372e0fd 
Comparing err variable to be not nil is redundant in this case.
The code above ensures that it is always not nil.

Updates golang#30208
marat-rkh added a commit to marat-rkh/go that referenced this issue Feb 13, 2019
@marat-rkh
net/textproto: prevent test from failing with nil pointer dereference
3ab20f6 
The variable err could have nil value when we call err.Error(),
because after we check it for nil above we continue the test
(t.Errorf doesn't stop the test execution).

Updates golang#30208
marat-rkh added a commit to marat-rkh/go that referenced this issue Feb 13, 2019
@marat-rkh
html/template: prevent test from failing with nil pointer dereference
3a5d9b1 
The variable err could have nil value when we call err.Error(),
because after we check it for nil above we continue the test
(t.Errorf doesn't stop the test execution).

Updates golang#30208
This was referenced Feb 13, 2019
Closed
Closed
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/162438 mentions this issue: go/cmd: remove redundant check for nil

@marat-rkh marat-rkh mentioned this issue Feb 13, 2019
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/162439 mentions this issue: crypto/x509: remove redundant check for nil in tests

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/162477 mentions this issue: html/template: prevent test from failing with nil pointer dereference

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/162457 mentions this issue: net/textproto: prevent test from failing with nil pointer dereference

@dgryski
Copy link
Contributor

dgryski commented Feb 13, 2019

The new data-flow based nilness check in vet finds the first two cases:

/Users/dgryski/go/src/go.googlesource.com/go/src/cmd/go/internal/work/exec.go:106:11: tautological condition: nil == nil
...
/Users/dgryski/go/src/go.googlesource.com/go/src/crypto/x509/name_constraints_test.go:2223:10: tautological condition: non-nil != nil

but I'm not aware of a tool that handles the third one. Nifty!

@bcmills bcmills added this to the Go1.13 milestone Feb 13, 2019
@mvdan
Copy link
Member

mvdan commented Feb 13, 2019

@octomarat please send a single PR for all these changes. Separate commits aren't necessary and just clutter the git history, in particular since most of the changes only touch test code.

@marat-rkh marat-rkh mentioned this issue Feb 14, 2019
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/162657 mentions this issue: go/cmd, crypto/x509, net/textproto, html/template: fix minor issues with nil values

@bcmills bcmills changed the title Minor issues with nil values in the source code all: redundant or missing nil checks Feb 14, 2019
gopherbot pushed a commit that referenced this issue Feb 26, 2019
@marat-rkh @mvdan
net/textproto: prevent test from failing with nil pointer dereference
c6e4706 
The variable err could have nil value when we call err.Error(),
because after we check it for nil above we continue the test
(t.Errorf doesn't stop the test execution).

Updates #30208

Change-Id: Ibcf38698326c69c06068989510311e37806995c6
GitHub-Last-Rev: 3ab20f6
GitHub-Pull-Request: #30214
Reviewed-on: https://go-review.googlesource.com/c/162457
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 26, 2019
@marat-rkh @mvdan
crypto/x509: remove redundant check for nil in tests
576a3c6 
Comparing err variable to be not nil is redundant in this case.
The code above ensures that it is always not nil.

Updates #30208

Change-Id: I0a41601273de36a05d22270a743c0bdedeb1d0bf
GitHub-Last-Rev: 372e0fd
GitHub-Pull-Request: #30213
Reviewed-on: https://go-review.googlesource.com/c/162439
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Mar 2, 2019
@marat-rkh
html/template: prevent test from failing with nil pointer dereference
aef1a7e 
The variable err could have nil value when we call err.Error(),
because after we check it for nil above we continue the test
(t.Errorf doesn't stop the test execution).

Updates #30208

Change-Id: I6f7a8609f2453f622a1fa94a50c99d2e04d5fbcd
GitHub-Last-Rev: 3a5d9b1
GitHub-Pull-Request: #30215
Reviewed-on: https://go-review.googlesource.com/c/162477
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@agnivade
Copy link
Contributor

agnivade commented Mar 7, 2019

@octomarat - I think only the cmd/go/internal fix is remaining. Would you like to push ahead and finish this one out ?

@marat-rkh
Copy link
Author

marat-rkh commented Mar 8, 2019 •
edited

Oh. I haven't noticed that some of the commits have been accepted and merged to the master already. Probably I am missing something, but what should I do to make the last one accepted and merged?

@golang golang locked and limited conversation to collaborators Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@dgryski @agnivade @mvdan @bcmills @marat-rkh @gopherbot