Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix display of shadowed variables while debugging #2254

Merged
merged 3 commits into from
Jan 23, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion src/debugAdapter/goDebug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ enum GoReflectKind {
UnsafePointer
}

enum GoVariableFlags {
VariableEscaped = 1,
VariableShadowed = 2,
VariableConstant = 4,
VariableArgument = 8,
VariableReturnArgument = 16
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment below about https://github.com/derekparker/delve/blob/master/service/api/types.go applies to the above as well. Also, since this is related to variables, can we move the above enum to appear either just before or right after the interface DebugVariable?

Copy link
Contributor

@ramya-rao-a ramya-rao-a Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, am just curious... from where are we getting these values of 1,2,4,8? I understand these refer to the the bits at different positions being 1. But where or how does delve define these?

I see https://github.com/go-delve/delve/blob/4c9a72e486f1f0d0c90ecede8415a871dced8117/service/api/types.go#L194

But I dont see how each of them got the value of 1,2,4 etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It starts here, the magic is in 1 << (iota); iota will start at zero and each line is incremented by 1. See the definition of iota here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've relocated GoVariableFlags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the operation iota = 1 << iota gets repeated for each line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, for the entirety of the const block. In a new const block iota is reset to zero.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats so cool

// These types should stay in sync with:
// https://github.com/derekparker/delve/blob/master/service/api/types.go

Expand Down Expand Up @@ -149,6 +157,8 @@ interface DebugVariable {
type: string;
realType: string;
kind: GoReflectKind;
flags: GoVariableFlags;
DeclLine: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its weird that this doesnt have struct tag at https://github.com/go-delve/delve/blame/4c9a72e486f1f0d0c90ecede8415a871dced8117/service/api/types.go#L264 to change it to camelcasing.
Should we log a bug for delve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it odd as well. We could open an issue however given that JSON is case-sensitive it would be a breaking change for consumers.

value: string;
len: number;
cap: number;
Expand Down Expand Up @@ -843,14 +853,44 @@ class GoDebugSession extends LoggingDebugSession {
log('functionArgs', args);
this.addFullyQualifiedName(args);
let vars = args.concat(locals);

// annotate shadowed variables in parentheses
const shadowedVars = new Map<string, Array<number>>();
for (let i = 0; i < vars.length; ++i) {
if ((vars[i].flags & GoVariableFlags.VariableShadowed) === 0) {
continue;
}
const varName = vars[i].name;
if (!shadowedVars.has(varName)) {
const indices = new Array<number>();
indices.push(i);
shadowedVars.set(varName, indices);
} else {
shadowedVars.get(varName).push(i);
}
}
for (const svIndices of shadowedVars.values()) {
// sort by declared line number in descending order
svIndices.sort((lhs: number, rhs: number) => {
return vars[rhs].DeclLine - vars[lhs].DeclLine;
});
// enclose in parentheses, one pair per scope
for (let scope = 0; scope < svIndices.length; ++scope) {
const svIndex = svIndices[scope];
// start at -1 so scope of 0 has one pair of parens
for (let count = -1; count < scope; ++count) {
vars[svIndex].name = `(${vars[svIndex].name})`;
}
}
}
let scopes = new Array<Scope>();
let localVariables = {
name: 'Local',
addr: 0,
type: '',
realType: '',
kind: 0,
flags: 0,
DeclLine: 0,
value: '',
len: 0,
cap: 0,
Expand Down Expand Up @@ -899,6 +939,8 @@ class GoDebugSession extends LoggingDebugSession {
type: '',
realType: '',
kind: 0,
flags: 0,
DeclLine: 0,
value: '',
len: 0,
cap: 0,
Expand Down