Found a bug using naml and vendoring valast 1.4.1

Looks like valast is now adding double {{ and }} in the valast.String output.

This is syntactically correct Go code from the compilers perspective. However this causes problems if this output is templated at all using Go's text/template package. The package reads the unusual literal coding as templating 🙁

Original output that caused the problem

panic: 
        // Adding a deployment: "{{ .KubeObject.Name }}"
        {{ .GoName }}Deployment := &v1.Deployment{
        TypeMeta: v1.TypeMeta{
                Kind:       "Deployment",
                APIVersion: "apps/v1",
        },
        ObjectMeta: v1.ObjectMeta{
                Name:        "nivenly",
                Namespace:   "public",
                Generation:  1,
                Labels:      map[string]string{"app": "nivenly"},
                Annotations: map[string]string{"deployment.kubernetes.io/revision": "1"},
        },
        Spec: v1.DeploymentSpec{
                Replicas: valast.Addr(int32(1)).(*int32),
                Selector: &v1.LabelSelector{MatchLabels: map[string]string{
                        "app": "nivenly",
                }},
                Template: v1.PodTemplateSpec{
                        ObjectMeta: v1.ObjectMeta{Labels: map[string]string{"app": "nivenly"}},
                        Spec: v1.PodSpec{
                                Volumes: []v1.Volume{{
                                        Name: "anchovies",
                                        VolumeSource: v1.VolumeSource{HostPath: &v1.HostPathVolumeSource{
                                                Path: "/data/anchovies",
                                        }},
                                }},
                                Containers: []v1.Container{{
                                        Name:  "nivenly",
                                        Image: "krisnova/nivenly.com:latest",
                                        Ports: []v1.ContainerPort{{
                                                ContainerPort: 1313,
                                                Protocol:      v1.Protocol("TCP"),
                                        }},
                                        Env: []v1.EnvVar{
                                                {
                                                        Name:  "BJORNODIR",
                                                        Value: "/public",
                                                },
                                                {
                                                        Name:  "BJORNO404PATH",
                                                        Value: "/public/404.html",
                                                },
                                                {
                                                        Name:  "BJORNO500PATH",
                                                        Value: "/public/404.html",
                                                },
                                                {
                                                        Name:  "BJORNO5XXPATH",
                                                        Value: "/public/404.html",
                                                },
                                                {
                                                        Name:  "BJORNOUSEDEFAULT",
                                                        Value: "false",
                                                },
                                        },
                                        VolumeMounts: []v1.VolumeMount{{
                                                Name:      "anchovies",
                                                MountPath: "/data/anchovies",
                                        }},
                                        TerminationMessagePath:   "/dev/termination-log",
                                        TerminationMessagePolicy: v1.TerminationMessagePolicy("File"),
                                        ImagePullPolicy:          v1.PullPolicy("Always"),
                                }},
                                RestartPolicy:                 v1.RestartPolicy("Always"),
                                TerminationGracePeriodSeconds: valast.Addr(int64(30)).(*int64),
                                DNSPolicy:                     v1.DNSPolicy("ClusterFirst"),
                                HostNetwork:                   true,
                                SecurityContext:               &v1.PodSecurityContext{},
                                SchedulerName:                 "default-scheduler",
                        },
                },
                Strategy: v1.DeploymentStrategy{
                        Type: v1.DeploymentStrategyType("RollingUpdate"),
                        RollingUpdate: &v1.RollingUpdateDeployment{
                                MaxUnavailable: &intstr.IntOrString{
                                        Type:   intstr.Type(1),
                                        StrVal: "25%",
                                },
                                MaxSurge: &intstr.IntOrString{
                                        Type:   intstr.Type(1),
                                        StrVal: "25%",
                                },
                        },
                },
                RevisionHistoryLimit:    valast.Addr(int32(10)).(*int32),
                ProgressDeadlineSeconds: valast.Addr(int32(600)).(*int32),
        },
}
        x.objects = append(x.objects, {{ .GoName }}Deployment)

        if client != nil {
                _, err = client.AppsV1().Deployments("{{ .KubeObject.Namespace }}").Create(context.TODO(), {{ .GoName }}Deployment, v1.CreateOptions{})
                if err != nil {
                        return err
                }
        }

