parser: fix persistent cache staleness and semanticdb_index order dependence

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)
+			}
+		})
+	}
+}