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: add comparable w/o interfaces #49587

Closed
Code-Hex opened this issue Nov 15, 2021 · 17 comments
Closed

proposal: spec: add comparable w/o interfaces #49587

Code-Hex opened this issue Nov 15, 2021 · 17 comments
Labels
generics Proposal
Projects
Milestone

Comments

@Code-Hex
Copy link

@Code-Hex Code-Hex commented Nov 15, 2021

What do I want?

  • I want a new comparable type that does not include interface{}.

Background

#49584

  • The current comparable type includes interface{}.
  • If an interface{} is passed to the function, we need to determine if it is comparable at runtime using reflect package. (to avoid panic)

I think comparable type which can only be determined at runtime, is almost the same as any type.

generics

import "reflect"

func Equal[T comparable](v1, v2 T) bool {
	if !reflect.TypeOf(v1).Comparable() {
		return false
	}
	if !reflect.TypeOf(v2).Comparable() {
		return false
	}
	return v1 == v2
}

func main() {
	v1 := interface{}(func() {})
	v2 := interface{}(func() {})
	Equal(v1, v2)
}

non-generics

import "reflect"

func Equal(v1, v2 interface{}) bool {
	if !reflect.TypeOf(v1).Comparable() {
		return false
	}
	if !reflect.TypeOf(v2).Comparable() {
		return false
	}
	return v1 == v2
}

func main() {
	v1 := interface{}(func() {})
	v2 := interface{}(func() {})
	Equal(v1, v2)
}
@gopherbot gopherbot added this to the Proposal milestone Nov 15, 2021
@dsnet
Copy link
Member

@dsnet dsnet commented Nov 15, 2021

Wouldn't this need to exclude any interface type, not just interface{}?

All interfaces are considered comparable:

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.

@AndrewHarrisSPU
Copy link

@AndrewHarrisSPU AndrewHarrisSPU commented Nov 15, 2021

I feel like there's some really useful headroom where preferring the proposedEqualFunc over Equal: https://gotipplay.golang.org/p/GzTn8g4jz8N

This approach doesn't require reflection and does set the compiler up to be useful. It doesn't prohibit accidents with Equal, though.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 15, 2021
@ianlancetaylor ianlancetaylor added the generics label Nov 15, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 16, 2021

I believe that writing a generic function that uses the comparable constraint, and then instantiating that function using an interface type, is not going to be a common case. You've already shown how code can handle that case explicitly when it matters. It's not clear to me that this is important enough to require a language change.

@ianlancetaylor ianlancetaylor changed the title proposal: Add another comparable type which is excluded interface{} type proposal: add another comparable constraint that excludes interface types Nov 16, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: add another comparable constraint that excludes interface types proposal: add another constraint like comparable but excluding interface types Nov 16, 2021
@Code-Hex
Copy link
Author

@Code-Hex Code-Hex commented Nov 16, 2021

Thanks, everyone.

If a provider of a third-party package does some kind of validation using comparable, I think a provider needs to consider interface{}. However, I understand that this is not a common case.

I have noticed something while writing this proposal. I thought that the real problem was that Go not provide a type that could not be configured no matter how hard Go user tried.

For example, a comparable struct.

If this type is provided, users who have the same problem as I do will be able to write their own definitions. For example, let's call it comparablestruct.

type NonInterfaceComparable interface {
	constraints.Integer | constraints.Float | constraints.Complex | ~string | comparablestruct
}

What do you all think?

@syumai
Copy link
Contributor

@syumai syumai commented Nov 16, 2021

I've considered NonInterfaceComparable in the comment (#49587 (comment)).
To implement this idea, we would need not only comparablestruct, but also comparablearray.
Using this, the interface definition can be written as follows

type NonInterfaceComparable[T any] interface {
	~bool |
		constraints.Integer |
		constraints.Float |
		constraints.Complex |
		~string |   // String values
		~*T |       // Pointer values
		~chan T |   // Channel values
		~<-chan T | // Receive only channel values
		~chan<- T | // Send only channel values
		comparablestruct |
		comparablearray
}

To represent pointers and channels, this interface requires a type argument.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

We should use the generics changes for a while before we decide we need two different kinds of comparable. I suggest we decline this and return in a few years if the need is still strongly felt.

@rsc rsc changed the title proposal: add another constraint like comparable but excluding interface types proposal: spec: add comparable w/o interfaces Dec 1, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

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 Dec 1, 2021
@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Dec 2, 2021

@ianlancetaylor

I believe that writing a generic function that uses the comparable constraint, and then instantiating that function using an interface type, is not going to be a common case. You've already shown how code can handle that case explicitly when it matters. It's not clear to me that this is important enough to require a language change.

