Skip to content

Use Origin() in Go extractor#13739

Merged
owen-mc merged 3 commits into
github:mainfrom
owen-mc:go/extractor-use-origin
Jul 18, 2023
Merged

Use Origin() in Go extractor#13739
owen-mc merged 3 commits into
github:mainfrom
owen-mc:go/extractor-use-origin

Conversation

@owen-mc

@owen-mc owen-mc commented Jul 13, 2023

Copy link
Copy Markdown
Contributor

When Go 1.18 was released with support for Generics, we had go to extreme lengths in the extractor to get some information. A much easier way to get that information was added in Go 1.19 in the form of the Origin() method on *types.Var and *types.Func. This PR uses those methods in the extractor.

@github-actions github-actions Bot added the Go label Jul 13, 2023
@owen-mc owen-mc force-pushed the go/extractor-use-origin branch from ab3a7bb to 54708ce Compare July 14, 2023 10:13
@owen-mc owen-mc force-pushed the go/extractor-use-origin branch from 54708ce to cff09d2 Compare July 14, 2023 12:53
@owen-mc owen-mc changed the title Test whether Object.Origin() does what we want Test using Origin() in Go extractor Jul 14, 2023
@owen-mc owen-mc force-pushed the go/extractor-use-origin branch from a006a09 to a2a2e93 Compare July 15, 2023 06:06
@owen-mc owen-mc marked this pull request as ready for review July 17, 2023 15:12
@owen-mc owen-mc requested a review from a team as a code owner July 17, 2023 15:12
@owen-mc owen-mc changed the title Test using Origin() in Go extractor Use Origin() in Go extractor Jul 17, 2023
@owen-mc

owen-mc commented Jul 17, 2023

Copy link
Copy Markdown
Contributor Author

Performance tests have shown no difference either way. The new code is simpler. This is ready for review.

@smowton

smowton commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

Suggest manually verifying if this works when the environmental Go is 1.17, since there's a risk packages.Load is getting some of this data from invoking go to dump the AST. It certainly uses the environmental Go for go list; unsure if it uses it for AST / type information, so it'd be good to be sure if we're effectively dropping 1.17 compatibility because that compiler might not provide the origin member. Otherwise LGTM.

@owen-mc

owen-mc commented Jul 18, 2023

Copy link
Copy Markdown
Contributor Author

@smowton I checked out this branch, ran make in go to build the extractor, uninstalled go and installed go 1.17, confirmed that was the only version of go installed, then ran all the tests. No problems extracting. A handful of tests failed but they were related to things missing from the libraries.

@owen-mc owen-mc merged commit 9b0d7f3 into github:main Jul 18, 2023
@owen-mc owen-mc deleted the go/extractor-use-origin branch July 18, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants