Merge pull request #3408 from jsternberg/dap-better-location-resolution

dap: improve location resolution for duplicate digests

dap/thread.go

@@ -33,13 +33,11 @@ type thread struct {
 	sourcePath string
 
 	// LLB state for the evaluate call.
-	def  *llb.Definition
+	def    *llb.Definition
-	ops  map[digest.Digest]*pb.Op
+	ops    map[digest.Digest]*pb.Op
-	head digest.Digest
+	head   digest.Digest
-	bps  map[digest.Digest]int
+	bps    map[digest.Digest]int
-
+	frames map[int32]*frame
-	frames         map[int32]*frame
-	framesByDigest map[digest.Digest]*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 {
+func (t *thread) getStackFrame(dgst digest.Digest, next *step) *frame {
-	if f := t.framesByDigest[dgst]; f != nil {
-		return f
-	}
-
 	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
+}