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

text/template: make "eq" support all comparable types #33740

Closed
a8m opened this issue Aug 20, 2019 · 7 comments

Comments

@a8m
Copy link
Contributor

commented Aug 20, 2019

The current eq function is limited only to "basic types", and it does not support other Go types that are comparable, like pointers or structs.
Also, it's not possible to decorate it, because it's private. Setting "eq": reflect.DeepEqual won't provide the same functionally, because cases like this:

package main

import (
	"log"
	"os"
	"reflect"
	"text/template"
)

func main() {
	tmpl, err := template.New("titleTest").
		Funcs(template.FuncMap{
			"goeq": reflect.DeepEqual,
		}).
		Parse(`{{ eq .A 1 }} != {{ goeq .A 1 }}`)
	if err != nil {
		log.Fatalf("parsing: %s", err)
	}
	err = tmpl.Execute(os.Stdout, struct{ A int64 }{1})
	if err != nil {
		log.Fatalf("execution: %s", err)
	}
	// Output: true != false
}

https://play.golang.org/p/zTg33Lggq0f

My current solution is just copying the current code from text/template/funcs.go, and adding the support by myself.

I would like to hear what others think about this, because it feels like a basic functionality that is missing when working with Go types.

If this proposal will be accepted, I will be more than happy to contribute my changes.

@gopherbot gopherbot added this to the Proposal milestone Aug 20, 2019
@gopherbot gopherbot added the Proposal label Aug 20, 2019
@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

I'm not sure I get what you're proposing; the title seems to suggest Go's == for pointers and structs as per the spec, but your example uses reflect.DeepEqual, which is quite a bit more complex than Go's equality operator.

See https://golang.org/pkg/reflect/#DeepEqual and https://golang.org/ref/spec#Comparison_operators.

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Hey @mvdan, sorry for not being clear.
My proposal is about adding support for all comparable types in the builtin eq function (or at least pointers).

About the example I've added above (with reflect.DeepEqual) -
I tried to demonstrate that as a user of this package, it's not possible for me to decorate (or extend) the existing eq function and add support for other comparable types. The current solution is just to override it.
The quickest way that came to my head was to override it with reflect.DeepEqual, but that was wrong, because the 2 functions don't provide the same functionally as I showed above (eq has this "loose comparison" for numeric types).
So I ended up with copying the eq function from funcs.go and added the support in my code.

Is there any objection about adding this support? or, at least part of it (pointers only)? If not, I would like to contribute my changes.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@robpike

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

It's too bad that comparison of interface values panic if the types aren't comparable, and there is no way to avoid that bar catching the panic. If we were willing to catch the panic, which would be expensive but probably very rare in practice, we could just do return v1 == v2 in the default clause of the switch k1 case in the eq function.

That might be the right solution. It wouldn't require baking in lots of information about what types are comparable.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 6, 2019

Change https://golang.org/cl/193837 mentions this issue: text/template: support all comparable types in eq

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Given @robpike's comment above, this sounds like a likely accept.
Leaving open for a week for final comments before accepting.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

No comments, so proposal accepted.

@rsc rsc changed the title proposal: text/template: make "eq" support all comparable types text/template: make "eq" support all comparable types Sep 18, 2019
@rsc rsc modified the milestones: Proposal, Go1.14 Sep 18, 2019
@gopherbot gopherbot closed this in 95cbcc5 Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.