To address your second point first: it's not quite as simple as that. The code there can still panic when one argument is nil. Getting it right is more awkward and has more runtime overhead: https://gotipplay.golang.org/p/hJQ1TzxVeLl; in fact it's still not right.

With respect to your first point, I'm not so sure. One pattern that I've seen emerging multiple times so far in the generic Go code that I've written is that of hiding type parameters behind interfaces (sometimes known as "type erasure", I believe). This pattern makes it possible to take a value with type parameters and put it inside a heterogeneous collection such as a slice or map. Here's a simple example: https://gotipplay.golang.org/p/WgMcZIkVxPs .

In general, given a type T with type parameter P constrained by interface C, when C is usable as a non-constraint type, it seems to be possible to write a transformation from T[P] to T[C], and that's actually quite a useful property.

Unfortunately, when C contains a comparable constraint, we can't apply the above transformation because comparable isn't usable as a non-constraint type. It's possible to work around this by defining an auxiliary interface type, say C1, which contains all the methods of C but not the comparable constraint, then define C as:

type C interface {
     C1
     comparable
}

and then we can write a transformation from T[P] to T[C1] because values of type C1 satisfy the constraint C because all interfaces are comparable.

My initial thought when I first came across this this was that we should just make comparable usable as a regular interface value. After all, all interfaces are comparable, so adding a comparable element to an interface wouldn't change its semantics at all.

Then I realised that perhaps there's a more interesting potential semantic here that could be introduced without loss of backward compatibility: we could make a comparable constraint on an interface only admit types that are actually comparable. So comparing two interface types that both embed comparable would be guaranteed not to panic.

There are various places in the standard library that would probably have used this constraint if they could (for example the key value provided to context.WithValue), and it would provide a way of assuring that some particular type could always be compared without a panic, which is hard to do currently. The only straightforward way of doing so is to catch the resulting panic.

The only problem that I can see with allowing comparable to mean "always comparable without panic" within regular interface types is that comparable within a type parameter constraint does not mean that currently, because comparable as a type parameter constraint does allow regular interface values which might panic when compared.

I propose that we leave the above possibility open without committing to it by tightening the current type parameter proposal so that it does not consider interface types to be comparable. For example, this code would not compile:

func main() {
	fmt.Println(Equal[interface{}](1, "hello"))   // interface{} does not satisfy comparable
}

func Equal[T comparable](a, b T) bool {
	return a == b
}

If at some future date we allow comparable in regular interface types, you'd be able to write this instead:

	fmt.Println(Equal[comparable](1, "hello"))

With respect to this proposal, I'm suggesting that instead of adding a new kind of comparable type, we can change the existing comparable constraint to mean what it says, and that we restrict the current type parameters implementation so that we can do this in the future without breaking backward compatibility.

@Code-Hex
Copy link
Author

@Code-Hex Code-Hex commented Dec 4, 2021

@rogpeppe Thanks for the detailed explanation.

My motivation for this proposal was to question the possibility of including any in comparable. This is absolutely misleading.

This is an example of any can be passed to comparable.

https://gotipplay.golang.org/p/ukN96RW8Pui

I also often see code that returns an interface as a return value and does something with the result.

