Skip to content
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

spec: any no longer implements comparable #51257

Closed
billinghamj opened this issue Feb 18, 2022 · 16 comments
Closed

spec: any no longer implements comparable #51257

billinghamj opened this issue Feb 18, 2022 · 16 comments
Assignees
Labels
generics NeedsFix release-blocker
Milestone

Comments

@billinghamj
Copy link

@billinghamj billinghamj commented Feb 18, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18rc1 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/jb/Library/Caches/go-build"
GOENV="/Users/jb/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jb/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jb/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.18rc1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.18rc1/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18rc1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/jb/code/golang-set/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qv/ll7pql3n7dbf5w8rf46bcttm0000gn/T/go-build4282451061=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/jML-W5X8dks?v=gotip

What did you expect to see?

Program exited.

(no error)

What did you see instead?

./prog.go:16:17: any does not implement comparable

Go build failed.

Alternatively, using comparable rather than any, this error:

./prog.go:16:20: interface is (or embeds) comparable

Go build failed.

Ref: #50646 (comment)

I understand that this seems to be intended, but I think it's going to make it practically quite difficult for developers to use generics in Go - as far as I can tell, there is simply no way to make this work

I think this should be reconsidered, or at least discussed more widely - as it does not seem to be specified in the proposal

This seems significant enough to be considered as a release blocker

A library which is affected in practice: deckarep/golang-set#79 (using its generics branch) - I have been trying to find workarounds to allow for this new behaviour, but have not managed to resolve it

@dmitshur dmitshur added this to the Go1.18 milestone Feb 18, 2022
@dmitshur dmitshur added the NeedsInvestigation label Feb 18, 2022
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Feb 18, 2022

@zephyrtronium
Copy link
Contributor

@zephyrtronium zephyrtronium commented Feb 18, 2022

Notably, if interfaces do not satisfy comparable, then a transition like this for sync.Map doesn't work:

type MapOf[K comparable, V any] struct { ... }
type Map = MapOf[any, any]

@findleyr
Copy link
Contributor

@findleyr findleyr commented Feb 18, 2022

CC @griesemer @rsc

I agree with the conclusions of #50646 -- that we should opt for logical consistency and type safety -- but also agree that this may warrant more discussion. In particular, we should probably analyze whether this decision is reversible: would it be possible to relax these restrictions in the future?

@billinghamj
Copy link
Author

@billinghamj billinghamj commented Feb 18, 2022

@findleyr If that conclusion remains, would an alternative be to allow comparable to be specified as a type and in non-constraint interfaces? I'm not too clear on what the thinking was to disallow that

If neither of those are allowed, is it the case that this kind of code is simply not allowed in Go? Or is there some workaround we're missing?

@ianlancetaylor ianlancetaylor added the generics label Feb 18, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Feb 18, 2022

Is there any reason to believe we can't relax this in a future release?
Being conservative for now seems like the best answer.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 18, 2022

I agree that we don't have to do anything now. I think the path forward is the suggestion above: permit comparable to be used as an ordinary type. It would be an interface type that can hold any comparable type. If we do that then I think this problem goes away; the original code doesn't actually want []any, it wants []comparable.

But we aren't going to do that for the 1.18 release.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 18, 2022

FWIW I am not sure that []comparable really fixes the inconsistency. For example this works today:

type T struct{ x any }

func main() {
	NewSetFromSlice([]T{
		{"foo"},
		{5},
	})
}

It is odd that []T (where T contains any) is allowed but []any is not. It feels like an inconsistent cut-out. But I completely agree about waiting until after Go 1.18 to figure this out.

@zephyrtronium
Copy link
Contributor

@zephyrtronium zephyrtronium commented Feb 18, 2022

Another hypothetical solution would be to add a constraint that expresses the set of all interface types. Then the constraint for "all types which have the == operator" is interface { comparable | interfaceTypes }.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 19, 2022

I believe this is just a bug: When we check if a type implements comparable, we must always use the same criteria for comparability. In this case:

type T struct{ x any }

func main() {
	NewSetFromSlice([]T{
		{"foo"},
		{5},
	})
}

the type T is not an interface and the code is using the traditional (before 1.18) predicate to determine if T is comparable. It should be using an updated version.

I think this needs to be fixed for 1.18 otherwise we'll have to maintain this inconsistency.

I agree that the long-term solution is to allow comparable as an ordinary type. See also my comment here:

Had we introduced a notation (some special mark) for interfaces for which we expect to use == and != in the beginning of Go, all this would have been a non-issue: clearly one wouldn't be able to compare two interfaces that don't support comparison. Such a mark would also have made it impossible to assign a non-comparable value to non-comparable interfaces, etc. Instead we relied on a run-time test, at considerable implementation cost, for that matter. This is something we cannot change. But generic code will now be type-safe with respect to this.

@griesemer griesemer self-assigned this Feb 19, 2022
@griesemer griesemer added NeedsFix release-blocker and removed NeedsInvestigation labels Feb 19, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 20, 2022

Change https://go.dev/cl/387055 mentions this issue: go/types, types2: implement (static) IsComparable predicate

@billinghamj
Copy link
Author

@billinghamj billinghamj commented Feb 21, 2022

Btw I also noticed I couldn't do this:

_ := Set[Set[string]]{}

With the same error - Set[string] does not implement comparable

Maybe that is a similar issue? As it seems that it's just a struct which contains entirely comparable fields. Hopefully that fix will work here too

@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 21, 2022

@billinghamj Note that Set is declared as follows:

type Set[T comparable] map[T]struct{}

A Set is a map, and maps are not comparable, they have never been. So you can't use a Set (i.e., a map) as a key for another map. This is also not possible in non-generic Go.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

I think this issue should have gone through the proposal process. The type parameters design doc explicitly states:

The type set of the comparable constraint is the set of all comparable types.

The definition of “comparable” at the time that proposal was approved states (emphasis mine):

Interface values are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.

By those definitions, every interface type should be included in the type-set of comparable, and thus any — as an interface type itself — should be in that type-set.

Redefining comparable not to include interface types is a very late and drastic departure from the design that was approved in #43651.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 3, 2022

@bcmills I agree that under normal circumstances this should have gone through the proposal process. But we've been long past the time where we should be making design decisions and we have run into what seems a clear inconsistency (#50646). As a matter or "real software engineering" (borrowing somewhat freely from "realpolitik"), being overly conservative (by being consistent) allows us to relax rules down the road while remaining backward compatible - at least that was the intent. The good news is that changing behavior, at least as far as the type system implementation is concerned, is simple. But first we need to settle on what the "right" behavior is. Again, given the imminence of the release, being overly cautious seems to be the prudent approach, even if it comes at the cost of not being able to write certain (or even many) programs.

@changkun
Copy link
Member

@changkun changkun commented Mar 18, 2022

Sorry for stumbling into this closed issue. This change may create code that depends on the behavior that any does not implement comparable, and it seems that being conservative or relaxed both make behavior that can potentially break the code compatibility. I understand the release doc emphasized the potential breakage, but this last-minute change is entirely unexpected and limits the ability of usage...

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 23, 2022

Change https://go.dev/cl/401874 mentions this issue: spec: clarify type set and interface are different

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generics NeedsFix release-blocker
Projects
None yet
Development

No branches or pull requests

10 participants