service/dap: Add support for package globals to scopes/variables requests (#2160)

* Support global variables

* Respond to review comments

* Clarify comment

* Add more details to test error messages

* Remove flaky main..inittask checks

* Rename globals flag to match vscode-go

* Normalize filepath with slash separator

* Improve handling for unknown package

* Tweak error message

* More refactoring, normalization and error details to deal with Win test failures

* Clean up optional launch args processing

* Add CurrentPackage to debugger and use instead of ListPackagesBuildInfo
Co-authored-by: NPolina Sokolova <polinasok@users.noreply.github.com>

service/dap/server.go

@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 
 	"github.com/go-delve/delve/pkg/gobuild"
 	"github.com/go-delve/delve/pkg/logflags"
@@ -72,12 +73,15 @@ type launchAttachArgs struct {
 	stopOnEntry bool
 	// stackTraceDepth is the maximum length of the returned list of stack frames.
 	stackTraceDepth int
+	// showGlobalVariables indicates if global package variables should be loaded.
+	showGlobalVariables bool
 }
 
 // defaultArgs borrows the defaults for the arguments from the original vscode-go adapter.
 var defaultArgs = launchAttachArgs{
-	stopOnEntry:     false,
-	stackTraceDepth: 50,
+	stopOnEntry:         false,
+	stackTraceDepth:     50,
+	showGlobalVariables: false,
 }
 
 // NewServer creates a new DAP Server. It takes an opened Listener
@@ -457,13 +461,19 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 		return
 	}
 
-	stop, ok := request.Arguments["stopOnEntry"]
-	s.args.stopOnEntry = ok && stop == true
-
+	// If user-specified, overwrite the defaults for optional args.
+	stop, ok := request.Arguments["stopOnEntry"].(bool)
+	if ok {
+		s.args.stopOnEntry = stop
+	}
 	depth, ok := request.Arguments["stackTraceDepth"].(float64)
 	if ok && depth > 0 {
 		s.args.stackTraceDepth = int(depth)
 	}
+	globals, ok := request.Arguments["showGlobalVariables"].(bool)
+	if ok {
+		s.args.showGlobalVariables = globals
+	}
 
 	var targetArgs []string
 	args, ok := request.Arguments["args"]
