Annotations generate a name collision - fixed (#539)

This is a partial fix of #46

> zombiezen commented on Aug 22, 2016:
>
> One of the notable examples is in persistent.capnp that the annotation and
> the type collide.

Also see capnproto/capnproto#323

This commit changes the generated code for all annotations that start with
a lowercase letter. Since the generated code in Go must start with an
uppercase letter to be valid Go, this change appends an "_" to the name,
regardless of whether there is a name collision already. (The thinking is
that a name collision can happen in the future, and the renaming must be
strictly an injective function, meaning that Go identifiers are always
chosen the same regardless of other names. This way the codegen will not
cause painful refactors in other code due to a name change later.)

The capnp files in this repo under `std` all have $Go.name() annotations
due to this bug. This removes one of them (std/capnp/persistent.capnp) and
replaces it with a unit test that compiles the generated code. Actually
doing a "go build" is one step father than previous unit tests.

The thinking is to not add too many calls to "go build," since there is a lot
of overhead and slow unit tests are discouraging. This commit appears to add
about 250ms to the time it takes to run a "go test" in the capnpc-go subdir.

This also tweaks templates/interfaceClient to fix a problem if an interface is
empty. The {{range .Methods -}} block in interfaceClient has a side effect of
marking the "context" package for import, and an empty interface skips that.
Since Resolve() also depends on the "context" package, this adds a call to
{{$.G.Imports.Context}}.Context to ensure it also marks "context" as used.

capnpc-go/capnpc-go.go

@@ -1,4 +1,7 @@
 /*
+Copyright (c) 2013-2023 Sandstorm Development Group, Inc. and contributors
+Licensed under the MIT License:
+
 capnpc-go is the Cap'n proto code generator for Go.  It reads a
 CodeGeneratorRequest from stdin and for a file foo.capnp it writes
 foo.capnp.go.  This is usually invoked from `capnp compile -ogo`.

capnpc-go/capnpc-go_test.go

@@ -1,3 +1,7 @@
+/*
+Copyright (c) 2013-2023 Sandstorm Development Group, Inc. and contributors
+Licensed under the MIT License:
+*/
 package main
 
 import (
@@ -6,6 +10,7 @@ import (
 	"go/parser"
 	"go/token"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"sort"
 	"strconv"
@@ -467,3 +472,146 @@ func nodeListString(n []*node) string {
 	b.WriteByte(']')
 	return b.String()
 }
+
+func setupTempDir() (string, error) {
+	dir, err := os.MkdirTemp("", "capnpc-go_test")
+	if err != nil {
+		return "", fmt.Errorf("setupTempDir(capnpc-go_test): %v", err)
+	}
+
+	// Add go.mod to the new dir, minimal contents
+	err = os.WriteFile(filepath.Join(dir, "go.mod"),
+		[]byte("module capnpc_go_test"), 0660)
+	if err != nil {
+		return "", fmt.Errorf("setupTempDir: write go.mod: %v", err)
+	}
+
+	// Add go.work to the new dir, minimal contents
+	modRoot, err := os.Getwd()
+	if err != nil {
+		return "", fmt.Errorf("setupTempDir: Getwd: %v", err)
+	}
+	modRoot = filepath.Dir(modRoot)
+
+	err = os.WriteFile(filepath.Join(dir, "go.work"),
+		[]byte(fmt.Sprintf("use %s", modRoot)), 0660)
+	if err != nil {
+		return "", fmt.Errorf("setupTempDir: write go.work: %v", err)
+	}
+	return dir, nil
+}
+
+// This test arises because capnproto-c++ has a file named:
+// * src/capnp/persistent.capnp
+// * Also found in this repo: std/capnp/persistent.capnp
+//
+// It contains two definitions:
+//   interface Persistent {}
+//   annotation persistent(interface, field) :Void;
+//
+// testdata/persistent-simple.capnp is a minimal reproducible test case for the
+// collision.
+func TestPersistent(t *testing.T) {
+	// This test is equivalent to:
+	// `capnp compile -ogo persistent-simple.capnp`
+	//
+	// Or the equivalent in-repo commands before a go install:
+	// ```
+	//    go build;
+	//    capnp compile --no-standard-import -I../std -o- \
+	//        testdata/persistent-simple.capnp | \
+	//        capnpc-go -promises=0 -schemas=0 -structstrings=0
+	// ```
+	t.Parallel()
+
+	defaultOptions := genoptions{
+		promises:      false,
+		schemas:       false,
+		structStrings: false,
+	}
+	tests := []struct {
+		fname string
+		opts  genoptions
+	}{
+		{"persistent-simple.capnp.out", defaultOptions},
+	}
+	for _, test := range tests {
+		dir, err := setupTempDir()
+		if err != nil {
+			t.Fatalf("%s: %v", test.fname, err)
+			break;
+		}
+		defer os.RemoveAll(dir)
+		genfname := test.fname+".go"
+
+		data, err := readTestFile(test.fname)
+		if err != nil {
+			t.Errorf("reading %s: %v", test.fname, err)
+			continue
+		}
+		msg, err := capnp.Unmarshal(data)
+		if err != nil {
+			t.Errorf("Unmarshaling %q: %v", test.fname, err)
+			continue
+		}
+		req, err := schema.ReadRootCodeGeneratorRequest(msg)
+		if err != nil {
+			t.Errorf("Reading code generator request %q: %v", test.fname, err)
+			continue
+		}
+		reqFiles, err := req.RequestedFiles()
+		if err != nil {
+			t.Errorf("Reading code generator request %q: RequestedFiles: %v", test.fname, err)
+			continue
+		}
+		if reqFiles.Len() < 1 {
+			t.Errorf("Reading code generator request %q: %d RequestedFiles", test.fname, reqFiles.Len())
+			continue
+		}
+		nodes, err := buildNodeMap(req)
+		if err != nil {
+			t.Errorf("buildNodeMap %q: %v", test.fname, err)
+			continue
+		}
+		g := newGenerator(reqFiles.At(0).Id(), nodes, test.opts)
+		err = g.defineFile()
+		if err != nil {
+			reqFname, _ := reqFiles.At(0).Filename()
+			t.Errorf("defineFile %q %+v: file %q: %v", test.fname, test.opts, reqFname, err)
+			continue
+		}
+		src := g.generate()
+		genfpath := filepath.Join(dir, genfname)
+		err = os.WriteFile(genfpath, []byte(src), 0660)
+		if err != nil {
+			t.Fatalf("Writing generated code %q: %v", genfpath, err)
+			break
+		}
+
+		// Relies on persistent-simple.capnp with $Go.package("persistent_simple")
+		// not being ("main"). Thus `go build` skips writing an executable.
+		args := []string{
+			"build", genfname,
+		}
+		cmd := exec.Command("go", args...)
+		cmd.Dir = dir
+		cmd.Stdin = strings.NewReader("")
+		var sout strings.Builder
+		cmd.Stdout = &sout
+		var serr strings.Builder
+		cmd.Stderr = &serr
+		if err = cmd.Run(); err != nil {
+			if gotcode, ok := err.(*exec.ExitError); ok {
+				exitcode := gotcode.ExitCode()
+				t.Errorf("go %+v exitcode:%d", args, exitcode)
+				t.Errorf("sout:\n%s", sout.String())
+				t.Errorf("serr:\n%s", serr.String())
+				t.Errorf("\n%s:\n%s", genfname, src)
+				continue
+			} else {
+				t.Errorf("go %+v: %v", args, err)
+				continue
+			}
+		}
+	}
+}

capnpc-go/nodes.go

@@ -258,6 +258,17 @@ func resolveName(nodes nodeMap, n *node, base, name string, file *node) error {
 	name = parseAnnotations(na).Rename(name)
 	if base == "" {
 		n.Name = strings.Title(name)
+		if n.Which() == schema.Node_Which_annotation && n.Name[0] != name[0] {
+			// Names that had a lowercase first letter change to uppercase and
+			// now might collide with a similar-named node.
+			//
+			// This rule forces Annotations to have a trailing underscore. The
+			// idea is to use a consistent naming rule that works even if there
+			// is no name collision yet. If a node is added later, names will
+			// not get mixed up or require a big refactor downstream.
+			// See also: persistent.capnp
+			n.Name = strings.Title(name) + "_"
+		}
 	} else {
 		n.Name = base + "_" + name
 	}

capnpc-go/templates/interfaceClient

@@ -60,7 +60,7 @@ func (c {{$.Node.Name}}) Release() {
 
 // Resolve blocks until the capability is fully resolved or the Context
 // expires.
-func (c {{$.Node.Name}}) Resolve(ctx context.Context) error {
+func (c {{$.Node.Name}}) Resolve(ctx {{$.G.Imports.Context}}.Context) error {
 	return capnp.Client(c).Resolve(ctx)
 }
 

capnpc-go/testdata/persistent-simple.capnp

@@ -0,0 +1,36 @@
+# Copyright (c) 2023 Sandstorm Development Group, Inc. and contributors
+# Licensed under the MIT License:
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+# THE SOFTWARE.
+
+using Go = import "go.capnp";
+
+@0xbcfe42e3392b05a8;
+
+$Go.package("persistent_simple");
+$Go.import("capnproto.org/go/capnp/v3/capnpc-go/testdata/persistent-simple");
+
+interface Persistent {
+}
+
+annotation persistent(interface, field) :Void;
+
+struct ThisStructOnlyNeededToGetImportsToWork {
+  value @0 :Int64;
+}

schemas/schemas_test.go

@@ -14,13 +14,13 @@ func TestDefaultFind(t *testing.T) {
 	if s := schemas.Find(0xdeadbeef); s != nil {
 		t.Errorf("schemas.Find(0xdeadbeef) = %d-byte slice; want nil", len(s))
 	}
-	s := schemas.Find(gocp.Package)
+	s := schemas.Find(gocp.Package_)
 	if s == nil {
-		t.Fatalf("schemas.Find(%#x) = nil", gocp.Package)
+		t.Fatalf("schemas.Find(%#x) = nil", gocp.Package_)
 	}
 	msg, err := capnp.Unmarshal(s)
 	if err != nil {
-		t.Fatalf("capnp.Unmarshal(schemas.Find(%#x)) error: %v", gocp.Package, err)
+		t.Fatalf("capnp.Unmarshal(schemas.Find(%#x)) error: %v", gocp.Package_, err)
 	}
 	req, err := schema.ReadRootCodeGeneratorRequest(msg)
 	if err != nil {
@@ -32,15 +32,15 @@ func TestDefaultFind(t *testing.T) {
 	}
 	for i := 0; i < nodes.Len(); i++ {
 		n := nodes.At(i)
-		if n.Id() == gocp.Package {
+		if n.Id() == gocp.Package_ {
 			// Found
 			if n.Which() != schema.Node_Which_annotation {
-				t.Errorf("found node %#x which = %v; want annotation", gocp.Package, n.Which())
+				t.Errorf("found node %#x which = %v; want annotation", gocp.Package_, n.Which())
 			}
 			return
 		}
 	}
-	t.Fatalf("could not find node %#x in registry", gocp.Package)
+	t.Fatalf("could not find node %#x in registry", gocp.Package_)
 }
 
 func TestNotFound(t *testing.T) {

std/capnp/persistent.capnp

@@ -108,7 +108,7 @@ interface Persistent@0xc8cb212fcd9f5691(SturdyRef, Owner) {
   }
 }
 
-annotation persistent(interface, field) :Void $Go.name("PersistentAnnotation");
+annotation persistent(interface, field) :Void;
 # Apply this annotation to interfaces for objects that will always be persistent, instead of
 # extending the Persistent capability, since the correct type parameters to Persistent depend on
 # the realm, which is orthogonal to the interface type and therefore should not be defined

std/fixups.patch

@@ -38,14 +38,3 @@
 
      interface @17 :Void;
      # The only interface value that can be represented statically is "null", whose methods always
---- a/capnp/persistent.capnp	2021-10-21 15:12:33.501158612 -0400
-+++ b/capnp/persistent.capnp	2021-10-21 15:13:31.930030219 -0400
-@@ -108,7 +108,7 @@
-   }
- }
-
--annotation persistent(interface, field) :Void;
-+annotation persistent(interface, field) :Void $Go.name("PersistentAnnotation");
- # Apply this annotation to interfaces for objects that will always be persistent, instead of
- # extending the Persistent capability, since the correct type parameters to Persistent depend on
- # the realm, which is orthogonal to the interface type and therefore should not be defined