From 2a85fea7042a00b4a88d9371e7a6f64d415325b2 Mon Sep 17 00:00:00 2001 From: davidhubbard Date: Mon, 11 Sep 2023 08:34:06 -0500 Subject: [PATCH] RegisterSchema once per golang package #490 (#544) Fixes https://github.com/capnproto/go-capnp/issues/490 by only generating one `RegisterSchema()` function per `$Go.package()`. For example, the unit tests persistent-simple.capnp and persistent-samepkg.capnp now both have the annotation: `$Go.package("persistent_simple");` Their generated go code thus starts with: `package persistent_simple` but only one of them will include a `RegisterSchema()`. Note: if `capnp` is invoked with only one .capnp file at a time on its command line, `capnpc-go` will not detect any *other* .capnp files that might have the same $Go.package. For that use case, the user might be able to cope by running `capnpc -o - persistent-simple.capnp | capnpc-go --forceschemasalways` and editing the generated code to rename all but one `RegisterSchema()` and add calls to the renamed code inside the one remainig `RegisterSchema()`... though that is not a supported solution. e.g. Use this: `capnpc -o go persistent-simple.capnp persistent-samepkg.capnp` Because the following will fail to compile due to duplicate `RegisterSchema()` definitions: ``` capnpc -o go persistent-simple.capnp capnpc -o go persistent-samepkg.capnp ``` --- capnpc-go/capnpc-go.go | 59 +++++++--- capnpc-go/capnpc-go_test.go | 106 +++++++++++------- capnpc-go/nodes.go | 54 +++++++-- capnpc-go/testdata/persistent-samepkg.capnp | 34 ++++++ .../persistent-simple-and-samepkg.capnp.out | Bin 0 -> 3408 bytes capnpc-go/testdata/persistent-simple.capnp | 2 +- .../testdata/persistent-simple.capnp.out | Bin 2056 -> 0 bytes 7 files changed, 186 insertions(+), 69 deletions(-) create mode 100644 capnpc-go/testdata/persistent-samepkg.capnp create mode 100644 capnpc-go/testdata/persistent-simple-and-samepkg.capnp.out delete mode 100644 capnpc-go/testdata/persistent-simple.capnp.out diff --git a/capnpc-go/capnpc-go.go b/capnpc-go/capnpc-go.go index 5b7627c5..7016a37f 100644 --- a/capnpc-go/capnpc-go.go +++ b/capnpc-go/capnpc-go.go @@ -43,9 +43,10 @@ const ( // genoptions are parameters that control code generation. // Usually passed on the command line. type genoptions struct { - promises bool - schemas bool - structStrings bool + promises bool + schemas bool + structStrings bool + forceSchemasAlways bool } type renderer interface { @@ -78,16 +79,18 @@ type generator struct { r renderer fileID uint64 nodes nodeMap + pkgs pkgMap imports imports data staticData opts genoptions } -func newGenerator(fileID uint64, nodes nodeMap, opts genoptions) *generator { +func newGenerator(fileID uint64, trees nodeTrees, opts genoptions) *generator { g := &generator{ r: &templateRenderer{t: templates}, fileID: fileID, - nodes: nodes, + nodes: trees.nodes, + pkgs: trees.pkgs, imports: newImports(), opts: opts, } @@ -141,14 +144,32 @@ func writeByteLiteral(out *bytes.Buffer, name string, data []byte) { } func (g *generator) defineSchemaVar() error { - msg, seg, _ := capnp.NewMessage(capnp.SingleSegment(nil)) - req, _ := schema.NewRootCodeGeneratorRequest(seg) - fnodes := g.nodes[g.fileID].nodes - ids := make([]uint64, len(fnodes)) - for i, n := range fnodes { - ids[i] = n.Id() + var ids []uint64 + // Unless g.opts.forceSchemasAlways overrides it, only call g.r.Render() if + // .done is false, then set .done = true + if !g.opts.forceSchemasAlways { + pkg, ok := g.pkgs[g.nodes[g.fileID].pkg] + if !ok { + return fmt.Errorf("BUG: file %v, $Go.package %q: not found", g.fileID, g.nodes[g.fileID].pkg) + } + if pkg.done { + return nil // RegisterSchema was written in another file + } + ids = make([]uint64, len(pkg.nodeId)) + copy(ids, pkg.nodeId) + pkg.done = true + } else { + // Restore the old behavior: every file has a RegisterSchema with only its own nodes + fnodes := g.nodes[g.fileID].nodes + ids = make([]uint64, len(fnodes)) + for i, n := range fnodes { + ids[i] = n.Id() + } } sort.Sort(uint64Slice(ids)) + + msg, seg, _ := capnp.NewMessage(capnp.SingleSegment(nil)) + req, _ := schema.NewRootCodeGeneratorRequest(seg) // TODO(light): find largest object size and use that to allocate list nodes, _ := req.NewNodes(int32(len(g.nodes))) i := 0 @@ -1171,13 +1192,13 @@ func (g *generator) defineFile() error { return nil } -func generateFile(reqf schema.CodeGeneratorRequest_RequestedFile, nodes nodeMap, opts genoptions) error { +func generateFile(reqf schema.CodeGeneratorRequest_RequestedFile, trees nodeTrees, opts genoptions) error { if opts.structStrings && !opts.schemas { return errors.New("cannot generate struct String() methods without embedding schemas") } id := reqf.Id() fname, _ := reqf.Filename() - g := newGenerator(id, nodes, opts) + g := newGenerator(id, trees, opts) if err := g.defineFile(); err != nil { return err } @@ -1218,6 +1239,7 @@ func main() { flag.BoolVar(&opts.promises, "promises", true, "generate code for promises") flag.BoolVar(&opts.schemas, "schemas", true, "embed schema information in generated code") flag.BoolVar(&opts.structStrings, "structstrings", true, "generate String() methods for structs (-schemas must be true)") + flag.BoolVar(&opts.forceSchemasAlways, "forceschemasalways", false, "(temporary, will be removed) force RegisterSchema() code in every generated .go file even if it is in the same package as another go file. Perhaps useful if the code generation erroneously omits a RegisterSchemas()") flag.Parse() msg, err := capnp.NewDecoder(os.Stdin).Decode() @@ -1230,7 +1252,7 @@ func main() { fmt.Fprintln(os.Stderr, "capnpc-go: reading input:", err) os.Exit(1) } - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(req) if err != nil { fmt.Fprintln(os.Stderr, "capnpc-go:", err) os.Exit(1) @@ -1239,9 +1261,14 @@ func main() { reqFiles, _ := req.RequestedFiles() for i := 0; i < reqFiles.Len(); i++ { reqf := reqFiles.At(i) - err := generateFile(reqf, nodes, opts) + fname, err := reqf.Filename() + if err != nil { + fmt.Fprintf(os.Stderr, "capnpc-go: failed to get filename %d/%d: %v\n", i + 1, reqFiles.Len(), err) + success = false + break + } + err = generateFile(reqf, trees, opts) if err != nil { - fname, _ := reqf.Filename() fmt.Fprintf(os.Stderr, "capnpc-go: generating %s: %v\n", fname, err) success = false } diff --git a/capnpc-go/capnpc-go_test.go b/capnpc-go/capnpc-go_test.go index ae6bf527..7e430cd1 100644 --- a/capnpc-go/capnpc-go_test.go +++ b/capnpc-go/capnpc-go_test.go @@ -92,11 +92,11 @@ func TestBuildNodeMap(t *testing.T) { t.Errorf("Reading code generator request %s: %v", test.name, err) continue } - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(req) if err != nil { t.Errorf("%s: buildNodeMap: %v", test.name, err) } - f := nodes[test.fileID] + f := trees.nodes[test.fileID] if f == nil { t.Errorf("%s: node map is missing file node @%#x", test.name, test.fileID) continue @@ -120,7 +120,7 @@ func TestBuildNodeMap(t *testing.T) { } // Test map lookup for _, k := range test.fileNodes { - n := nodes[k] + n := trees.nodes[k] if n == nil { t.Errorf("%s: missing @%#x from node map", test.name, k) } else if n.Id() != k { @@ -182,13 +182,13 @@ func TestRemoteScope(t *testing.T) { }, } req := mustReadGeneratorRequest(t, "scopes.capnp.out") - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(req) if err != nil { t.Fatal("buildNodeMap:", err) } collect := func(test scopeTest) (g *generator, typ schema.Type, from *node, ok bool) { - g = newGenerator(0xd68755941d99d05e, nodes, genoptions{}) - v := nodes[test.constID] + g = newGenerator(0xd68755941d99d05e, trees, genoptions{}) + v := trees.nodes[test.constID] if v == nil { t.Errorf("Can't find const @%#x for %s test", test.constID, test.name) return nil, schema.Type{}, nil, false @@ -273,13 +273,13 @@ func formatImportSpecs(specs []importSpec) string { func TestDefineConstNodes(t *testing.T) { req := mustReadGeneratorRequest(t, "const.capnp.out") - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(req) if err != nil { t.Fatal("buildNodeMap:", err) } - g := newGenerator(0xc260cb50ae622e10, nodes, genoptions{}) + g := newGenerator(0xc260cb50ae622e10, trees, genoptions{}) getCalls := traceGenerator(g) - err = g.defineConstNodes(nodes[0xc260cb50ae622e10].nodes) + err = g.defineConstNodes(trees.nodes[0xc260cb50ae622e10].nodes) if err != nil { t.Fatal("defineConstNodes:", err) } @@ -369,12 +369,12 @@ func TestDefineFile(t *testing.T) { t.Errorf("Reading code generator request %q: %d RequestedFiles", test.fname, reqFiles.Len()) continue } - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(req) if err != nil { t.Errorf("buildNodeMap %s: %v", test.fname, err) continue } - g := newGenerator(reqFiles.At(0).Id(), nodes, test.opts) + g := newGenerator(reqFiles.At(0).Id(), trees, test.opts) if err := g.defineFile(); err != nil { t.Errorf("defineFile %s %+v: %v", test.fname, test.opts, err) continue @@ -387,7 +387,10 @@ func TestDefineFile(t *testing.T) { // Generation should be deterministic between runs. for i := 0; i < iterations-1; i++ { - g := newGenerator(reqFiles.At(0).Id(), nodes, test.opts) + for _, v := range trees.pkgs { + v.done = false + } + g := newGenerator(reqFiles.At(0).Id(), trees, test.opts) if err := g.defineFile(); err != nil { t.Errorf("defineFile %s %+v [iteration %d]: %v", test.fname, test.opts, i+2, err) continue @@ -531,6 +534,11 @@ func TestPersistent(t *testing.T) { // capnpc-go -promises=0 -schemas=0 -structstrings=0 // ``` t.Parallel() + dir, err := setupTempDir() + if err != nil { + panic(err) + } + defer os.RemoveAll(dir) defaultOptions := genoptions{ promises: true, @@ -541,17 +549,13 @@ func TestPersistent(t *testing.T) { fname string opts genoptions }{ - {"persistent-simple.capnp.out", defaultOptions}, + // The capnp.out is generated with: + // capnp compile --no-standard-import -I../../std -o- persistent-simple.capnp \ + // persistent-samepkg.capnp > persistent-simple-and-samepkg.capnp.out + {"persistent-simple-and-samepkg.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" - + var tobuild []string + for testIndex, test := range tests { data, err := readTestFile(test.fname) if err != nil { t.Errorf("reading %s: %v", test.fname, err) @@ -576,32 +580,49 @@ func TestPersistent(t *testing.T) { t.Errorf("Reading code generator request %q: %d RequestedFiles", test.fname, reqFiles.Len()) continue } - nodes, err := buildNodeMap(req) + trees, err := makeNodeTrees(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 + var srcDump string + ok := false + for i := 0; i < reqFiles.Len(); i++ { + reqf := reqFiles.At(i) + reqFname, err := reqf.Filename() + if err != nil { + t.Errorf("Reading code generator request %q: reqFiles[%d] failed: %v", test.fname, i, err) + break + } + genfname := reqFname+".go" + g := newGenerator(reqf.Id(), trees, test.opts) + err = g.defineFile() + if err != nil { + t.Errorf("defineFile %q %+v: file[%d] %q: %v", test.fname, test.opts, i, reqFname, err) + break + } + 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) + return + } + tobuild = append(tobuild, genfname) + ok = true + srcDump += fmt.Sprintf("\n%s:\n%s", genfname, src) } - 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 + if !ok { + continue } + if testIndex + 1 < len(tests) { + continue + } // 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...) + tobuild = append([]string{"build"}, tobuild...) + cmd := exec.Command("go", tobuild...) cmd.Dir = dir cmd.Stdin = strings.NewReader("") var sout strings.Builder @@ -611,13 +632,12 @@ func TestPersistent(t *testing.T) { 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("go %+v exitcode:%d", tobuild, exitcode) t.Errorf("sout:\n%s", sout.String()) - t.Errorf("serr:\n%s", serr.String()) - t.Errorf("\n%s:\n%s", genfname, src) + t.Errorf("serr:\n%s%s", serr.String(), srcDump) continue } else { - t.Errorf("go %+v: %v", args, err) + t.Errorf("go %+v: %v", tobuild, err) continue } } diff --git a/capnpc-go/nodes.go b/capnpc-go/nodes.go index 817bffb6..4c8efb44 100644 --- a/capnpc-go/nodes.go +++ b/capnpc-go/nodes.go @@ -225,19 +225,43 @@ func (ann *annotations) Rename(given string) string { return ann.Name } +type pkgSchema struct { + // nodeId has the Ids of all the schema nodes in this package. + // (use nodeMap to locate the nodes by their ID) + nodeId []uint64 + // done is true if this schema has been written, i.e. do not + // write it to multiple files in a Go package. + done bool +} + type nodeMap map[uint64]*node +type pkgMap map[string]*pkgSchema + +// nodeTrees contains the abstract syntax tree plus any compiled +// intermediate representation trees so that any call to +// buildNodeTrees() gets a completely filled-out nodeTrees instance +// with all trees ready to generate the output files in a single pass. +type nodeTrees struct { + // nodes has all schema.CodeGeneratorRequest.Nodes indexed by Id + nodes nodeMap + // pkgs maps each $Go.package annotation to the schema node Ids + // used to write a RegisterSchemas block + pkgs pkgMap +} -func buildNodeMap(req schema.CodeGeneratorRequest) (nodeMap, error) { +func makeNodeTrees(req schema.CodeGeneratorRequest) (nodeTrees, error) { + ret := nodeTrees{} rnodes, err := req.Nodes() if err != nil { - return nil, err + return ret, err } - nodes := make(nodeMap, rnodes.Len()) + ret.nodes = make(nodeMap, rnodes.Len()) + ret.pkgs = make(pkgMap) var allfiles []*node for i := 0; i < rnodes.Len(); i++ { ni := rnodes.At(i) n := &node{Node: ni} - nodes[n.Id()] = n + ret.nodes[n.Id()] = n if n.Which() == schema.Node_Which_file { allfiles = append(allfiles, n) } @@ -245,23 +269,35 @@ func buildNodeMap(req schema.CodeGeneratorRequest) (nodeMap, error) { for _, f := range allfiles { fann, err := f.Annotations() if err != nil { - return nil, fmt.Errorf("reading annotations for %v: %v", f, err) + return ret, fmt.Errorf("reading annotations for %v: %v", f, err) } ann := parseAnnotations(fann) f.pkg = ann.Package f.imp = ann.Import nnodes, _ := f.NestedNodes() + ids := make([]uint64, nnodes.Len()) for i := 0; i < nnodes.Len(); i++ { nn := nnodes.At(i) - if ni := nodes[nn.Id()]; ni != nil { + ids[i] = nn.Id() + if ni := ret.nodes[nn.Id()]; ni != nil { nname, _ := nn.Name() - if err := resolveName(nodes, ni, "", nname, f); err != nil { - return nil, err + if err := resolveName(ret.nodes, ni, "", nname, f); err != nil { + return ret, err } } } + + // add this file's nodes to the f.pkg (the $Go.package annotation value) + pkg, ok := ret.pkgs[f.pkg] + if !ok { + pkg = &pkgSchema{} + } + // Note that this can collect ids from multiple files if they are in the + // same $Go.package. + pkg.nodeId = append(pkg.nodeId, ids...) + ret.pkgs[f.pkg] = pkg } - return nodes, nil + return ret, nil } // resolveName is called as part of building up a node map to populate the name field of n. diff --git a/capnpc-go/testdata/persistent-samepkg.capnp b/capnpc-go/testdata/persistent-samepkg.capnp new file mode 100644 index 00000000..f8fbe064 --- /dev/null +++ b/capnpc-go/testdata/persistent-samepkg.capnp @@ -0,0 +1,34 @@ +# 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"; + +@0x941fad1fe66eeb50; + +$Go.package("persistent_simple"); +$Go.import("capnproto.org/go/capnp/v3/capnpc-go/testdata/persistent-simple"); + +interface SomeOtherFile { +} + +struct ThisIsInTheSamePackageAsTheOtherfile { + string @0 :Text; +} diff --git a/capnpc-go/testdata/persistent-simple-and-samepkg.capnp.out b/capnpc-go/testdata/persistent-simple-and-samepkg.capnp.out new file mode 100644 index 0000000000000000000000000000000000000000..ce9c48cc3cd806e4680358ea7df8366740f87efc GIT binary patch literal 3408 zcmdT`U1%It6uz^`Rt;KXTC9|`f3IR3kLYy0`_CpT%l zH1s0$uVP^l@HxP{fUD(g)3xb8fB2<-p&POkU)~4kit!2-i}3d~Ks~-Aie|`4#Uq<1 zZVW75C%Y5}w%ZzqKIs3IVK*%fffGh}jj)G*{?9jr;c3*9m8R=5GZ;dK{k@K<~ojxC{a> zV!m_Yi-~W4I=0vc{AtX~cdqW}{!*UpQFPbRzCY$Zn7Q&4=oc|hn1!Wzr*nB5aLV)D zj|*G64qaRzx*wC>#~JZMDMZP)!4*PUNE(LcCzyRWl73Cj1?o>dq?*>V@=E_hf5iS% zom0MFR{XU5kUwSL4q_4YNt5eXHIK+0@~Yb)oa0^O9|dncdAjcisI-r_)C287_Gq-> zrldi1mVT2Ti#Ft&R65qwTT$1=wd#+okw4U@+u|?VPkd(T5g>HVcu;=n{@x5o`HN)e zQa!ikh2?w8CT0Ydv`y_fiX$?^>OzU%{I zo`R|d_XYC%J*{LD5seYcGo92sWu6D>!QAVc$t`FGmaLc(9UC9VEIXbPTGA<*R4t|( znU-xkQRKc0YAW<=Lmh8LSe1T5lnq4^dNsT@pyQQHyW-f>raNWIS>L3l0d@$B9_`Z+ z;hPBi~cU6 cI8uFZUMOPwFwsg@Tu{2cUM~=h$9nDj3vK*O8~^|S literal 0 HcmV?d00001 diff --git a/capnpc-go/testdata/persistent-simple.capnp b/capnpc-go/testdata/persistent-simple.capnp index eb681d48..cd9a55c0 100644 --- a/capnpc-go/testdata/persistent-simple.capnp +++ b/capnpc-go/testdata/persistent-simple.capnp @@ -19,7 +19,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN # THE SOFTWARE. -using Go = import "go.capnp"; +using Go = import "/go.capnp"; @0xbcfe42e3392b05a8; diff --git a/capnpc-go/testdata/persistent-simple.capnp.out b/capnpc-go/testdata/persistent-simple.capnp.out deleted file mode 100644 index 7c8840d444d4337e295812c9dfe6a519b0aca238..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 2056 zcmah~O=uHQ5Pq8w^Y!-k5oZE}CCIg&$p^r}YPRkK$H8`1q-pq{gvDLR zE09Z&R1ZITI=E?{^6bFEYWY%}YY6fe(wpxe;AZ&_Zc9!NEsmD9!-sO_de`T%0RNp} zitmm4+rcc^?`{9Tz{k*!Ns1d59g1JvPfpzLb#cqU&GH?ZyncJ8@69)g3zKTTN=;Tm z%wzHe_$Sz#FRjzhNqS^nE0`HmYPyV?nOr`r^(NK4o);UebrivMqW(P#=KV3|ya-po z!_{~IAUD2^k7u-O>Xe$(&RkSw!c6EzHJeE#^bvIrz0L*oXLXU&bP2C%L z^?71$KlDA|cZ(l7cRY~~yByq7KK*O_M)Cb_;6vc4j#zm{i&Wa7(>X8QinX>}cs@(G z8_emWM~s3LB4s2oDuh&VXdvCNwd7?}8aX+hcXq1dsA)a~_MR(YI#)VhnrU_pl$YI& zmXY=0dc-*q%e&Fv^{};uBXW*qxUOHxn(9jw#0@4Q?C0-}Tc%Ze7d6*hznXg&U+sQS zG|GqS7qq?{-_DuQMkv$=Omwg4UD*mrbvI=pL-(z|?rhJ}Z{F_1K=nOfdzSoeFg^x_ z-Xnf5c>D;I&0xQGlKYzNS@QpeF-yJ|jBBM`H>w~L_a(1SCr