Will be attaching recreation steps in just a moment

We're running into scenarios where the emitted code from valast is not valid due to missing types. We traced this down to the generated string representation not adding type information. This patch causes most emitted types to perform a typecast to seed the information properly to the compiler.

See #15 for details. Fixes #15

Signed-off-by: Frederick F. Kautz IV [email protected]

In naml, we need to know what packages are included in the AST so that we can handle imports properly.

This patch adds Result.Packages which informs the caller of AST about which packages the returned AST uses.

Signed-off-by: Frederick F. Kautz IV [email protected]

I was wondering if you could provide an option to disable gofumpt?

I use valast in tests and calling out to gofumpt slows them down quite a bit. I also don't need the newline capabilities, so I'd love to able to turn this off.

For reference, I'm currently using [email protected]. This version is perfect for me right now, but it'd be nice to stay up-to-date.

Stringifying a pointer to integer in a struct yields incorrect results. Here is an example for int64.

simple use case works

	i64 := int64(3607)
	test_str := String(&i64)

yields

valast.Addr(int64(3607)).(*int64)

nested in struct results in invalid output

	i64 := int64(3607)
	got := str{
		ExpirationSeconds: &i64,
	}

	test_str := String(got)

yields

valast.str{ExpirationSeconds: valast.Addr(3607).(*int64)}

This breaks for other integer types too.

Hello! Thanks for you job!

As recommended we include you package by specifying tag:
require ( github.com/hexops/valast v1.4.0 ) but github.com/hexops/valast/go.mod in the tag v1.4.0 requires: mvdan.cc/gofumpt v0.0.0-20210107193838-d24d34e18d44

the d24d34e18d44 cannot be found. it seems dependency bug was fixed in the master go.mod: mvdan.cc/gofumpt v0.1.1

Could you please release another tag v1.4.x with correct line in go.mod? Thank you

Regards, Robert

I'm not sure how this happened, but it looks like the current version of gofumpt is available on the default proxy https://proxy.golang.org, but not directly.

You can test this by running the valast tests using the following command:

GOMODCACHE=/tmp/gomodcache GOPROXY=direct go test ./...

This will setup a local empty cache and directly hit mvdan.cc/gofumpt which I guess will hit github. You should see the following error:

go: mvdan.cc/[email protected]: invalid version: unknown revision d24d34e18d44

For this PR, I just bumped gofumpt to v0.1.1 and ran the tests again with the environment variables above. This appears to have fixed the problem!

This is a hack (but a quite functional one) to split composite literals across multiple lines to avoid extra long struct values - which are a major problem with Valast today and e.g. block me from releasing the autogold package.

We will defer this to gofumpt once it can perform this functionality.

Signed-off-by: Stephen Gutekanst [email protected]

go fmt does nothing to handle very-long-lines, of which valast will often produce due to it being a generated AST.

gofumpt is a stricter version of gofmt, which does a much better job of formatting generated code than gofmt, and has in-progress work for formatting very-long-lines.

This PR changes the default String and StringOptions output of valast to always use gofumpt formatting.

With complex data types, this can provide a 66% performance improvement.

Before:

$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       2	 564812395 ns/op
PASS
ok  	github.com/hexops/valast	9.372s

After:

$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       6	 189646854 ns/op
PASS
ok  	github.com/hexops/valast	6.379s

In practice this can result in tests running much faster, before:

