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

proposal: spec: permit values to have type "comparable" #51338

Open
ianlancetaylor opened this issue Feb 24, 2022 · 56 comments
Open

proposal: spec: permit values to have type "comparable" #51338

ianlancetaylor opened this issue Feb 24, 2022 · 56 comments
Labels
Projects
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 24, 2022

As part of adding generics, Go 1.18 introduces a new predeclared interface type comparable. That interface type is implemented by any non-interface type that is comparable, and by any interface type that is or embeds comparable. Comparable non-interface types are numeric, string, boolean, pointer, and channel types, structs all of whose field types are comparable, and arrays whose element types are comparable. Slice, map, and function types are not comparable.

In Go, interface types are comparable in the sense that they can be compared with the == and != operators. However, interface types do not in general implement the predeclared interface type comparable. An interface type only implements comparable if it is or embeds comparable.

Developing this distinction between the predeclared type comparable and the general language notion of comparable has been confusing; see #50646. The distinction makes it hard to write certain kinds of generic code; see #51257.

For a specific example, you can today write a generic Set type of some specific (comparable) element type and write functions that work on sets of any element type:

type Set[E comparable] map[E]bool
func Union[E comparable](s1, s2 Set[E]) Set[E] { ... }

But there is no way today to instantiate this Set type to create a general set that works for any (comparable) value. That is, you can't write Set[any], because any does not satisfy the constraint comparable. You can get a very similar effect by writing map[any]bool, but then all the functions like Union have to be written anew for this new version.

We can reduce this kind of problem by permitting comparable to be an ordinary type. It then becomes possible to write Set[comparable].

As an ordinary type, comparable would be an interface type that is implemented by any comparable type.

  • Any comparable non-interface type could be assigned to a variable of type comparable.
  • A value of an interface type that is or embeds comparable could be assigned to a variable of type comparable.
  • A type assertion to comparable, as in x.(comparable), would succeed if the dynamic type of x is a comparable type.
  • Similarly for a type switch case comparable.
@ianlancetaylor ianlancetaylor added this to the Proposal milestone Feb 24, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 24, 2022
@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Feb 24, 2022

To be clear, this proposal is for Go 1.19 or later.

CC @griesemer

@apparentlymart
Copy link

@apparentlymart apparentlymart commented Feb 24, 2022

Amusingly, I wrote my first real generic code today and then immediately ran into this situation, in almost exactly the way described in the writeup.

My generic set type:

type Set[T comparable] map[T]T

I initially tested it with some simple cases involving T being a struct type containing only comparable fields, and I was just getting ready to call it done when I decided to write the following declarations as static assertions to verify it could support all of the types I wanted:

var _ Set[exampleInterface]
var _ Set[exampleComparableStruct]
var _ Set[*exampleUncomparableStruct] // pointers are comparable regardless of target type

type exampleInterface interface {
	comparable
	Boop()
}

type exampleComparableStruct struct {
	v [2]int
}

type exampleUncomparableStruct struct {
	f func(int)
}

This fails at compile time with the following error:

set_test.go:3:11: interface is (or embeds) comparable

I understand that this is one of the cases you already listed as what this proposal would allow:

A value of an interface type that is or embeds comparable could be assigned to a variable of type comparable.

However, I ended up here mostly by luck, because this error message happened to exactly match the text you used to describe the situation that would become valid. This error message was confusing to me because it just states something I know to be true: I intentionally embedded comparable in that interface type to try to make it possible to declare a set of that interface type. It doesn't say anything about why that statement is relevant, since it does seem to be valid to declare an interface which embeds comparable, just that it's not valid to then declare a value of that type.

I totally understand that making this work is not on the table until Go 1.19 at the earliest, but I wonder if it would be possible in the meantime to improve this error message to at least make a more direct statement about what I did wrong here:

set_test.go:3:11: cannot declare variable of interface type that is (or embeds) comparable

Perhaps I'm misunderstanding, but I believe from this writeup (and from my own further experimentation) that it's the declaration of the variable that is invalid, not the interface declaration itself.


In case it's helpful in evaluating the proposal, my underlying goal here is to make a set of values that are all comparable and all implement a specific interface, but that have different dynamic types.

This is actually part of an implementation of a directed acyclic graph (type Graph[V comparable], with Set[V] fields) where the nodes represent different actions, so in real usage there would be a type Action interface representing the idea of an action, and concrete implementations of that interface representing specific action types. The program will then walk the graph in a topological sort order and call a method of Action on each node, which is of course then a virtual call to perform the specific action represented by that value.

Action will actually be implemented only by pointers to the action types in practice, which will therefore all be comparable by pointer identity.

I'm not planning to pursue this any further for the moment, since it seems unlikely I will be able to achieve this goal with Go 1.18. I hope these details are helpful in evaluating the proposal.


Next day update: FWIW, I was able to work around this for now by separating the idea of a graph node (a pointer to a specific struct type) from the idea of an action, so that a graph node has an action, rather than is an action. For my purposes that seems to be sufficient for the moment, although this wrapping graph node struct isn't really adding anything right now except something concrete to save a pointer to.

type GraphNode struct {
	Action Action
}

type Action interface {
	// ...
}

type ActionGraph = graph.Graph[*GraphNode]

@go101
Copy link

@go101 go101 commented Feb 24, 2022

Is it possible to disallow comparisons with incomparable interfaces, to avoid runtime panics during comparisons? (looks hard).

@apparentlymart
Copy link

@apparentlymart apparentlymart commented Feb 24, 2022

I would think that disallowing comparisons with interfaces that aren't statically incomparable would be a breaking change at this point, since it's always been possible so far to carefully compare interface values and just make sure that you don't write any implementations that aren't themselves comparable. 🤔 I have several examples in the codebases I maintain in my dayjob which would immediately fail compile under that rule, even though in practice they can never panic at runtime today.

It does seem like a shame, since this seems like a clear win if this were a green field problem, but I think I'd rather have the ability to declare that all values of a particular interface type are guaranteed never to panic on comparison, even if it remains possible to accept the risk and try comparing interface values of a non-statically-guaranteed-comparable interface type.

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Feb 25, 2022

@go101 We can't change the existing language, so it will continue to be possible to compare values of interface type which can possibly panic. That said, if we adopt this proposal, then programs that care can convert their interface types to be comparable, or embed comparable, and thus ensure that their interface comparisons can't panic. It would also, after a while, be possible to have vet or some other analyzer warn about comparisons of interface types that are not comparable.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 25, 2022

