Merge pull request #287 from bytecodealliance/ydnar/issue281

wit/bindgen: fix wasm-tools WIT generation bugs

Author: ydnar

CHANGELOG.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Fixed
 
+- [#281](https://github.com/bytecodealliance/go-modules/issues/281): errors from internal `wasm-tools` calls are no longer silently ignored. This required fixing a number of related issues, including synthetic world packages for Component Model metadata generation, WIT generation, and WIT keyword escaping in WIT package or interface names.
 - [#284](https://github.com/bytecodealliance/go-modules/issues/284): do not use `bool` for `variant` or `result` GC shapes. TinyGo returns `result` and `variant` values with `bool` as 0 or 1, which breaks the memory representation of tagged unions (variants).
 
 ## [v0.5.0] — 2024-12-14

testdata/issues/issue281.wit

@@ -0,0 +1,11 @@
+package issues:issue281@0.1.0;
+
+world imports {
+    import wasi:http/types@0.2.1;
+}
+
+package wasi:http@0.2.1 {
+	interface types {
+		type t = u8;
+	}
+}

testdata/issues/issue281.wit.json

@@ -0,0 +1,53 @@
+{
+  "worlds": [
+    {
+      "name": "imports",
+      "imports": {
+        "interface-0": {
+          "interface": {
+            "id": 0
+          }
+        }
+      },
+      "exports": {},
+      "package": 1
+    }
+  ],
+  "interfaces": [
+    {
+      "name": "types",
+      "types": {
+        "t": 0
+      },
+      "functions": {},
+      "package": 0
+    }
+  ],
+  "types": [
+    {
+      "name": "t",
+      "kind": {
+        "type": "u8"
+      },
+      "owner": {
+        "interface": 0
+      }
+    }
+  ],
+  "packages": [
+    {
+      "name": "wasi:[email protected]",
+      "interfaces": {
+        "types": 0
+      },
+      "worlds": {}
+    },
+    {
+      "name": "issues:[email protected]",
+      "interfaces": {},
+      "worlds": {
+        "imports": 0
+      }
+    }
+  ]
+}

testdata/issues/issue281.wit.json.golden.wit

@@ -0,0 +1,11 @@
+package issues:issue281@0.1.0;
+
+world imports {
+	import wasi:http/types@0.2.1;
+}
+
+package wasi:http@0.2.1 {
+	interface types {
+		type t = u8;
+	}
+}

wit/bindgen/generator.go

@@ -20,6 +20,7 @@ import (
 	"testing/fstest"
 	"time"
 
+	"github.com/tetratelabs/wazero/sys"
 	"go.bytecodealliance.org/cm"
 	"go.bytecodealliance.org/internal/codec"
 	"go.bytecodealliance.org/internal/go/gen"
@@ -2364,13 +2365,15 @@ func (g *generator) newPackage(w *wit.World, i *wit.Interface, name string) (*ge
 		// Generate wasm file
 		res, world := synthesizeWorld(g.res, w, worldName)
 		witText := res.WIT(wit.Filter(world, i), "")
+		world.Package.Worlds.Delete(worldName) // Undo mutation
 		if g.opts.generateWIT {
 			witFile := g.witFileFor(owner)
 			witFile.WriteString(witText)
 		}
 		content, err := g.componentEmbed(witText)
 		if err != nil {
-			// return nil, err
+			g.opts.logger.Errorf("WIT:\n%s\n\n", witText)
+			return nil, err
 		}
 		componentType := &wasm.CustomSection{
 			Name:     "component-type:" + worldName,
@@ -2390,43 +2393,41 @@ func (g *generator) newPackage(w *wit.World, i *wit.Interface, name string) (*ge
 	return pkg, nil
 }
 
-var replacer = strings.NewReplacer("/", "-", ":", "-", "@", "-v", ".", "")
+var replacer = strings.NewReplacer("/", "-", ":", "-", "@", "-v", ".", "", "%", "")
 
 // componentEmbed runs generated WIT through wasm-tools to generate a wasm file with a component-type custom section.
 func (g *generator) componentEmbed(witData string) ([]byte, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
 	// TODO: --all-features?
-	filename := "./component.wit"
+	filename := "component.wit"
 	args := []string{"component", "embed", "--only-custom", filename}
 	fsMap := map[string]fs.FS{
-		".": fstest.MapFS{
+		"": fstest.MapFS{
 			filename: &fstest.MapFile{Data: []byte(witData)},
 		},
 	}
 	stdout := &bytes.Buffer{}
-	err := g.wasmTools.Run(ctx, nil, stdout, nil, fsMap, args...)
+	stderr := &bytes.Buffer{}
+	err := g.wasmTools.Run(ctx, nil, stdout, stderr, fsMap, args...)
 	if err != nil {
+		if _, ok := err.(*sys.ExitError); ok {
+			return nil, fmt.Errorf("wasm-tools: %s", stderr.String())
+		}
 		return nil, fmt.Errorf("wasm-tools: %w", err)
 	}
 	return stdout.Bytes(), err
 }
 
 func synthesizeWorld(r *wit.Resolve, w *wit.World, name string) (*wit.Resolve, *wit.World) {
-	p := &wit.Package{}
-	p.Name.Namespace = "go"
-	p.Name.Package = "bindgen"
-
 	w = w.Clone()
 	w.Name = name
 	w.Docs = wit.Docs{}
-	w.Package = p
-	p.Worlds.Set(name, w)
+	w.Package.Worlds.Set(name, w)
 
 	r = r.Clone()
 	r.Worlds = append(r.Worlds, w)
-	r.Packages = append(r.Packages, p)
 
 	return r, w
 }

wit/bindgen/testdata_test.go

@@ -21,6 +21,7 @@ import (
 	"go.bytecodealliance.org/internal/go/gen"
 	"go.bytecodealliance.org/internal/relpath"
 	"go.bytecodealliance.org/wit"
+	"go.bytecodealliance.org/wit/logging"
 )
 
 var writeGoFiles = flag.Bool("write", false, "write generated Go files")
@@ -108,6 +109,7 @@ func validateGeneratedGo(t *testing.T, res *wit.Resolve, origin string) {
 		GeneratedBy("test"),
 		PackageRoot(pkgPath),
 		Versioned(true),
+		Logger(logging.NewLogger(os.Stderr, logging.LevelWarn)),
 	)
 	if err != nil {
 		t.Error(err)

wit/ident.go

@@ -2,6 +2,7 @@ package wit
 
 import (
 	"errors"
+	"strconv"
 	"strings"
 
 	"github.com/coreos/go-semver/semver"
@@ -36,21 +37,30 @@ type Ident struct {
 func ParseIdent(s string) (Ident, error) {
 	var id Ident
 	name, ver, hasVer := strings.Cut(s, "@")
-	base, ext, hasExt := strings.Cut(name, "/")
-	id.Namespace, id.Package, _ = strings.Cut(base, ":")
 	if hasVer {
 		var err error
 		id.Version, err = semver.NewVersion(ver)
 		if err != nil {
 			return id, err
 		}
 	}
+	base, ext, hasExt := strings.Cut(name, "/")
+	ns, pkg, _ := strings.Cut(base, ":")
+	id.Namespace = trimPercent(ns)
+	id.Package = trimPercent(pkg)
 	if hasExt {
-		id.Extension = ext
+		id.Extension = trimPercent(ext)
 	}
 	return id, id.Validate()
 }
 
+func trimPercent(s string) string {
+	if len(s) > 0 && s[0] == '%' {
+		return s[1:]
+	}
+	return s
+}
+
 // Validate validates id, returning any errors.
 func (id *Ident) Validate() error {
 	switch {
@@ -59,6 +69,59 @@ func (id *Ident) Validate() error {
 	case id.Package == "":
 		return errors.New("missing package name")
 	}
+	if err := validateName(id.Namespace); err != nil {
+		return err
+	}
+	if err := validateName(id.Package); err != nil {
+		return err
+	}
+	return validateName(id.Extension)
+}
+
+func validateName(s string) error {
+	if len(s) == 0 {
+		return nil
+	}
+	var prev rune
+	for _, c := range s {
+		switch {
+		case c >= 'a' && c <= 'z':
+			switch {
+			case prev >= 'A' && prev <= 'Z':
+				return errors.New("invalid character " + strconv.Quote(string(c)))
+			}
+		case c >= 'A' && c <= 'Z':
+			switch {
+			case prev == 0: // start of string
+			case prev >= 'A' && prev <= 'Z':
+			case prev >= '0' && prev <= '9':
+			case prev == '-':
+			default:
+				return errors.New("invalid character " + strconv.Quote(string(c)))
+			}
+		case c >= '0' && c <= '9':
+			switch {
+			case prev >= 'a' && prev <= 'z':
+			case prev >= 'A' && prev <= 'Z':
+			case prev >= '0' && prev <= '9':
+			default:
+				return errors.New("invalid character " + strconv.Quote(string(c)))
+			}
+		case c == '-':
+			switch prev {
+			case 0: // start of string
+				return errors.New("invalid leading -")
+			case '-':
+				return errors.New("invalid double --")
+			}
+		default:
+			return errors.New("invalid character " + strconv.Quote(string(c)))
+		}
+		prev = c
+	}
+	if prev == '-' {
+		return errors.New("invalid trailing -")
+	}
 	return nil
 }
 
@@ -68,15 +131,15 @@ func (id *Ident) String() string {
 		return id.UnversionedString()
 	}
 	if id.Extension == "" {
-		return id.Namespace + ":" + id.Package + "@" + id.Version.String()
+		return escape(id.Namespace) + ":" + escape(id.Package) + "@" + id.Version.String()
 	}
-	return id.Namespace + ":" + id.Package + "/" + id.Extension + "@" + id.Version.String()
+	return escape(id.Namespace) + ":" + escape(id.Package) + "/" + escape(id.Extension) + "@" + id.Version.String()
 }
 
 // UnversionedString returns a string representation of an [Ident] without version information.
 func (id *Ident) UnversionedString() string {
 	if id.Extension == "" {
-		return id.Namespace + ":" + id.Package
+		return escape(id.Namespace) + ":" + escape(id.Package)
 	}
-	return id.Namespace + ":" + id.Package + "/" + id.Extension
+	return escape(id.Namespace) + ":" + escape(id.Package) + "/" + escape(id.Extension)
 }

wit/ident_test.go

@@ -18,6 +18,16 @@ func TestIdent(t *testing.T) {
 		{"wasi:io/streams", Ident{Namespace: "wasi", Package: "io", Extension: "streams"}, false},
 		{"wasi:io/[email protected]", Ident{Namespace: "wasi", Package: "io", Extension: "streams", Version: semver.New("0.2.0")}, false},
 
+		// Escaping
+		{"%use:%own", Ident{Namespace: "use", Package: "own"}, false},
+		{"%use:%[email protected]", Ident{Namespace: "use", Package: "own", Version: semver.New("0.2.0")}, false},
+		{"%use:%own/%type", Ident{Namespace: "use", Package: "own", Extension: "type"}, false},
+		{"%use:%own/%[email protected]", Ident{Namespace: "use", Package: "own", Extension: "type", Version: semver.New("0.2.0")}, false},
+
+		// Mixed-case
+		{"ABC:def-GHI", Ident{Namespace: "ABC", Package: "def-GHI"}, false},
+		{"ABC1:def2-GHI3", Ident{Namespace: "ABC1", Package: "def2-GHI3"}, false},
+
 		// Errors
 		{"", Ident{}, true},
 		{":", Ident{}, true},
@@ -28,6 +38,15 @@ func TestIdent(t *testing.T) {
 		{"wasi:/", Ident{}, true},
 		{"wasi:clocks@", Ident{}, true},
 		{"wasi:clocks/wall-clock@", Ident{}, true},
+		{"foo%:bar%baz", Ident{Namespace: "foo%", Package: "bar%baz"}, true},
+		{"-foo:bar", Ident{Namespace: "-foo", Package: "bar"}, true},
+		{"foo-:bar", Ident{Namespace: "foo-", Package: "bar"}, true},
+		{"foo--foo:bar", Ident{Namespace: "foo--foo", Package: "bar"}, true},
+		{"aBc:bar", Ident{Namespace: "aBc", Package: "bar"}, true},
+		{"1:2", Ident{Namespace: "1", Package: "2"}, true},
+		{"1a:2b", Ident{Namespace: "1a", Package: "2b"}, true},
+		{"foo-1:bar", Ident{Namespace: "foo-1", Package: "bar"}, true},
+		{"foo:bar-1", Ident{Namespace: "foo", Package: "bar-2"}, true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.s, func(t *testing.T) {

wit/wit.go

@@ -578,7 +578,7 @@ func relativeName(o TypeOwner, p *Package) string {
 		return ""
 	}
 	qualifiedName := op.Name
-	qualifiedName.Package += "/" + name
+	qualifiedName.Package += "/" + escape(name)
 	return qualifiedName.String()
 }