go: reflect: do not deprecate Ptr APIs just yet

Deprecated does not imply the code isn’t valid. If a tool is suggesting that, it’s a problem with the tool and not with the deprecation mark. Deprecated just means it’s no longer the recommended way to do something.

We should remove the // Deprecated here. We don’t want tools to complain about perfectly valid code.

Oh, and CC @dominikh for staticcheck, though I’m still fairly sure that it’s correct here. At least as long as deprecation notices don’t carry Go version information.

FWIW, Staticcheck maintains a handwritten mapping of Go identifiers to 1) when they were deprecated 2) when their recommended alternative became available. As such, Staticcheck won’t flag uses of a deprecated identifier if the user is targeting an older version of Go.

We usually update that mapping close to a Go release (either closely before or closely after…) and make a new release.

I thought this rule was already common knowledge, as https://go-review.googlesource.com/c/go/+/284777/ was rejected for deprecating io/ioutil too early.

Generally speaking, Go has always immediately deprecated things when alternatives became available. io/ioutil is the odd one out here.

io/ioutil is the odd one out here.

Now I’m curious what makes ioutil special so that we should wait a few releases to deprecate it.

Thanks all for the input and solution. Just to double check - are we ending up with clear guidelines on how and when standard library APIs should be deprecated? I would hope so, and that they would be documented somewhere, so we don’t end up here again next cycle 😃

Fair enough, but we don’t want people to start replacing Ptr by Pointer until Pointer is available in all supported versions. So I guess the correct approach is to only mark an identifier as deprecated if the replacement is available in all supported versions of Go.

If an identifier is deprecated because it is unsafe or dangerous, then we want that deprecation warning to be issued in all cases.

Staticcheck actually does do that for some deprecated objects, such as insecure cryptographic functions. These will get flagged regardless of the version in which they were deprecated. These are the objects marked with DeprecatedNeverUse in https://github.com/dominikh/go-tools/blob/8b9edfdb7a54d16418dda13a02fe1864c34624c4/knowledge/deprecated.go.

This is because writing "go 1.N’ in a go.mod file does not mean that only go 1.N is supported. It means that we should use Go 1.N semantics, but in general it may still be OK to build with Go 1.N-1.

You’re right that go 1.N doesn’t mean that only >=1.N is supported. That’s especially true because the version specified has to be at least as high as the highest version required by any package or (build-tagged) file in the module. But I don’t think that assuming 1.N-1 improves on that. Defaulting to the version specified in go.mod is a guess, but it’s an easy to understand one. I’d rather not add an offset to the value.

Probably related: #30791.

If an identifier is deprecated because we have a newer, cooler, replacement, and the replacement is first available in Go 1.N, then it seems to me that the warning should be issued if compiling for Go 1.N+2. This is because when we are targeting Go 1.N+2, then Go 1.N is no longer supported and it’s not so important to ensure that things still work for people using it.

In Staticcheck, we make this the user’s responsibility. Instead of us subtracting 2 from the determined version, we expect the user to either use the desired version in their go.mod – which works for most simpler modules that target a single minimum version across all files – or to use the -go flag to overwrite the targeted version.

Separately, perhaps if staticcheck can’t get the Go version from a go.mod file, it should use 1.16 (or perhaps 1.11), as that is what cmd/go does

This will probably become much less of a concern once Go drops support for GOPATH, as all packages will have a go.mod file, and we get the version in that from go list (which means we also get its fallbacks.)

Thinking out loud.

If an identifier is deprecated because it is unsafe or dangerous, then we want that deprecation warning to be issued in all cases.

If an identifier is deprecated because we have a newer, cooler, replacement, and the replacement is first available in Go 1.N, then it seems to me that the warning should be issued if compiling for Go 1.N+2. This is because when we are targeting Go 1.N+2, then Go 1.N is no longer supported and it’s not so important to ensure that things still work for people using it.

This is because writing "go 1.N’ in a go.mod file does not mean that only go 1.N is supported. It means that we should use Go 1.N semantics, but in general it may still be OK to build with Go 1.N-1.

I’m not sure how to make that work with staticcheck.

Separately, perhaps if staticcheck can’t get the Go version from a go.mod file, it should use 1.16 (or perhaps 1.11), as that is what cmd/go does (https://go.googlesource.com/go/+/refs/heads/master/src/cmd/go/internal/modload/modfile.go#84).

Deprecated does not imply the code isn’t valid. If a tool is suggesting that, it’s a problem with the tool and not with the deprecation mark.

Here is how Staticcheck handles deprecation, based on an example:

package pkg

import (
	"io"
	"os"
)

func Fn(r io.ReadSeeker) {
	r.Seek(0, os.SEEK_SET)
}

$ staticcheck -go 1.5 # os.SEEK_SET was first deprecated in Go 1.7, so using it in 1.5 is fine
$ staticcheck -go 1.7 # targeting Go 1.7, we should respect the deprecation
sand.go:9:12: os.SEEK_SET has been deprecated since Go 1.7: Use io.SeekStart, io.SeekCurrent, and io.SeekEnd.  (SA1019)

Feel free to tell me if you disagree with that behavior.