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

net/http: some dead code and warnings #19850

Closed
rgeronimi opened this issue Apr 5, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@rgeronimi
Copy link

commented Apr 5, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/raphaelgeronimi/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/yv/6zwrsm6j6kqgj6vr3hf0_zbw0000gn/T/go-build128646901=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I looked at the code of MaxBytesReader (src/net/http/request.go:1013).

What did you expect to see?

No dead code.

What did you see instead?

The method maxBytesReader.tooLarge seems unused. It is confirmed by a grep on the go1.8.src.tar: no piece of code calls this method (see below)

go $ grep -Rn "tooLarge" .
./src/cmd/compile/internal/gc/esc.go:685: e.escassignSinkWhy(n, n, "too large for stack") // TODO category: tooLarge
./src/fmt/print.go:281:// tooLarge reports whether the magnitude of the integer is
./src/fmt/print.go:283:func tooLarge(x int) bool {
./src/fmt/print.go:294: if tooLarge(num) {
./src/fmt/print.go:885: if tooLarge(num) {
./src/net/http/request.go:1024:func (l *maxBytesReader) tooLarge() (n int, err error) {

The forest behind the tree: such dead code shall not exist (=be tolerated on an automatic basis) in such a large and important codebase. Tools exist now. A linter which catches it immediately is https://github.com/dominikh/go-tools/tree/master/cmd/unused

$ unused net/http
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:1024:26: func (*maxBytesReader).tooLarge is unused (U1000)

So either a tool such as the one above shall be added to the build chain, or if it is already the case the build chain or acceptance procedure shall be strengthened to not tolerate such defects. On that topic, running the go linter (golint) on net/http doesn't catch the dead code, but returns many other - typically linter - items:

/usr/local/Cellar/go/1.8/libexec/src/net/http/client.go:216:39: error strings should not be capitalized or end with punctuation or a newline
/usr/local/Cellar/go/1.8/libexec/src/net/http/cookie.go:34:2: struct field HttpOnly should be HTTPOnly
/usr/local/Cellar/go/1.8/libexec/src/net/http/export_test.go:28:2: var ExportHttp2ConfigureServer should be ExportHTTP2ConfigureServer
/usr/local/Cellar/go/1.8/libexec/src/net/http/export_test.go:110:21: don't use underscores in Go names; method IdleConnStrsForTesting_h2 should be IdleConnStrsForTestingH2
/usr/local/Cellar/go/1.8/libexec/src/net/http/export_test.go:180:6: func ExportHttp2ConfigureTransport should be ExportHTTP2ConfigureTransport
/usr/local/Cellar/go/1.8/libexec/src/net/http/export_test.go:189:5: don't use underscores in Go names; var Export_shouldCopyHeaderOnRedirect should be ExportShouldCopyHeaderOnRedirect
/usr/local/Cellar/go/1.8/libexec/src/net/http/fs.go:36:1: exported method Dir.Open should have comment or be unexported
/usr/local/Cellar/go/1.8/libexec/src/net/http/fs.go:429:10: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/fs.go:493:10: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:468:2: error var http2errMixPseudoHeaderTypes should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:469:2: error var http2errPseudoAfterRegular should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:480:2: error var http2errReadEmpty should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:481:2: error var http2errWriteFull should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:873:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:887:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:908:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:925:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:927:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:929:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:931:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:985:5: error var http2ErrFrameTooLarge should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1132:2: error var http2errStreamID should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1133:2: error var http2errDepStreamID should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1134:2: error var http2errPadLength should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1150:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1162:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1255:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1268:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1296:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1339:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1397:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1504:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1592:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1630:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1666:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1756:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1782:1: receiver name f should be consistent with previous receiver name fr for http2Framer
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:1978:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2565:2: error var http2errInvalidHeaderFieldName should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2566:2: error var http2errInvalidHeaderFieldValue should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2832:5: error var http2errClosedPipeWrite should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2930:2: error var http2errClientDisconnected should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2931:2: error var http2errClosedBody should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2932:2: error var http2errHandlerComplete should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:2933:2: error var http2errStreamClosed should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:3034:23: error strings should not be capitalized or end with punctuation or a newline
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:3770:5: error var http2errHandlerPanicked should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:4510:6: don't use underscores in Go names; var url_ should be url
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5084:9: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5099:2: error var http2ErrRecursivePush should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5100:2: error var http2ErrPushLimitReached should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5471:5: error var http2errTransportVersion should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5641:5: error var http2ErrNoCachedConn should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5715:2: error var http2errClientConnClosed should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5716:2: error var http2errClientConnUnusable should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5718:2: error var http2errClientConnGotGoAway should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5719:2: error var http2errClientConnGotGoAwayAfterSomeReqBody should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:5992:5: error var http2errRequestCanceled should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:6239:2: error var http2errStopReqBodyWrite should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:6242:2: error var http2errStopReqBodyWriteAndCancel should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:6391:12: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:6917:5: error var http2errClosedResponseBody should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7013:5: error var http2errInvalidTrailers should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7225:2: error var http2errResponseHeaderListSize should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7226:2: error var http2errPseudoTrailers should have name of the form errFoo
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7247:17: should omit type io.ReadCloser from declaration of var http2noBody; it will be inferred from the right-hand side
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7486:1: receiver name se should be consistent with previous receiver name e for http2StreamError
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7490:1: receiver name se should be consistent with previous receiver name e for http2StreamError
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7599:9: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/h2_bundle.go:7648:9: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/readrequest_test.go:29:24: should drop = nil from declaration of var noTrailer; it is the zero value
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:70:2: comment on exported var ErrHeaderTooLong should be of the form "ErrHeaderTooLong ..."
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:72:2: comment on exported var ErrShortBody should be of the form "ErrShortBody ..."
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:74:2: comment on exported var ErrMissingContentLength should be of the form "ErrMissingContentLength ..."
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:497:1: receiver name req should be consistent with previous receiver name r for Request
/usr/local/Cellar/go/1.8/libexec/src/net/http/request.go:1282:19: method wantsHttp10KeepAlive should be wantsHTTP10KeepAlive
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:1526:9: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2384:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2390:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2397:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2475:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2491:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2612:5: exported var ErrServerClosed should have comment or be unexported
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2672:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2690:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2703:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2710:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2717:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2721:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2749:1: receiver name s should be consistent with previous receiver name srv for Server
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2876:20: don't use underscores in Go names; method setupHTTP2_ListenAndServeTLS should be setupHTTP2ListenAndServeTLS
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2889:20: don't use underscores in Go names; method setupHTTP2_Serve should be setupHTTP2Serve
/usr/local/Cellar/go/1.8/libexec/src/net/http/server.go:2894:20: don't use underscores in Go names; method onceSetNextProtoDefaults_Serve should be onceSetNextProtoDefaultsServe
/usr/local/Cellar/go/1.8/libexec/src/net/http/transfer.go:452:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
/usr/local/Cellar/go/1.8/libexec/src/net/http/transfer.go:648:9: if block ends with a return statement, so drop this else and outdent its block
/usr/local/Cellar/go/1.8/libexec/src/net/http/transport.go:1158:2: don't use underscores in Go names; var no_proxy should be noProxy

@bradfitz bradfitz changed the title Dead code and linter warnings in net/http net/http: some dead code and warnings Apr 5, 2017

@bradfitz bradfitz added this to the Go1.9Maybe milestone Apr 5, 2017

@mrajashree

This comment has been minimized.

Copy link

commented Apr 5, 2017

@bradfitz can I work on this?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

All yours!

@bradfitz bradfitz self-assigned this Jun 7, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jun 7, 2017

CL https://golang.org/cl/45077 mentions this issue.

@gopherbot gopherbot closed this in 755fd93 Jun 7, 2017

@golang golang locked and limited conversation to collaborators Jun 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.