U1000: reduce false positive that relates to type parameters #1448

seiyab · 2023-09-24T09:54:08Z

Addresses #1294.

NOTE
Probably some false-positives for complex generic types still exist. And this doesn't address other similar problems such as #1440.

Though I consider this makes a improvement, feel free to reject this if you feel this incomplete fix can be harmful.

seiyab · 2023-11-01T11:24:37Z

@dominikh any chance to get reviewed?

dominikh · 2023-11-01T12:42:51Z

I'll take a look when I get the chance

dominikh · 2023-11-06T13:36:42Z

I'm wondering if instead we can use https://pkg.go.dev/golang.org/x/exp/typeparams#GenericAssignableTo — that is, replace our implementation of implements with a modified version of GenericAssignableTo.

seiyab · 2023-11-06T23:04:27Z

Thank you for your review. I'll take a look.

seiyab · 2023-11-07T04:02:53Z

~~I locally tried https://pkg.go.dev/golang.org/x/exp/typeparams#GenericAssignableTo and it successfully passed the tests.~~
~~Perhaps it might also resolve opposite false positive as #1440 reports. I'll investigate more and might add more tests.~~

seiyab · 2023-11-07T09:36:18Z

Ah, I mistakenly tested ./statickcheck/... instead of ./unused/.... Testing ./unused/..., it fails. I'll investigate it anyway.

seiyab · 2023-11-07T12:49:10Z

@dominikh
I found that we can't utilize types.GenericAssignableTo.
types.GenericAssignableTo looks to consider V and T have same numbers of type parameters. (If not, it falls back types.AssignableTo)

If V and T are generic named types, then V is considered assignable to T if, for every possible instantation of V[A_1, ..., A_N], the instantiation T[A_1, ..., A_N] is valid and V[A_1, ..., A_N] implements T[A_1, ..., A_N].

In contrast, to resolve #1294, we want to check whether concrete type V is assignable to one of instantiation of T[A_1, ..., A_N] or not.

dominikh

Overall the code looks fine.

I'm very torn on the overall change, however. On the one hand, this fixes some of the common false positives. On the other hand, the incomplete nature of the change means that minor changes to an interface can reintroduce a false positive.

For example, we'd be able to correctly handle type I[T any] interface { foo() T }, but not type I[T any] interface { foo() []T }. This might be more confusing for users, who will not understand why a minor change to the interface introduces a new false positive.

/cc @mvdan for his opinion.

Note that I don't think we can realistically solve this problem unless we reuse the whole unification step of go/types, which unfortunately isn't currently exported. I'll look into rectifying that.

dominikh · 2023-11-02T22:11:00Z

unused/implements.go

