dap: improve location resolution for duplicate digests

This improves location resolution for duplicate digests by making some
assumptions about the file structure to determine a "best match" when
there are multiple possible locations.

It uses the next operation (that this digest is the input for) to
determine a location. If the location of the current operation is before
the next operation, this location is preferred. If there are multiple
locations that happen before the next operation, the one closest to the
next operation (aka later in the file) is used instead.

This resolves the most common case of multiple identical `FROM`
statements without adding more code to the frontends themselves.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>

dap/thread.go

@@ -33,13 +33,11 @@ type thread struct {
 	sourcePath string
 
 	// LLB state for the evaluate call.
-	def  *llb.Definition
-	ops  map[digest.Digest]*pb.Op
-	head digest.Digest
-	bps  map[digest.Digest]int
-
-	frames         map[int32]*frame
-	framesByDigest map[digest.Digest]*frame
+	def    *llb.Definition
+	ops    map[digest.Digest]*pb.Op
+	head   digest.Digest
+	bps    map[digest.Digest]int
+	frames map[int32]*frame
 
 	// Runtime state for the evaluate call.
 	entrypoint *step
@@ -141,14 +139,13 @@ type step struct {
 }
 
 func (t *thread) createProgram() error {
-	t.framesByDigest = make(map[digest.Digest]*frame)
 	t.frames = make(map[int32]*frame)
 
 	// Create the entrypoint by using the last node.
 	// We will build on top of that.
 	head := &step{
 		dgst:  t.head,
-		frame: t.getStackFrame(t.head),
+		frame: t.getStackFrame(t.head, nil),
 	}
 	t.entrypoint = t.createBranch(head)
 	return nil
@@ -166,7 +163,7 @@ func (t *thread) createBranch(last *step) (first *step) {
 			// exit point always matches the one set on first
 			out: first.out,
 			// always set to the same as next which is always first
-			frame: t.getStackFrame(first.dgst),
+			frame: t.getStackFrame(first.dgst, first),
 		}
 
 		op := t.ops[first.dgst]
@@ -195,7 +192,7 @@ func (t *thread) createBranch(last *step) (first *step) {
 					in:    exit,
 					next:  exit,
 					out:   exit,
-					frame: t.getStackFrame(digest.Digest(inp.Digest)),
+					frame: t.getStackFrame(digest.Digest(inp.Digest), nil),
 				}
 				prev.in = t.createBranch(head)
 			}
@@ -213,11 +210,7 @@ func (t *thread) createBranch(last *step) (first *step) {
 	return first
 }
 
-func (t *thread) getStackFrame(dgst digest.Digest) *frame {
-	if f := t.framesByDigest[dgst]; f != nil {
-		return f
-	}
-
+func (t *thread) getStackFrame(dgst digest.Digest, next *step) *frame {
 	f := &frame{
 		op: t.ops[dgst],
 	}
@@ -226,7 +219,7 @@ func (t *thread) getStackFrame(dgst digest.Digest) *frame {
 		f.setNameFromMeta(meta)
 	}
 	if loc, ok := t.def.Source.Locations[string(dgst)]; ok {
-		f.fillLocation(t.def, loc, t.sourcePath)
+		f.fillLocation(t.def, loc, t.sourcePath, next)
 	}
 	t.frames[int32(f.Id)] = f
 	return f

dap/variables.go

@@ -35,9 +35,17 @@ func (f *frame) setNameFromMeta(meta llb.OpMetadata) {
 	// TODO: should we infer the name from somewhere else?
 }
 
-func (f *frame) fillLocation(def *llb.Definition, loc *pb.Locations, ws string) {
+func (f *frame) fillLocation(def *llb.Definition, loc *pb.Locations, ws string, next *step) {
 	for _, l := range loc.Locations {
 		for _, r := range l.Ranges {
+			if next != nil && f.Line != 0 {
+				// We have location information. See if the new location
+				// information matches with our location better.
+				if !betterLocation(r, f, next) {
+					continue
+				}
+			}
+
 			f.Line = int(r.Start.Line)
 			f.Column = int(r.Start.Character)
 			f.EndLine = int(r.End.Line)
@@ -48,7 +56,13 @@ func (f *frame) fillLocation(def *llb.Definition, loc *pb.Locations, ws string)
 				Name: path.Base(info.Filename),
 				Path: filepath.Join(ws, info.Filename),
 			}
-			return
+
+			// If we do not have a next operation, then we don't have
+			// any information to make a determination about the "best" fit
+			// that happens at the beginning of this section. Exit early.
+			if next == nil {
+				return
+			}
 		}
 	}
 }
@@ -402,3 +416,23 @@ func brief(s string) string {
 	}
 	return s
 }
+
+func betterLocation(r *pb.Range, f *frame, next *step) bool {
+	// Ideal guess is one that is before the next frame.
+	if int(r.Start.Line) <= next.frame.Line {
+		// And is later than our current guess.
+		if int(r.Start.Line) > f.Line {
+			return true
+		}
+	}
+
+	// We're after the next frame so this is a bad guess.
+	// Was our original one even worse?
+	if int(r.Start.Line) < f.Line {
+		// Yes it was. We'll consider this a better location.
+		return true
+	}
+
+	// Doesn't seem to be a better location.
+	return false
+}