golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.3k stars 17.58k forks source link

proposal: all: have a policy for sweeping updates #67510

Open robpike opened 4 months ago

robpike commented 4 months ago

Proposal Details

See https://go-review.googlesource.com/c/go/+/586616 for example. There's nothing wrong with the change in itself, but there is no guidance about whether it should happen. It's a code rewrite with no semantic effect. It's also a reasonable thing to do, but there are surely many places throughout std that could have a similar change applied. That in turn could lead to many tiny CLs coming in, creating an undue load on maintainers and the trybots to check them.

It's reasonable to suggest that new significant packages, such as "slices", be used in std to show how best to use them. The standard library acts as an exemplar of good Go style.

So I propose two things:

  1. A policy concisely (if imperfectly) defining when a new package, function, or language feature be deployed throughout std.
  2. A decision that when such a possibility arises, it be done wholesale, in one or maybe a handful of big CLs, to get it done quickly and efficiently.
earthboundkid commented 4 months ago

An example of this not happening is cmp.Or, which I added, but I didn't go back and look for all the places where it could be swapped in. (It also wasn't clear if doing that would make sense stylistically, since it's usually slower than a simple if statement anyway.) A lot of times someone will propose something and say "Look we do this N times in the standard library with unexported internal functions" and that's what gives it the evidence it needs to be approved, but actually fixing the N unexported functions is a secondary task that ends up out of scope for the proposer, who just wants to add their thing.

With reflect.TypeFor (which Josh proposed and I implemented), the refactor did actually happen, but it was separate from my initial implementation.

seankhliao commented 4 months ago

consider also #32816 perhaps significant new additions should have corresponding updates in go fix?

aimuz commented 4 months ago

There is also a new feature, iter, which can be rewritten in most places as for range b.N

bjorndm commented 4 months ago

As long as the changes are benchmarked to be of same performance and memory useas before, this seems like a good idea.

aimuz commented 4 months ago

In most cases, there is a performance gain of about 2% and fewer allocation

robpike commented 4 months ago

Please confine your comments to discussion of the proposal itself rather than turning this thread into a list of recent changes, which can be curated just from scanning git log.

ianlancetaylor commented 4 months ago

I agree that point 1 is important. Point 2 seems less important to me. As an experiment inspired by this proposal I tried writing a large CL to convert from sort.Sort to slices.SortFunc where appropriate. The result was https://go.dev/cl/587655, which was somewhat awkward to write and definitely hard to review. Nobody wants to look at a change to 89 files where some of the changes are trivial and some require thought. A purely mechanical change can be large, but I think it's reasonable to break up a more complex one into a series of small targeted CLs.

So to me the important point is a way to decide the direction that we want to go, and a way to avoid backsliding and, especially, changing back and forth. I tentatively suggest a Transitions wiki page that describes the intended direction of the source tree. For example, the GCC project has https://gcc.gnu.org/wiki/Partial_Transitions.

robpike commented 4 months ago

Thanks for doing the experiment @ianlancetaylor. You don't discuss the breakdown but let's say it's 50:50. I'd happily review a CL containing consistent trivial fixes to 45 files, or even 88, followed by however many it takes to get the trickier ones in, which could presumably still be grouped a bit.

The key point is to get it done by an approved process rather than saying, "let's do this" and then not seeing it through efficiently.