--- PASS: TestUnionMerge (6.38s)
    --- PASS: TestUnionMerge/#00 (1.42s)
    --- PASS: TestUnionMerge/#01 (1.17s)
    --- PASS: TestUnionMerge/#02 (2.32s)
    --- PASS: TestUnionMerge/#03 (0.00s)
    --- PASS: TestUnionMerge/#04 (0.00s)
    --- PASS: TestUnionMerge/#05 (1.46s)

After:

--- PASS: TestUnionMerge (2.96s)
    --- PASS: TestUnionMerge/#00 (1.05s)
    --- PASS: TestUnionMerge/#01 (0.77s)
    --- PASS: TestUnionMerge/#02 (0.77s)
    --- PASS: TestUnionMerge/#03 (0.00s)
    --- PASS: TestUnionMerge/#04 (0.00s)
    --- PASS: TestUnionMerge/#05 (0.37s)

Also appears to fix https://github.com/hexops/autogold/issues/16

Signed-off-by: Stephen Gutekanst [email protected]

This benchmark is indicative of the performance issue reported by a user in https://github.com/sourcegraph/sourcegraph/pull/18189

$ go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/hexops/valast
BenchmarkComplexType-16    	       2	 564812395 ns/op
PASS
ok  	github.com/hexops/valast	9.372s

Signed-off-by: Stephen Gutekanst [email protected]

I like this :-)

However, currently I am building an app based on a list of records where each record has several time.Time fields. Turns out dumping such fields produces a very long output. I am not interested in the value of those fields and sadly I cannot easily remove them.

An option to disable, override or shorting the time.Time value would be much appreciated :-)

This is just one field:

UpdatedAt: &time.Time{ wall: 1339000, ext: 63784765750, loc: &time.Location{ name: "Local", zone: []time.zone{ { name: "LMT", offset: -2205, }, { name: "WEST", offset: 3600, isDST: true, }, {name: "WET"}, { name: "WEST", offset: 3600, isDST: true, }, {name: "WET"}, { name: "WEMT", offset: 7200, isDST: true, }, {name: "WET"}, { name: "CET", offset: 3600, }, { name: "CET", offset: 3600, }, { name: "CEST", offset: 7200, isDST: true, }, { name: "WEST", offset: 3600, isDST: true, }, {name: "WET"}, }, tx: []time.zoneTrans{ { when: -1830384000, index: 6, }, { when: -1689555600, index: 1, }, { when: -1677801600, index: 2, }, { when: -1667437200, index: 3, isstd: true, }, { when: -1647738000, index: 4, isstd: true, }, { when: -1635814800, index: 3, isstd: true, }, { when: -1616202000, index: 4, isstd: true, isutc: true, }, { when: -1604365200, index: 3, }, { when: -1584666000, index: 4, isstd: true, isutc: true, }, { when: -1572742800, index: 3, isstd: true, isutc: true, }, { when: -1553043600, index: 4, isstd: true, isutc: true, }, { when: -1541206800, index: 3, isstd: true, isutc: true, }, { when: -1521507600, index: 4, }, { when: -1442451600, index: 3, }, { when: -1426813200, index: 4, }, { when: -1379293200, index: 3, }, { when: -1364778000, index: 4, }, { when: -1348448400, index: 3, }, { when: -1333328400, index: 4, }, { when: -1316394000, index: 3, }, { when: -1301274000, index: 4, }, { when: -1284339600, index: 3, }, { when: -1269824400, index: 4, }, { when: -1221440400, index: 3, }, { when: -1206925200, index: 4, }, { when: -1191200400, index: 3, }, { when: -1175475600, index: 4, }, { when: -1127696400, index: 3, }, { when: -1111971600, index: 4, }, { when: -1096851600, index: 3, }, { when: -1080522000, index: 4, }, { when: -1063587600, index: 3, }, { when: -1049072400, index: 4, }, { when: -1033347600, index: 3, }, { when: -1017622800, index: 4, }, { when: -1002502800, index: 3, }, { when: -986173200, index: 4, }, { when: -969238800, index: 3, }, { when: -950490000, index: 4, }, { when: -942022800, index: 3, }, { when: -922669200, index: 4, }, { when: -906944400, index: 3, }, { when: -891133200, index: 4, }, { when: -877309200, index: 3, }, { when: -873684000, index: 5, }, { when: -864007200, index: 3, }, { when: -857955600, index: 4, }, { when: -845859600, index: 3, }, { when: -842839200, index: 5, }, { when: -831348000, index: 3, }, { when: -825901200, index: 4, }, { when: -814410000, index: 3, }, { when: -810784800, index: 5, }, { when: -799898400, index: 3, }, { when: -794451600, index: 4, }, { when: -782960400, index: 3, }, { when: -779335200, index: 5, }, { when: -768448800, index: 3, }, { when: -763002000, index: 4, }, { when: -749091600, index: 3, }, { when: -733366800, index: 4, }, { when: -717631200, index: 3, }, { when: -701906400, index: 4, }, { when: -686181600, index: 3, }, { when: -670456800, index: 4, }, { when: -654732000, index: 3, }, { when: -639007200, index: 4, }, { when: -623282400, index: 3, }, { when: -607557600, index: 4, }, { when: -591832800, index: 3, }, { when: -575503200, index: 4, }, { when: -559778400, index: 3, }, { when: -544053600, index: 4, }, { when: -528328800, index: 3, }, { when: -512604000, index: 4, }, { when: -496879200, index: 3, }, { when: -481154400, index: 4, }, { when: -465429600, index: 3, }, { when: -449704800, index: 4, }, { when: -433980000, index: 3, }, { when: -417650400, index: 4, }, { when: -401925600, index: 3, }, { when: -386200800, index: 4, }, { when: -370476000, index: 3, }, { when: -354751200, index: 4, }, { when: -339026400, index: 3, }, { when: -323301600, index: 4, }, { when: -307576800, index: 3, }, { when: -291852000, index: 4, }, { when: -276127200, index: 3, }, { when: -260402400, index: 4, }, { when: -244677600, index: 3, }, { when: -228348000, index: 4, }, { when: -212623200, index: 3, }, { when: -196898400, index: 4, }, { when: -181173600, index: 3, }, { when: -165448800, index: 4, }, { when: -149724000, index: 3, }, { when: -133999200, index: 4, }, { when: -118274400, index: 7, }, { when: 212544000, index: 2, }, { when: 228268800, index: 3, }, { when: 243993600, index: 4, }, { when: 260323200, index: 3, }, { when: 276048000, index: 4, }, { when: 291772800, index: 3, }, { when: 307501200, index: 4, }, { when: 323222400, index: 3, }, { when: 338950800, index: 4, }, { when: 354675600, index: 3, }, { when: 370400400, index: 4, }, { when: 386125200, index: 3, }, { when: 401850000, index: 4, }, { when: 417578400, index: 3, }, { when: 433299600, index: 4, }, { when: 449024400, index: 3, }, { when: 465354000, index: 4, }, { when: 481078800, index: 3, }, { when: 496803600, index: 4, }, { when: 512528400, index: 3, }, { when: 528253200, index: 4, }, { when: 543978000, index: 3, }, { when: 559702800, index: 4, }, { when: 575427600, index: 3, }, { when: 591152400, index: 4, }, { when: 606877200, index: 3, }, { when: 622602000, index: 4, }, { when: 638326800, index: 3, }, { when: 654656400, index: 4, }, { when: 670381200, index: 3, }, { when: 686106000, index: 4, }, { when: 701830800, index: 3, }, { when: 717555600, index: 8, }, { when: 733280400, index: 9, }, { when: 749005200, index: 8, }, { when: 764730000, index: 9, }, { when: 780454800, index: 8, }, { when: 796179600, index: 9, }, { when: 811904400, index: 8, }, { when: 828234000, index: 10, }, { when: 846378000, index: 6, }, { when: 859683600, index: 10, }, { when: 877827600, index: 6, }, { when: 891133200, index: 10, }, { when: 909277200, index: 6, }, { when: 922582800, index: 10, }, { when: 941331600, index: 6, }, { when: 954032400, index: 10, }, { when: 972781200, index: 6, }, { when: 985482000, index: 10, }, { when: 1004230800, index: 6, }, { when: 1017536400, index: 10, }, { when: 1035680400, index: 6, }, { when: 1048986000, index: 10, }, { when: 1067130000, index: 6, }, { when: 1080435600, index: 10, }, { when: 1099184400, index: 6, }, { when: 1111885200, index: 10, }, { when: 1130634000, index: 6, }, { when: 1143334800, index: 10, }, { when: 1162083600, index: 6, }, { when: 1174784400, index: 10, }, { when: 1193533200, index: 6, }, { when: 1206838800, index: 10, }, { when: 1224982800, index: 6, }, { when: 1238288400, index: 10, }, { when: 1256432400, index: 6, }, { when: 1269738000, index: 10, }, { when: 1288486800, index: 6, }, { when: 1301187600, index: 10, }, { when: 1319936400, index: 6, }, { when: 1332637200, index: 10, }, { when: 1351386000, index: 6, }, { when: 1364691600, index: 10, }, { when: 1382835600, index: 6, }, { when: 1396141200, index: 10, }, { when: 1414285200, index: 6, }, { when: 1427590800, index: 10, }, { when: 1445734800, index: 6, }, { when: 1459040400, index: 10, }, { when: 1477789200, index: 6, }, { when: 1490490000, index: 10, }, { when: 1509238800, index: 6, }, { when: 1521939600, index: 10, }, { when: 1540688400, index: 6, }, { when: 1553994000, index: 10, }, { when: 1572138000, index: 6, }, { when: 1585443600, index: 10, }, { when: 1603587600, index: 6, }, { when: 1616893200, index: 10, }, { when: 1635642000, index: 6, }, { when: 1648342800, index: 10, }, { when: 1667091600, index: 6, }, { when: 1679792400, index: 10, }, { when: 1698541200, index: 6, }, { when: 1711846800, index: 10, }, { when: 1729990800, index: 6, }, { when: 1743296400, index: 10, }, { when: 1761440400, index: 6, }, { when: 1774746000, index: 10, }, { when: 1792890000, index: 6, }, { when: 1806195600, index: 10, }, { when: 1824944400, index: 6, }, { when: 1837645200, index: 10, }, { when: 1856394000, index: 6, }, { when: 1869094800, index: 10, }, { when: 1887843600, index: 6, }, { when: 1901149200, index: 10, }, { when: 1919293200, index: 6, }, { when: 1932598800, index: 10, }, { when: 1950742800, index: 6, }, { when: 1964048400, index: 10, }, { when: 1982797200, index: 6, }, { when: 1995498000, index: 10, }, { when: 2014246800, index: 6, }, { when: 2026947600, index: 10, }, { when: 2045696400, index: 6, }, { when: 2058397200, index: 10, }, { when: 2077146000, index: 6, }, { when: 2090451600, index: 10, }, { when: 2108595600, index: 6, }, { when: 2121901200, index: 10, }, { when: 2140045200, index: 6, }, }, extend: "WET0WEST,M3.5.0/1,M10.5.0", cacheStart: 1648342800, cacheEnd: 1667091600, cacheZone: &time.zone{ name: "WEST", offset: 3600, isDST: true, }, },

Consider the following (minified) example program that tries to print a value of type font.Metrics:

package main

import (
	"fmt"
	"log"

	"github.com/golang/freetype"
	"github.com/golang/freetype/truetype"
	"github.com/hexops/valast"
	"golang.org/x/image/font/gofont/gomono"
)

func main() {
	f, err := freetype.ParseFont(gomono.TTF)
	if err != nil {
		log.Fatalln(err)
	}

	face := truetype.NewFace(f, &truetype.Options{
		Size:              12,
		GlyphCacheEntries: 0,
		SubPixelsX:        1,
		SubPixelsY:        1,
	})
	fmt.Println("metrics =", valast.String(face.Metrics()))

	// Output:
	// metrics = err: go command required, not found: exec: "go": executable file not found in $PATH: stderr: 
}

(https://play.golang.org/p/6nQBdwaHo5q)

It prints:

metrics = err: go command required, not found: exec: "go": executable file not found in $PATH: stderr: 

When executed in an environment where an executable go is unavailable, such as inside a browser via WebAssembly (GOOS=js GOARCH=wasm), or inside the Go Playground.

It's possible to provide a custom PackagePathToName function via an option, such as:

fmt.Println("metrics =", valast.StringWithOptions(face.Metrics(), &valast.Options{
	PackagePathToName: func(path string) (string, error) { return pathpkg.Base(path), nil }, // TODO: Handle paths like example.com/foo/v2, where the name is 'foo' not 'v2'.
}))

Then it doesn't get the aforementioned error.

Is this intended behavior for String, or should valast know that GOOS=js GOARCH=wasm environment cannot execute a go binary and should automatically use a different PackagePathToName implementation, so that it's possible to use String?

Note that both spew.Dump(face.Metrics()) and goon.Dump(face.Metrics()) print an output without an error inside a browser.

time.Duration will cause ( missing ',' in argument list ) error i wrote a hack and it can work well. change method gofumptFormatExpr

func gofumptFormatExpr(w io.Writer, fset *token.FileSet, expr ast.Expr, opt gofumpt.Options) error {
	// First use go/format to convert the expression to Go syntax.
	var tmp bytes.Buffer
	if err := format.Node(&tmp, fset, expr); err != nil {
		return err
	}
	rs := []rune(tmp.String())
	var firstNum int
	for i, v := range rs {
		if v == '(' {
			if rs[i+1] >= '0' && rs[i+1] <= '9' {
				firstNum = i + 1
				break
			}
		}
	}
	if firstNum > 0 {
		rs0 := rs[:firstNum]
		rs1 := rs[firstNum:]
		var hasChar bool
		for _, v := range rs1 {
			if v < '0' || v > '9' {
				hasChar = true
				break
			}
		}
		if hasChar {
			rs1 = append([]rune{'_'}, rs1...)
		}
		rs = append(rs0, rs1...)
	}
	tmpString := string(formatCompositeLiterals(rs))

Today valast uses a dumb heuristic to determine if a string should be formatting as a Go "string literal" or `raw string literal`:

		if len(s) > 40 && strings.Contains(s, "\n") && !strings.Contains(s, "`") {
			return basicLit(vv, token.STRING, "string", "`"+s+"`", opt)
		}
		return basicLit(vv, token.STRING, "string", strconv.Quote(v.String()), opt)

No doubt there are cases where the formatting provided here will be less than optimal. Problematic cases include:

• Very long single-line strings.
• Multi-line strings with long lines.
• Strings with lots of unicode where escape sequences (or not) may be more desirable.
• ...

The goal of this issue is to find out how we can improve the default formatting to meet most use cases. It would then additionally be nice to have the ability for users of the package to provide a string for matter of their own, but we should do this after exhausting possibilities of improving the default.

Map keys are sorted according to regular Go rules, but a number of Go values cannot be sorted according to those rules. As it stands, valast will produce unstable output in such situations.

It would be nice to find a better way to resolve this. Feedback on how to handle this very welcome.

• Panic or error when such a map key is encountered, ask the user provide a comparison function
• See if go-cmp does anything for this - I suspect they don't have to because they can just compare map keys for equality
• Take into consideration the drawbacks and benefits with the go-spew approach, output not being sorted by default, sorting by spewed keys is interesting but may have edge cases that are not handled, etc.