There's a lot of examples in test code that are shadowing package names in test libraries like testify.

For example:

import (
  "testing"
  "github.com/stretchr/testify/assert"
  "github.com/stretchr/testify/require"
)

func TestSomething(t *testing.T) {
  assert := assert.New(t)
  require := require.New(t)
  var a string = "Hello"
  var b string = "Hello"

  assert.Equal(a, b, "The two words should be the same.")
}

Shouldn't it be an explicit decision and rare occurrence to override the value of a package namespace with a local variable?

import (
  "testing"
  "github.com/stretchr/testify/assert"
  "github.com/stretchr/testify/require"
)

func TestSomething(t *testing.T) {
  assertions := assert.New(t)
  req := require.New(t)
  var a string = "Hello"
  var b string = "Hello"

  assert.Equal(a, b, "The two words should be the same.")
}

I'd expect the proper behavior to be aliasing the package like pkgassert which I doesn't feel idiomatic, or ensuring that variables are not set to the same name as the package: assertions := assert.New(t).

Other examples:

• Ok: buf := bytes.Buffer{}
• Bad: bytes := bytes.Buffer{}

Note: I'm not focused on variable naming patterns, as Go tends to recommend single letter, or short variables in the narrowed scope you might use the buffer for. Assume the longer name is acceptable and desired due to the function length.

If this is a desired behavior in testing packages I'd like to know why? It appears to violate a key principle of idiomatic Go in "no magic" and being readable.

If there are undesired side effects from this I'd like to know of that as well. My assumption is that is means after import that any calls to the package in the same scope wouldn't work due to variable shadowing

Edit

Scope Shadowing in Go was a nice read and gave some great points on usage. The main takeaway I had from this and from Stack commenters:

1. It can enhance readability. (I think this is probably due to Go preferring small concise naming for packages, making package names in Go variable length.)

2. Parallel Test Issues was something I recall reading a while back. I didn't understand the recommendation to "hide" the loop variable by putting in:

    for _, tc := range tests {
        tc := tc
        t.Run(tc.name, func(t *testing.T) {
            t.Parallel()
            // Here you test tc.value against a test function.
            // Let's use t.Log as our test function :-)
            t.Log(tc.value)
        })
    }
   

   }

With a better understanding of shadowing, I'm seeing this "hiding" was talking about shadowing the caller scope variable of tc by assigning inside the loop, thereby shadowing.

This seems like a good use case, though I still feel it is being "clever" rather if not commented, and violates the idiomatic Go goals of being clear and readable with no "magic".

CodePudding user response：

Wow, that is awful code. While it might not be a compiler enforced rule, if I see this:

import "github.com/stretchr/testify/assert"

Then in that file, as far as Im concerned, any identifer spelled assert is the top level import from that package, period. Anyone who breaks that pattern, in the name of being "clever" or whatever, is just asking for trouble. Peoples muscle memory is trained to recognize top level imports, so please, do not do stuff like this. If you want to avoid thinking of a new name, just remove some letters:

asrt := assert.New(t)

or add a letter:

nAssert := assert.New(t)

or alias the import:

import tAssert "github.com/stretchr/testify/assert"

Just please, do not follow the poor example given in that code.

CodePudding user response：

After reviewing all the feedback and writing I've seen I'm going to summarize the findings here.

If you are investigating this behavior after seeing Go linting alerts from a tool like Goland, hopefully this answer will save you same time and make clear that it's ok, idiomatic, and only in some cases of reusing the actual package calls after initializing would any issues be experienced.

Shadowing Package Names With Local Variables

• This is a matter of opinion, there is no official statement indicating this is considered non-idiomatic code.

• Opinions seem to overall lean towards avoiding shadowing unless intentionally done for clarity & readability, but this is again opinion, not a requirement.

• Once overridden, the package would be unavailable in that scope as the package name has been shadowed by the local variable.

• I've observed that this behavior seems more likely in Go than some languages, because Go prompts package names that are short, not multi-word, and easy to type. It seems inevitable that assert := assert.New(t) could result when this happens.

• Consistency, as always in matters like this, is the key. If a team determines to never shadow the package name, then assertions := assert.New(t) could be used, or as mentioned the package could be aliased such as import tassert "github.com/stretchr/testify/assert". This is an opinion standard, and either way is legal and considered idiomatic.

• In the case of testing, this is commonly done as further usage of the package after the initial variable set isn't used further. Example below using the is is package. Package author comment on this (again this is opinionated, but it doesn't break anything to use it with is := is.New(t).

package package_test

import (
    "testing"
    "package"
    
    iz "github.com/matryer/is"
)

func TestFuncName(t *testing.T) {
    is := iz.New(t)
    
    got := FuncName()
    want := ""
    
    is.Equal(got,want) // AssertMessage
}

Shadowing Behavior Can Address Goroutine Variable Issues

Shadowing is a known behavior and documented clearly in Effective Go.

The bug is that in a Go for loop, the loop variable is reused for each iteration, so the req variable is shared across all goroutines. That's not what we want.

It may seem odd to write req := req but it's legal and idiomatic in Go to do this. You get a fresh version of the variable with the same name, deliberately shadowing the loop variable locally but unique to each goroutine.

This can be solved without shadowing by passing in the variable as a parameter into the embedded func as well, simplifying the code. Both are legal and considered idiomatic per the guide.

Shadowing for Tests

Related to this goroutine behavior, but in the context of a testing, parallel tests run in goroutines, and therefore benefit from this pattern as well.

The tc := tc statement is required in the parallel test example because the closure runs in a goroutine. More generally, the tc := tc idiom used in the context of closures where the closure can be executed after the start of the next loop iteration - credit @gopher in comments

The gist on Be Careful With Table Driven Tests mentions this behavior as well in the How To Solve This.

Detecting

The check go vet can check for shadowing, but is not considered part of the stable package, and was removed here related to GitHub Issue 29260 and GitHub Issue 34053. The change seems to have been prompted for compatibility purposes due to the requirement of unsafeptr flag. Additional comments seem to point towards also requiring better heuristics before allowing in go vet without being experimental.

• Go Vet Shadow
• Instructions on running via go vet: Package Variables With Instructions:

go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
go vet -vettool=$(which shadow)

For those wanting to lint this, golangci-lint does have an option to configure shadowing checks in the configuration (search for check-shadowing as no bookmark to that text block`.

Further Reading

• Scope and Shadowing in Go
• Scopes and Closures