golang / go

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

x/exp/slices: "backport" slices.SortFunc #61374

Closed ghost closed 1 year ago

ghost commented 1 year ago

slices.SortFunc is coming soon:

https://pkg.go.dev/slices#SortFunc

but its not here yet. meanwhile, the signature:

func SortFunc[S ~[]E, E any](x S, cmp func(a, b E) int)

is different from x/exp/slices:

func SortFunc[E any](x []E, less func(a, b E) bool)

https://pkg.go.dev/golang.org/x/exp/slices#SortFunc

namely, the callback returns int instead of bool.

ianlancetaylor commented 1 year ago

We can just do this, it doesn't need to go through proposal review. Taking it out of the proposal process. Thanks.

gopherbot commented 1 year ago

Change https://go.dev/cl/511395 mentions this issue: backport: exp/slices.sortFunc to "slices"

ianlancetaylor commented 1 year ago

To be clear, changing golang.org/x/exp/slices to look like the standard library slices package doesn't require a proposal.

Changing the standard library slices package does require a proposal.

gopherbot commented 1 year ago

Change https://go.dev/cl/511895 mentions this issue: slices: update to current standard library version

gopherbot commented 1 year ago

Change https://go.dev/cl/511660 mentions this issue: sort: add gen_sort_variants support for x/exp/slices

eandre commented 1 year ago

This is causing lots of breakage for us. It results in a need for all your dependencies that rely on x/exp/slices.SortFunc to be upgraded to the new signature, or none of them.

Maybe it's too late to revert this, but it would be a much cleaner migration to keep x/exp/slices.SortFunc with a bool return.

gophun commented 1 year ago

All packages under "exp" is are experimental packages. You say "us" as if you have used this in production.

eandre commented 1 year ago

The golang.org/x/exp/slices package in particular is what we, and a massive amount of other popular packages, used. See https://pkg.go.dev/golang.org/x/exp/slices?tab=importedby.

Importers include:

I don't see how the amount of breakage justifies the change, regardless of whether or not x/exp is technically experimental.

gophun commented 1 year ago

This list paints an unfavourable picture of the industry. Incorporating dependencies without vetting and lacking comprehension of their maintenance model appears to me like picking up arbitrary food from the street and eating it.

The Go project needs a space for exploring APIs and sharing them with other enthusiastic experimenters. That place is x/exp, and it comes with a warning:

Warning: Packages here are experimental and unreliable. Some may one day be promoted to the main repository or other subrepository, or they may be modified arbitrarily or even disappear altogether. -- https://github.com/golang/exp

ianlancetaylor commented 1 year ago

I agree with other comments that the API is explicitly experimental, and that coordinating the x/exp package with the finally adopted package is the right approach.

That said, if it helps, we could consider adding some simple adapter functions to the x/exp package. That would let people who expect the old API use the new one with a simple renaming.

eandre commented 1 year ago

this seems to essentially say that the people writing the software don't get to decide whats experimental and whats not. it communicates that the end users get to make that decision. of course that's not the case.

I agree there is value in having a place for experimenting without committing to backwards compatibility.

That said, this package and a few others (x/exp/maps, x/exp/constraints, x/exp/slog, x/exp/errors) have all been proposed for inclusion in the standard library by prominent members of the Go team, and that has created (perhaps unintentionally) more of a perception that adopting them is simply a matter of "backporting" the functionality for use (a) earlier than when they become part of the standard library, and (b) for use in libraries that don't want to force their importers to immediately depend on Go 1.N.

For example, consider the removal of maps.Keys from the standard library maps package in Go 1.21. In #61538 there's many comments to the effect of "if you need it, just use x/exp/maps for another 6 months, it's no big deal". For example this comment by @rsc. I also note that the two policies are seemingly inconsistent: why are we keeping x/exp/maps.Keys around if the goal is to immediately unify the experimental package with what is in the standard library?

For what it's worth, I think the x/exp/errors package makes it vastly more clear that the package is experimental, in a way that was never done for x/exp/{slices,maps}: "This is an EXPERIMENTAL package, and may change in arbitrary ways without notice."

