reflect: deprecate SliceHeader and StringHeader

[rsc]

reflect.SliceHeader and reflect.StringHeader were marked deprecated in the Go 1.19 cycle and in the Go 1.20 cycle, both times without a proposal discussion. Filing this issue for the proposal discussion.

[gopherbot]

Change https://go.dev/cl/452762 mentions this issue: reflect: deprecate SliceHeader and StringHeader

[gopherbot]

Change https://go.dev/cl/452761 mentions this issue: reflect: remove deprecation notices from SliceHeader, StringHeader

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

It would be very helpful to know how much code this will affect. Despite the dire warnings in the commentary right now, there are compiler special cases (which we can't remove) that make the code correct today. If there is a lot of code like this, flagging it with a deprecation warning is a lot of churn.

[rsc]

Per https://github.com/golang/go/wiki/Deprecated we cannot add deprecated notices until Go 1.22. I'd still like to know more about how much code this will flag.

[rsc]

I downloaded the latest release and prerelease version of each module known to proxy.golang.org (using index.golang.org's list) as of a few weeks ago. Grepping for StringHeader or SliceHeader in that code turns up many many many matches (attached). Since the vast majority of existing uses are correct, I am not sure that it makes sense to mark these types deprecated and trigger warnings in all this code. If we ever decide to change the representation of strings or slices, then it might make sense to go through that pain. Today I am not sure it does.

headers.txt.gz

[rsc]

It still seems like we don't know the full impact, but we do know that much code using these is incorrect, and there are much better alternatives now, so marking them deprecated seems at least worth trying.

Have all concerns about this proposal been addressed?

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[mdempsky]

According to https://github.com/golang/go/wiki/Deprecated, "An API feature should only be marked deprecated if its use is problematic in some way, not simply because something newer exists."

I agree that reflect.{String,Slice}Header are problematic in the sense that they're much easier to misuse than the newer unsafe.{String,Slice}{,Data} APIs, and that misuse can lead to very subtle memory corruption. But I also believe the cmd/vet checks added for #40701 are reasonably precise at catching all misuse patterns that appear in practice. So arguably users that pay attention to warnings should already have correct code bases, and the deprecation warning would serve just as noise/churn for them.

So as much as I dislike the reflect header types, it's not obvious to me that formally marking them deprecated is actually a net-win for end users. Though I'm not against it either.

[bcmills]

@mdempsky, are the cmd/vet checks from #40701 included in the subset run automatically by go test?

If not, perhaps including them would be a good starting point, and we could reassess whether to fully deprecate them after we give that a release or two to soak.

[mdempsky]

@bcmills No, it looks like it's not included in go test by default:

go/src/cmd/go/internal/test/test.go

Line 655 in 88a36c9

// "-unsafeptr",

[randall77]

I think the question is whether whatever tool surfaces warnings about using deprecated API (staticcheck?) would also run the vet check.

[bcmills]

@mdempsky, #41205 suggests that the cmd/vet check might not be precise enough.
(That, or we may have some existing violations to clean up ourselves. 😅)

[rsc]

Re #56906 (comment), you're right that we don't know the impact. I addressed that in #56906 (comment):

It still seems like we don't know the full impact, but we do know that much code using these is incorrect, and there are much better alternatives now, so marking them deprecated seems at least worth trying.

We can roll this back if we need to.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[FiloSottile]

It might be because I didn't follow the unsafe.{String,Slice}{,Data} proposals closely enough, but I feel like there's hidden context in the decision process here that would be worth linking to or spelling out.

#56906 (comment) says

Since the vast majority of existing uses are correct, [...]

and the next comment #56906 (comment) says

we do know that much code using these is incorrect

Those two can just both be true at the same time, but maybe there was a discussion in between?

Also, are we officially amending the https://go.dev/wiki/Deprecated rule

If function F1 is being replaced by function F2 and the first release in which F2 is available is Go 1.N, then an official deprecation notice for F1 should not be added until Go 1.N+2.

to "Go 1.N+1", since we didn't find a better explanation than an off-by-one error for Go 1.N+2?

[ianlancetaylor]

I changed https://go.dev/wiki/Deprecated.

I don't think there was any big discussion in between, I think it's more what you say: it does seem that most current uses are correct, but there are also many (but a minority) of incorrect uses.

[gopherbot]

Change https://go.dev/cl/498757 mentions this issue: doc/go1.21: mention changes to the reflect package