Update for Go 1.19 and support generics. #49

paddycarver · 2022-12-19T04:19:55Z

Update go.mod to indicate support for Go 1.19.

Add support for generic interfaces, i.e. interfaces that accept a type parameter.

For example, you can now have this kind of interface:

type Interface[Kind any] interface {
	DoTheThing() Kind
}

and if you run impl 's StringImpl' 'Interface[string] it will generate the following code:

func (s StringImpl) DoTheThing() string {
	// normal impl stub here
}

Fixes #44.

josharian

I haven't looked at this in any depth, just took a superficial pass. Gotta return to family now. Will look more carefully once the first round trip is complete. :)

go.mod

impl.go

paddycarver · 2022-12-22T23:40:48Z

Thanks for the review! I'll follow up on it when the chosen fam isn't making cookies with me, so I'll likely get to it this week. It's your repo so I'll do whatever you like, just want to explain a couple of the choices I made you raised questions about to make sure the suggested fix is actually what you want. 🙂

I am using this in anger locally, and while I think there are some iterative improvements that could happen (I'd love to be able to inject the types into the generated comments! Though I think some design work there may be needed...) it's been awesome to have working. (Though I gotta follow up with a highly suspect PR to vim-go as well. 😬)

Enjoy the family time! This will keep. 🙂

josharian · 2022-12-23T04:31:57Z

just want to explain a couple of the choices I made you raised questions about to make sure the suggested fix is actually what you want.

Absolutely. Code review is an iterative process in which multiple people collectively try to write clear, simple code. :)

This will keep.

Indeed. :) Happy holidays!

paddycarver · 2023-01-02T03:29:01Z

go.mod

-	golang.org/x/tools v0.4.0
-	golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
-)
+require golang.org/x/tools v0.4.0


I think this is just the result of running go mod tidy on main. If you get something different, please shout?

paddycarver · 2023-01-02T07:33:12Z

Think I resolved all the comments. Sorry, this has evolved a bit (I realized my handling of parsing out the type params was a bit too naive and switched it up for proper AST parsing) but hopefully I've commented things well enough that it's easier to follow, and hopefully the expansion makes it more robust.

josharian · 2023-01-21T19:22:01Z

Hi! Just letting you know I haven't forgotten about this. :)

paddycarver · 2023-01-21T22:17:23Z

Appreciate the update, but also no worries! You set clear expectations at the outset that it would get reviewed when you had time, and I'm content using my fork (and giving it some mileage in anger, seeing if I can break it/if I missed anything), so no pressure here. 😄

nieomylnieja · 2024-01-14T15:55:19Z

a year passed, is the project abandoned?

josharian · 2024-01-14T15:59:08Z

is the project abandoned?

No, its maintainer is just pretty seriously distractible. But amenable to gentle pings.

I'll aim to take a look sometime over the coming weeks.

Do you need this particular improvement?

nieomylnieja · 2024-01-15T17:23:36Z

@josharian I'd really appreciate it, although, as @paddycarver stated, not a blocker, I've built it with his changes :)

paddycarver · 2024-01-16T13:47:54Z

It happens to all of us!

I'll take a look at the conflicts here and see if I can resolve them quickly, but I know this turned into more of A Thing than I had originally expected. If it's too invasive/too complicated, I'd love that feedback, and I'll see if I can find a less-technically-correct-but-easier-to-implement version.

I have been using the version of impl this PR represents for about a year now, with some pretty heavy generics usage, and it seems to be working for me. vim-go requires some updates to allow quoting the input, but that's not a problem for this repo. 😁

Update go.mod to indicate support for Go 1.19. Add support for generic interfaces, i.e. interfaces that accept a type parameter. For example, you can now have this kind of interface: ```go type Interface[Kind any] interface { DoTheThing() Kind } ``` and if you run `impl 's StringImpl' 'Interface[string]` it will generate the following code: ```go func (s StringImpl) DoTheThing() string { // normal impl stub here } ``` Fixes josharian#44.

