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

Fix display of shadowed variables while debugging #2254

merged 3 commits into from
Jan 23, 2019

Conversation

jhendrixMSFT
Copy link
Member

Ensure that shadowed variables have a unique name so they are properly
displayed in the variables pane. Each nested scope will enclose the
variable name in a pair of parentheses.

@jhendrixMSFT
Fix display of shadowed variables while debugging
2d2e7cd 
Ensure that shadowed variables have a unique name so they are properly
displayed in the variables pane.  Each nested scope will enclose the
variable name in a pair of parentheses.
@jhendrixMSFT jhendrixMSFT self-assigned this Jan 17, 2019
@jhendrixMSFT
Copy link
Member Author

Fixes #1974

@jhendrixMSFT
Copy link
Member Author

image

ramya-rao-a
ramya-rao-a suggested changes Jan 19, 2019
View reviewed changes
src/debugAdapter/goDebug.ts Outdated
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 •
edited
Loading

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

src/debugAdapter/goDebug.ts
@@ -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.

@ramya-rao-a
Copy link
Contributor

Do you know if something changed with gometalinter? The corresponding tests are failing...

@jhendrixMSFT
Copy link
Member Author

If I run gometalinter locally on the test fixture I see the following output.

WARNING: failed to make error: failed to check packages: errors while loading package test/testfixture/errorsTest: [d:\work\src\test\testfixture\errorsTest\errors.go a relative path: GetFileAttributesEx error: failed to check packages: errors while loading package test: The filename, directory name, or volume label syntax is incorrect.
error: failed to check packages: errors while loading package test/testfixture/errorsTest: [d:\work\src\test\testfixture\errorsTest\errors.go:11:2:warning: error return value not checked (undeclared name: prin]) (errcheck)
errors.go:11:2:warning: unused variable or constant undeclared name: prin (varcheck)

The warning messages as expected by the test are included in the output however it appears that there's a bunch of extra preceding output which I suspect is the root problem. Have you seen this before?

@jhendrixMSFT
Copy link
Member Author

The error is coming from a dependent tool errcheck. Here's the offending commit alecthomas/gometalinter@f64e0e4#diff-c15d9d82fc36434e5329fe18954eaa32

@jhendrixMSFT
Copy link
Member Author

I think we should switch to using the latest stable version of gometalinter so that we aren't just getting latest master and hoping it works.

@ramya-rao-a ramya-rao-a merged commit e07df5a into microsoft:master Jan 23, 2019
@jhendrixMSFT jhendrixMSFT deleted the debug1974 branch January 23, 2019 18:25
@ramya-rao-a ramya-rao-a mentioned this pull request Jan 29, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

Assignees

@jhendrixMSFT jhendrixMSFT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhendrixMSFT @ramya-rao-a