feat: include canonical node info for identifier service (#5917)

Allow the user to request that only canonical nodes be returned. This is useful when there is more than one node for a given symbol. This happens in c++ where we have a node for the declaration and a node for the definition.
kythe · Nov 3, 2023 · 3db7739 · 3db7739
1 parent a4489f5
commit 3db7739
Show file tree

Hide file tree

Showing 6 changed files with 246 additions and 127 deletions.
diff --git a/kythe/go/serving/identifiers/identifiers.go b/kythe/go/serving/identifiers/identifiers.go
@@ -49,6 +49,11 @@ type Table struct {
 	table.Proto
 }
 
+type matchNode struct {
+	match           *ipb.FindReply_Match
+	canonicalTicket string
+}
+
 // Find implements the Service interface for Table
 func (it *Table) Find(ctx context.Context, req *ipb.FindRequest) (*ipb.FindReply, error) {
 	var (
@@ -58,25 +63,54 @@ func (it *Table) Find(ctx context.Context, req *ipb.FindRequest) (*ipb.FindReply
 		reply     ipb.FindReply
 	)
 
-	return &reply, it.LookupValues(ctx, []byte(qname), (*srvpb.IdentifierMatch)(nil), func(msg proto.Message) error {
+	// If clustering is done across language boundaries (e.g. proto), then it is possible the
+	// canonical node has a different qualified name (e.g. build_event_stream.BuildEvent vs
+	// build_event_stream::BuildEvent). If the user asked for the qualified name that isn't the
+	// canonical node, return all results since we won't be returning the canonical node.
+	allNodes := make(map[string]*matchNode)
+	var orderedTickets []string
+
+	if err := it.LookupValues(ctx, []byte(qname), (*srvpb.IdentifierMatch)(nil), func(msg proto.Message) error {
 		match := msg.(*srvpb.IdentifierMatch)
 		for _, node := range match.GetNode() {
 			if !validCorpusAndLang(corpora, languages, node) {
 				continue
 			}
 
-			matchNode := &ipb.FindReply_Match{
+			replyMatch := &ipb.FindReply_Match{
 				Ticket:        node.GetTicket(),
 				NodeKind:      node.GetNodeKind(),
 				NodeSubkind:   node.GetNodeSubkind(),
 				BaseName:      match.GetBaseName(),
 				QualifiedName: match.GetQualifiedName(),
 			}
-
-			reply.Matches = append(reply.GetMatches(), matchNode)
+			if n := allNodes[node.GetTicket()]; n != nil {
+				log.Errorf("Two matches found for the same ticket: %v, %v", n, node)
+			}
+			allNodes[node.GetTicket()] = &matchNode{
+				match:           replyMatch,
+				canonicalTicket: node.GetCanonicalNodeTicket(),
+			}
+			orderedTickets = append(orderedTickets, node.GetTicket())
 		}
 		return nil
-	})
+	}); err != nil {
+		return nil, err
+	}
+
+	for _, t := range orderedTickets {
+		ticketMatch := allNodes[t]
+		if req.GetPickCanonicalNodes() &&
+			ticketMatch.canonicalTicket != "" &&
+			ticketMatch.canonicalTicket != ticketMatch.match.GetTicket() &&
+			allNodes[ticketMatch.canonicalTicket] != nil {
+			continue
+		}
+
+		reply.Matches = append(reply.GetMatches(), ticketMatch.match)
+	}
+
+	return &reply, nil
 }
 
 func validCorpusAndLang(corpora, langs []string, node *srvpb.IdentifierMatch_Node) bool {

diff --git a/kythe/go/serving/identifiers/identifiers_test.go b/kythe/go/serving/identifiers/identifiers_test.go
@@ -30,81 +30,132 @@ import (
 )
 
 var matchTable = Table{testProtoTable{
-	"foo::bar": []proto.Message{&srvpb.IdentifierMatch{
-		Node: []*srvpb.IdentifierMatch_Node{
-			node("kythe://corpus?lang=c++", "record", "class"),
+	"foo::bar": []proto.Message{
+		&srvpb.IdentifierMatch{
+			Node: []*srvpb.IdentifierMatch_Node{
+				node("kythe://corpus?lang=c++", "", "record", "class"),
+			},
+			BaseName:      "bar",
+			QualifiedName: "foo::bar",
 		},
-		BaseName:      "bar",
-		QualifiedName: "foo::bar",
-	}, &srvpb.IdentifierMatch{
-		Node: []*srvpb.IdentifierMatch_Node{
-			node("kythe://corpus?lang=rust", "record", "struct"),
+		&srvpb.IdentifierMatch{
+			Node: []*srvpb.IdentifierMatch_Node{
+				node("kythe://corpus?lang=c++#decl", "kythe://corpus?lang=c++", "record", "class"),
+			},
+			BaseName:      "bar",
+			QualifiedName: "foo::bar",
 		},
-		BaseName:      "bar",
-		QualifiedName: "foo::bar",
-	}},
+		&srvpb.IdentifierMatch{
+			Node: []*srvpb.IdentifierMatch_Node{
+				node("kythe://corpus?lang=rust", "", "record", "struct"),
+			},
+			BaseName:      "bar",
+			QualifiedName: "foo::bar",
+		},
+	},
+
+	"foo.bar": []proto.Message{
+		&srvpb.IdentifierMatch{
+			Node: []*srvpb.IdentifierMatch_Node{
+				node("kythe://corpus?lang=java", "kythe://corpus?lang=c++", "record", "interface"),
+			},
+			BaseName:      "bar",
+			QualifiedName: "foo.bar",
+		},
+	},
 
-	"com.java.package.Interface": []proto.Message{&srvpb.IdentifierMatch{
-		Node: []*srvpb.IdentifierMatch_Node{
-			node("kythe://habeas?lang=java", "record", "interface"),
+	"com.java.package.Interface": []proto.Message{
+		&srvpb.IdentifierMatch{
+			Node: []*srvpb.IdentifierMatch_Node{
+				node("kythe://habeas?lang=java", "", "record", "interface"),
+			},
+			BaseName:      "Interface",
+			QualifiedName: "com.java.package.Interface",
 		},
-		BaseName:      "Interface",
-		QualifiedName: "com.java.package.Interface",
-	}},
+	},
 }}
 
 var tests = []testCase{
 	{
-		findRequest("foo::bar", nil, nil),
+		"Qualified name across languages",
+		findRequest("foo::bar", nil, nil, false),
+		[]*ipb.FindReply_Match{
+			match("kythe://corpus?lang=c++", "record", "class", "bar", "foo::bar"),
+			match("kythe://corpus?lang=c++#decl", "record", "class", "bar", "foo::bar"),
+			match("kythe://corpus?lang=rust", "record", "struct", "bar", "foo::bar"),
+		},
+	},
+	{
+		"Canonical node for Qualified name across languages",
+		findRequest("foo::bar", nil, nil, true),
 		[]*ipb.FindReply_Match{
 			match("kythe://corpus?lang=c++", "record", "class", "bar", "foo::bar"),
 			match("kythe://corpus?lang=rust", "record", "struct", "bar", "foo::bar"),
 		},
 	},
 	{
-		findRequest("foo::bar", nil, []string{"rust"}),
+		"Java qualified name",
+		findRequest("foo.bar", nil, nil, false),
+		[]*ipb.FindReply_Match{
+			match("kythe://corpus?lang=java", "record", "interface", "bar", "foo.bar"),
+		},
+	},
+	{
+		"Java qualified name with canonical node with a different qualified name",
+		findRequest("foo.bar", nil, nil, true),
+		[]*ipb.FindReply_Match{
+			match("kythe://corpus?lang=java", "record", "interface", "bar", "foo.bar"),
+		},
+	},
+	{
+		"Rust only",
+		findRequest("foo::bar", nil, []string{"rust"}, false),
 		[]*ipb.FindReply_Match{
 			match("kythe://corpus?lang=rust", "record", "struct", "bar", "foo::bar"),
 		},
 	},
 	{
-		findRequest("com.java.package.Interface", []string{"habeas"}, nil),
+		"Corpus filter matches",
+		findRequest("com.java.package.Interface", []string{"habeas"}, nil, false),
 		[]*ipb.FindReply_Match{
 			match("kythe://habeas?lang=java", "record", "interface", "Interface", "com.java.package.Interface"),
 		},
 	},
 	{
-		findRequest("com.java.package.Interface", []string{"corpus"}, nil),
+		"Corpus filter does not match",
+		findRequest("com.java.package.Interface", []string{"corpus"}, nil, false),
 		nil,
 	},
 }
 
 func TestFind(t *testing.T) {
 	for _, test := range tests {
-		reply, err := matchTable.Find(context.TODO(), &test.FindRequest)
+		reply, err := matchTable.Find(context.TODO(), test.Request)
 		if err != nil {
-			t.Errorf("unexpected error for request %v: %v", test.FindRequest, err)
+			t.Errorf("unexpected error for request %v: %v", test.Request, err)
 		}
 
 		if err := testutil.DeepEqual(test.Matches, reply.Matches); err != nil {
-			t.Error(err)
+			t.Errorf("Failed %s\n%v", test.Name, err)
 		}
 	}
 }
 
-func findRequest(qname string, corpora, langs []string) ipb.FindRequest {
-	return ipb.FindRequest{
-		Identifier: qname,
-		Corpus:     corpora,
-		Languages:  langs,
+func findRequest(qname string, corpora, langs []string, canonicalOnly bool) *ipb.FindRequest {
+	return &ipb.FindRequest{
+		Identifier:         qname,
+		PickCanonicalNodes: canonicalOnly,
+		Corpus:             corpora,
+		Languages:          langs,
 	}
 }
 
-func node(ticket, kind, subkind string) *srvpb.IdentifierMatch_Node {
+func node(ticket, canonicalNodeTicket, kind, subkind string) *srvpb.IdentifierMatch_Node {
 	return &srvpb.IdentifierMatch_Node{
-		Ticket:      ticket,
-		NodeKind:    kind,
-		NodeSubkind: subkind,
+		Ticket:              ticket,
+		CanonicalNodeTicket: canonicalNodeTicket,
+		NodeKind:            kind,
+		NodeSubkind:         subkind,
 	}
 }
 
@@ -119,7 +170,8 @@ func match(ticket, kind, subkind, bname, qname string) *ipb.FindReply_Match {
 }
 
 type testCase struct {
-	ipb.FindRequest
+	Name    string
+	Request *ipb.FindRequest
 	Matches []*ipb.FindReply_Match
 }
 

diff --git a/kythe/proto/identifier.proto b/kythe/proto/identifier.proto
@@ -35,6 +35,11 @@ message FindRequest {
 
   // Restricts the match to the given languages.
   repeated string languages = 3;
+
+  // When two or more nodes are present for the same logical symbol (e.g. the
+  // definition and declaration of a field in C++) and if a canonical node is
+  // known, only return the canonical node.
+  bool pick_canonical_nodes = 4;
 }
 
 message FindReply {

diff --git a/kythe/proto/identifier_go_proto/identifier.pb.go b/kythe/proto/identifier_go_proto/identifier.pb.go
diff --git a/kythe/proto/serving.proto b/kythe/proto/serving.proto
@@ -412,6 +412,11 @@ message IdentifierMatch {
     // Kythe ticket for the matched node.
     string ticket = 1;
 
+    // Ticket for the canonical node, if there is one. For example, if there is
+    // a definition node and a declaration node only one will be the canonical
+    // node.
+    string canonical_node_ticket = 4;
+
     // Kind of the node being referenced.
     string node_kind = 2;