I think this is a good change, but mostly because I see it as a step on the path to making any constraint useable as a normal interface value, which in turn opens the door to tagged union types. I know that using constraints as interface values has some wrinkles to work out (given type number interface{ int | uint }, var n number would be initially nil, rather than 0 [but what else could it be, since it's not specified as an int nor a uint yet?]), but it seems like the logical implication of the systems introduced in Go 1.18, so I'm not sure how long it can be excluded.

@abligh
Copy link

@abligh abligh commented Feb 27, 2022

I fell at (nearly) the first hurdle at trying to make a generic sync.Map (details in #51384 ). More precisely my GenericMap[K,V] works fine except for things like GenericMap[interface{},bool] because interface{}/any do not implement comparable. So I support this proposal.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 2, 2022

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 rsc moved this from Incoming to Active in Proposals Mar 2, 2022
@Merovius
Copy link

@Merovius Merovius commented Mar 3, 2022

However, interface types do not in general implement the predeclared interface type comparable. An interface type only implements comparable if it is or embeds comparable.
[…]
But there is no way today to instantiate this Set type to create a general set that works for any (comparable) value. That is, you can't write Set[any], because any does not satisfy the constraint comparable.

TIL. This surprised me, I missed that discussion.

IMO that was a mistake and the answer to this problem is "interface types are comparable, so they should satisfy comparable".

Personally, I'm against this proposal for the reasons I mentioned when I last saw this idea being floated. Namely that embedding comparable becomes viral. For any interface-type:

  1. Embedding comparable is bad. It disallows implementations with non-comparable values (e.g. http.HandlerFunc). In general, the author of an interface-types should have no opinion over what underlying type is used to implement it.
  2. Not embedding comparable is bad. It disallows instantiating Set with it, even if the user of the interface knows all their actual values are comparable.

So you'll need both versions, in practice, for every interface. That's a color.

Moreover, the rules as stated don't actually seem to give meaningful additional guarantees about safety. For example, struct{ F any } would still implement comparable, AIUI. But its comparison is obviously not, in any way, safer than comparison of any. So, the claim that

That said, if we adopt this proposal, then programs that care can convert their interface types to be comparable, or embed comparable, and thus ensure that their interface comparisons can't panic.

doesn't seem to be actually true. [edit] Well, when testing this, I realized that this isn't actually true - struct{any} does not implement comparable in Go 1.18, so this option is actually already the case.[/edit] Of course, we could extend the virality of comparable to struct-fields and array-elements. But that would also make the virality-problem above much worse. While it's not that unreasonable, to have to declare type ComparableType interface { reflect.Type; comparable } to instantiate Set[Type], if I want to use a struct with a reflect.Type field, I'd have to

  1. Declare my own version of that struct, with the same fields, except all interfaces replaced with their comparable colors, potentially recursively.
  2. Hope that definition stays in sync.
  3. Presumably, manually write conversions between my comparable struct-flavor and the one I actually want (unless we do something like we did for struct-tags and allow conversions to ignore comparable coloring).
  4. Convert back and forth, every time I want to put the struct I'm interested in into a Set or take it out from it.

That seems an unreasonable amount of effort, to me.


I understand why the decision was made, to have interface-types not satisfy comparable. It is confusing to be able to instantiate Set[any], even if the constraint is comparable. But confusion can be solved by education. We are living with the nil-pointers-in-interfaces-are-not-nil confusion, because it might be confusing, but the language is better for the semantics of it. The mess we are getting ourselves into with distinguishing comparable interface types from non-comparable ones, however, can't be solved by education. We are making it more cumbersome to use the language, to avoid having to explain to people that interface-types are in fact comparable. That seems a bad tradeoff.

Deciding that interface-types don't satisfy comparable was the conservative decision. We can't become more restrictive, but we can always become less restrictive. So starting out more restrictive until we learn more made sense. IMO, the arguments in favor of this issue really are arguments that we should exercise the option to become less restrictive and have interface-types satisfy comparable.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 3, 2022

Is this proposal significantly different from the idea I floated here? #49587 (comment)

In that comment, I was under the impression that interface types would satisfy the comparable constraint. I guess I never actually tried it out!

Although I have a soft spot for this idea, I don't think it fits well with the language as is. reflect.Type is a good example - it's commonly used as a comparable value, but we can't currently use it as a comparable type parameter, which seems wrong. If this had all been done at the start, we'd probably have added comparable to the reflect.Type interface definition, but I don't believe that can be done now without breaking backward compatibility.

As a general rule, I think that if one is allowed to use == on a type outside generic code, one should be allowed to use it inside generic code.

That said, I would support a change to the language that's compatible with the goal expressed in the subject of this proposal: I think that values should be permitted to have type "comparable". I propose a much simpler rule: when used in a non-constraint context, comparable is a synonym for any.

Given:

type Set[E comparable] map[E]bool

it would be possible to write Set[comparable] but that would be exactly the same as Set[any].

The main benefit of doing this is that it would be possible to use a comparable constraint interface
as the parameter type too. For example given these definitions:

type MC interface {
    comparable
    M()
}

type MSet[T MC] map[T] bool

MSet[MC] would be a valid instantiation of MSet, but MSet[struct{_ [0]func(); MC] would not.

@Merovius

Embedding comparable is bad. It disallows implementations with non-comparable values (e.g. http.HandlerFunc). In general, the author of an interface-types should have no opinion over what underlying type is used to implement it.

For the record, I disagree with this. It's an important part of some interfaces that their implementations are comparable - that's part of the contract. reflect.Type is one example. Another is context.WithValue: if comparable as proposed here had been available at the time, we'd almost certainly have used it for that first argument.

@Merovius
Copy link

@Merovius Merovius commented Mar 3, 2022

@rogpeppe I agree that for some interfaces it's important. But for most it's counterproductive. By making interface types not satisfy comparable unless they embed it, we make most interface types impossible to use with generic types like Set even if that usage would be totally safe. And just because the author didn't want to require comparable, doesn't mean the user of an interface doesn't know all their values are.

That's kind of my point. comparable bisects the set of interface types into two universes, one with "types where it is considered essential that the interface is comparable" and one with "types where the author just wants to do some operations and doesn't care about the concrete type" - and a gray area in-between. In usage, they become incompatible. But there is overlap between them, which means duplication. The proposal text makes the argument:

But there is no way today to instantiate this Set type to create a general set that works for any (comparable) value. That is, you can't write Set[any], because any does not satisfy the constraint comparable. You can get a very similar effect by writing map[any]bool, but then all the functions like Union have to be written anew for this new version.

But I'd argue, we still need this duplication under the proposal, if we want to be able to use e.g. reflect.Type (or any other gray-area usage of an interface) in a Set. With the second being

// SetAny is a Set[T], which doesn't statically guarantee its values are comparable, for cases where
// you want to use an interface type which doesn't categorically require comparable, but where you know
// the values revelant for your case are. It panics at runtime, if you try adding non-comparable values to it.
type SetAny[T any] struct { m map[comparable]bool }
func (s SetAny[T]) Contains(v T) bool { return s.m[v.(comparable)] }
func (s SetAny[T]) Add(v T) { s.m[v.(comparable)] = true }
// etc

I also think that we need to keep current usage in mind. reflect.Type does not embed comparable and context.WithValue doesn't accept comparable and we can't really change that under compatibility rules. But that also means we can't have a Set[T comparable] which can take a reflect.Type. "If we had comparable from the beginning, we would've done that" is all well and good, but we didn't and we should consider how we go from here.

@Merovius
Copy link

@Merovius Merovius commented Mar 3, 2022

@rogpeppe Let me clarify: In my comment above, when I said "X is bad", i didn't mean "it is categorically bad, for all types", but "it has downsides". Both embedding and not embedding comparable has downsides, so no is strictly better, so we'll sometimes/often/usually/… need both.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 3, 2022

I proposed that:

when used in a non-constraint context, comparable is a synonym for any.

For the record, that would imply that the following would be OK:

var x *comparable
var y *any = x

An alternative might be to consider comparable as a separate named type, as if it we defined as follows:

type comparable any

@atdiar
Copy link

@atdiar atdiar commented Mar 3, 2022

Is there really a problem appart from reflect.Type ?

How often do people use any potentially non-comparable interface values in Set and why would they not type assert to comparable before insertion?

Embedding comparable does not seem like an absolute necessity every time. The proposal as it stands would enable us to test the dynamic type.

@Merovius
Copy link

@Merovius Merovius commented Mar 3, 2022

@atdiar

How often do people use any potentially non-comparable interface values in Set

The majority of interface values should be non-comparable. There are only two reasons I can think of, to embed comparable: 1. You accept it and need to put it in a map (or otherwise compare it) yourself and 2. it's effectively a closed sum (like reflect.Type or ast.Node) and you want to guarantee that all implementations are comparable.

Those are relatively rare. Most interface usages are to abstract over behavior, which shouldn't care about the dynamic type of its value.

why would they not type assert to comparable before insertion?

Of course, they can do that. That's what I said here, under "we still need this duplication". Of course the author of the Set type might chose not to do that duplication, putting the onus instead on the user of the type to do it. But avoiding that was one of the benefits given for this proposal and I just don't see that.

And, of course, the even larger problem of using struct-types with interface-fields still persists after that. That's not something that can even be solved with a type-assertion.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

I am still mulling over the implications, but it seems to me that the migration for interfaces to comparable is analogous to the migration to const values in C.

Like const, comparable sets up a situation in which new APIs are more restrictive than old ones, and in which values must be cast or converted from the less-restrictive to the more-restrictive type in order to use them.

With const, if you have a const value, and you know that a function does not modify it, you can cast away the const and pass the value as a non-const type. With comparable, if you have an interface value, and you know that the caller always passes a value that is actually comparable, you can (in many cases) explicitly convert it to a comparable type.

However, I'm not sure that the comparable conversion is even always an option for, say, struct types with fields of interface types. You may know that the contents of a given field are always comparable, but you can't convert the struct type to an equivalent comparable type because struct types are not interfaces. (The comparable interface conversion would lose the struct fields.)

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

It seems to me that the virality problems could be at least partially addressed by defining “comparable” as a parameterized type rather than (just) an interface type. Its behavior would be roughly analogous to the convertible.To and assignable.To interface types I described in my assignability writeup.

Here I'll describe that type as Comparable[T any].

Comparable[T] as a built-in type

Comparable[T] is a type with the same representation as T that holds the subset of values of type T for which the == operation is guaranteed not to panic.

  1. A variable of type T is convertible to Comparable[T]. If the value held by the variable is not comparable, the conversion panics. (This essentially front-loads the panic that could later result from the == operation.)
  2. A variable of type Comparable[T] is convertible to T.
  3. The zero-value of Comparable[T] is the zero-value of T (which is itself always comparable).
  4. If T is an interface type, Comparable[T] implements T. However, T only implements Comparable[T] if T is or embeds a Comparable interface type, or if the underlying types of all types in type-set of T are comparable.
  5. The method set of Comparable[T] is the method set of T.
  6. (optional) If T not an interface type and is itself comparable, T is assignable to Comparable[T]. (That is: because the conversion to Comparable[T] cannot fail, it may be implicit.)
  7. (optional) Because every Comparable[T] is a T, a variable of type Comparable[T] is assignable to T.
  8. (optional) If T is a comparable composite type, then T is assignable to Comparable[T]

I have not yet assessed whether this definition of Comparable[T] is coherent as an interface type. (The zero-value in particular is odd, since it is not nil like the usual zero-value for an interface type.)

This definition of Comparable is more useful for integrating generic types, because it allows, say, struct types with a field of an interface type to be used as a type with a comparable bound. However, I'm not sure whether Comparable[T] replaces or merely augments the non-parameterized comparable bound.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

At the very least, Comparable[T] could be used to adapt existing interface types to generic APIs with comparable constraints:

type MapOf[K comparable, V any] map[K]V

func assign(m MapOf[Comparable[any], any], k, v interface{}) {
	kc := Comparable[any](v)
	m[kc] = v
}

The ergonomics still aren't great, but at least the conversion becomes possible to write at all.

@atdiar
Copy link

@atdiar atdiar commented Mar 3, 2022

What if we just don't apply comparable in a retroactive fashion. For map keys, we in effect wouldn't retroactively constrain with comparable.

On the other hand, it would be possible to offer a wrapping interface to maps that uses the comparable constraint if such a stronger constraint was needed. (henceforth eliminating all non-comparable interfaces from the list of potential values for type arguments)

Meaning that in Go, in general, map keys would remain unconstrained (using any) leaving the usage of comparable for future APIs.

Edit: Perhaps that the current constraint is not yet well defined and does not equal to comparable.
Perhaps a new predicate isInterface and the overall constraint would be equal to: {comparable | isInterface} rather.

Edit2: needs something more to deal with struct fields...

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

I guess, to summarize my comments above: defining comparable as an interface type as proposed seems coherent, but as far as I can tell does almost nothing to address the problems described in #51257.

Consider the implementation of the Union function for a Set type that allows arbitrary key-types checked at runtime:

type Set[E any] map[comparable]bool

func Union[E any](s1, s2 Set[E]) Set[E] {
	result := make(Set[E])
	for k, ok := range s1 {
		if ok {
			result[k.(comparable)] = true
		}
	}
	for k, ok := range s2 {
		if ok {
			result[k.(comparable)] = true
		}
	}
}

Now consider what happens if we instantiate above with the type:

type S struct {
	I any
}

As currently proposed in #51338 (comment), the above implementation of Union would always panic for the type Set[S].

The problem is that the type-assertion rule is too weak (emphasis mine):

A type assertion to comparable, as in x.(comparable), would succeed if the dynamic type of x is a comparable type.


In order for the Union implementation to be viable, that rule would need to be revised to:

A type assertion to comparable, as in x.(comparable), would succeed if an interface comparison on the dynamic value of x would not panic.

But if we make that revision, now we have a bit of a paradox: comparable is no longer a coherent interface type, because a variable of type comparable could contain a value of a type that does not implement the comparable interface!

@atdiar
Copy link

@atdiar atdiar commented Mar 3, 2022

What if a struct required all its fields to implement comparable?
That was how I was envisioning comparable.

Isn't that the only way to be sure at compile-time that a composite type implements comparable?

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 3, 2022

@atdiar

What if a struct required all its fields to implement comparable?

Because comparable does not exist as an interface type today, no existing struct type has such a requirement, and adding such a requirement to any struct type would be a breaking change. I don't see a viable migration path to there from where we are today — hence the analogy with const in C. 😩

@Merovius
Copy link

@Merovius Merovius commented Mar 3, 2022

@bcmills

At the very least, Comparable[T] could be used to adapt existing interface types to generic APIs with comparable constraints:

ISTM this isn't substantially different than the proposal, except that the proposal spells the conversion as a type-assertion. i.e. it spells x.(comparable), instead of Comparable[T](x).

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Mar 4, 2022

Thanks for the example. Personally I would always write code like that using a map from the key (I guess URL + ContentType) to a *Work. It would never occur to me that the io.Reader as part of the map key, because that just seems weird. But maybe that's just me.

And there is always this approach https://go.dev/play/p/HzhWw23rpja?v=gotip ? Maybe?

I dunno, it's suggestive, but is it convincing? And it's not an existing type.

@apparentlymart
Copy link

@apparentlymart apparentlymart commented Mar 4, 2022

The hypothetical @Merovius shared could easily have applied to what I was describing in my original comment above if what I were implementing were across multiple libraries intended for reuse, rather than a closed system.

I have a graph of actions, but the graph is built in terms of sets, which are themselves implemented as Go maps. If this were not a closed system where the graph representation and the actions mechanism were implemented together and tightly coupled, there would be no particular reason why the Action interface itself would deserve to be comparable in isolation. It's only once I want to put them in a set that the additional property of being comparable becomes important, but I wouldn't be able to do that if the definition of Action were not under my control, or if there are unrelated implementations of Action that don't need to be in the graph.

With that said, there are a lot of hypotheticals in that scenario. It's also true that I already solved it in a way that I think would also be satisfying in the less-tightly-coupled scenario: rather than trying to make Action also be a graph node, I made another type that is comparable and has an action. As far as I can tell this works even if the following three parts are controlled by different maintainers:

  • In a graph library: the generic graph/set implementation itself digraph.Graph[T comparable] and digraph.Set[T comparable].
  • In some sort of workflow modelling library: The workflow.Action interface, which is not comparable and has no justification to be within the realm of what this library provides.
  • In a graph-based workflow executor which uses both of the above: type GraphNode struct { Action workflow.Action }, with digraph.Graph[*GraphNode] as the graph type.

As long as you're willing to accept pointer identity as the comparison, it's always possible to wrap something in a pointer to make it comparable. If you need some other definition of identity then I think that invokes Ian's situation where the digraph.Set instance must have its own custom key-generating function (or similar), which takes a T and returns something stable and comparable that represents it.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 4, 2022

(I should add that I don't feel too strongly about any of this. My main concern today is that we are making the conservative choice for 1.18, a choice that we can change later without invalidating code that works with 1.18. As far as I can see, we are.)

Yup. It looks like https://go-review.googlesource.com/c/go/+/381254 has changed that choice since I suggested doing so, which gives us some breathing room to consider this proposal, for which I'm grateful, thanks!

To speak in favour of this proposal for a moment, I believe that having a way of statically declaring that an interface type is comparable would be a significant benefit to Go, providing useful assurance that the code won't panic and also acting as user-visible documentation where that might not be clear otherwise. I think it's unlikely that this constraint would be significantly viral.

That said, I believe this statement is technically false:

reflect.Type is not an issue here. The reflect.Type interface already includes unexported methods, and as such can only be implemented by types defined in the reflect package. Therefore, we can change reflect.Type to embed comparable with no concerns about backward compatibility.

Here's an example where embedding comparable inside reflect.Type would result in a compilation failure:
https://go.dev/play/p/Bvd1uht3Gei?v=gotip

That is, the fact that unexported methods are included in it doesn't preclude other types implementing reflect.Type, and some such implementations might not themselves be comparable.

All that said, it is well known that reflect.Type should be comparable and any such implementation probably deserves that compilation failure.

Like Ian, I'd still like to see concrete examples where this proposal would actually cause problems.

@beoran
Copy link

@beoran beoran commented Mar 4, 2022

@Merovius Thanks for the example. I think the current behaviour is correct there, since Work contains a io.Reader and is definitely not comparable in non generic Go. I would argue that comparable is the wrong type for the elements of a genetic set.

In java, for example there is the Hashable generic type and that is used as the basis for many other generic types. In go, i would expect a Set to likewise use a generic Hashable interface.

Thinking about what this issue is trying to fix, perhaps it is better to wait and see if we can develop some best practices with a generics library with tye language as it is now, and after a year or two we can probably see more clearly what are the real problems to fix.

@Merovius
Copy link

@Merovius Merovius commented Mar 4, 2022

I think the probability of a specific person having encountered a specific problem in real code is relatively low - much less, that they remember it, potentially years after the fact. For example, I don't know if I ever encountered the nil vs. nil problem and while I definitely have encountered the loop variable closure problem, I couldn't actually name the occasion it happened in. So, the best I can do is try and come up with code I would plausibly write, which I tried to do above.

I thought about all of this some more. Another example (aside from reflect.Type) of an interface-type which tends to end up in maps is ast.Node. There's code in the stdlib doing it. ast.Node does not (AFAICT) contain unexported methods. But, to be fair, it falls into the same class of "it should really be a union type, but we don't have those", so it can at least be argued that it should have been given a comparable embedding.

But that made me think of another kind of ast.Node. In this case, it seems less clear. goldmark is a markdown parser and it is extensible. So an ast.Node here is not a closed union, but it is intended to be implementable by users of the library (at least AIUI). From what I can tell, the goldmark library itself does not store it in a map (I only grepped map[Node] and map[ast.Node] though, so I might've missed it), so there is no a priori reason for the library to declare the interface as comparable. But it's conceivable for an extension or user of the library to use these ast.Nodes in a map, for example to store annotations (similar to how go/types does it for go/ast.Node). That would, of course, require ast.Node to be comparable, strictly speaking.

So, in this case, the tradeoff I mentioned above comes up for the goldmark authors - should they limit the implementers of the ast.Node interface, by embedding comparable, or should they limit its users, by not doing it? Keeping in mind that the decision can't be reversed later, for backwards compatibility reasons.

These are the best, most plausible examples I could come up with so far. I will continue to idly think about this for a while, maybe I can come up with more. Maybe I can't. I honestly don't know how big of a problem this is going to be. Maybe it's fine.

Personally, I just find it strange, that interface-types are comparable, as defined by the spec, but don't implement comparable. ISTM that by choosing to not have them implement comparable, the generics design introduced an incongruency and special case, instead of trying to fit itself into the language. But maybe that's just me.

@atdiar
Copy link

@atdiar atdiar commented Mar 4, 2022

Personally, I just find it strange, that interface-types are comparable, as defined by the spec, but don't implement comparable. ISTM that by choosing to not have them implement comparable, the generics design introduced an incongruency and special case, instead of trying to fit itself into the language. But maybe that's just me.

I think it can be explained.

The change is probably motivated by the difference in dealing with interface value comparison vs. non-interface value comparison.
These are two different notions of comparability.

One is allowed to proceed and possibly fail at runtime.
The other one has its failures caught at compile-time.

It would be problematic to not distinguish the two:

var c comparable
var f func() 

i:= interface{} (f) 

c=i // ?? 

Unless I am wildly mistaken, if interfaces were comparable we should be able to assign i to c.

Meaning f would implement comparable?!

So I guess, now we would need to make sure that f implements comparable too for the assignment c=i to be valid.

Meaning that depending on the type that is boxed, an interface value would or would not be assignable to a comparable variable.

So I guess, perhaps that interface types are not comparable in general but only maybe-comparable.
Which would correspond to their behavior of being comparable until a runtime failure.

@Merovius
Copy link

@Merovius Merovius commented Mar 4, 2022

@atdiar I understand the logic. I just find the outcome strange.

It would be problematic to not distinguish the two: […] Unless I am wildly mistaken, if interfaces were comparable we should be able to assign i to c.

You are assuming the change of this proposal already happened and comparable is a type - otherwise you couldn't write var c comparable. I'm arguing against doing that. Obviously, if comparable is a type, then i should not be assignable to c in that code.

What I find strange, is that it is valid to write func eq(a, b any) bool { return a == b }, but it is invalid to make that function generic. i.e. func eq[T comparable](a, b T) bool { return a == b} can't be instantiated with any, even though the manual expansion of that function is valid.

This proposal is to fix the problem that Set[T comparable] can't be instantiated with interface-types, because interface types don't implement comparable. My counter-proposal is to fix that problem by making comparable use the definition of "comparable" from the spec (which is to say, go back to how the design used to work).

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Mar 4, 2022

Here's an example where embedding comparable inside reflect.Type would result in a compilation failure:

That's clever. I'd rather not worry about that, but, hmmm.

@atdiar
Copy link

@atdiar atdiar commented Mar 5, 2022

@Merovius

The problem is that if we allow operations on interfaces that are not available to every member of the respective typesets they define, we lose the equivalence relation between interface implementation and constraint satisfaction.

Then, as you accurately note, we won't be able to use some constraint interfaces as regular interfaces.
That's unfortunate asconstraint == typeset inclusion appears to be a simpler rule.

This clash is because of interfaces such as any which are being compared in spite of their typeset.

If generic-go allows to write a sound subset of Go code that is sufficient in most cases, it might be preferable.

In any case, I think that implementing the current proposal might cement the definition of comparable somewhat irreversibly.

@changkun
Copy link
Member

@changkun changkun commented Mar 28, 2022

Sorry for being understanding this proposal too slowly. I have two questions about this proposal:

  1. What are the differences between saying "permit values to have type 'comparable'" and "permit 'any' to implement type 'comparable'"?
  2. If we have code in Go 1.18 that relies on func New[T comparable]() T can never return an empty interface, does this proposal mean a break of backward compatibility?

@Merovius
Copy link

@Merovius Merovius commented Mar 28, 2022

What are the differences between saying "permit values to have type 'comparable'" and "permit 'any' to implement type 'comparable'"?

The former means you can write var x comparable without getting a compilation error. This is not currently possible.

The latter would mean that you can instantiate func F[T any]() with any (or any other interface type, would be the goal), getting the same semantics of comparison as the rest of the language.

Those are basically the only two choice for making it possible to instantiate func F[T comparable]() with interface types - either you a) allow all interface types, or b) you introduce a special kind of interface type which restricts to comparable values.

If we have code in Go 1.18 that relies on func New[T comparable]() T can never return an empty interface, does this proposal mean a break of backward compatibility?

Not as stated. Because comparable would not be an empty interface. But if you can write code which relies on this being the empty interface, that code might also break if that function returns comparable. I can't really imagine how code would rely on that fact though. Do you have example code, which would demonstrate such an assumption?

@hherman1
Copy link

@hherman1 hherman1 commented Mar 31, 2022

@Merovius in the proposal where comparable is a specially defined interface, would it be possible to write a constrain such as:

`type ComparableIntf interface {
comparable
}

func Example[T ComparableIntf](T t) {…}`

@Merovius
Copy link

@Merovius Merovius commented Mar 31, 2022

@hherman1 Yes, but why? That's the same as just writing func Example[T comparable](T t) {…}.

@hherman1
Copy link

@hherman1 hherman1 commented Mar 31, 2022

@Merovius sorry, my example was incomplete. I meant something like this:

`type ComparableIntf interface {
comparable
OtherMethod()
}

func Example[T ComparableIntf](T t) {…}`

@Merovius
Copy link

@Merovius Merovius commented Mar 31, 2022

@hherman1 Yes, that's possible. It has been mentioned above a couple of times, see for example ComparableType in this comment.

@rittneje
Copy link

@rittneje rittneje commented Apr 12, 2022

Maybe the language spec should just change to define comparability for slices, maps, and functions?

  1. Slices are equal if they have the same slice header.
  2. Maps are equal if they are literally the same map, from the same call to make.
  3. Funcs are never equal to other funcs, or even themselves.

Then all types in Go are "comparable". Consequently any and comparable are the same thing, so the latter is essentially deprecated. (Or maybe it takes on new life as an alias of any that more directly conveys your intent.)

I haven't thought through any negative implications this might have, but it sidesteps the issues mentioned throughout this thread about the two flavors of "comparable".

@Merovius
Copy link

@Merovius Merovius commented Apr 12, 2022

@rittneje Non-comparable types where made non-comparable for a reason. I don't think generics change the equation on that significantly.

Porges added a commit to Azure/azure-service-operator that referenced this issue Apr 19, 2022
Based upon expanding the `StringSet` type. Note that we cannot substitute uses of `map[T]struct{}` where `T` is an interface because interface types are [not comparable](golang/go#51338), even though the `map[T]struct{}` version works.

`AreEqual` is a standalone function at the moment because making it a method [crashes the compiler](golang/go#51840).
@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Apr 21, 2022

Thanks for all the discussion. We've tried to address the most pressing issue on #52474. If that proposal is accepted, I will most likely close this one and recreate it in simpler form.

@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

@ianlancetaylor
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Apr 29, 2022

See #52614 for another take on this problem.

@Merovius
Copy link

@Merovius Merovius commented Apr 30, 2022

FWIW at this point, after all the discussion, I think I've fully come around to this proposal. I think my concerns are largely valid, but a) they happen relatively rarely, b) we might find ways to somewhat alleviate them and c) overall, after some transition pains, I think this leads to the most consistent language in the long term.

So, at least my personal opinion on this has changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests