delve icon indicating copy to clipboard operation
delve copied to clipboard

proc: make PushPackageVarOrSelect check local variables first

Open aarzilli opened this issue 3 months ago • 10 comments

Fixes #4179

aarzilli avatar Oct 12 '25 14:10 aarzilli

To be honest I'm not sure it's worth doing this. It matches better the semantics of Go but It brings in some complication and it's mostly about collisions between local variables and global variables (which are rare anyway).

aarzilli avatar Oct 12 '25 16:10 aarzilli

This PR seems to address another issue I discovered the other day. When the local structure variable and the package name in dwarf are the same, when calling the method of the local structure variable, it will be displayed that the method cannot be found. 🤣 I'll test it tonight when I have some free time to see if I can solve another problem.

wenxuan70 avatar Oct 13 '25 16:10 wenxuan70

I tried debugging the unit tests using the Delve command fixed by this patch and found other issues. When I try to call ctx.Value("ok"), I receive an error message: could not find symbol. This seems to be because the local variable name conflicts with another package of the same name. I took a cursory look at the dlv source code and its associated fncall logs. When compiling to an ast expression, ctx is replaced by another name.

example: image

fncall logs: image

wenxuan70 avatar Nov 08 '25 11:11 wenxuan70

What's code.byted.org/gin/ginex/ctx.Value?

aarzilli avatar Nov 14 '25 16:11 aarzilli

the code.byted.org/gin/ginex/ctx is the package name. No code.byted.org/gin/ginex/ctx.Value.

wenxuan70 avatar Nov 14 '25 16:11 wenxuan70

I've determined that this code is causing the problem. But I'm not sure if this scenario makes sense.

https://github.com/go-delve/delve/blob/b271ba362b43fa0cdaff0899cf076393a436c0d9/pkg/proc/bininfo.go#L3002-L3009

wenxuan70 avatar Nov 14 '25 16:11 wenxuan70

Should be fixed.

aarzilli avatar Nov 15 '25 14:11 aarzilli

I used your patch and it worked fine locally.

wenxuan70 avatar Nov 16 '25 10:11 wenxuan70

To be honest I'm not sure it's worth doing this. It matches better the semantics of Go but It brings in some complication and it's mostly about collisions between local variables and global variables (which are rare anyway).

I've been holding off on review due to this concern. Do you still feel this way?

derekparker avatar Nov 24 '25 17:11 derekparker

I've been holding off on review due to this concern. Do you still feel this way?

I'm inclined to merge this.

aarzilli avatar Nov 28 '25 15:11 aarzilli