I also noticed that x/exp/errors is a separate module (unlike x/exp/{slices,maps,constraints}) which further reduces the likelihood of ecosystem breakage when an incompatible change is made, since you only need your program's module graph to agree on a consistent version for that package (as a single-package module) as opposed to a whole set of packages (everything else under x/exp) in the case of x/exp/{slices,maps}. Perhaps that's a better approach going forward for proposals for inclusion into the standard library.

I agree with other comments that the API is explicitly experimental, and that coordinating the x/exp package with the finally adopted package is the right approach.

I disagree in this case, at least. It seems to me the packages in question were published with the explicit goal of getting real-world feedback before including them in the standard library. The change to SortFunc and friends to have the sort function return an integer, and the introduction of cmp.Compare, is clearly a better design than what was initially proposed.

But, while I haven't traced the lineage of what caused that change to be proposed, isn't it reasonable to assume such a change came about precisely because of the widespread adoption of x/exp/slices, with thousands of real-world users using the proposed APIs? That seems like something we should encourage and celebrate, not discourage.

Finally, I'm not really sure what we win by unifying the APIs? In this particular case it seems to cause widespread ecosystem breakage for very little benefit. It's natural to make large, incompatible changes in the early stages of any design. But in this case a backwards incompatible change was made to a package with thousands of importers, for a proposed design by the Go team for inclusion in the standard library, that had existed for almost two years in stable form.

Under other circumstances (if there were no "experimental" label on the x/exp module) the argument would be: "make the backwards incompatible change by publishing a new major version under a different import path". But that's exactly what the standard-library slices package is. Why go to extra lengths to break users of the "previous major version" living in x/exp/slices, in the name of unifying what in effect is two different major versions of the same package?

Edit: I should clarify that I don't believe reverting the change would help but rather just contribute more confusion. I don't think adapter functions are worth adding: if you need to change your code anyways you might as well update to the new signature. The problem is for large programs to get all their dependencies to agree on a single version of x/exp and adapter functions wouldn't really help with that. I'm just arguing for a better (different?) process for future changes.

gophun commented 1 year ago

I'm just arguing for a better (different?) process for future changes.

The next experimental package will be x/exp/xiter: #61898

Maybe linters like staticcheck or golangci-lint should flag x/exp dependencies. Large projects often use these linters.

eandre commented 1 year ago

I'm just arguing for a better (different?) process for future changes.

The next experimental package will be x/exp/xiter: #61898

Maybe linters like staticcheck or golangci-lint should flag x/exp dependencies. Large projects often use these linters.

Yes, perhaps. But as I noted, I think perhaps the widespread adoption of x/exp/slices contributed to the improved design of slices.SortFunc in the first place, and having linters complain would discourage that.

It seems to me we could just have taken the lessons learned and applied them to the standard library slices and left x/exp/slices with the old signature. I agree there does need to be ways of experimenting, but all the evidence points to x/exp/slices not really being treated as experimental by anyone other than as a technicality by virtue of the module it's in.

ghost commented 1 year ago

this package and a few others (x/exp/maps, x/exp/constraints, x/exp/slog, x/exp/errors) have all been proposed for inclusion in the standard library by prominent members of the Go team, and that has created (perhaps unintentionally) more of a perception that adopting them is simply a matter of "backporting" the functionality for use (a) earlier than when they become part of the standard library, and (b) for use in libraries that don't want to force their importers to immediately depend on Go 1.N.

Its unfortunate that some people had this incorrect perception. The package description makes it painfully clear thats not the case:

This subrepository holds experimental and deprecated (in the old directory) packages.

Warning: Packages here are experimental and unreliable. Some may one day be promoted to the main repository or other subrepository, or they may be modified arbitrarily or even disappear altogether.

In short, code in this subrepository is not subject to the Go 1 compatibility promise. (No subrepo is, but the promise is even more likely to be violated by go.exp than the others.)

Caveat emptor.

https://github.com/golang/exp

I honestly dont know how they could make it more clear. I would like to go back to this as well:

I don't see how the amount of breakage justifies the change, regardless of whether or not x/exp is technically experimental.

Thats the thing, once they dump something into x/exp, they dont have to justify any change. thats the whole point. Caveat emptor is Latin for "Let the buyer beware". It seems some people are of the mind that they can adopt a package, and then expect that package has the Go 1 promise, even though it explicitly says it does not offer that promise.