Turns out we need to use 1.18 because we have generics in our testdata package now, meaning `go test` won't run unless our go.mod is set to 1.18.

Add a comment documenting the purpose of parseTypeParams. Add tests exercising parseTypeParams. Simplify parseTypeParams by using strings.Cut instead of index math.

Instead of parsing generic types using string manipulation, parse them using the go/parse package to get the AST. Rather than bespokely handling the AST stuff everywhere, pull it out into a recursive helper function (we need it to be recursive because generics allow for some pretty complex AST constructs). Rather than passing around a type ID and its params everywhere, create a Type struct that contains both. Pull our type param matching out into a helper function.

Add a test for the interface assertion that was failing.

Had a lingering type assertion that should have used the ID from the parsed AST type instead. Also noticed that we were losing the package names on the type parameters, which would lead to potentially-subtle incorrect results; we should always be using the package if the user submits it. This requires an update to typeFromAST to preserve the package, and then updating findInterface to strip the package off when dealing with qualified interfaces.

paddycarver · 2024-01-16T13:56:25Z

Think I got the conflicts resolved. 😄 Tests still pass.

and let gofumpt scribble on the files

josharian

I pushed a handful of fixes / nitpicks. Just a few open questions...

josharian · 2024-01-16T18:17:30Z

impl.go