@@ -732,18 +742,51 @@ func (s *Server) onScopesRequest(request *dap.ScopesRequest) {
 	// Retrieve local variables
 	locals, err := s.debugger.LocalVariables(goid, frame, 0, cfg)
 	if err != nil {
-		s.sendErrorResponse(request.Request, UnableToListLocals, "Unable to list local vars", err.Error())
+		s.sendErrorResponse(request.Request, UnableToListLocals, "Unable to list locals", err.Error())
 		return
 	}
 	locScope := &proc.Variable{Name: "Locals", Children: slicePtrVarToSliceVar(locals)}
 
 	// TODO(polina): Annotate shadowed variables
-	// TODO(polina): Retrieve global variables
 
 	scopeArgs := dap.Scope{Name: argScope.Name, VariablesReference: s.variableHandles.create(argScope)}
 	scopeLocals := dap.Scope{Name: locScope.Name, VariablesReference: s.variableHandles.create(locScope)}
 	scopes := []dap.Scope{scopeArgs, scopeLocals}
 
+	if s.args.showGlobalVariables {
+		// Limit what global variables we will return to the current package only.
+		// TODO(polina): This is how vscode-go currently does it to make
+		// the amount of the returned data manageable. In fact, this is
+		// considered so expensive even with the package filter, that
+		// the default for showGlobalVariables was recently flipped to
+		// not showing. If we delay loading of the globals until the corresponding
+		// scope is expanded, generating an explicit variable request,
+		// should we consider making all globals accessible with a scope per package?
+		// Or users can just rely on watch variables.
+		currPkg, err := s.debugger.CurrentPackage()
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToListGlobals, "Unable to list globals", err.Error())
+			return
+		}
+		currPkgFilter := fmt.Sprintf("^%s\\.", currPkg)
+		globals, err := s.debugger.PackageVariables(s.debugger.CurrentThread().ThreadID(), currPkgFilter, cfg)
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToListGlobals, "Unable to list globals", err.Error())
+			return
+		}
+		// Remove package prefix from the fully-qualified variable names.
+		// We will include the package info once in the name of the scope instead.
+		for i, g := range globals {
+			globals[i].Name = strings.TrimPrefix(g.Name, currPkg+".")
+		}
+
+		globScope := &proc.Variable{
+			Name:     fmt.Sprintf("Globals (package %s)", currPkg),
+			Children: slicePtrVarToSliceVar(globals),
+		}
+		scopeGlobals := dap.Scope{Name: globScope.Name, VariablesReference: s.variableHandles.create(globScope)}
+		scopes = append(scopes, scopeGlobals)
+	}
 	response := &dap.ScopesResponse{
 		Response: *newResponse(request.Request),
 		Body:     dap.ScopesResponseBody{Scopes: scopes},

service/dap/server_test.go

@@ -341,6 +341,9 @@ func TestSetBreakpoint(t *testing.T) {
 
 		client.ScopesRequest(1000)
 		scopes := client.ExpectScopesResponse(t)
+		if len(scopes.Body.Scopes) > 2 {
+			t.Errorf("\ngot  %#v\nwant len(Scopes)=2 (Arguments & Locals)", scopes)
+		}
 		expectScope(t, scopes, 0, "Arguments", 1000)
 		expectScope(t, scopes, 1, "Locals", 1001)
 
@@ -413,12 +416,12 @@ func expectScope(t *testing.T, got *dap.ScopesResponse, i int, name string, varR
 }
 
 // expectChildren is a helper for verifying the number of variables within a VariablesResponse.
-//      parentName - name of the enclosing variable or scope
+//      parentName - pseudoname of the enclosing variable or scope (used for error message only)
 //      numChildren - number of variables/fields/elements of this variable
 func expectChildren(t *testing.T, got *dap.VariablesResponse, parentName string, numChildren int) {
 	t.Helper()
 	if len(got.Body.Variables) != numChildren {
-		t.Errorf("\ngot  len(%s)=%d\nwant %d", parentName, len(got.Body.Variables), numChildren)
+		t.Errorf("\ngot  len(%s)=%d (children=%#v)\nwant len=%d", parentName, len(got.Body.Variables), got.Body.Variables, numChildren)
 	}
 }
 
@@ -432,7 +435,7 @@ func expectChildren(t *testing.T, got *dap.VariablesResponse, parentName string,
 func expectVar(t *testing.T, got *dap.VariablesResponse, i int, name, value string, useExactMatch, hasRef bool) (ref int) {
 	t.Helper()
 	if len(got.Body.Variables) <= i {
-		t.Errorf("\ngot  len=%d\nwant len>%d", len(got.Body.Variables), i)
+		t.Errorf("\ngot  len=%d (children=%#v)\nwant len>%d", len(got.Body.Variables), got.Body.Variables, i)
 		return
 	}
 	if i < 0 {
@@ -556,7 +559,9 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 		runDebugSessionWithBPs(t, client,
 			// Launch
 			func() {
-				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode": "exec", "program": fixture.Path, "showGlobalVariables": true,
+				})
 			},
 			// Breakpoints are set within the program
 			fixture.Source, []int{},
@@ -580,6 +585,7 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					scopes := client.ExpectScopesResponse(t)
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
 
 					// Arguments
 
@@ -596,6 +602,12 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 						expectVarExact(t, bar, 1, "Bur", `"lorem"`, noChildren)
 					}
 
+					// Globals
+
+					client.VariablesRequest(1002)
+					globals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, globals, 0, "p1", "10", noChildren)
+
 					// Locals
 
 					client.VariablesRequest(1001)
@@ -775,6 +787,7 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					scopes := client.ExpectScopesResponse(t)
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
 
 					client.ScopesRequest(1111)
 					erres := client.ExpectErrorResponse(t)
@@ -791,6 +804,10 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					expectChildren(t, locals, "Locals", 1)
 					expectVarExact(t, locals, -1, "a1", `"bur"`, noChildren)
 
+					client.VariablesRequest(1002) // Globals
+					globals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, globals, 0, "p1", "10", noChildren)
+
 					client.VariablesRequest(7777)
 					erres = client.ExpectErrorResponse(t)
 					if erres.Body.Error.Format != "Unable to lookup variable: unknown reference 7777" {
@@ -835,6 +852,9 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 
 					client.ScopesRequest(1000)
 					scopes := client.ExpectScopesResponse(t)
+					if len(scopes.Body.Scopes) > 2 {
+						t.Errorf("\ngot  %#v\nwant len(scopes)=2 (Argumes & Locals)", scopes)
+					}
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
 
@@ -964,18 +984,18 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 						ref = expectVarExact(t, m4, 2, "[key 1]", "<main.astruct>", hasChildren)
 						if ref > 0 {
 							client.VariablesRequest(ref)
-							m4_key1 := client.ExpectVariablesResponse(t)
-							expectChildren(t, m4_key1, "m4_key1", 2)
-							expectVarExact(t, m4_key1, 0, "A", "2", noChildren)
-							expectVarExact(t, m4_key1, 1, "B", "2", noChildren)
+							m4Key1 := client.ExpectVariablesResponse(t)
+							expectChildren(t, m4Key1, "m4Key1", 2)
+							expectVarExact(t, m4Key1, 0, "A", "2", noChildren)
+							expectVarExact(t, m4Key1, 1, "B", "2", noChildren)
 						}
 						ref = expectVarExact(t, m4, 3, "[val 1]", "<main.astruct>", hasChildren)
 						if ref > 0 {
 							client.VariablesRequest(ref)
-							m4_val1 := client.ExpectVariablesResponse(t)
-							expectChildren(t, m4_val1, "m4_val1", 2)
-							expectVarExact(t, m4_val1, 0, "A", "22", noChildren)
-							expectVarExact(t, m4_val1, 1, "B", "22", noChildren)
+							m4Val1 := client.ExpectVariablesResponse(t)
+							expectChildren(t, m4Val1, "m4Val1", 2)
+							expectVarExact(t, m4Val1, 0, "A", "22", noChildren)
+							expectVarExact(t, m4Val1, 1, "B", "22", noChildren)
 						}
 					}
 					expectVarExact(t, locals, -1, "emptymap", "<map[string]string> (length: 0)", noChildren)
@@ -1020,6 +1040,73 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 	})
 }
 
