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

String function is not viable in environments where executable go is unavailable #18

Open
dmitshur opened this issue Sep 21, 2021 · 3 comments

Comments

@dmitshur
Copy link

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.

@slimsag
Copy link
Member

slimsag commented Sep 21, 2021

👋 hey! Nice to hear from you!

:( bummer, this is definitely unintentional. I didn’t think we depended on the go binary at all. I think that what may be happening is our usage of gofumpt has brought this in by accident: https://github.com/hexops/valast/blob/main/valast.go#L113

@dmitshur
Copy link
Author

dmitshur commented Sep 21, 2021

Heya! 😀 Sorry that it's via an issue report! 😅

this is definitely unintentional.

Thanks for confirming.

what may be happening is our usage of gofumpt

From what I can tell, this particular dependence on the go binary comes from DefaultPackagePathToName, which uses golang.org/x/tools/go/packages. When the packages.Load call with its current &packages.Config{Mode: packages.NeedName} config selects the goListDriver driver, that driver uses go list to load information about packages.

@dmitshur dmitshur changed the title String method is not viable in environments where executable go is unavailable String function is not viable in environments where executable go is unavailable Sep 21, 2021
@slimsag
Copy link
Member

slimsag commented Sep 22, 2021

Ahhh, that makes more sense. For context, through reflection we are able to get a types’ package import path - but we aren’t sure how we should refer to it canonically in code (import path != name, as you know) so that is what DefaultPackagePathToName is doing.

I wonder what go-spew and go-goon do in this case? I guess your original comment about using a different default implementation under ‘ GOOS=js GOARCH=wasm’ may be the right path forward!

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

No branches or pull requests

2 participants