In this example, we return []bytes and string with some kind of interface{} (Here I'm using fmt.Stringer). We want to do comparisons within a function, so we specify comparable as a type parameter. The program is compiled. So we are convinced that it works, but in fact, it panics at runtime and terminates abnormally. I assume that a case similar to this will occur.

https://gotipplay.golang.org/p/f3LMqLbIRcx


I would be happy if comparable did not include any. Looking back at the process so far, it seems that there are several options.

  1. Define a new comparable without any
  2. Provide comparable for struct and array. And users can define comparable without any
  3. Modify the existing comparable without any

if we choose the third option, I think this should be in time for the release of Generics. Because it's related to maintaining backward compatibility.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 4, 2021

@Code-Hex Looking specifically at any seems like a bit of a sidetrack. The question is whether interface types in general should be considered as implementing comparable. If we change comparable, then people will want "comparable plus interface types," which there is no way to write. So no matter what, if we think that this is an important issue, we need two different kinds of comparable. Given that, the big advantage of the current definition is that it matches how the rest of the language behaves. You can use == with interface types. If we only have one kind of comparable it would be strange if it did not work the same way.

Changing comparable to only apply to struct and array types doesn't help because we would still have to decide what to do about [3]fmt.Stringer.

So let's wait and see whether this is a real problem in practice before we introduce a new kind of comparable.

@Code-Hex
Copy link
Author

@Code-Hex Code-Hex commented Dec 4, 2021

@ianlancetaylor Thank you for your answer. And sorry about that. I was confused because any is a type alias for interface{}.

Looking specifically at any seems like a bit of a sidetrack. The question is whether interface types in general should be considered as implementing comparable.

I agree with you on this point as well. I also think that adding a new comparable is a better way if deal with this problem.

the big advantage of the current definition is that it matches how the rest of the language behaves.

I agree with rsc's opinion. Because I can't think of anything that would really be a problem now.

I suggest we decline this and return in a few years if the need is still strongly felt.

However, it would obviously be helpful to have a comparable without interfaces. I believe some of the upvotes are of the same opinion.

I've been experimenting with generics in this repository. It is intended to be provided as a library. The comparable in this repository has no reason to accept interfaces. So I thought it would be nice if it could be detected at compile-time.

@Merovius
Copy link

@Merovius Merovius commented Dec 5, 2021

@Code-Hex

I've been experimenting with generics in this repository. It is intended to be provided as a library. The comparable in this repository has no reason to accept interfaces.

Why? From what I can tell this is a generic caching library. It seems very useful to me, to instantiate a cache using interface-types. For example, I might have an encoding library, which caches encoders for reflect.Type (which is an interface-type). They are comparable, so nothing would panic. I don't have a concrete type to put in.

IMO your specific use-case should just use comparable and let the user decide, if they want to instantiate it with an interface-type or not. And if they do and then pass a non-comparable dynamic value, your library should just panic - because passing such a value is a bug. It shouldn't treat them as unequal using reflect-workarounds because that's the wrong answer. Treating them as unequal leads to strange, unexpected behaviors. In the case of a caching-library, it would mean every lookup of an incomparable value would be interpreted as a cache-miss, which needs back-filling, in the worst case making the cache completely pointless, as it's fully filled with unusable values. That's clearly not right.

Obviously it would be preferable (all things being equal) to be able to catch buggy usage of your library statically. But that's just not how the language at large works. The panic you generate is akin to the panic generated by using an incomparable value as a key in a map[interface{}]T. It's not great we can't detect that statically - but Go just doesn't try to catch all bugs statically.

@Code-Hex
Copy link
Author

@Code-Hex Code-Hex commented Dec 5, 2021

@Merovius Thanks. This is because interfaces are not statically guaranteed to be comparable. I thought it would be best if the compiler could guarantee that only comparable interfaces would come across (like ~comparableliterals | ~interface{ comparable }.

Unfortunately, it is not. (it's acceptable like interface{}(func() {})) However, I thought it was inevitable because it was in accordance with the language specification. So I thought that if there was a comparable without an interface, I could enforce the usage to avoid runtime panic of the library.

Why?

As I write this message, I am thinking that what I really wanted was what @rogpeppe said. I know this is a bit off-topic, but does the feasibility exist for this?

I propose that we leave the above possibility open without committing to it by tightening the current type parameter proposal so that it does not consider interface types to be comparable.

@Merovius
Copy link

@Merovius Merovius commented Dec 5, 2021

I'm not sold on the idea of comparable being a normal interface. It feels an awful lot like a color to me.
As an example, if we did that and you used it in your library, I could no longer use it for my encoding example, as reflect.Type does not embed comparable. Of course, reflect.Type does know that all its valid implementations are comparable, so it might embed it. But other interface-types might not. So every interface type now has to decide if it

  1. Embeds comparable, disallowing third-party implementations using non-comparable values (which is a significant restriction, implementations using func are common)
  2. Does not embed comparable, making it unusable with any generic function which uses it as a constraint even if the actual used types all are comparable.

I'd guess we would end up with a lot of type ComparableReader interface { io.Reader; comparable } kind of interfaces, for the sole purpose of using a pre-existing interface-type with a library which uses comparable constraints.

Then there's also the question Ian brings up here - what about composite types? Currently they are comparable and we can't break that. But for this idea to be effective, we definitely would need composite types to not be assignable to comparable (or fulfill the comparable constraint), if not all interface types they use (recursively) are comparable themselves. So, comparable doesn't actually mean "the type is comparable" anymore.

Personally, I don't think this is a huge problem, TBH. Note that we've been living with panics when non-comparable dynamic types are compared since Go 1.0 and I don't think it's an issue that comes up terribly often. I don't really see how this is going to be much different with the advent of generics.

@rsc rsc moved this from Active to Likely Decline in Proposals Dec 8, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 8, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals Dec 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 15, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@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 Proposal
Projects
Proposals
Declined
Development

No branches or pull requests

9 participants