parser: fix persistent cache staleness and semanticdb_index order dependence (#162)

Two related correctness fixes for -scala_gazelle_cache_file:

1. Cached rules no longer enter the symbol scope at startup. Previously
   readScalaRuleCacheFile loaded every cached rule's symbols before the
   walk, so rules deleted from the tree kept providing symbols, kept
   their semanticdb_index membership, and never aged out of the cache
   file. The cache now seeds the memo only (SeedScalaRule); a rule's
   symbols load on its first ParseScalaRule hit, in walk order like a
   fresh parse, and ScalaRules() returns only rules seen this run so
   deletions age out of a persistent cache.

2. semanticdb_index membership now traverses Symbol.Conflicts.
   TrieScope.Put is first-wins per symbol name, so which label holds a
   duplicated name depends on symbol insertion order; membership
   computed from scope winners differed between walk-order runs and
   sorted cache preloads. Including conflict losers makes the generated
   deps order-independent.

Author: zackerydev

language/scala/cache.go

@@ -34,9 +34,7 @@ func (sl *scalaLang) readScalaRuleCacheFile() error {
 		if err != nil {
 			return err
 		}
-		if err := sl.parser.LoadScalaRule(from, rule); err != nil {
-			return err
-		}
+		sl.parser.SeedScalaRule(from, rule)
 	}
 
 	if debugCache {

pkg/parser/BUILD.bazel

@@ -39,6 +39,7 @@ go_test(
     srcs = [
         "assets_test.go",
         "exec_test.go",
+        "memo_parser_test.go",
         "scalameta_parser_test.go",
     ],
     data = glob(["testdata/**/*"]),
@@ -47,6 +48,7 @@ go_test(
         "//build/stack/gazelle/scala/parse",
         "//pkg/bazel",
         "//pkg/collections",
+        "@bazel_gazelle//label",
         "@bazel_gazelle//testtools",
         "@com_github_google_go_cmp//cmp",
         "@com_github_google_go_cmp//cmp/cmpopts",

pkg/parser/memo_parser.go

@@ -19,12 +19,14 @@ const debugMemoParser = false
 type MemoParser struct {
 	next  Parser
 	rules map[label.Label]*sppb.Rule
+	used  map[label.Label]bool
 }
 
 func NewMemoParser(next Parser) *MemoParser {
 	return &MemoParser{
 		next:  next,
 		rules: make(map[label.Label]*sppb.Rule),
+		used:  make(map[label.Label]bool),
 	}
 }
 
@@ -53,6 +55,15 @@ func (p *MemoParser) ParseScalaRule(kind string, from label.Label, dir string, s
 		if debugMemoParser {
 			log.Printf("rule cache hit: %s", from)
 		}
+		if !p.used[from] {
+			p.used[from] = true
+			// Seeded rules carry no symbols so that rules deleted from the
+			// tree cannot pollute the resolution scope; load symbols on first
+			// use, in walk order like a fresh parse.
+			if err := p.next.LoadScalaRule(from, rule); err != nil {
+				return nil, err
+			}
+		}
 		return rule, nil
 	}
 	if debugMemoParser {
@@ -71,6 +82,7 @@ func (p *MemoParser) ParseScalaRule(kind string, from label.Label, dir string, s
 	}
 	rule.Sha256 = sha256
 	p.rules[from] = rule
+	p.used[from] = true
 
 	if debugMemoParser {
 		log.Printf("rule cache save: %s (%s)", from, sha256)
@@ -82,14 +94,26 @@ func (p *MemoParser) ParseScalaRule(kind string, from label.Label, dir string, s
 // LoadScalaRule loads the given state.
 func (p *MemoParser) LoadScalaRule(from label.Label, rule *sppb.Rule) error {
 	p.rules[from] = rule
+	p.used[from] = true
 	return p.next.LoadScalaRule(from, rule)
 }
 
-// ScalaRules returns a list of all scala rules sorted by label
+// SeedScalaRule primes the memo with a cached rule without loading its
+// symbols into scope.  Symbols load on the first ParseScalaRule hit, so
+// cached rules that no longer exist in the tree never contribute symbols.
+func (p *MemoParser) SeedScalaRule(from label.Label, rule *sppb.Rule) {
+	p.rules[from] = rule
+}
+
+// ScalaRules returns the rules used this run (freshly parsed or cache-hit)
+// sorted by label.  Seeded-but-unused rules are dropped so that deleted
+// rules age out of a persistent cache file.
 func (p *MemoParser) ScalaRules() []*sppb.Rule {
 	rules := make([]*sppb.Rule, 0, len(p.rules))
-	for _, rule := range p.rules {
-		rules = append(rules, rule)
+	for from, rule := range p.rules {
+		if p.used[from] {
+			rules = append(rules, rule)
+		}
 	}
 	SortRules(rules)
 	return rules

pkg/parser/memo_parser_test.go

@@ -0,0 +1,168 @@
+package parser
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/bazelbuild/bazel-gazelle/label"
+	"github.com/google/go-cmp/cmp"
+
+	sppb "github.com/stackb/scala-gazelle/build/stack/gazelle/scala/parse"
+)
+
+// fakeParser records LoadScalaRule and ParseScalaRule calls.
+type fakeParser struct {
+	loadCalls  []string
+	parseCalls []string
+}
+
+func (f *fakeParser) LoadScalaRule(from label.Label, rule *sppb.Rule) error {
+	f.loadCalls = append(f.loadCalls, from.String())
+	return nil
+}
+
+func (f *fakeParser) ParseScalaRule(kind string, from label.Label, dir string, srcs ...string) (*sppb.Rule, error) {
+	f.parseCalls = append(f.parseCalls, from.String())
+	files := make([]*sppb.File, len(srcs))
+	for i, src := range srcs {
+		files[i] = &sppb.File{Filename: src}
+	}
+	return &sppb.Rule{
+		Label: from.String(),
+		Kind:  kind,
+		Files: files,
+	}, nil
+}
+
+// writeSrc writes a fake scala source file and returns its dir.
+func writeSrc(t *testing.T, dir, name, content string) {
+	t.Helper()
+	if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// parseOnce runs a fresh MemoParser over the given srcs and returns the
+// memoized rule (carrying the correct Sha256 for those files on disk).
+func parseOnce(t *testing.T, from label.Label, dir string, srcs ...string) *sppb.Rule {
+	t.Helper()
+	parser := NewMemoParser(&fakeParser{})
+	if _, err := parser.ParseScalaRule("scala_library", from, dir, srcs...); err != nil {
+		t.Fatal(err)
+	}
+	rules := parser.ScalaRules()
+	if len(rules) != 1 {
+		t.Fatalf("want 1 memoized rule, got %d", len(rules))
+	}
+	return rules[0]
+}
+
+func TestMemoParserSeedDoesNotLoadSymbols(t *testing.T) {
+	dir := t.TempDir()
+	writeSrc(t, dir, "a.scala", "class A")
+	from := label.New("", "pkg", "a")
+	cached := parseOnce(t, from, dir, "a.scala")
+
+	next := &fakeParser{}
+	parser := NewMemoParser(next)
+	parser.SeedScalaRule(from, cached)
+
+	if len(next.loadCalls) != 0 {
+		t.Fatalf("seeding must not load symbols, got LoadScalaRule calls: %v", next.loadCalls)
+	}
+}
+
+func TestMemoParserHitLoadsSymbolsExactlyOnce(t *testing.T) {
+	dir := t.TempDir()
+	writeSrc(t, dir, "a.scala", "class A")
+	from := label.New("", "pkg", "a")
+	cached := parseOnce(t, from, dir, "a.scala")
+
+	next := &fakeParser{}
+	parser := NewMemoParser(next)
+	parser.SeedScalaRule(from, cached)
+
+	for i := 0; i < 2; i++ {
+		got, err := parser.ParseScalaRule("scala_library", from, dir, "a.scala")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if diff := cmp.Diff(cached.Label, got.Label); diff != "" {
+			t.Fatalf("rule label (-want +got):\n%s", diff)
+		}
+	}
+
+	if len(next.parseCalls) != 0 {
+		t.Fatalf("sha256 match must not re-parse, got ParseScalaRule calls: %v", next.parseCalls)
+	}
+	if diff := cmp.Diff([]string{"//pkg:a"}, next.loadCalls); diff != "" {
+		t.Fatalf("symbols must load exactly once, on first hit (-want +got):\n%s", diff)
+	}
+}
+
+func TestMemoParserStaleSeedReparses(t *testing.T) {
+	dir := t.TempDir()
+	writeSrc(t, dir, "a.scala", "class A")
+	from := label.New("", "pkg", "a")
+	cached := parseOnce(t, from, dir, "a.scala")
+
+	// file content changes after the rule was cached
+	writeSrc(t, dir, "a.scala", "class A { def b = 1 }")
+
+	next := &fakeParser{}
+	parser := NewMemoParser(next)
+	parser.SeedScalaRule(from, cached)
+
+	if _, err := parser.ParseScalaRule("scala_library", from, dir, "a.scala"); err != nil {
+		t.Fatal(err)
+	}
+
+	if diff := cmp.Diff([]string{"//pkg:a"}, next.parseCalls); diff != "" {
+		t.Fatalf("sha256 mismatch must re-parse (-want +got):\n%s", diff)
+	}
+}
+
+func TestMemoParserScalaRulesOmitsUnusedSeeds(t *testing.T) {
+	dir := t.TempDir()
+	writeSrc(t, dir, "a.scala", "class A")
+	live := label.New("", "pkg", "live")
+	deleted := label.New("", "pkg", "deleted")
+	cached := parseOnce(t, live, dir, "a.scala")
+
+	next := &fakeParser{}
+	parser := NewMemoParser(next)
+	parser.SeedScalaRule(live, cached)
+	// a rule that was cached on a previous run but no longer exists in the
+	// tree: it is seeded but never visited by the walk
+	parser.SeedScalaRule(deleted, &sppb.Rule{Label: deleted.String(), Kind: "scala_library"})
+
+	if _, err := parser.ParseScalaRule("scala_library", live, dir, "a.scala"); err != nil {
+		t.Fatal(err)
+	}
+
+	var got []string
+	for _, r := range parser.ScalaRules() {
+		got = append(got, r.Label)
+	}
+	if diff := cmp.Diff([]string{"//pkg:live"}, got); diff != "" {
+		t.Fatalf("ScalaRules must omit seeded-but-unused rules (-want +got):\n%s", diff)
+	}
+}
+
+func TestMemoParserLoadScalaRuleMarksUsed(t *testing.T) {
+	from := label.New("", "pkg", "a")
+	next := &fakeParser{}
+	parser := NewMemoParser(next)
+
+	if err := parser.LoadScalaRule(from, &sppb.Rule{Label: from.String(), Kind: "scala_library"}); err != nil {
+		t.Fatal(err)
+	}
+
+	if diff := cmp.Diff([]string{"//pkg:a"}, next.loadCalls); diff != "" {
+		t.Fatalf("LoadScalaRule must delegate (-want +got):\n%s", diff)
+	}
+	if len(parser.ScalaRules()) != 1 {
+		t.Fatal("explicitly loaded rules must be retained by ScalaRules")
+	}
+}

pkg/semanticdb/BUILD.bazel

@@ -29,6 +29,7 @@ go_library(
 go_test(
     name = "semanticdb_test",
     srcs = [
+        "semanticdb_index_test.go",
         "semanticdb_test.go",
         "visitor_test.go",
     ],
@@ -37,7 +38,11 @@ go_test(
     deps = [
         "//build/stack/gazelle/scala/parse",
         "//pkg/protobuf",
+        "//pkg/resolver",
+        "//pkg/scalarule",
         "//scala/meta/semanticdb",
+        "@bazel_gazelle//label",
+        "@bazel_gazelle//rule",
         "@com_github_google_go_cmp//cmp",
         "@com_github_google_go_cmp//cmp/cmpopts",
     ],

pkg/semanticdb/semanticdb_index.go

@@ -111,9 +111,22 @@ func (s *semanticdbIndexRule) Resolve(rctx *scalarule.ResolveContext, importsRaw
 		kinds[kind] = true
 	}
 
+	// Collect symbols by label, including conflict losers: TrieScope.Put is
+	// first-wins, so which label holds a duplicated symbol name depends on
+	// symbol insertion order (package walk order vs. sorted cache preload).
+	// Index membership must be order-independent, so traverse Conflicts too.
 	symbols := make(map[label.Label]*resolver.Symbol)
+	var addSymbol func(sym *resolver.Symbol)
+	addSymbol = func(sym *resolver.Symbol) {
+		if _, ok := symbols[sym.Label]; !ok {
+			symbols[sym.Label] = sym
+		}
+		for _, conflict := range sym.Conflicts {
+			addSymbol(conflict)
+		}
+	}
 	for _, sym := range GetGlobalScope().GetSymbols("") {
-		symbols[sym.Label] = sym
+		addSymbol(sym)
 	}
 
 	deps := make([]string, 0, len(symbols))

pkg/semanticdb/semanticdb_index_test.go

@@ -0,0 +1,74 @@
+package semanticdb
+
+import (
+	"testing"
+
+	"github.com/bazelbuild/bazel-gazelle/label"
+	"github.com/bazelbuild/bazel-gazelle/rule"
+	"github.com/google/go-cmp/cmp"
+
+	sppb "github.com/stackb/scala-gazelle/build/stack/gazelle/scala/parse"
+	"github.com/stackb/scala-gazelle/pkg/resolver"
+	"github.com/stackb/scala-gazelle/pkg/scalarule"
+)
+
+// TestSemanticdbIndexRuleResolveDeps asserts that index membership is
+// independent of symbol insertion order: TrieScope.Put is first-wins for a
+// given symbol name, so a label whose every symbol name is already claimed
+// by another label only appears in the scope as a Conflict.  Membership must
+// include conflict losers, otherwise the generated deps differ between runs
+// that insert symbols in package-walk order and runs that preload them from
+// a cache in sorted order.
+func TestSemanticdbIndexRuleResolveDeps(t *testing.T) {
+	for name, tc := range map[string]struct {
+		kinds   []string
+		symbols []*resolver.Symbol
+		want    []string
+	}{
+		"conflict loser is a member": {
+			kinds: []string{"scala_library"},
+			symbols: []*resolver.Symbol{
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_library", label.New("", "a", "a")),
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_library", label.New("", "b", "b")),
+			},
+			want: []string{"//a:a_semanticdb", "//b:b_semanticdb"},
+		},
+		"kind filter still applies to conflict losers": {
+			kinds: []string{"scala_library"},
+			symbols: []*resolver.Symbol{
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_library", label.New("", "a", "a")),
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_binary", label.New("", "b", "b")),
+			},
+			want: []string{"//a:a_semanticdb"},
+		},
+		"external labels are excluded": {
+			kinds: []string{"scala_library"},
+			symbols: []*resolver.Symbol{
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_library", label.New("", "a", "a")),
+				resolver.NewSymbol(sppb.ImportType_CLASS, "com.foo.Dup", "scala_library", label.New("maven", "", "jar")),
+			},
+			want: []string{"//a:a_semanticdb"},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			scope := resolver.NewTrieScope()
+			for _, sym := range tc.symbols {
+				if err := scope.PutSymbol(sym); err != nil {
+					t.Fatal(err)
+				}
+			}
+			SetGlobalScope(scope)
+
+			r := rule.NewRule(SemanticdbIndexRuleKind, "semanticdb_index")
+			r.SetAttr("kinds", tc.kinds)
+
+			provider := NewSemanticdbIndexRuleProvider(SemanticdbIndexRuleLoad, SemanticdbIndexRuleKind)
+			ruleProvider := provider.ResolveRule(nil, nil, r)
+			ruleProvider.Resolve(&scalarule.ResolveContext{Rule: r}, nil)
+
+			if diff := cmp.Diff(tc.want, r.AttrStrings("deps")); diff != "" {
+				t.Fatalf("deps (-want +got):\n%s", diff)
+			}
+		})
+	}
+}