Skip to content

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

Closed
buglloc opened this issue Jan 11, 2022 · 5 comments
Closed
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@buglloc
Copy link

buglloc commented Jan 11, 2022

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:

package tst

import (
	"sync"
)

type mut struct{ mu sync.Mutex }

func foo(a interface{}) {
	if _, ok := a.(mut); ok {
		_ = ok
	}
}

copylock reports:

/home/buglloc/work/projects/copylock-fp/tst/tst.go:10:14: assignment copies lock value to _: (github.com/buglloc/copylock-fp/tst.mut, bool) contains github.com/buglloc/copylock-fp/tst.mut contains sync.Mutex

Is there any reason not to check that lhs in checkCopyLocksAssign is points to blank identifier?

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 11, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jan 11, 2022
@bcmills
Copy link
Contributor

bcmills commented Jan 11, 2022

Why would you need to do that kind of type-assertion? If mut is non-copyable, then it also isn't safe to copy a mut value into a variable of type interface{} — so the type that you're checking for should itself be impossible in this scenario.

(CC @timothy-king)

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 11, 2022
@timothy-king
Copy link
Contributor

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 _ identifiers should be straight-forward if it is desirable.

@buglloc
Copy link
Author

buglloc commented Jan 12, 2022

Sorry for the bad example :(
In my case, we use this type-assertion in reflection, just to ignore unnecessary struct.
Unfortunately, the code is private, but I have tried to rewrite it as closely as possible:

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.

@bcmills
Copy link
Contributor

bcmills commented Jan 12, 2022

The call to val.Interface() in that sequence copies the lock value that isn't supposed to be copied.

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()
	}

@buglloc
Copy link
Author

buglloc commented Jan 14, 2022

Yeah, you're right. It turns out that there really is no possible scenario :)

Thanks!

@buglloc buglloc closed this as completed Jan 14, 2022
@golang golang locked and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants