bundler: remove mutable Rootname global; derive and return the entry name (#95)

bundler/luabundler.go

@@ -59,10 +59,12 @@ end)(nil)`
 	funcsuffix        string = `end)`
 )
 
-// Rootname is the bundle name for the batch of raw lua
-var (
-	Rootname string = `__root`
-)
+// Rootname is the canonical bundle name for the entry-point module. luabundle
+// output produced by this package always uses it, and it is the default assumed
+// when a bundle does not declare a different entry. It is an immutable constant:
+// the actual entry name of a foreign bundle is detected per call and returned by
+// UnbundleAll, never stored globally.
+const Rootname string = `__root`
 
 // IsBundled keeps regex bundling logic to this file
 func IsBundled(rawlua string) bool {
@@ -79,7 +81,7 @@ func AnalyzeBundle(rawlua string, log func(s string, a ...interface{})) {
 		log("script is not bundled\n")
 		return
 	}
-	results, err := UnbundleAll(rawlua)
+	results, _, err := UnbundleAll(rawlua)
 	if err != nil {
 		log("Couldn't unbundle to analyze: %v", err)
 		return
@@ -89,37 +91,47 @@ func AnalyzeBundle(rawlua string, log func(s string, a ...interface{})) {
 	}
 }
 
-// UnbundleAll takes luacode generates all bundlenames and bundles
-func UnbundleAll(rawlua string) (map[string]string, error) {
+// UnbundleAll takes luacode and returns every registered module keyed by name,
+// alongside the name of the entry-point ("root") module. The root name is
+// detected locally from the bundle's `return __bundle_require("...")` line rather
+// than read from or written to package state, so concurrent or repeated calls
+// cannot contaminate one another. When the input is not bundled it is returned
+// as the sole module under the canonical Rootname.
+func UnbundleAll(rawlua string) (map[string]string, string, error) {
 	if !IsBundled(rawlua) {
-		return map[string]string{Rootname: rawlua}, nil
+		return map[string]string{Rootname: rawlua}, Rootname, nil
 	}
-	newRootInd := regexp.MustCompile(`__bundle_require\(".*"\)`).FindStringIndex(rawlua)
-	if newRootInd != nil {
-		Rootname = rawlua[newRootInd[0]+18 : newRootInd[1]-2]
+	rootName := Rootname
+	if m := regexp.MustCompile(`__bundle_require\("(.*)"\)`).FindStringSubmatch(rawlua); m != nil {
+		rootName = m[1]
 	}
 	scripts := map[string]string{}
-	r, err := findNextBundledScript(rawlua)
+	r, err := findNextBundledScript(rawlua, rootName)
 	for r.leftover != "" {
 		if err != nil {
-			return nil, fmt.Errorf("findNextBundledScript(%s): %v", rawlua, err)
+			return nil, "", fmt.Errorf("findNextBundledScript(%s): %v", rawlua, err)
 		}
 		scripts[r.name] = r.body
-		r, err = findNextBundledScript(r.leftover)
+		r, err = findNextBundledScript(r.leftover, rootName)
 	}
-	if _, ok := scripts[Rootname]; !ok {
-		return nil, fmt.Errorf("Failed to find root bundle")
+	if _, ok := scripts[rootName]; !ok {
+		return nil, "", fmt.Errorf("Failed to find root bundle")
 	}
 
-	return scripts, nil
+	return scripts, rootName, nil
 }
 
 type result struct {
 	name, body, leftover string
 }
 
-func findNextBundledScript(rawlua string) (result, error) {
-	root := regexp.MustCompile(`(?s)__bundle_register\("(.*?)", function\(require, _LOADED, __bundle_register, __bundle_modules\)[\r\n\s]+(.*?)[\r\n ]+end\)[\n\r]+(return __bundle_require\(\"__root\"\)|__bundle_register)+`)
+func findNextBundledScript(rawlua, rootName string) (result, error) {
+	// The final module in a bundle is terminated by the entry line
+	// `return __bundle_require("<rootName>")`; every earlier module is
+	// terminated by the next `__bundle_register`. Deriving the terminator from
+	// the detected root name (rather than hardcoding "__root") is what lets a
+	// bundle with a foreign entry name be scanned without losing its last module.
+	root := regexp.MustCompile(`(?s)__bundle_register\("(.*?)", function\(require, _LOADED, __bundle_register, __bundle_modules\)[\r\n\s]+(.*?)[\r\n ]+end\)[\n\r]+(return __bundle_require\("` + regexp.QuoteMeta(rootName) + `"\)|__bundle_register)+`)
 	m := root.FindStringSubmatchIndex(rawlua)
 	if m == nil {
 		return result{}, nil
@@ -140,13 +152,13 @@ func findNextBundledScript(rawlua string) (result, error) {
 
 // Unbundle extracts the root bundle per
 func Unbundle(rawlua string) (string, error) {
-	srcmap, err := UnbundleAll(rawlua)
+	srcmap, rootName, err := UnbundleAll(rawlua)
 	if err != nil {
 		return "", err
 	}
-	rt, ok := srcmap[Rootname]
+	rt, ok := srcmap[rootName]
 	if !ok {
-		return "", fmt.Errorf("Rootname not found in unbundled map")
+		return "", fmt.Errorf("root module %q not found in unbundled map", rootName)
 	}
 	return rt, nil
 }

bundler/luabundler_test.go

@@ -2,6 +2,7 @@ package bundler
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -253,10 +254,13 @@ var b = '3'
 end)
 return __bundle_require("__root")
 `
-	got, err := UnbundleAll(raw)
+	got, gotRoot, err := UnbundleAll(raw)
 	if err != nil {
 		t.Fatalf("expected no err, got %v", err)
 	}
+	if gotRoot != Rootname {
+		t.Errorf("want root %q, got %q", Rootname, gotRoot)
+	}
 	want := map[string]string{
 		Rootname: `require("core/AgendaDeck-other.foo")
 var a = '2'
@@ -268,6 +272,72 @@ require("core/AgendaDeck-other.foo")`,
 	}
 }
 
+// foreignEntryBundle is a luabundle whose entry module is named "custom_entry"
+// rather than the usual "__root". Its alphabetically-last module, "zzz", is the
+// final registered block, so it is the block terminated by the entry line
+// `return __bundle_require("custom_entry")`.
+var foreignEntryBundle = metaprefix + "\n" +
+	`__bundle_register("custom_entry", function(require, _LOADED, __bundle_register, __bundle_modules)
+require("zzz")
+end)
+__bundle_register("zzz", function(require, _LOADED, __bundle_register, __bundle_modules)
+zzz_value = 1
+end)
+return __bundle_require("custom_entry")`
+
+// TestUnbundleForeignEntryName covers issue #95 failure mode (a): a bundle whose
+// entry module is not "__root" used to silently drop its alphabetically-last
+// module, because the scan regex hardcoded "__root" in its trailing alternation.
+func TestUnbundleForeignEntryName(t *testing.T) {
+	got, gotRoot, err := UnbundleAll(foreignEntryBundle)
+	if err != nil {
+		t.Fatalf("expected no err, got %v", err)
+	}
+	if gotRoot != "custom_entry" {
+		t.Errorf("expected detected root %q, got %q", "custom_entry", gotRoot)
+	}
+	want := map[string]string{
+		"custom_entry": `require("zzz")`,
+		// "zzz" must survive: it is the alphabetically-last module and was the
+		// one dropped before the fix.
+		"zzz": "zzz_value = 1",
+	}
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("want != got:\n%v\n", diff)
+	}
+}
+
+// TestNoCrossCallContamination covers issue #95 failure mode (b): unbundling a
+// bundle with a foreign entry name used to rewrite a mutable package global,
+// corrupting the entry name emitted by any later Bundle call. Now that the root
+// name is derived locally and returned, a subsequent Bundle is unaffected.
+func TestNoCrossCallContamination(t *testing.T) {
+	// This call rewrote the global Rootname in the buggy implementation.
+	if _, _, err := UnbundleAll(foreignEntryBundle); err != nil {
+		t.Fatalf("unbundling foreign-entry bundle: %v", err)
+	}
+
+	fr := &fakeLuaReader{fs: map[string]string{"dep.ttslua": "dep_value = 2"}}
+	got, err := Bundle(`require("dep")`, fr)
+	if err != nil {
+		t.Fatalf("expected no err, got %v", err)
+	}
+	if !strings.Contains(got, `__bundle_register("__root",`) {
+		t.Errorf("later Bundle output did not register a \"__root\" module:\n%s", got)
+	}
+	if !strings.Contains(got, `return __bundle_require("__root")`) {
+		t.Errorf("later Bundle output did not use \"__root\" as its entry line:\n%s", got)
+	}
+	// The bundle must round-trip: its entry must resolve to a registered module.
+	back, backRoot, err := UnbundleAll(got)
+	if err != nil {
+		t.Fatalf("re-unbundling later Bundle output: %v", err)
+	}
+	if _, ok := back[backRoot]; !ok {
+		t.Errorf("entry module %q not registered in re-bundled output", backRoot)
+	}
+}
+
 // This is still being non-deterministic
 func DisabledTestSmartBundle(t *testing.T) {
 	fr := &fakeLuaReader{

bundler/xmlbundler.go

@@ -59,8 +59,11 @@ func indentString(s string, indent string) string {
 	return strings.Join(final, "\n")
 }
 
-// UnbundleAllXML converts a bundled xml file to mapping of filenames to contents
-func UnbundleAllXML(rawxml string) (map[string]string, error) {
+// UnbundleAllXML converts a bundled xml file to mapping of filenames to
+// contents, alongside the name of the entry-point ("root") module. XML has no
+// per-bundle entry declaration, so the root is always the canonical Rootname;
+// it is returned so callers can treat Lua and XML unbundling uniformly.
+func UnbundleAllXML(rawxml string) (map[string]string, string, error) {
 	type inc struct {
 		name  string
 		start int
@@ -98,10 +101,10 @@ func UnbundleAllXML(rawxml string) (map[string]string, error) {
 		}
 	}
 	if len(stack) != 0 {
-		return nil, fmt.Errorf("Bundled xml left after finished reading file: %v", stack)
+		return nil, "", fmt.Errorf("Bundled xml left after finished reading file: %v", stack)
 	}
 	store[Rootname] = unindentAndJoin(xmlarray, "")
-	return store, nil
+	return store, Rootname, nil
 }
 
 func unindentAndJoin(raw []string, indent string) string {

bundler/xmlbundler_test.go

@@ -62,7 +62,7 @@ func TestUnbundleXML(t *testing.T) {
 `,
 	}
 
-	got, err := UnbundleAllXML(input)
+	got, _, err := UnbundleAllXML(input)
 	if err != nil {
 		t.Fatalf("UnbundleAllXML(): %v", err)
 	}

handler/bundlehandler.go

@@ -17,7 +17,7 @@ type Handler struct {
 
 	key, keypath, extension string
 	bundle                  func(string, file.TextReader) (string, error)
-	unbundle                func(string) (map[string]string, error)
+	unbundle                func(string) (map[string]string, string, error)
 }
 
 // NewLuaHandler fills in relevant info for lua bundling
@@ -102,12 +102,12 @@ func (h *Handler) WhileWritingToFile(rawj map[string]interface{}, possiblefname
 			h.key, rawscript, rawscript)
 	}
 
-	allScripts, err := h.unbundle(script)
+	allScripts, rootName, err := h.unbundle(script)
 	if err != nil {
 		return HandleAction{}, fmt.Errorf("UnbundleAll(...): %v", err)
 	}
 	// root bundle is promised to exist
-	rootscript, _ := allScripts[bundler.Rootname]
+	rootscript, _ := allScripts[rootName]
 	returnAction := HandleAction{Noop: false}
 	if len(rootscript) > 80 {
 		err = h.DefaultWriter.EncodeToFile(rootscript, possiblefname)
@@ -120,7 +120,7 @@ func (h *Handler) WhileWritingToFile(rawj map[string]interface{}, possiblefname
 		returnAction.Key = h.key
 		returnAction.Value = rootscript
 	}
-	delete(allScripts, bundler.Rootname)
+	delete(allScripts, rootName)
 
 	for k, script := range allScripts {
 		fname := k

tests/e2e_test.go

@@ -114,11 +114,11 @@ func TestAllReverseThenBuild(t *testing.T) {
 				if !ok {
 					t.Fatalf("non string found in luascript, found %T", gls)
 				}
-				wantBundles, err := bundler.UnbundleAll(wlss)
+				wantBundles, _, err := bundler.UnbundleAll(wlss)
 				if err != nil {
 					t.Fatalf("unbundle want : %v", err)
 				}
-				gotBundles, err := bundler.UnbundleAll(glss)
+				gotBundles, _, err := bundler.UnbundleAll(glss)
 				if err != nil {
 					t.Fatalf("unbundle got : %v", err)
 				}