Resolve name shadowing between imports and params

[novas0x2a]

If the interface to be mocked requires an import that is shadowed by a
param with the same name, it's unlikely to be a problem for the upstream
interface (and thus go undetected), but it causes moq to generate
invalid code.

Example in the wild:
https://github.com/kubernetes/helm/blob/534193640179fe3c9ffc3012a7bc4e8b23fbe832/pkg/helm/interface.go#L28 (the conflict is between the chart param and the chart import)

[matryer]

What kind of code does moq generate here?

[novas0x2a]

It only changes anything if it detects a conflict with an import name, and even then it only changes the generated local name of the argument, not the interface on the exposed mock (since those are exported, the names don't conflict)

package main

import "context"

type Thing interface {
        WithConflict(context context.Context)
        NoConflict(ctx context.Context)
}

$ ./moq.fbe042a -out before . Thing
$ ./moq.2e9ae34 -out after . Thing
$ diff -u before after

--- before      2018-07-24 12:42:55.253016061 -0700
+++ after       2018-07-24 12:43:06.617187737 -0700
@@ -22,7 +22,7 @@
 //             NoConflictFunc: func(ctx context.Context)  {
 //                    panic("TODO: mock out the NoConflict method")
 //             },
-//             WithConflictFunc: func(context context.Context)  {
+//             WithConflictFunc: func(contextMoqParam context.Context)  {
 //                    panic("TODO: mock out the WithConflict method")
 //             },
 //         }
@@ -36,7 +36,7 @@
        NoConflictFunc func(ctx context.Context)
 
        // WithConflictFunc mocks the WithConflict method.
-       WithConflictFunc func(context context.Context)
+       WithConflictFunc func(contextMoqParam context.Context)
 
        // calls tracks calls to the methods.
        calls struct {
@@ -85,19 +85,19 @@
 }
 
 // WithConflict calls WithConflictFunc.
-func (mock *ThingMock) WithConflict(context context.Context) {
+func (mock *ThingMock) WithConflict(contextMoqParam context.Context) {
        if mock.WithConflictFunc == nil {
                panic("ThingMock.WithConflictFunc: method is nil but Thing.WithConflict was just called")
        }
        callInfo := struct {
                Context context.Context
        }{
-               Context: context,
+               Context: contextMoqParam,
        }
        lockThingMockWithConflict.Lock()
        mock.calls.WithConflict = append(mock.calls.WithConflict, callInfo)
        lockThingMockWithConflict.Unlock()
-       mock.WithConflictFunc(context)
+       mock.WithConflictFunc(contextMoqParam)
 }
 
 // WithConflictCalls gets all the calls that were made to WithConflict.

[novas0x2a]

The before copy in this example fails to compile because the context arg shadows the context import, so the context.Context in the callInfo struct decl blows up; this code makes sure that in that case, the context arg is deconflicted so there's no shadowing

[novas0x2a]

Finally got back to this again, with a test this time.

[novas0x2a]

@matryer resolved the conflict, can you give this another look?

[matryer]

Hey @novas0x2a thanks for your work on this. If the situations occurs without this PR, what does the user need to do to fix it? Is it as simple as just changing the variable name from context to ctx or is it more complex?

[novas0x2a]

Yeah, a parameter rename would resolve it. The main issue is when the param is in another project the user doesn't control (that's what motivated the change :))

[novas0x2a]

Ping @matryer (and maybe @sudo-suhas)?

[sudo-suhas]

Hey @novas0x2a, I have been a bit occupied with professional and personal things. I will get around to this, please give me some time 🙏

[novas0x2a]

@sudo-suhas updated my branch to fix the conflicts!

[novas0x2a]

We're about to hit the 2 year anniversary of this PR. Am I wasting my time resolving the conflicts on this?

[matryer]

Hey @novas0x2a - since it's so easy for the programmer to solve this problem, I don't think it's worth the complexity to have the moq tool do this. What do you think?

I don't want to undermine the effort that's gone into this, and I have not experienced this issue myself (so I don't quite have a grasp for how frustrating this could be) which is why I think we've been reluctant to make any progress on this PR.

[novas0x2a]

since it's so easy for the programmer to solve this problem, I don't think it's worth the complexity to have the moq tool do this. What do you think?

I'm not sure what you mean "so easy"- in order to solve this problem, I had to either fork moq or helm. There's no workaround if I don't control the upstream.

[matryer]

Oh right, if you don’t control the code. Yeah good point.

…

On Thu, 17 Sep 2020 at 17:32, Mike Lundy ***@***.***> wrote: since it's so easy for the programmer to solve this problem, I don't think it's worth the complexity to have the moq tool do this. What do you think? I'm not sure what you mean "so easy"- in order to solve this problem, I had to either fork moq or helm. There's no workaround if I don't control the upstream. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAY2G5HUB7HVVMRKUD3MSDSGI2ZLANCNFSM4FJ6JFSQ> .