Fix gocognit output when a function has generic receiver type #10

ivanruski · 2022-05-01T17:15:36Z

recvString func in gocognit returns BADRECV when the function's receiver is a
generic type.

uudashr · 2022-06-16T17:08:02Z

Do you have a file to test?

ivanruski · 2022-06-22T05:38:49Z

Yes, below is my testing file. I realised that I wasn't handling the scenario when the receiver has multiple type params.

I've implemented the missing piece. I can close this PR and open another one with single commit if you'd like.

package main

type Node[T any] struct {
}

func (n *Node[T]) String() string {
	if n != nil {
		return "Node"
	}

	return ""
}

type Pair[K any, V any] struct {
	Key   K
	Value V
}

func (p *Pair[K, V]) String() string {
	if p != nil {
		return "Pair"
	}

	return ""
}

type Triple[K any, V any, T any] struct {
}

func (t *Triple[K, V, T]) String() string {
	if t != nil {
		return "Triple"
	}

	return ""
}

func main() {
}

uudashr · 2022-06-22T15:06:16Z

You can modify the existing PR or create the new one.

ivanruski · 2022-06-23T07:23:48Z

It's a single commit now.

uudashr · 2022-06-28T03:39:13Z

Hi @ivanruski, thanks for the awesome work.
Btw, I've seen ast.IndexListExpr cannot be found on go1.16. How to make this compatible with go1.16?

uudashr

Also need to make sure this will compatible previous golang version. At least start from 1.16

uudashr · 2022-06-29T00:40:33Z

gocognit.go