+
+func satisfiesConstraint(t types.Type, tp *types.TypeParam) bool {
+	bound, ok := tp.Underlying().(*types.Interface)
+	if !ok {


types.TypeParam.Underlying is guaranteed to be a *types.Interface (for valid programs, and only valid programs reach our checks), so we can drop the , ok — I'd rather this panic if that invariant is ever violated.

dominikh · 2023-11-02T22:15:44Z

unused/implements.go

+	if !ok {
+		return false
+	}
+	return types.Implements(t, bound)


This should probably use types.Satisfies

Thanks, changing it into types.Satisfies() make some tests fail and clarify that I'm misunderstanding something around here. Probably bound should also be tp.Constraint() or something but it sometimes returns Type which isn't *types.Interface so it can't obviously passed to types.Satisfies().
I'll investigate it and add more test cases.

dominikh · 2023-11-06T02:22:33Z

unused/implements.go

+}
+
+func newMethodChecker() *methodsChecker {
+	return &methodsChecker{


I'd like to avoid this allocation when there are no type parameters.

dominikh · 2023-11-06T02:26:20Z

unused/implements.go

+	if types.Identical(implType, interfaceType) {
+		return true
+	}
+	tp, ok := interfaceType.(*types.TypeParam)


This won't support methods like foo(x []T). We should at least have a comment here to document this limitation.

This still needs a comment stating that we only support trivial uses of type parameters and that this doesn't constitute full unification.

dominikh · 2023-11-07T14:03:54Z

@dominikh I found that we can't utilize types.GenericAssignableTo. types.GenericAssignableTo looks to consider V and T have same numbers of type parameters. (If not, it falls back types.AssignableTo)

If V and T are generic named types, then V is considered assignable to T if, for every possible instantation of V[A_1, ..., A_N], the instantiation T[A_1, ..., A_N] is valid and V[A_1, ..., A_N] implements T[A_1, ..., A_N].

In contrast, to resolve #1294, we want to check whether concrete type V is assignable to one of instantiation of T[A_1, ..., A_N] or not.

Thank you for looking into this.

dominikh · 2023-11-07T14:13:27Z

Thinking more about this, I think we should merge this PR, then improve upon it with better/full unification in a future change.

seiyab · 2023-11-09T14:27:14Z

@dominikh Please take another look.
Thank you for your review. Some my confusions are solved and it get better.

For avoiding allocation, I can't find a lightweight approach to know whether a *type.Interface has any type parameters or not. So I use a value of methodChecker instead its reference. It can't go out of the function implements() scope and should be put on stack. And I also make() typeParams map lazily.

mvdan · 2023-11-17T13:48:39Z

@dominikh I think your analysis above is sensible. I think we should push go/types to expose more APIs if they are useful for tools.

dominikh · 2023-11-17T15:36:41Z

@dominikh I think your analysis above is sensible. I think we should push go/types to expose more APIs if they are useful for tools.

FWIW, I've filed golang/go#63982

dominikh · 2023-11-17T16:27:04Z

unused/implements.go

+	typeParams map[*types.TypeParam]types.Type
+}
+
+func newMethodChecker() methodsChecker {


We don't need this constructor.

dominikh · 2023-11-17T16:27:19Z

unused/implements.go

@@ -60,6 +62,7 @@ func implements(V types.Type, T *types.Interface, msV *types.MethodSet) ([]*type

 	// A concrete type implements T if it implements all methods of T.
 	var sels []*types.Selection
+	c := newMethodChecker()


This can simply be var c methodsChecker

dominikh · 2023-11-17T16:28:52Z

unused/implements.go

+func satisfiesConstraint(t types.Type, tp *types.TypeParam) bool {
+	bound, ok := tp.Constraint().Underlying().(*types.Interface)
+	if !ok {
+		panic("unexpected failure on type assertion (types.Type to *types.Interface)")


Just do bound := tp.Constraint().Underlying().(*types.Interface) — we don't need to check for ok just to manually panic — the panic we get on the failed type assertion is fine.

dominikh · 2023-11-17T16:29:55Z

unused/implements.go

+	if types.Identical(implType, interfaceType) {
+		return true
+	}
+	tp, ok := interfaceType.(*types.TypeParam)


This still needs a comment stating that we only support trivial uses of type parameters and that this doesn't constitute full unification.

- remove constructor - remove manual panic - add a comment

seiyab · 2023-11-18T04:10:07Z

@dominikh Thank you for your review. I fixed them.

dominikh

LGTM, will merge in a couple days.

seiyab added 4 commits September 24, 2023 18:46

U1000: reduce false positive that relates to type parameters

bd354bd

conservative error handling

9b6be6f

refactor (reduce duplication)

d3cb406

refactor: reduce duplication

f60de78

dominikh requested changes Nov 7, 2023

View reviewed changes

seiyab added 5 commits November 9, 2023 21:59

meet easy change requests

1d6b0b5

improve type check and add tests

a099c11

add tests

2026271

add test case

3d02c4e

reduce heap allocation

34b901a

seiyab requested a review from dominikh November 17, 2023 14:48

dominikh requested changes Nov 17, 2023

View reviewed changes

meet change requests

e730679

- remove constructor - remove manual panic - add a comment

seiyab requested a review from dominikh November 18, 2023 04:09

dominikh approved these changes Nov 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

U1000: reduce false positive that relates to type parameters #1448

U1000: reduce false positive that relates to type parameters #1448

seiyab commented Sep 24, 2023 •

edited

seiyab commented Nov 1, 2023

dominikh commented Nov 1, 2023

dominikh commented Nov 6, 2023

seiyab commented Nov 6, 2023

seiyab commented Nov 7, 2023 •

edited

seiyab commented Nov 7, 2023

seiyab commented Nov 7, 2023

dominikh left a comment

dominikh Nov 2, 2023

dominikh Nov 2, 2023

seiyab Nov 8, 2023

dominikh Nov 6, 2023

dominikh Nov 6, 2023

dominikh Nov 17, 2023

dominikh commented Nov 7, 2023

dominikh commented Nov 7, 2023

seiyab commented Nov 9, 2023

mvdan commented Nov 17, 2023

dominikh commented Nov 17, 2023

dominikh Nov 17, 2023

dominikh Nov 17, 2023

dominikh Nov 17, 2023

dominikh Nov 17, 2023

seiyab commented Nov 18, 2023

dominikh left a comment

U1000: reduce false positive that relates to type parameters #1448

Are you sure you want to change the base?

U1000: reduce false positive that relates to type parameters #1448

Conversation

seiyab commented Sep 24, 2023 • edited

seiyab commented Nov 1, 2023

dominikh commented Nov 1, 2023

dominikh commented Nov 6, 2023

seiyab commented Nov 6, 2023

seiyab commented Nov 7, 2023 • edited

seiyab commented Nov 7, 2023

seiyab commented Nov 7, 2023

dominikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikh commented Nov 7, 2023

dominikh commented Nov 7, 2023

seiyab commented Nov 9, 2023

mvdan commented Nov 17, 2023

dominikh commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seiyab commented Nov 18, 2023

dominikh left a comment

Choose a reason for hiding this comment

seiyab commented Sep 24, 2023 •

edited

seiyab commented Nov 7, 2023 •

edited