For example, consider the removal of maps.Keys from the standard library maps package in Go 1.21. In #61538 there's many comments to the effect of "if you need it, just use x/exp/maps for another 6 months, it's no big deal". For example this comment by @rsc. I also note that the two policies are seemingly inconsistent: why are we keeping x/exp/maps.Keys around if the goal is to immediately unify the experimental package with what is in the standard library?

strawman. this issue is about slices.SortFunc, which is in both currently, so thats not the same situation as maps.Keys. once we get to the point where maps.Keys exists in the standard library, yes I would expect both signatures to match at that point as well.

For what it's worth, I think the x/exp/errors package makes it vastly more clear that the package is experimental, in a way that was never done for x/exp/{slices,maps}: "This is an EXPERIMENTAL package, and may change in arbitrary ways without notice."

the text I quoted above from the README, to me is vividly clear. if others dont agree, then perhaps we can work on improving the language, but I dont think that gives an inroad to "language lawyer" a path into reverting this change.

I disagree in this case, at least. It seems to me the packages in question were published with the explicit goal of getting real-world feedback before including them in the standard library. The change to SortFunc and friends to have the sort function return an integer, and the introduction of cmp.Compare, is clearly a better design than what was initially proposed.

I would encourage users with this viewpoint to consider other viewpoints. from this viewpoint, people are wanting consistency on a package that is marked as experimental. however another viewpoint is my own. my viewpoint is that now that the slices package HAS made it into the standard library, I want to use it NOW. but until Go 1.21, I couldn't easily do that, I would need to vendor the code. So since we already have x/exp/slices, and the API is NOT locked like normal packages, to me it makes sense for functions that exist in both to have the same signature. but I knew before slices made it into the standard library, that the signature could change at any time or the function could disappear entirely. it was only once the package was merged that I had confidence on the API. I would encourage others to adopt this viewpoint. once an experimental package is merged into standard library, THEN the API can be relied on.

But, while I haven't traced the lineage of what caused that change to be proposed, isn't it reasonable to assume such a change came about precisely because of the widespread adoption of x/exp/slices, with thousands of real-world users using the proposed APIs? That seems like something we should encourage and celebrate, not discourage.

another strawman. we can encourage people to use a package, while at the same time make it clear that a package is experimental. thats exactly what was done here. the package was proposed:

https://github.com/golang/go/issues/45955

then it was put into the x/exp, which exempts the code from any promise of API stability.

Finally, I'm not really sure what we win by unifying the APIs? In this particular case it seems to cause widespread ecosystem breakage for very little benefit. It's natural to make large, incompatible changes in the early stages of any design. But in this case a backwards incompatible change was made to a package with thousands of importers, for a proposed design by the Go team for inclusion in the standard library, that had existed for almost two years in stable form.

I cant speak for the ecosystem, but only myself. my plan was to move to slices proper once Go 1.21 released. after the slices merge, but BEFORE Go 1.21, I was in limbo because the two function signatures were different. to me during that time period it makes sense for them to be the same signature, that way once Go 1.21 is released, all I have to do is rename the imports. from my view this suggestion has no drawback, because x/exp APIs are marked as experimental, so nothing is stopping the Go team from making the change I am wanting.

Under other circumstances (if there were no "experimental" label on the x/exp module) the argument would be: "make the backwards incompatible change by publishing a new major version under a different import path". But that's exactly what the standard-library slices package is. Why go to extra lengths to break users of the "previous major version" living in x/exp/slices, in the name of unifying what in effect is two different major versions of the same package?

see my previous comment.

Edit: I should clarify that I don't believe reverting the change would help but rather just contribute more confusion. I don't think adapter functions are worth adding: if you need to change your code anyways you might as well update to the new signature. The problem is for large programs to get all their dependencies to agree on a single version of x/exp and adapter functions wouldn't really help with that. I'm just arguing for a better (different?) process for future changes.

personally I am quite pleased with the result here. the Go team was quick to harmonize the two APIs once slices was merged, which made my transition once Go 1.21 dropped quite smooth.

It seems to me we could just have taken the lessons learned and applied them to the standard library slices and left x/exp/slices with the old signature. I agree there does need to be ways of experimenting, but all the evidence points to x/exp/slices not really being treated as experimental by anyone other than as a technicality by virtue of the module it's in.

