Repository: go-delve/delve
Pull request #2513 (2021-05-26 19:29:33)

dap: add 'clipboard' support, and truncate a long value
* add 'clipboard' capability
* apply a larger string limit for 'hover' and 'clipboard' context
* truncate the string representation of a compound type (or pointer of a compound type) variable if the length of the value is larger than 256. But when a variable is returned as a result of Evaluate Request for hover or copy value feature (clipboard or variables context), do not truncate the value.

Updates golang/vscode-go#1318
Fixes golang/vscode-go#1435

hyangah: Arbitrarily chosen 1K, Still 1K is too large for my taste.

Before:
![Screen Shot 2021-05-26 at 2 58 52 PM](https://user-images.githubusercontent.com/4999471/119719778-3107b700-be37-11eb-95ad-bb779321aabc.png)

After: 
![Screen Shot 2021-05-26 at 2 59 42 PM](https://user-images.githubusercontent.com/4999471/119719785-32d17a80-be37-11eb-8c68-26e3c4cc8c31.png)

Still it's possible that we'd end up sending a large JSON message for variables response if the requested variable/scope contains many long variables. This PR doesn't mean to prevent that.
derekparker: > Arbitrarily chosen 1K, Still 1K is too large for my taste.

Would you like time to live with it and possibly change this before it gets submitted?

suzmue: > Arbitrarily chosen 1K, Still 1K is too large for my taste.

I agree I think that these strings become unnecessarily long.

When I was testing paging, this was the cause of some things being so slow that it was unusable for investigating the values.

I would say drop it to ~100 (or even less) especially if it has children to expand to get more information.
hyangah: Lowered the max limit to 256. Tried 128 and it broke some tests and some didn't look nice with 128 (e.g. slice of pointers to a compound type, ...) due to all the type info and len/cap info before the actual data. :-(
I think a better solution is to truncate more intelligently, but I don't want to go too deep into it now.
hyangah: @polinasok is this 256 limit ok? Or should we look into further to do better than SinglelineString and avoid truncating informational messages (e.g. failed load, or partial load...)? 
suzmue: LGTM
suzmue: When the server declares the capability `supportsClipboardContext`, `Copy Value` from both the variable pane and WATCH use the clipboard context.
hyangah: @polinasok It looks like you accidentally edited [my comment](https://github.com/go-delve/delve/pull/2513#issuecomment-850876556) instead of responding to it.

> I am not sure what you mean by "truncating information messages".

- information other than the variable's value such as len/cap of slice/array, or type information or address expression
- information that indicates the loading states, such as " ... 2 more", "- FAILED TO LOAD: some error", ...

> Could you remind me please what actions in the vscode UI generate evaluate requests with "hover" and "clipboard" context. I know that right click + "Copy to Value" is "variabels" and using Debug Console is "repl". 

"hover" -> when you hover over the variable from the editor.
"clipboard" -> "Copy Value" context menu from VARIABLES or WATCH view
hyangah: PR description is updated. Seems like we all agreed on 256 for now.
@suzmue has a prototype of removing type info and slice/array's len/cap info from the value presentation and making them available in a different way (type info is already supplied as the separate type attribute)

Patch:
diff --git a/service/dap/server.go b/service/dap/server.go
index 6dcbd4e8..be5913a9 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -688,6 +688,7 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) {
 	response.Body.SupportsExceptionInfoRequest = true
 	response.Body.SupportsSetVariable = true
 	response.Body.SupportsEvaluateForHovers = true
+	response.Body.SupportsClipboardContext = true
 	// TODO(polina): support these requests in addition to vscode-go feature parity
 	response.Body.SupportsTerminateRequest = false
 	response.Body.SupportsRestartRequest = false
@@ -1814,26 +1815,42 @@ func (s *Server) getTypeIfSupported(v *proc.Variable) string {
 // custom, a zero reference, reminiscent of a zero pointer, is used to indicate that
 // a scalar variable cannot be "dereferenced" to get its elements (as there are none).
 func (s *Server) convertVariable(v *proc.Variable, qualifiedNameOrExpr string) (value string, variablesReference int) {
-	return s.convertVariableWithOpts(v, qualifiedNameOrExpr, false)
+	return s.convertVariableWithOpts(v, qualifiedNameOrExpr, 0)
 }
 
 func (s *Server) convertVariableToString(v *proc.Variable) string {
-	val, _ := s.convertVariableWithOpts(v, "", true)
+	val, _ := s.convertVariableWithOpts(v, "", skipRef)
 	return val
 }
 
+// defaultMaxValueLen is the max length of a string representation of a compound or reference
+// type variable.
+const defaultMaxValueLen = 1 << 8 // 256
+
+// Flags for convertVariableWithOpts option.
+type convertVariableFlags uint8
+
+const (
+	skipRef convertVariableFlags = 1 << iota
+	showFullValue
+)
+
 // convertVariableWithOpts allows to skip reference generation in case all we need is
-// a string representation of the variable.
-func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr string, skipRef bool) (value string, variablesReference int) {
+// a string representation of the variable. When the variable is a compound or reference
+// type variable and its full string representation can be larger than defaultMaxValueLen,
+// this returns a truncated value unless showFull option flag is set.
+func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr string, opts convertVariableFlags) (value string, variablesReference int) {
+	canHaveRef := false
 	maybeCreateVariableHandle := func(v *proc.Variable) int {
-		if skipRef {
+		canHaveRef = true
+		if opts&skipRef != 0 {
 			return 0
 		}
 		return s.variableHandles.create(&fullyQualifiedVariable{v, qualifiedNameOrExpr, false /*not a scope*/})
 	}
 	value = api.ConvertVar(v).SinglelineString()
 	if v.Unreadable != nil {
-		return value, variablesReference
+		return value, 0
 	}
 
 	// Some of the types might be fully or partially not loaded based on LoadConfig.
@@ -1962,6 +1979,10 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 			variablesReference = maybeCreateVariableHandle(v)
 		}
 	}
+	canTruncateValue := showFullValue&opts == 0
+	if len(value) > defaultMaxValueLen && canTruncateValue && canHaveRef {
+		value = value[:defaultMaxValueLen] + "..."
+	}
 	return value, variablesReference
 }
 
@@ -2019,21 +2040,27 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
 			return
 		}
 
-		if exprVar.Kind == reflect.String && (request.Arguments.Context == "repl" || request.Arguments.Context == "variables") {
-			if strVal := constant.StringVal(exprVar.Value); exprVar.Len > int64(len(strVal)) {
-				// reload string value with a bigger limit.
-				loadCfg := DefaultLoadConfig
-				loadCfg.MaxStringLen = 4 << 10
-				if v, err := s.debugger.EvalVariableInScope(goid, frame, 0, request.Arguments.Expression, loadCfg); err != nil {
-					s.log.Debugf("Failed to load more for %v: %v", request.Arguments.Expression, err)
-				} else {
-					exprVar = v
+		ctxt := request.Arguments.Context
+		switch ctxt {
+		case "repl", "variables", "hover", "clipboard":
+			if exprVar.Kind == reflect.String {
+				if strVal := constant.StringVal(exprVar.Value); exprVar.Len > int64(len(strVal)) {
+					// reload string value with a bigger limit.
+					loadCfg := DefaultLoadConfig
+					loadCfg.MaxStringLen = 4 << 10
+					if v, err := s.debugger.EvalVariableInScope(goid, frame, 0, request.Arguments.Expression, loadCfg); err != nil {
+						s.log.Debugf("Failed to load more for %v: %v", request.Arguments.Expression, err)
+					} else {
+						exprVar = v
+					}
 				}
 			}
 		}
-		// TODO(polina): as far as I can tell, evaluateName is ignored by vscode for expression variables.
-		// Should it be skipped altogether for all levels?
-		exprVal, exprRef := s.convertVariable(exprVar, fmt.Sprintf("(%s)", request.Arguments.Expression))
+		var opts convertVariableFlags
+		if ctxt == "variables" || ctxt == "hover" || ctxt == "clipboard" {
+			opts |= showFullValue
+		}
+		exprVal, exprRef := s.convertVariableWithOpts(exprVar, fmt.Sprintf("(%s)", request.Arguments.Expression), opts)
 		response.Body = dap.EvaluateResponseBody{Result: exprVal, VariablesReference: exprRef}
 	}
 	s.send(response)


Test patch:
diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index 2b897241..8419f1c5 100644
--- a/service/dap/daptest/client.go
+++ b/service/dap/daptest/client.go
@@ -104,6 +104,7 @@ func (c *Client) ExpectInitializeResponseAndCapabilities(t *testing.T) *dap.Init
 		SupportsSetVariable:              true,
 		SupportsFunctionBreakpoints:      true,
 		SupportsEvaluateForHovers:        true,
+		SupportsClipboardContext:         true,
 	}
 	if !reflect.DeepEqual(initResp.Body, wantCapabilities) {
 		t.Errorf("capabilities in initializeResponse: got %+v, want %v", pretty(initResp.Body), pretty(wantCapabilities))
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index bdf43135..10e21a6b 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -1493,7 +1493,7 @@ func TestVariablesLoading(t *testing.T) {
 					}
 
 					// Map partially missing based on LoadConfig.MaxArrayValues
-					ref = checkVarRegex(t, locals, -1, "m1", "m1", `\(loaded 64/66\) map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`, `map\[string\]main\.astruct`, hasChildren)
+					ref = checkVarRegex(t, locals, -1, "m1", "m1", `\(loaded 64/66\) map\[string\]main\.astruct \[.+\.\.\.`, `map\[string\]main\.astruct`, hasChildren)
 					if ref > 0 {
 						client.VariablesRequest(ref)
 						m1 := client.ExpectVariablesResponse(t)
@@ -1572,7 +1572,7 @@ func TestVariablesLoading(t *testing.T) {
 							client.VariablesRequest(ref)
 							tmV := client.ExpectVariablesResponse(t)
 							checkChildren(t, tmV, "tm.v", 1)
-							ref = checkVarRegex(t, tmV, 0, `\[0\]`, `tm\.v\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`, `map\[string\]main\.astruct`, hasChildren)
+							ref = checkVarRegex(t, tmV, 0, `\[0\]`, `tm\.v\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.`, `map\[string\]main\.astruct`, hasChildren)
 							if ref > 0 {
 								client.VariablesRequest(ref)
 								tmV0 := client.ExpectVariablesResponse(t)
@@ -1602,7 +1602,7 @@ func TestVariablesLoading(t *testing.T) {
 								tmV := client.ExpectVariablesResponse(t)
 								checkChildren(t, tmV, "tm.v", 1)
 								// TODO(polina): this evaluate name is not usable - it should be empty
-								ref = checkVarRegex(t, tmV, 0, `\[0\]`, `\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`, `map\[string\]main\.astruct`, hasChildren)
+								ref = checkVarRegex(t, tmV, 0, `\[0\]`, `\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.`, `map\[string\]main\.astruct`, hasChildren)
 								if ref > 0 {
 									client.VariablesRequest(ref)
 									tmV0 := client.ExpectVariablesResponse(t)
@@ -2723,13 +2723,18 @@ func TestEvaluateRequest(t *testing.T) {
 					if erres.Body.Error.Format != "Unable to evaluate expression: could not find symbol value for a1" {
 						t.Errorf("\ngot %#v\nwant Format=\"Unable to evaluate expression: could not find symbol value for a1\"", erres)
 					}
+					client.EvaluateRequest("a1", 1002, "clipboard")
+					erres = client.ExpectVisibleErrorResponse(t)
+					if erres.Body.Error.Format != "Unable to evaluate expression: could not find symbol value for a1" {
+						t.Errorf("\ngot %#v\nwant Format=\"Unable to evaluate expression: could not find symbol value for a1\"", erres)
+					}
 				},
 				disconnect: false,
 			}})
 	})
 }
 
-func TestEvaluateRequestLongStr(t *testing.T) {
+func TestEvaluateRequestLongStrLargeValue(t *testing.T) {
 	runTest(t, "testvariables2", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client, "launch",
 			// Launch
@@ -2741,22 +2746,24 @@ func TestEvaluateRequestLongStr(t *testing.T) {
 			[]onBreakpoint{{
 				execute: func() {
 
-					longstr := `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`
-					longstrTruncated := `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"`
-
 					checkStop(t, client, 1, "main.main", -1)
 
 					client.VariablesRequest(1001) // Locals
 					locals := client.ExpectVariablesResponse(t)
 
 					// reflect.Kind == String, load with longer load limit if evaluated in repl/variables context.
-					for _, evalContext := range []string{"", "watch", "repl", "variables", "somethingelse"} {
+					for _, evalContext := range []string{"", "watch", "repl", "variables", "hover", "clipboard", "somethingelse"} {
 						t.Run(evalContext, func(t *testing.T) {
 							// long string
+
+							longstr := `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`
+							longstrTruncated := `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"`
+
 							client.EvaluateRequest("longstr", 0, evalContext)
 							got := client.ExpectEvaluateResponse(t)
 							want := longstrTruncated
-							if evalContext == "repl" || evalContext == "variables" {
+							switch evalContext {
+							case "repl", "variables", "hover", "clipboard":
 								want = longstr
 							}
 							checkEval(t, got, want, false)
@@ -2769,9 +2776,22 @@ func TestEvaluateRequestLongStr(t *testing.T) {
 							// variables are not affected.
 							checkVarExact(t, locals, -1, "longstr", "longstr", longstrTruncated, "string", noChildren)
 							checkVarExact(t, locals, -1, "m6", "m6", `main.C {s: `+longstrTruncated+`}`, "main.C", hasChildren)
+
+							// large array
+							m1 := `\(loaded 64/66\) map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`
+							m1Truncated := `\(loaded 64/66\) map\[string\]main\.astruct \[.+\.\.\.`
+
+							client.EvaluateRequest("m1", 0, evalContext)
+							got3 := client.ExpectEvaluateResponse(t)
+							want3 := m1Truncated
+							switch evalContext {
+							case "variables", "hover", "clipboard":
+								want3 = m1
+							}
+							checkEvalRegex(t, got3, want3, true)
+							checkVarRegex(t, locals, -1, "m1", "m1", m1Truncated, `map\[string\]main\.astruct`, true)
 						})
 					}
-
 				},
 				disconnect: true,
 			}})