From 7b8a344528812840a976903c6f80c532f627d7b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Fri, 27 Dec 2024 08:58:25 +0000
Subject: [PATCH] cue/token: add Pos.Compare from toposort
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now that Go is moving towards int-based compare functions,
such as the general cmp.Compare or specific methods like
net/netip.AddrPort.Compare, add one of our own as well.

Sorting by position is rather common, particularly when wanting to
sort nodes, errors, or values by position. One such example is
toposort, where we borrow the initial code from, as it seems
like the most principled out of them.

Follow-up commits will switch other position compare uses to
cue/token.Pos.Compare, split up for the sake of being able to
more easily bisect or investigate any potential issues.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Iaa8483c11cb1ef784c704630631a167140b25efe
Dispatch-Trailer: {"type":"trybot","CL":1206368,"patchset":2,"ref":"refs/changes/68/1206368/2","targetBranch":"master"}
---
 cue/token/position.go            | 25 +++++++++++++++++++++++++
 internal/core/toposort/vertex.go | 17 +----------------
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/cue/token/position.go b/cue/token/position.go
index 99d81b84b..f0cfd62f5 100644
--- a/cue/token/position.go
+++ b/cue/token/position.go
@@ -15,6 +15,7 @@
 package token
 
 import (
+	"cmp"
 	"fmt"
 	"sort"
 	"sync"
@@ -74,6 +75,10 @@ func (p Pos) File() *File {
 	return p.file
 }
 
+// TODO(mvdan): The methods below don't need to build an entire Position
+// just to access some of the information. This could matter particularly for
+// Compare, as it is called many times when sorting by position.
+
 func (p Pos) Line() int {
 	if p.file == nil {
 		return 0
@@ -106,6 +111,26 @@ func (p Pos) String() string {
 	return p.Position().String()
 }
 
+// Compare returns an integer comparing two positions. The result will be 0 if p == p2,
+// -1 if p < p2, and +1 if p > p2. Note that [NoPos] is always smaller than any valid position.
+func (p Pos) Compare(p2 Pos) int {
+	if p == p2 {
+		return 0
+	} else if p == NoPos {
+		return 1
+	} else if p2 == NoPos {
+		return -1
+	}
+	pos, pos2 := p.Position(), p2.Position()
+	if c := cmp.Compare(pos.Filename, pos2.Filename); c != 0 {
+		return c
+	}
+	// Note that CUE doesn't currently use any directives which alter
+	// position information, like Go's //line, so comparing by offset is enough.
+	return cmp.Compare(pos.Offset, pos2.Offset)
+
+}
+
 // NoPos is the zero value for Pos; there is no file and line information
 // associated with it, and [Pos.IsValid] is false. NoPos is always
 // smaller than any other Pos value. The corresponding Position value
diff --git a/internal/core/toposort/vertex.go b/internal/core/toposort/vertex.go
index 7abdd7cad..348ae420f 100644
--- a/internal/core/toposort/vertex.go
+++ b/internal/core/toposort/vertex.go
@@ -130,7 +130,6 @@ package toposort
 // Vertex's slice of StructInfos.
 
 import (
-	"cmp"
 	"fmt"
 	"slices"
 
@@ -368,7 +367,7 @@ type vertexFeatures struct {
 }
 
 func (vf *vertexFeatures) compareStructMeta(a, b *structMeta) int {
-	if c := comparePos(a.pos, b.pos); c != 0 {
+	if c := a.pos.Compare(b.pos); c != 0 {
 		return c
 	}
 	aHasDyn := a.hasDynamic(vf.dynFieldsMap)
@@ -383,20 +382,6 @@ func (vf *vertexFeatures) compareStructMeta(a, b *structMeta) int {
 	}
 }
 
-func comparePos(aPos, bPos token.Pos) int {
-	if aPos == bPos {
-		return 0
-	} else if aPos == token.NoPos {
-		return 1
-	} else if bPos == token.NoPos {
-		return -1
-	}
-	if c := cmp.Compare(aPos.Filename(), bPos.Filename()); c != 0 {
-		return c
-	}
-	return cmp.Compare(aPos.Offset(), bPos.Offset())
-}
-
 func VertexFeatures(ctx *adt.OpContext, v *adt.Vertex) []adt.Feature {
 	debug("\n*** V (%s %v %p) ***\n", v.Label.RawString(ctx), v.Label, v)