I would say again, the motivation for this is once slices was merged, users want to use it. in that case, the simplest way to make that happen is to harmonize the two APIs. we dont have to concern ourselves with any fallout of changes to x/exp, because we dont offer any promises to that code.

I would say in closing, it just seems that some people need to respect the experimental nature of the code in x/exp, and maybe just dont even use it going forward. I think that would yield better results than using it, then expecting it to have the same code promise as standard library code.

eandre commented 1 year ago

Your argument seems to boil down to "the repository is marked as experimental, so all these people made a mistake". I think the reality is much more nuanced than that.

If it were a matter of "this is in the beginning stages of designing the x/exp/slices package and we expect to be making breaking changes on an ongoing basis" then I'd agree, but that's not how either the Go team or the package's importers have treated the package (judging from the comments on the maps.Keys removal issue). It was stable. People were filing proposals to make additions to it.

I am merely pointing out real downsides and costs to the action taken in this issue. "It was clearly marked as experimental" does not reduce those costs and downsides, but only serves to disparage users who used the package.

But perhaps more importantly, as far as I can tell you (@1268) haven't really provided a good argument for why making this change is beneficial, only that it is permissible under the stated (lack of) compatibility promises. And even less so have you made a case for why the benefits outweigh the very real costs.

personally I am quite pleased with the result here. the Go team was quick to harmonize the two APIs once slices was merged, which made my transition once Go 1.21 dropped quite smooth.

I don't see how the actions taken in this issue made for a smoother migration than the alternative (not making this change).

Say you're an author of a large program, with, say 100 module dependencies. Some of those dependencies will use x/exp/slices.SortFunc. Until two weeks ago when the signature change landed, all of those dependencies would have the old signature (returning a bool). Now, minimum version selection (MVS) means that if any one of your dependencies upgrades their x/exp dependency to the latest commit, every other dependency breaks. I fail to see how this is a smooth migration outcome.

mvdan commented 1 year ago

If we can't experiment in the x/exp repo, where can we?

eandre commented 1 year ago

If we can't experiment in the x/exp repo, where can we?

It's not binary. Just because x/exp is designed for experimentation doesn't mean there aren't very real costs to making a backwards incompatible change to a package imported by thousands of public packages and untold private packages.

If it were early in the design phase of x/exp/slices, and we came up with a better prototype design, that would be another matter. There would still be costs associated with such breakage, but there would be real benefits: getting additional feedback on the new design. But in this case the new design already shipped in the standard library, so there's much less benefit to changing x/exp/slices.

Mission accomplished, and the design that shipped in the standard library is better for it. It's probably better because x/exp/slices saw tons of use. But now that we have a better design, thanks to all those users, do we then need to go and break those same users?

It's all a trade-off, but the costs don't go away because there's an "experimental" sticker on the box. You may disagree with me and think that the benefits of making the change outweigh the cost, but I have yet to hear a compelling argument for why it's so important that the x/exp/slices be identical to slices.

ghost commented 1 year ago

It's not binary. Just because x/exp is designed for experimentation doesn't mean there aren't very real costs to making a backwards incompatible change to a package imported by thousands of public packages and untold private packages.

I think my issue with this is that it suggests that the costs of experimentation should be borne by the developers, rather than the end users. to me, honestly I am just grateful the code even exists publicly. if I was a developer and received the blowback that this issue is getting, I might just decide to dump similar code into internal or even keep it private in the future. so hopefully people can see how placing these demands on the stability of experimental code can be taxing to the developer.

It's all a trade-off, but the costs don't go away because there's an "experimental" sticker on the box. You may disagree with me and think that the benefits of making the change outweigh the cost, but I have yet to hear a compelling argument for why it's so important that the x/exp/slices be identical to slices.

another strawman. my issue was never about making the two packages identical, it was about making two functions have the same signature. the scope of that doesn't even bleed into the function body itself, let alone the rest of the package.

eandre commented 1 year ago

I think my issue with this is that it suggests that the costs of experimentation should be borne by the developers, rather than the end users. to me, honestly I am just grateful the code even exists publicly. if I was a developer and received the blowback that this issue is getting, I might just decide to dump similar code into internal or even keep it private in the future. so hopefully people can see how placing these demands on the stability of experimental code can be taxing to the developer.

