-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/exp/slices: "backport" slices.SortFunc #61374
Comments
We can just do this, it doesn't need to go through proposal review. Taking it out of the proposal process. Thanks. |
Change https://go.dev/cl/511395 mentions this issue: |
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. |
Change https://go.dev/cl/511895 mentions this issue: |
Change https://go.dev/cl/511660 mentions this issue: |
Also add a go:generate command to the standard library slices package. For #61374 Change-Id: I7aae8e451b7c6c4390e0344257478d1a96a14189 Reviewed-on: https://go-review.googlesource.com/c/go/+/511660 Run-TryBot: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Eli Bendersky <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
This is causing lots of breakage for us. It results in a need for all your dependencies that rely on Maybe it's too late to revert this, but it would be a much cleaner migration to keep |
All packages under "exp" is are experimental packages. You say "us" as if you have used this in production. |
The Importers include:
I don't see how the amount of breakage justifies the change, regardless of whether or not |
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:
|
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. |
I agree there is value in having a place for experimenting without committing to backwards compatibility. That said, this package and a few others ( For example, consider the removal of For what it's worth, I think the I also noticed that
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 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 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 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 |
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 It seems to me we could just have taken the lessons learned and applied them to the standard library |
Its unfortunate that some people had this incorrect perception. The package description makes it painfully clear thats not the case:
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:
Thats the thing, once they dump something into
strawman. this issue is about
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 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
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: then it was put into the
I cant speak for the ecosystem, but only myself. my plan was to move to
see my previous comment.
personally I am quite pleased with the result here. the Go team was quick to harmonize the two APIs once
I would say again, the motivation for this is once I would say in closing, it just seems that some people need to respect the experimental nature of the code in |
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 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.
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 |
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 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 |
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
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. |
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.
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 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. |
If that's your cost/benefit model I rest my case. |
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. |
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
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.
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 |
Please explain. Surely the entirely voluntary decision to change from |
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. |
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 Anyway, I think we've exhausted this discussion at this point. |
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. |
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. |
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. |
I think the Go team is squarely in agreement with Ian. |
<!-- Describe what has changed in this PR --> **What changed?** The golang.org/x/exp module was updated to use the same function signature as Go 1.21's upcoming slices.SortFunc function. This changed the return value from a bool (less than) to an int (compare to). Update Temporal to the latest version and fix the uses. <!-- Tell your future self why have you made these changes --> **Why?** For more details about the upstream change, see the following issue and commit: - golang/go#61374 - https://cs.opensource.google/go/x/exp/+/302865e7556b4ae5de27248ce625d443ef4ad3ed <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** I ran the tests locally on my machine. <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** Worst case I may have switched the sort order somewhere accidentally. <!-- Is this PR a hotfix candidate or require that a notification be sent to the broader community? (Yes/No) --> **Is hotfix candidate?** No --------- Co-authored-by: Yichao Yang <[email protected]>
<!-- Describe what has changed in this PR --> **What changed?** The golang.org/x/exp module was updated to use the same function signature as Go 1.21's upcoming slices.SortFunc function. This changed the return value from a bool (less than) to an int (compare to). Update Temporal to the latest version and fix the uses. <!-- Tell your future self why have you made these changes --> **Why?** For more details about the upstream change, see the following issue and commit: - golang/go#61374 - https://cs.opensource.google/go/x/exp/+/302865e7556b4ae5de27248ce625d443ef4ad3ed <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** I ran the tests locally on my machine. <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** Worst case I may have switched the sort order somewhere accidentally. <!-- Is this PR a hotfix candidate or require that a notification be sent to the broader community? (Yes/No) --> **Is hotfix candidate?** No --------- Co-authored-by: Yichao Yang <[email protected]>
<!-- Describe what has changed in this PR --> **What changed?** The golang.org/x/exp module was updated to use the same function signature as Go 1.21's upcoming slices.SortFunc function. This changed the return value from a bool (less than) to an int (compare to). Update Temporal to the latest version and fix the uses. <!-- Tell your future self why have you made these changes --> **Why?** For more details about the upstream change, see the following issue and commit: - golang/go#61374 - https://cs.opensource.google/go/x/exp/+/302865e7556b4ae5de27248ce625d443ef4ad3ed <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** I ran the tests locally on my machine. <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** Worst case I may have switched the sort order somewhere accidentally. <!-- Is this PR a hotfix candidate or require that a notification be sent to the broader community? (Yes/No) --> **Is hotfix candidate?** No --------- Co-authored-by: Yichao Yang <[email protected]>
* was seeing build failures locally and in CI due to golang/go#61374 * had to bump to go 1.21 to leverage the `cmp` lib Signed-off-by: Arko Dasgupta <[email protected]>
Update go exp pkg to latest until go version is updated to v1.21 Ref: golang/go#61374
Update go exp pkg to latest until go version is updated to v1.21 Ref: golang/go#61374
slices.SortFunc is coming soon:
https://pkg.go.dev/slices#SortFunc
but its not here yet. meanwhile, the signature:
is different from
x/exp/slices
:https://pkg.go.dev/golang.org/x/exp/slices#SortFunc
namely, the callback returns
int
instead ofbool
.The text was updated successfully, but these errors were encountered: