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: wrap errors returned by custom template functions #34201

Open
snowcall opened this issue Sep 10, 2019 · 9 comments
Open

text/template: wrap errors returned by custom template functions #34201

snowcall opened this issue Sep 10, 2019 · 9 comments

Comments

@snowcall
Copy link

@snowcall snowcall commented Sep 10, 2019

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

go version go1.13 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\User\AppData\Local\go-build
set GOENV=C:\Users\User\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\Go
set GOPRIVATE=
set GOPROXY=direct
set GOROOT=C:\Go\go1.13
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Go\go1.13\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=E:\Projects\test\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\User\AppData\Local\Temp\go-build112356450=/tmp/go-build -gno-record-gcc-switches

What did you do?

I'm trying to get the error returned by my custom template function. The error is a struct implementing the error interface.

Execute returns template.ExecError and despite the fact that it implements Unwrap() error, calling Unwrap returns *errors.errorsString, not my error implementation.

Thus, there is no way to get the original error.

Code:

package main

import (
	"errors"
	"fmt"
	"os"
	"text/template"
)

type MyError struct {
	Value int
}

func (e *MyError) Error() string { return "error message" }

func test(val int) (string, error) {
	return "", &MyError{
		Value: val,
	}
}

func main() {
	t := template.Must(template.New("test").Funcs(template.FuncMap{
		"test": test,
	}).Parse("{{ test 10 }}"))

	err := t.Execute(os.Stdout, nil)

	if err != nil {
		var e *MyError
		fmt.Println(errors.As(err, &e)) // <-- false
	}
}

What did you expect to see?

true

What did you see instead?

false

Additional notes

  1. Documentation for template.FuncMap states that:

if the second (error) return value evaluates to non-nil during execution, execution terminates and Execute returns that error

  1. The same applies for method calls like {{ .Object.Method }}.

  2. Both text/template and html/template are affected.

@snowcall

This comment has been minimized.

Copy link
Author

@snowcall snowcall commented Sep 10, 2019

Originally posted by @jba in #34179 (comment)

For that new issue: I can confirm that at least since 1.5, the Funcs doc comment says "Execute returns that error " (emphasis mine), and also that it does not in fact do so.

@tmthrgd

This comment has been minimized.

Copy link
Contributor

@tmthrgd tmthrgd commented Sep 10, 2019

I believe this is caused by this line using %v not %w:

s.errorf("error calling %s: %v", name, err)

I'm happy to open a CL for this.

@toothrot toothrot changed the title text/template: Execute hides errors returned by custom template functions text/template: wrap errors returned by custom template functions Sep 16, 2019
@toothrot toothrot added this to the Unreleased milestone Sep 16, 2019
@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Sep 16, 2019

Thanks for filing the updated issue, @snowcall.

The "errors" package (and corresponding %w wrap) is new in the standard library, and has only been used selectively so far in the standard library. I believe that documentation copy (execution terminates and Execute returns that error) predates error wrapping.

I'm cc'ing the owners of text/template who can provide some more context.

/cc @mvdan @robpike

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Oct 8, 2019

I think this should be a proper proposal to change text/template to use error wrapping. The proposal should explain what scenarios would start wrapping errors and why.

@tj

This comment has been minimized.

Copy link

@tj tj commented Dec 11, 2019

I ran into this as well, I need to get my error back from a template but you can't unwrap this &errors.errorString{s:"template: :4:6:... it's returning. IMO it should be all or nothing in the stdlib, it was really jarring and unintuitive to see that it only works with some errors.

In my case I have a {{compile .Query}}, and I need to pass back the user-friendly parse error, not the text/template error.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Dec 11, 2019

@tj sorry, do you mean that error wrapping is inconsistent within text/template itself? If so, we should definitely fix that sooner than later. I'd be happy to review a patch.

If you mean with other packages in the standard library, I think it's expected that it will take a few releases for each one of them to be adapted to error wrapping.

@tj

This comment has been minimized.

Copy link

@tj tj commented Dec 11, 2019

@mvdan I'm not sure if text/template wraps in other scenarios, but in this case the {{compile .Query}} is returning a errors.errorString, so I can't get access to the underlying parser/compiler error, which is more human-friendly for customers than:

template: :4:6: executing \"\" at <compile .Query>: error calling compile: parsing query: Parse error near \"\\\"ping and host = \\\"\""

For now I'll just parse it out of the string 😆easier than moving compile() outside of all the templates.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Dec 11, 2019

Yeah, I think the package just hasn't been adapted for error wrapping at all. It's certainly something we can look at for 1.15, but as you can imagine, that's eight months away from being generally available. Changing the types of errors is just far too risky to do during code freeze.

If anyone wants to take a stab at this work, feel free. Otherwise I might get started once the tree reopens for 1.15. I think the biggest part of the work will be figuring out what APIs should start using error wrapping, and making sure that no tests are broken.

@tj

This comment has been minimized.

Copy link

@tj tj commented Dec 11, 2019

Fair enough! I didn't realize they weren't mostly converted when the wrapping was introduced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.