It sounds like you're trying to make this about a general principle, but my whole point is that context matters. Making the change to the signature here was not done in the name of "experimentation" but in the name of unifying the two signatures. The experimentation was completed: the revised design was shipped in the standard library as the outcome.

another strawman. my issue was never about making the two packages identical, it was about making two functions have the same signature. the scope of that doesn't even bleed into the function body itself, let alone the rest of the package.

I'm explaining the very real costs associated with that decision and I'm arguing why I believe the benefits weren't worth the costs in this particular situation. Can you elaborate on why, in this particular situation, you believe the benefits of unifying the two function signatures to outweigh the costs?

ghost commented 1 year ago

I'm explaining the very real costs associated with that decision and I'm arguing why I believe the benefits weren't worth the costs in this particular situation. Can you elaborate on why, in this particular situation, you believe the benefits of unifying the two function signatures to outweigh the costs?

when the cost is zero, any benefit will outweigh it. in this situation, since the software offers no guarantee or promise, no cost is set upon the developers to make this change, beyond the time needed to actually commit the code change. the benefit is that during the time period after slices merge, but before Go 1.21, it allowed users to set their code in a way that once Go 1.21 hits, its just a matter of changing the import.

in regards to cost of the end user, I don't really care about that. I had to update my own code to the new signature, but I always knew it was a possibility when opting into experimental code. I hope other users can come to understand that when they use experimental code, they do so at their own risk. I will support any method to better communicate that fact, so that we can avoid any confusion in the future.

eandre commented 1 year ago

If that's your cost/benefit model I rest my case.

ianlancetaylor commented 1 year ago

The reason to backport the change to x/exp/slices is so that people still using older versions of Go, which do not have the slices package, can choose to update to the current x/exp/slices and can then seamlessly change to the standard library slices package when they update to Go 1.21.

Yes, we can do things in other ways, and there are different costs and benefits to the different decisions, but the choice of updating x/exp/slices still seems like a reasonable choice to me. Even while understanding that it is not cost-free. Because none of the options are cost-free.

eandre commented 1 year ago

The reason to backport the change to x/exp/slices is so that people still using older versions of Go, which do not have the slices package, can choose to update to the current x/exp/slices and can then seamlessly change to the standard library slices package when they update to Go 1.21.

But the total work is the same: either they do the work of converting their signature now while still using x/exp/slices, or they do it when they eventually migrate to the standard library slices. There is no efficiency gain in front-loading the work of changing the signature. But compared to the alternative it does cause extensive breakage. Neither option may be cost-free but it seems to be orders of magnitude different amounts.

Anyway, I've made my case and if you still think this is the appropriate trade-off I don't have much more to add.

ghost commented 1 year ago

But the total work is the same: either they do the work of converting their signature now while still using x/exp/slices, or they do it when they eventually migrate to the standard library slices. There is no efficiency gain in front-loading the work of changing the signature. But compared to the alternative it does cause extensive breakage. Neither option may be cost-free but it seems to be orders of magnitude different amounts.

it seems some people in their zeal are hyper focused on the cost/benefit specifically to slices, and are not considering other cost/benefits. as Ian alluded to, the cost of updating to a new Go version, while much less than something like C, is not zero, and might be significant especially if a user is still pre generics. So a holistic view should be taken of costs involved.

eandre commented 1 year ago

it seems some people in their zeal are hyper focused on the cost/benefit specifically to slices, and are not considering other cost/benefits. as Ian alluded to, the cost of updating to a new Go version, while much less than something like C, is not zero, and might be significant especially if a user is still pre generics. So a holistic view should be taken of costs involved.

No need to be disparaging; there is no zeal here. I will note that <upgrading to Go 1.21> and <replacing imports of x/exp/slices with slices> are entirely decoupled. Whether or not x/exp/slices.SortFunc shares the same signature as slices.SortFunc has no effect on the ease of upgrading to Go 1.21.

ghost commented 1 year ago

Whether or not x/exp/slices.SortFunc shares the same signature as slices.SortFunc has no effect on the ease of upgrading to Go 1.21.