+		}
+		prefix := "["
+		if specType.Len != nil {
+			prefix += specType.Len.(*ast.BasicLit).Value


Does this work when the type is [...]T?

josharian · 2024-01-16T18:19:00Z

impl.go

+			return Type{}, err
+		}
+		return Type{
+			ID: "map[" + key.String() + "]" + value.String(),


in other places you're careful to only touch the ID, not the params, of subtypes but here, the params get clobbered. makes me feel unsure i understand what this code is doing.

josharian · 2024-01-16T18:19:26Z

impl.go

+}
+
+func typeFromAST(in ast.Expr) (Type, error) {
+	switch specType := in.(type) {


is there any way to avoid rebuilding an entire node printing infrastructure? it'd be nice to somehow use go/format or go/printer.

josharian · 2024-01-16T18:20:19Z

impl.go

+		// func Foo[Param1, Param2](context.Context, Param1) Param2
+		// We're gonna need to complicate everything to support that
+		// construction, and we don't actually need the deconstructed
+		// bits, so we're just... not going to deconstruct it at all.


can we use go/printer or go/format here? this is an awful lot of new code.

josharian · 2024-01-17T00:51:54Z

Oops. Of course, I can't push changes to your fork. The changes I added are in the generics branch on this repo: main...generics

paddycarver · 2024-01-17T05:51:47Z

2024-01-16 21_48_13-Update for Go 1 19 and support generics by paddycarver · Pull Request #49 · jos

Hmmm, you should be able to push to my branch.... I can pull those edits in when I get a chance if we can't figure out how to give you push access.

Since I opened this PR I became a dad, so while your questions are eminently fair, I don't know the answers without diving into it again and reloading a bunch of context, and that's a harder thing to achieve now than it was before. 😅 I'll put it on my to-do list, but please feel free to nudge me (anyone reading this) with a ping if it seems like I may have forgotten about it.

josharian · 2024-01-17T18:32:13Z

Since I opened this PR I became a dad

Congrats! Go be a father, I'll just wrap this up now.

The code can in fact be considerably simplified. I'll do that and then merge.

nieomylnieja · 2024-01-18T10:43:10Z

Thanks a lot for this one @paddycarver @josharian <3

paddycarver · 2024-01-19T01:24:01Z

Thank you! I'm glad you knew of a simpler way, that part of the PR is where I looked up and realized this might've gotten away from me a bit, so I appreciate the pointer to where the reusable functionality lives. 😀

paddycarver mentioned this pull request Dec 19, 2022

Add support for generics #44

Closed

josharian reviewed Dec 22, 2022

View reviewed changes

paddycarver force-pushed the generics branch from 965a125 to 7aa3e96 Compare January 2, 2023 03:24

paddycarver commented Jan 2, 2023

View reviewed changes

paddycarver requested a review from josharian January 2, 2023 07:48

paddycarver added 11 commits January 16, 2024 05:53

Remove some debugging changes.

e36c596

Revert go.mod to 1.14.

43f209f

Set go.mod back to 1.18.

cff8333

Turns out we need to use 1.18 because we have generics in our testdata package now, meaning `go test` won't run unless our go.mod is set to 1.18.

Document, simplify, and test parseTypeParams.

bc03717

Add a comment documenting the purpose of parseTypeParams. Add tests exercising parseTypeParams. Simplify parseTypeParams by using strings.Cut instead of index math.

Fix outdated comment.

2d3f13f

Update oudated comment.

e5ca4b6

Switch lingering if/elseif chain to a type switch.

e9c31be

Add test for panic.

9f0a8ce

Add a test for the interface assertion that was failing.

paddycarver force-pushed the generics branch from b783b8f to 50818c6 Compare January 16, 2024 13:54

josharian added 3 commits January 16, 2024 09:55

go.mod: update all deps

a52407f

rename type.ID to type.Name

902a46e

and let gofumpt scribble on the files

simplify code

9da095f

josharian added 4 commits January 16, 2024 10:15

clean docs

cce7cbd

add a test case

1e06e72

simplify error message

0b02767

reduce indentation

cc2523e

josharian reviewed Jan 16, 2024

View reviewed changes

fix CI, I hope

e0454a7

simplify code by using format.Node

5fa0fa9

josharian merged commit e6bec82 into josharian:main Jan 17, 2024
1 check passed

paddycarver deleted the generics branch January 19, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for Go 1.19 and support generics. #49

Update for Go 1.19 and support generics. #49

paddycarver commented Dec 19, 2022

josharian left a comment

paddycarver commented Dec 22, 2022

josharian commented Dec 23, 2022

paddycarver Jan 2, 2023

paddycarver commented Jan 2, 2023

josharian commented Jan 21, 2023

paddycarver commented Jan 21, 2023

nieomylnieja commented Jan 14, 2024

josharian commented Jan 14, 2024 •

edited

nieomylnieja commented Jan 15, 2024

paddycarver commented Jan 16, 2024

paddycarver commented Jan 16, 2024

josharian left a comment

josharian Jan 16, 2024

josharian Jan 16, 2024

josharian Jan 16, 2024

josharian Jan 16, 2024

josharian commented Jan 17, 2024

paddycarver commented Jan 17, 2024

josharian commented Jan 17, 2024

nieomylnieja commented Jan 18, 2024

paddycarver commented Jan 19, 2024

Update for Go 1.19 and support generics. #49

Update for Go 1.19 and support generics. #49

Conversation

paddycarver commented Dec 19, 2022

josharian left a comment

Choose a reason for hiding this comment

paddycarver commented Dec 22, 2022

josharian commented Dec 23, 2022

paddycarver Jan 2, 2023

Choose a reason for hiding this comment

paddycarver commented Jan 2, 2023

josharian commented Jan 21, 2023

paddycarver commented Jan 21, 2023

nieomylnieja commented Jan 14, 2024

josharian commented Jan 14, 2024 • edited

nieomylnieja commented Jan 15, 2024

paddycarver commented Jan 16, 2024

paddycarver commented Jan 16, 2024

josharian left a comment

Choose a reason for hiding this comment

josharian Jan 16, 2024

Choose a reason for hiding this comment

josharian Jan 16, 2024

Choose a reason for hiding this comment

josharian Jan 16, 2024

Choose a reason for hiding this comment

josharian Jan 16, 2024

Choose a reason for hiding this comment

josharian commented Jan 17, 2024

paddycarver commented Jan 17, 2024

josharian commented Jan 17, 2024

nieomylnieja commented Jan 18, 2024

paddycarver commented Jan 19, 2024

josharian commented Jan 14, 2024 •

edited