reflect: do not deprecate Ptr APIs just yet

[mvdan]

In basically all of my modules, I support the two latest major Go versions - right now, this means 1.16 and 1.17.
I develop on Go master, for the sake of helping with testing and developing new features.

In one of the codebases that must still support Go 1.16, I got the following warning from staticcheck:

$ staticcheck ./...
cmd/shfmt/json.go:27:7: reflect.Ptr is deprecated: use the new spelling, Pointer.  (SA1019)
syntax/walk.go:269:7: reflect.Ptr is deprecated: use the new spelling, Pointer.  (SA1019)

Staticcheck is right: the type is marked as deprecated.
Unfortunately for me, this is a warning that will be bothering me for six to twelve months before I can fix it.
reflect.Pointer won't appear until Go 1.18, and I won't drop support for 1.17 until 1.19 comes out.

I think the standard library needs to at least wait one major release before formally deprecating APIs.
That is, the earliest we could deprecate reflect.Ptr would be in Go 1.19, when the majority of module authors can stop worrying about Go 1.17 and older.

Alternatively, to be extra conservative towards module authors who move slowly, we could make the general rule to wait two major releases.

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.
We seem to have dealt with the reflect API changes differently, though.
Perhaps this means we should write down this general standard library deprecation rule?

Marking as NeedsDecision for 1.18, as making a decision after 1.18 would be a bit late :)

cc @ianlancetaylor @bradfitz @rsc

[mvdan]

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.

[bcmills]

At least as long as deprecation notices don't carry Go version information.

See previously #38149.

[mvdan]

I'm going to mark this as a release blocker, because some time has passed and I fear it might go under the radar given all that's happening for 1.18. We definitely need to make a choice before 1.18 comes out, and ideally before the first beta is out as well.

[bradfitz]

I didn't realize tools would be so aggressive about this.

I'm fine tweaking the docs to a pattern recognizable to humans but not to tools for a few releases.

Want to send a change?

[randall77]

We'll need the same for CL 350691 which deprecated reflect.Value.Pointer.

[dominikh]

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.

[dominikh]

(as an aside, the reflect documentation still mentions Ptr and PtrTo in various places, such as in the documentation of New and UnsafePointer; someone should fix that)

[gopherbot]

Change https://golang.org/cl/358454 mentions this issue: all: use reflect.{Ptr,PtrTo}

[mvdan]

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

I see, I didn't know that. Would there be a way to not issue warnings for deprecations that haven't been released yet?

Generally speaking, Go has always immediately deprecated things when alternatives became available.

If that is our policy in a consistent way, then that's fine by me - as long as tools follow it as well. I shouldn't be getting warnings to fix my code before I am able to do so, even if I happen to be developing on tip.

[mvdan]

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.

[dominikh]

I see, I didn't know that. Would there be a way to not issue warnings for deprecations that haven't been released yet?

A way for users right now? No. A way for me to change Staticcheck to behave that way? Trivially.

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

Hm, seeing these messages, I'm not sure reflect.Ptr should be deprecated, either.

• https://go-review.googlesource.com/c/go/+/284777/1#message-f4c5090b3a565d8e28abfa2ab036ed29e94235da
• https://go-review.googlesource.com/c/go/+/284777/1#message-241c85847541f38f2ae44e61ef84eac2487508f5

/cc @rsc @ianlancetaylor

[rsc]

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

[cuonglm]

@rsc to be clear, and also don't deprecate Value.Pointer?

[ianlancetaylor]

@cuonglm That is a separate issue, but it does seem reasonable to me.

[gopherbot]

Change https://golang.org/cl/359175 mentions this issue: reflect: undeprecate Ptr, PtrTo

[randall77]

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.

[dominikh]

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.

[randall77]

I kind of agree, but with the following caveat. The version you pass to staticcheck should be the minimum Go version you support, not the Go version you're currently compiling with.
@mvdan's complaint was that when compiling with master he got the deprecation warning, but he does support 1.16, and there's no way to square that circle (the api that you're supposed to move to in 1.18 doesn't exist in 1.16).
So it seems to me there needs to be a way to specify the minimum supported version. Maybe there's a way to configure that? Maybe by default staticcheck can assume the standard Go support period? (Don't complain about deprecations until the deprecation note appears in both the version you're compiling with and version n-1.)

[randall77]

Or maybe @mvdan should just be using -go 1.16 explicitly.

[dominikh]

The version you pass to staticcheck should be the minimum Go version you support, not the Go version you're currently compiling with.

Right, if you use the -go flag, it denotes the minimum version you want to support. If you don't pass the flag, we look at the go directive in go.mod. If you're not using modules, we have to default to something, which is the version of Go that Staticcheck was compiled with.

@mvdan's complaint was that when compiling with master he got the deprecation warning, but he does support 1.16, and there's no way to square that circle (the api that you're supposed to move to in 1.18 doesn't exist in 1.16).

The problem here is that Staticcheck refers to a list of deprecated objects in the standard library, and if it can't find an entry, falls back to always reporting the deprecation. That means that between Go deprecating something, and us updating the list, Staticcheck will complain about it, even if you were to target Go 1.0. Or, in other words: if we don't know that we shouldn't report it, we'll report it.

It seems pretty obvious that this behavior is bad, and we should instead not report it.

However, that doesn't answer the following question: should reflect.Ptr get flagged as deprecated when targeting Go 1.18 as the minimum version? There doesn't seem to be consensus on that matter. I believe that traditionally, we've deprecated things the moment an alternative became available. We have broken with that tradition with io/ioutil, and Russ and Ian seem to advocate for not deprecating things that aren't harmful to use. I am, however, worried that Staticcheck being too noisy about deprecations in the past has affected their opinion.

[ianlancetaylor]

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).

[dominikh]

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.)

[ianlancetaylor]

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.

[mvdan]

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 :)

[ianlancetaylor]

I think the guidelines wind up being: if function F1 is being replaced by function F2, and the first release in which F2 is available is Go 1.N, then we can add an official deprecation notice in release Go 1.N+2.

[mvdan]

That's fair. Where could we document that for future reference?

[ianlancetaylor]

At https://golang.org/wiki/Deprecated ?

[mvdan]

Good point - I've made an edit. Feel free to tweak it as necessary. I reused your wording with a bit of context.

[ianlancetaylor]

Thanks.