+	case *ast.IndexListExpr:
+		targs := make([]string, 0, len(t.Indices))
+		for _, exp := range t.Indices {
+			targs = append(targs, recvString(exp))


The targs slice was created using make with the non-zero length. Why do we still use append?

I am using the second version of make for creating slices, where the second argument is the length of the slice and the third argument is the capacity of the underlying array. In this case go won't resize the underlying array, because I specified enough capacity. https://go.dev/play/p/rZmxsFAvmF1.

I can change it to:

targs := make([]string, len(t.Indices)) for i, exp := range t.Indices { targs[i] = recvString(exp) }

You are absolutely right. The code works.

Btw, it seems that using assignments is faster (most of the time). What do you think?

see: https://github.com/uudashr/go-bench and try to run

$ go test -benchmem -run="^$" -bench="^BenchmarkSlice_PreAllocated" goos: darwin goarch: arm64 pkg: github.com/uudashr/go-bench BenchmarkSlice_PreAllocated_Assign-8 1000000000 0.0000005 ns/op 0 B/op 0 allocs/op BenchmarkSlice_PreAllocated_Append-8 1000000000 0.0000010 ns/op 0 B/op 0 allocs/op PASS ok github.com/uudashr/go-bench 0.110s

Yes, I observe the same thing (that assigning is faster). I will changed it.

goos: darwin goarch: arm64 pkg: github.com/uudashr/go-bench BenchmarkSlice_PreAllocated_Assign-10 1000000000 0.0000004 ns/op BenchmarkSlice_PreAllocated_Append-10 1000000000 0.0000006 ns/op PASS ok github.com/uudashr/go-bench 0.239s

ivanruski · 2022-06-29T20:40:49Z

Hi @ivanruski, thanks for the awesome work.
Btw, I've seen ast.IndexListExpr cannot be found on go1.16. How to make this compatible with go1.16?

Hi @uudashr, good catch I've tested it only with go1.18 and missed that.
Right now if we try to run gocognit against the testing file above on a machine with go1.16, line 99 in main.go (...parser.ParseFile...) will return an error telling us that the file is invalid.

I am mentioning this because I am not sure how to make it work for 1.16.

If someone is using just the build output, they won't have any issues even if the version was upgraded to 1.18, however if someone with go version < 1.18 is trying to import the project they won't be able to.

ivanruski · 2022-06-29T21:03:00Z

I've updated the go version in the go.mod in this commit because the changes here will work only for go1.18 or higher.

uudashr · 2022-06-30T11:35:32Z

Hi @uudashr, good catch I've tested it only with go1.18 and missed that. Right now if we try to run gocognit against the testing file above on a machine with go1.16, line 99 in main.go (...parser.ParseFile...) will return an error telling us that the file is invalid.

I am mentioning this because I am not sure how to make it work for 1.16.

If someone is using just the build output, they won't have any issues even if the version was upgraded to 1.18, however if someone with go version < 1.18 is trying to import the project they won't be able to.

Maybe we can use build constraints/build tag, for example:
By having recv_pre118.go for the pre 1.18 and recv.go for 1.18 or higher

See:

uudashr · 2022-06-30T10:03:41Z

go.mod

@@ -1,5 +1,11 @@
 module github.com/uudashr/gocognit

-go 1.16
+go 1.18


Go 1.16 is the minimum required version, I don't think we need to change this to 1.18. Let's make it backward compatible. I've put a comment on the PR.
ref: https://go.dev/doc/modules/gomod-ref

uudashr · 2022-06-30T10:44:52Z

gocognit.go

+	case *ast.IndexListExpr:
+		targs := make([]string, 0, len(t.Indices))
+		for _, exp := range t.Indices {
+			targs = append(targs, recvString(exp))


You are absolutely right. The code works.

Btw, it seems that using assignments is faster (most of the time). What do you think?

see: https://github.com/uudashr/go-bench and try to run

$ go test -benchmem -run="^$" -bench="^BenchmarkSlice_PreAllocated" goos: darwin goarch: arm64 pkg: github.com/uudashr/go-bench BenchmarkSlice_PreAllocated_Assign-8 1000000000 0.0000005 ns/op 0 B/op 0 allocs/op BenchmarkSlice_PreAllocated_Append-8 1000000000 0.0000010 ns/op 0 B/op 0 allocs/op PASS ok github.com/uudashr/go-bench 0.110s

uudashr · 2022-06-30T11:40:45Z

Can we put the test file under the testdata

Yes, below is my testing file. I realised that I wasn't handling the scenario when the receiver has multiple type params.

I've implemented the missing piece. I can close this PR and open another one with single commit if you'd like.

package main

type Node[T any] struct {
}

func (n *Node[T]) String() string {
	if n != nil {
		return "Node"
	}

	return ""
}

type Pair[K any, V any] struct {
	Key   K
	Value V
}

func (p *Pair[K, V]) String() string {
	if p != nil {
		return "Pair"
	}

	return ""
}

type Triple[K any, V any, T any] struct {
}

func (t *Triple[K, V, T]) String() string {
	if t != nil {
		return "Triple"
	}

	return ""
}

func main() {
}

uudashr · 2022-06-30T13:45:19Z

I already setup github actions to run tests on various go versions on every PR.

If you want to test it locally try to use

docker run --rm -i -v $(pwd):/src uudashr/golang-test:1.16

recvString func in gocognit returns BADRECV when the function's receiver is a generic type. Implement recvString in two variants in order to keep the code compatible with go versions less than 1.18

ivanruski · 2022-06-30T22:07:02Z

Thanks for all of the references and info. I've changed my code based on your feedback. I've tested it with go 1.18 and go 1.16.

Regarding the test file - should I create a third test and keep the same structure (to have a separate file in it's own dir e.g. testdata/src/c.go)?

uudashr · 2022-07-01T03:46:23Z

I think we can create it under testdata/src/c only to test generic function. Just to ensure it can calculate the complexity and not return the BADRECV

Regarding the test file - should I create a third test and keep the same structure (to have a separate file in it's own dir e.g. testdata/src/c.go)?

uudashr

Awesome work. Just need to have unit test for the go1.18 generic

ivanruski · 2022-07-01T12:35:31Z

Hi @uudashr , I've added the test in another file with build tag for go1.18. I didn't put the other tests in *pre118_test.go file because they should be executed regardless of the go version. Is that okay ?

uudashr · 2022-07-01T13:06:56Z

gocognit118_test.go

+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzerWithGenericMethods(t *testing.T) {


What if we name it TestAnalyzer_Generic or something not specific to methods?
The test that we have so far is to test the ability to show the struct/receiver name which has generics declaration. The method itself does not use the generic at all.

So in the future, we can start to add test/capability for gocognit to also works with generic methods and functions.
Ex:

func SumNumbers[K comparable, V Number](m map[K]V) V { var s V for _, v := range m { s += v } return s }

Sure.

I was wondering whether to add an example with generic function, but I didn't do it because recvString only cares about the functions name.

changed the name to be plural - TestAnalyzer_Generics.

uudashr

Looks awesome

uudashr · 2022-07-01T17:39:50Z

HI @ivanruski for such a case

package testdata

type Number interface {
	int64 | float64
}

func SumNumbers[K comparable, V Number](m map[K]V) V {
	var s V
	for _, v := range m {
		s += v
	}
	return s
}

And run the gocognit

$ gocognit file.go
1 testdata SumNumbers file.go:7:1

SumNumbers function shows no generic information. Do you think we need to show the generic information on the receiver string representation?

Without generic information, we still can identify it clearly and it has the same string representation standard as the generic functions/methods

Ex: without generic information

1 testdata (*Triple).String c.go:30:1
1 testdata (*Pair).String c.go:19:1
1 testdata (*Node).String c.go:6:1

compare to this with generic information

1 testdata (*Triple[K, V, T]).String c.go:30:1
1 testdata (*Pair[K, V]).String c.go:19:1
1 testdata (*Node[T]).String c.go:6:1

ivanruski · 2022-07-02T08:07:18Z

Hi @uudashr,

At first when I found out that generic types are represented as ast.IndexExpr and ast.IndexListExpr I've decided to use all of the information provided by these types, because I said to my self - type parameters are part of the type definition so I will use them (the same is true for functions though).

Giving it a second thought now, I see your point and I think that omitting the generic part of the receivers won't negatively impact the output of the program. On the contrary as you said, we will still be able to identify the method, and the output of the program will be simpler.

So I'm onboard with changing the recvString to something like:

func recvString(recv ast.Expr) string {
	switch t := recv.(type) {
	case *ast.Ident:
		return t.Name
	case *ast.StarExpr:
		return "*" + recvString(t.X)
	case *ast.IndexExpr:
		return recvString(t.X)
	case *ast.IndexListExpr:
		return recvString(t.X)
	}
	return "BADRECV"
}

I can open another PR if you'd like.

uudashr · 2022-07-02T15:06:56Z

Another PR would be great. Thanks man

ivanruski force-pushed the master branch from d79a89f to 6ed2d52 Compare June 23, 2022 07:23

uudashr requested changes Jun 29, 2022

View reviewed changes

ivanruski force-pushed the master branch from 6ed2d52 to 9e93e1a Compare June 29, 2022 20:59

uudashr requested changes Jun 30, 2022

View reviewed changes

Fix gocognit output when a function has generic receiver type

bf94da4

recvString func in gocognit returns BADRECV when the function's receiver is a generic type. Implement recvString in two variants in order to keep the code compatible with go versions less than 1.18

ivanruski force-pushed the master branch from 9e93e1a to bf94da4 Compare June 30, 2022 22:02

ivanruski requested a review from uudashr June 30, 2022 22:07

uudashr approved these changes Jul 1, 2022

View reviewed changes

uudashr requested changes Jul 1, 2022

View reviewed changes

ivanruski requested a review from uudashr July 1, 2022 12:35

uudashr requested changes Jul 1, 2022

View reviewed changes

ivanruski force-pushed the master branch from 8df0047 to 92e616a Compare July 1, 2022 13:21

Add unit test for methods with generic receivers

c660021

ivanruski force-pushed the master branch from 92e616a to c660021 Compare July 1, 2022 13:25

ivanruski requested a review from uudashr July 1, 2022 13:26

uudashr approved these changes Jul 1, 2022

View reviewed changes

uudashr merged commit 150dee4 into uudashr:master Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gocognit output when a function has generic receiver type #10

Fix gocognit output when a function has generic receiver type #10

ivanruski commented May 1, 2022

uudashr commented Jun 16, 2022

ivanruski commented Jun 22, 2022 •

edited

uudashr commented Jun 22, 2022

ivanruski commented Jun 23, 2022

uudashr commented Jun 28, 2022

uudashr left a comment

uudashr Jun 29, 2022

ivanruski Jun 29, 2022

uudashr Jun 30, 2022

ivanruski Jun 30, 2022

ivanruski commented Jun 29, 2022 •

edited

ivanruski commented Jun 29, 2022

uudashr commented Jun 30, 2022

uudashr Jun 30, 2022

uudashr Jun 30, 2022

uudashr commented Jun 30, 2022 •

edited

uudashr commented Jun 30, 2022

ivanruski commented Jun 30, 2022 •

edited

uudashr commented Jul 1, 2022

uudashr left a comment

ivanruski commented Jul 1, 2022

uudashr Jul 1, 2022

ivanruski Jul 1, 2022

ivanruski Jul 1, 2022

uudashr left a comment

uudashr commented Jul 1, 2022

ivanruski commented Jul 2, 2022

uudashr commented Jul 2, 2022

Fix gocognit output when a function has generic receiver type #10

Fix gocognit output when a function has generic receiver type #10

Conversation

ivanruski commented May 1, 2022

uudashr commented Jun 16, 2022

ivanruski commented Jun 22, 2022 • edited

uudashr commented Jun 22, 2022

ivanruski commented Jun 23, 2022

uudashr commented Jun 28, 2022

uudashr 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

ivanruski commented Jun 29, 2022 • edited

ivanruski commented Jun 29, 2022

uudashr commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr commented Jun 30, 2022 • edited

uudashr commented Jun 30, 2022

ivanruski commented Jun 30, 2022 • edited

uudashr commented Jul 1, 2022

uudashr left a comment

Choose a reason for hiding this comment

ivanruski commented Jul 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uudashr left a comment

Choose a reason for hiding this comment

uudashr commented Jul 1, 2022

ivanruski commented Jul 2, 2022

uudashr commented Jul 2, 2022

ivanruski commented Jun 22, 2022 •

edited

ivanruski commented Jun 29, 2022 •

edited

uudashr commented Jun 30, 2022 •

edited

ivanruski commented Jun 30, 2022 •

edited