-
Notifications
You must be signed in to change notification settings - Fork 18k
x/tools/go/analysis/passes/copylock: allow assignment to blank identifier #50551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Why would you need to do that kind of type-assertion? If (CC @timothy-king) |
Like @bcmills, I am not yet really sure why we would not want to warn on this code example though. More details or a more realistic example might help here. FWIW suppressing the check on assignment to |
Sorry for the bad example :( type Registrator interface {
Register()
}
type Struct struct {
sync.Mutex
Nested map[string]Registrator
}
func register(v reflect.Value) bool {
c, ok := v.Interface().(Registrator)
if ok {
c.Register()
}
return ok
}
func RegisterRecursively(value interface{}) {
val := reflect.ValueOf(value)
if val.Kind() == reflect.Ptr {
val = val.Elem()
}
if val.Kind() != reflect.Struct {
return
}
if _, ok := val.Interface().(Struct); ok {
// A Struct handles registration on its own
return
}
for i := 0; i < val.NumField(); i++ {
field := val.Field(i)
if !field.CanInterface() {
continue
}
if register(field) {
continue
}
if field.Kind() == reflect.Ptr {
field = field.Elem()
}
switch field.Kind() {
case reflect.Map:
for _, item := range field.MapKeys() {
register(field.MapIndex(item))
}
case reflect.Struct:
RegisterRecursively(val.Field(i).Interface())
}
}
} I agree with you that this code looks rather weird and can be rewritten, but safe for non-copyable objects I guess. |
The call to IMO one clearer (and more correct) way to write that reflection code is: if val.Kind() == reflect.Ptr {
if val.Type() == reflect.TypeOf((*Struct)(nil)) {
// A *Struct handles registration on its own.
return
}
val = val.Elem()
} |
Yeah, you're right. It turns out that there really is no possible scenario :) Thanks! |
After copylock was able to detect copylock in multi-assignment (golang/tools#319) it starts report false positives on test type assertions.
For example, for code:
copylock reports:
Is there any reason not to check that lhs in checkCopyLocksAssign is points to blank identifier?
The text was updated successfully, but these errors were encountered: