Skip to content

text/template/parse: Unexpected Behaviour of ActionNode.Position in text/template - Potential Bug? #66423

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.

Sign up for GitHub

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

Jump to bottom
Closed
shallowclouds opened this issue Mar 20, 2024 · 7 comments
Closed

text/template/parse: Unexpected Behaviour of ActionNode.Position in text/template - Potential Bug? #66423

shallowclouds opened this issue Mar 20, 2024 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@shallowclouds
Copy link

Go version

go version go1.21.4 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxx/projects/gopath/pkg/mod'
GOOS='darwin'
GOPATH='/Users/xxx/projects/gopath'
GOROOT='/opt/homebrew/opt/go/libexec'
GOSUMDB='sum.golang.google.cn'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/opt/go/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/xxx/projects/xxx/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/mt/w_9vxxsn0ssfxz46n8lbrykh0000gp/T/go-build2022525700=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

https://go.dev/play/p/U8UabjbhJsN

// You can edit this code!
// Click here and start typing.
package main

import (
	"fmt"
	"reflect"
	"text/template"
)

func main() {
	temp := `{{.locale}}`
	tree, err := template.New("test").Parse(temp)
	if err != nil {
		panic(err)
	}

	node := tree.Root.Nodes[0]
	fmt.Println(reflect.TypeOf(node))
	// expect: {{.locale}} 0
	fmt.Println(node.String(), node.Position())
	// expect: {{.locale}}
	fmt.Println(temp[node.Position():])
}

What did you see happen?

I am currently developing a highlighter for text/template scripts. To accomplish this, I require an Abstract Syntax Tree (AST) for the provided template. During this process, I encountered an unexpected behaviour with the Position method of the ActionNode.

Contrary to my expectations, the Position of the ActionNode does not include the delimiter ('{{'). However, the String method does return the full {{.locale}}, inclusive of the delimiter. This discrepancy has prompted me to question whether there is a bug, or if I have misunderstood the purpose of the Position function in the Action interface.

According to the given documentation comment, Position() Pos should represent the 'byte position of the start of node in full original input string'. This, in my understanding, should include the delimiter.

Could you please clarify this discrepancy, or confirm if this is indeed a bug? Thank you for your time and assistance.

What did you expect to see?

temp := `{{.locale}}`
tree, err := template.New("test").Parse(temp)
if err != nil {
  panic(err)
}

node := tree.Root.Nodes[0]
fmt.Println(reflect.TypeOf(node))
// expect: {{.locale}} 0
fmt.Println(node.String(), node.Position())
// expect: {{.locale}}
fmt.Println(temp[node.Position():])
@dr2chase
Copy link
Contributor

@robpike, @mvdan, can you give this a look?

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 20, 2024
@robpike
Copy link
Contributor

robpike commented Mar 20, 2024

The action starts inside the delimiter, so the position field is correct. The String method includes the delimiter for ease of use. So yes, they are inconsistent and perhaps should be consistent, but it's more likely to break things than improve them to try to change this. Plus, you'd need to decide what change to make.

I think it can stay as is.

@shallowclouds
Copy link
Author

The action starts inside the delimiter, so the position field is correct. The String method includes the delimiter for ease of use. So yes, they are inconsistent and perhaps should be consistent, but it's more likely to break things than improve them to try to change this. Plus, you'd need to decide what change to make.

I think it can stay as is.

Thanks for replying, I understanded. May I ask why the position does not contains "{{ if" and other things, without these, it is nearly impossible to map nodes range to the original text for highlighter, and is not like other common ast parsers.

@robpike
Copy link
Contributor

robpike commented Mar 20, 2024

As I said, the action starts inside the delimiter, that's why.

@shallowclouds
Copy link
Author

shallowclouds commented Mar 21, 2024 •
edited
Loading

As I said, the action starts inside the delimiter, that's why.

Sorry I may not have made it clear, I mean if there should be a node wraps the action node but contains the delimiter and prefix such as "if" and "range" to make the AST nodes ranges cover all the original text.

@robpike
Copy link
Contributor

robpike commented Mar 21, 2024

It's impractical to change the parse tree given the compatibility promise. You'll just have to code around it, which shouldn't be too inconvenient.

@shallowclouds
Copy link
Author

It's impractical to change the parse tree given the compatibility promise. You'll just have to code around it, which shouldn't be too inconvenient.

Got it, thanks.

@golang golang locked and limited conversation to collaborators Mar 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@dr2chase @robpike @gopherbot @shallowclouds