+// TestGlobalScopeAndVariables launches the program with showGlobalVariables
+// arg set, executes to a breakpoint in the main package and tests that global
+// package main variables got loaded. It then steps into a function
+// in another package and tests that globals scope got updated to those vars.
+func TestGlobalScopeAndVariables(t *testing.T) {
+	runTest(t, "consts", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode": "exec", "program": fixture.Path, "showGlobalVariables": true,
+				})
+			},
+			// Breakpoints are set within the program
+			fixture.Source, []int{},
+			[]onBreakpoint{{
+				// Stop at line 36
+				execute: func() {
+					client.StackTraceRequest(1, 0, 20)
+					stack := client.ExpectStackTraceResponse(t)
+					expectStackFrames(t, stack, 36, 1000, 3, 3)
+
+					client.ScopesRequest(1000)
+					scopes := client.ExpectScopesResponse(t)
+					expectScope(t, scopes, 0, "Arguments", 1000)
+					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
+
+					client.VariablesRequest(1002)
+					client.ExpectVariablesResponse(t)
+					// The program has no user-defined globals.
+					// Depending on the Go version, there might
+					// be some runtime globals (e.g. main..inittask)
+					// so testing for the total number is too fragile.
+
+					// Step into pkg.AnotherMethod()
+					client.StepInRequest(1)
+					client.ExpectStepInResponse(t)
+					client.ExpectStoppedEvent(t)
+
+					client.StackTraceRequest(1, 0, 20)
+					stack = client.ExpectStackTraceResponse(t)
+					expectStackFrames(t, stack, 14, 1000, 4, 4)
+
+					client.ScopesRequest(1000)
+					scopes = client.ExpectScopesResponse(t)
+					expectScope(t, scopes, 0, "Arguments", 1000)
+					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package github.com/go-delve/delve/_fixtures/internal/dir0/pkg)", 1002)
+
+					client.VariablesRequest(1002)
+					globals := client.ExpectVariablesResponse(t)
+					expectChildren(t, globals, "Globals", 1)
+					ref := expectVarExact(t, globals, 0, "SomeVar", "<github.com/go-delve/delve/_fixtures/internal/dir0/pkg.SomeType>", hasChildren)
+
+					if ref > 0 {
+						client.VariablesRequest(ref)
+						somevar := client.ExpectVariablesResponse(t)
+						expectChildren(t, somevar, "SomeVar", 1)
+						expectVarExact(t, somevar, 0, "X", "0", noChildren)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // Tests that 'stackTraceDepth' from LaunchRequest is parsed and passed to
 // stacktrace requests handlers.
 func TestLaunchRequestWithStackTraceDepth(t *testing.T) {

service/debugger/debugger.go

@@ -1415,6 +1415,26 @@ func (d *Debugger) convertDefers(defers []*proc.Defer) []api.Defer {
 	return r
 }
 
+// CurrentPackage returns the fully qualified name of the
+// package corresponding to the function location of the
+// current thread.
+func (d *Debugger) CurrentPackage() (string, error) {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+
+	if _, err := d.target.Valid(); err != nil {
+		return "", err
+	}
+	loc, err := d.target.CurrentThread().Location()
+	if err != nil {
+		return "", err
+	}
+	if loc.Fn == nil {
+		return "", fmt.Errorf("unable to determine current package due to unspecified function location")
+	}
+	return loc.Fn.PackageName(), nil
+}
+
 // FindLocation will find the location specified by 'locStr'.
 func (d *Debugger) FindLocation(goid, frame, deferredCall int, locStr string, includeNonExecutableLines bool) ([]api.Location, error) {
 	d.targetMutex.Lock()
@@ -1490,6 +1510,13 @@ func (d *Debugger) Recorded() (recorded bool, tracedir string) {
 	return d.target.Recorded()
 }
 
+// CurrentThread returns the current thread.
+func (d *Debugger) CurrentThread() proc.Thread {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+	return d.target.CurrentThread()
+}
+
 // Checkpoint will set a checkpoint specified by the locspec.
 func (d *Debugger) Checkpoint(where string) (int, error) {
 	d.targetMutex.Lock()