the above comment is not considering the use case I presented originally, and have repeated several times. if a user wanted to use "master" slices before the Go 1.21 release, their only option was to vendor the code. however since the issue on this page was addressed, that is not longer the case. people who were pre Go 1.21 can now use x/exp/slices and have the same experiences as master slices. then once Go 1.21 was released, those users could seamlessly update by simply changing the import. further, as Ian mentioned even now that Go 1.21 is released, having both with the same signature is STILL useful, as people can STILL choose to stay on Go 1.20 or older, then when THEY are ready then can upgrade to Go 1.21 and simply have to swap the import path.

No need to be disparaging; there is no zeal here.

putting the word "zeal" into the category of insults seems to be a stretch by any use of the word. I have tried very hard to be civil here, avoiding use of "you" based language and insults in general.

I will note that <upgrading to Go 1.21> and <replacing imports of x/exp/slices with slices> are entirely decoupled.

they aren't, see previous comment. most people that choose to upgrade to Go 1.21, are likely going to immediately or soon after swap imports. in fact in my case slices is the primary reason I upgraded Go 1.21. so while one doesn't require they other, yes that are inextricably linked.

eandre commented 1 year ago

they aren't, see previous comment. most people that choose to upgrade to Go 1.21, are likely going to immediately are soon after swap imports. in fact in my case slices is the primary reason I upgraded Go 1.21. so while one doesn't require they other, yes that are inextricably linked.

Please explain. Surely the entirely voluntary decision to change from x/exp/slices to slices can't be considered to be part of the cost of upgrading.

ianlancetaylor commented 1 year ago

Everybody with existing code is going to want to change one way or another.

People who do not have existing code, and are using 1.20, will have to use x/exp/slices. For them, it is convenient if x/exp/slices provides the same API as slices, so that when they update to 1.21 or later, they can easily change to use slices.

eandre commented 1 year ago

People who do not have existing code, and are using 1.20, will have to use x/exp/slices. For them, it is convenient if x/exp/slices provides the same API as slices, so that when they update to 1.21 or later, they can easily change to use slices.

That's true. But given how quickly the Go community upgrades to new releases (largely thanks to the thoughtfulness around avoiding breaking changes), prioritizing addressing a minor inconvenience (making the change from x/exp/slices to slices slightly less work) for not-yet-written code for a small segment of people, over a very real pain for provably-thousands of packages, still feels like a strange decision to me.

Anyway, I think we've exhausted this discussion at this point.

ghost commented 1 year ago

But given how quickly the Go community upgrades to new releases (largely thanks to the thoughtfulness around avoiding breaking changes), prioritizing addressing a minor inconvenience (making the change from x/exp/slices to slices slightly less work) for not-yet-written code for a small segment of people, over a very real pain for provably-thousands of packages, still feels like a strange decision to me.

the real issue here is precedent.

honestly the concerns presented are valid. however if we were to rollback this change, or have rejected it in the first place, it sets precedent, or depending on the situation adds to existing precedent. thats the insidious nature of a request like this. on the surface its presented as reasonable, and if one takes it only in the context of this issue, it is. but as soon as one starts making ANY concessions regarding experimental code in the interest of stability, people are going to link right to this issue and say "LOOK IT WAS DONE BEFORE SEE? WHY CANT IT BE DONE NOW?" months and years in the future from now.

so thats why, as petty or unreasonable or whatever adjective one wants to apply to this action, that its important that we make NO concessions to serve the stability of experimental packages. any requests for stability of experimental code, must fall ineffectually like waves against a rock. otherwise the expectation becomes that experimental code offers the same promise as standard library code, or close to it.

eandre commented 1 year ago

That's one perspective, but is that the official stance of the Go project?

ghost commented 1 year ago

That's one perspective, but is that the official stance of the Go project?

I would say history has answered that question already:

these are quotes from the Go team, and in some cases quotes from a Principal Engineer at Google. I would say that is well good enough.

eandre commented 1 year ago

I don't believe @ianlancetaylor's comments amount to even something remotely as general a statement or philosophy around backwards compatibility as you're suggesting they do. I would prefer to let the Go team speak for themselves.

randall77 commented 1 year ago

I think the Go team is squarely in agreement with Ian.