Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmd/vet: audit / update vet analyzer handling of generic code #48704

Closed
28 tasks done
findleyr opened this issue Sep 30, 2021 · 25 comments
Closed
28 tasks done

cmd/vet: audit / update vet analyzer handling of generic code #48704

findleyr opened this issue Sep 30, 2021 · 25 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.18

Comments

@findleyr
Copy link
Member

findleyr commented Sep 30, 2021 •
edited
Loading

Umbrella issue: we need to verify that the vet analyzers in x/tools are well behaved with generic code, before they are vendored into the standard library as part of the Go 1.18 release cycle.

The majority of the analyzers require no additional logic for the new language constructs. In most of these cases, we should still add test coverage. This coverage needs to be added both in x/tools, and in cmd/vet/testdata. We can start with x/tools and add tests to cmd/vet once our changes have been vendored.

In some cases, analyzers will require new logic to handle type parameters. For example, the printf analyzer could consider the structural restrictions on a type parameter (e.g. interface { ~int }), and offer diagnostics if a particular verb is incompatible with this structural restriction (I put together a proof of concept CL for this). But the solution in that CL is not obviously correct: we can't know whether a type argument implements fmt.Formatter. Where such non-trivial decisions are required, we should have a separate proposal issue that will be referenced here.

I'll keep this top comment updated with the current state of analyzers:

CC @timothy-king @golang/tools-team

(N.B.: I'm filing this issue during quiet week, because we need it to track ongoing CLs. Please note that we might not be responsive to discussion about vet changes this week, but that there will be time for discussion later).
@findleyr findleyr added this to the Go1.18 milestone Sep 30, 2021
@findleyr findleyr added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 30, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353210 mentions this issue: go/analysis/passes/unmarshal: allow unmarshalling to a type parameter

gopherbot pushed a commit to golang/tools that referenced this issue Oct 4, 2021
@stamblerre
go/analysis/passes/unmarshal: allow unmarshalling to a type parameter
ccaa907 
We can also unmarshal data to a type parameter, in addition to a pointer
and an interface.

This analyzer probably requires more discussion, but this solution
should be sufficient for now.

Updates golang/go#48704

Change-Id: I333f919109295e80a04e59df131713553cdbe612
Reviewed-on: https://go-review.googlesource.com/c/tools/+/353210
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353550 mentions this issue: go/analysis/passes/atomic: add a test that uses typeparams

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353629 mentions this issue: go/analysis/passes/bools: add typeparams test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 5, 2021
@hyangah
go/analysis/passes/atomic: add a test that uses typeparams
d477ef3 
This atomic check simply inspects whether sync/atomic.Add*
are used and LHS looks identical to the first arg of the call.
In the current implementation, this does not require handling
of type params. This test shows the addition of typeparams
doesn't crash the vet.

Update golang/go#48704

Change-Id: I79f9d782595f6bf2db82237afdbef1ffdf6f808e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/353550
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/354610 mentions this issue: go/analysis/passes/assign: add typeparams test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 10, 2021
@zpavlinovic @odeke-em
go/analysis/passes/assign: add typeparams test
1af23bd 
This CL adds a test for the assign pass that involves use of generics.

Update golang/go#48704

Change-Id: I355e73130c54bdc2363c686a5b28fe3140a307b5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/354610
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
@findleyr findleyr mentioned this issue Oct 14, 2021
Closed
jmesyou added a commit to jmesyou/golang-tools that referenced this issue Oct 20, 2021
@jmesyou
go/analysis/passes/unusedresult: add test for typeparams
e039fca 
This CL adds a test for unused results inside a function using generics.

Update golang/go#48704
jmesyou added a commit to jmesyou/golang-tools that referenced this issue Oct 20, 2021
@jmesyou
go/analysis/passes/unusedresult: add test for typeparams
ed4c84e 
This CL adds a test for unused results inside a function using generics.

Update golang/go#48704
@jmesyou jmesyou mentioned this issue Oct 20, 2021
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357411 mentions this issue: go/analysis/passes/unusedresult: add test for typeparams

jmesyou pushed a commit to jmesyou/golang-tools that referenced this issue Oct 20, 2021
@jmesyou
go/analysis/passes/unusedresult: add test for typeparams
d60ca4a 
This CL adds a test for unused results inside a function using generics.

Update golang/go#48704
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357412 mentions this issue: go/analysis/passes/deepequalerrors: add typeparams test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 21, 2021
@zpavlinovic
go/analysis/passes/deepequalerrors: add typeparams test
4100dac 
This CL adds a test for the assign pass that involves use of generics.

Update golang/go#48704

Change-Id: I1aa51aed24d63d42b6b0150d64925b97dfd59a92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/357412
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357777 mentions this issue: go/analysis/passes/ctrlflow: add typeparams test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357756 mentions this issue: go/analysis/passes/composite: update for generics

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357758 mentions this issue: go/analysis/passes/shift: update for generics

gopherbot pushed a commit to golang/tools that referenced this issue Oct 25, 2021
@findleyr
go/analysis/passes/composite: update for generics
1050b5c 
Warn about unkeyed composite literals via literals of type parameter
type.

Updates golang/go#48704

Change-Id: Ia75139b56cb73288c133bd547d71664464015813
Reviewed-on: https://go-review.googlesource.com/c/tools/+/357756
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/358695 mentions this issue: go/analysis/passes/cgocall: add typeparams test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/358619 mentions this issue: go/analysis/passes/ifaceassert: add a typeparams test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/358694 mentions this issue: go/analysis/passes/lostcancel: add typeparams test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/358696 mentions this issue: go/analysis/passes/httpresponse: add typeparams test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 26, 2021
@guodongli-google
go/analysis/passes/httpresponse: add typeparams test
903c757 
testdata/src/typeparams is similar to testdata/src/a but
uses type parameters.

For golang/go#48704

Change-Id: I91b101bda6e1da5b2de6830896a4b13508f31322
Reviewed-on: https://go-review.googlesource.com/c/tools/+/358696
Trust: Guodong Li <guodongli@google.com>
Run-TryBot: Guodong Li <guodongli@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
jmesyou pushed a commit to jmesyou/golang-tools that referenced this issue Oct 26, 2021
go/analysis/passes/unusedresult: update and add test for typeparams
0a69700 
This CL implements  support for setting unused generic funcs via flags.
A test for unused results with generics is added.

Update golang/go#48704
gopherbot pushed a commit to golang/tools that referenced this issue Oct 26, 2021
@guodongli-google
go/analysis/passes/lostcancel: add typeparams test
26dbf47 
testdata/src/typeparams is similar to testdata/src/a but
uses type parameters.

For golang/go#48704

Change-Id: Id6911e0e18b31a0de917835e4bf83c50adea1599
Reviewed-on: https://go-review.googlesource.com/c/tools/+/358694
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Guodong Li <guodongli@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Oct 26, 2021
@zpavlinovic @odeke-em
go/analysis/passes/cgocall: add typeparams test
591e12a 
This CL adds a test for the cgocall pass that involves use of generics.

Updates golang/go#48704

Change-Id: I521708d607c5f32ca24fe370b7d6436147bae6a5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/358695
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359401 mentions this issue: go/analysis/passes/testinggoroutine: update for generics

jmesyou pushed a commit to jmesyou/golang-tools that referenced this issue Oct 28, 2021
go/analysis/passes/unusedresult: update and add test for typeparams
1379ff8 
This CL implements  support for setting unused generic funcs via flags.
A test for unused results with generics is added.

Update golang/go#48704
jmesyou pushed a commit to jmesyou/golang-tools that referenced this issue Oct 28, 2021
go/analysis/passes/unusedresult: update and add test for typeparams
1c6db19 
This CL implements  support for setting unused generic funcs via flags.
A test for unused results with generics is added.

Updates golang/go#48704
gopherbot pushed a commit to golang/tools that referenced this issue Oct 28, 2021
@timothy-king
go/analysis/passes/unusedresult: add test for typeparams
e7eb6f6 
This CL adds a test for unused results inside a function using generics.

Update golang/go#48704

Change-Id: I3703cd6bbc40142b4a667d3cd069ea0721a045ec
GitHub-Last-Rev: 1c6db19
GitHub-Pull-Request: #345
Reviewed-on: https://go-review.googlesource.com/c/tools/+/357411
Reviewed-by: Tim King <taking@google.com>
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359494 mentions this issue: go/analysis/passes/tests: update for generics

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359409 mentions this issue: go/analysis/passes/stdmethods: add tests with generic receivers

gopherbot pushed a commit to golang/tools that referenced this issue Oct 29, 2021
@findleyr
go/analysis/passes/stdmethods: add tests with generic receivers
170abdd 
Ensure that the logic for detecting incorrect signatures of standard
methods applies to methods on generic types.

For golang/go#48704

Change-Id: I1862238ad1ff013137b12674fbc0068a7e8a3c60
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359409
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Oct 29, 2021
@findleyr
go/analysis/passes/testinggoroutine: update for generics
4d2571b 
Update the testinggoroutine analyzer to find bad calls via instantiated
function calls.

For golang/go#48704

Change-Id: I39ef43090a0e5dd04dfe094376517ac25441ad27
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359401
Trust: Robert Findley <rfindley@google.com>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Oct 29, 2021
@timothy-king
go/analysis/passes/tests: update for generics
a2be0cd 
Warn about Test functions that contain type parameters.

For golang/go#48704

Change-Id: I3f6852613482ec6f33e5650a014564915f11b920
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359494
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Tim King <taking@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359894 mentions this issue: go/analysis/passes/nilfunc: add typeparams test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 30, 2021
@guodongli-google
go/analysis/passes/nilfunc: add typeparams test
a6c6f4b 
testdata/src/typeparams is similar to testdata/src/a but
uses type parameters.

For golang/go#48704

Change-Id: I2d49190b606d6cdcfe79af5f29bba1117b07b94a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359894
Run-TryBot: Guodong Li <guodongli@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Guodong Li <guodongli@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359974 mentions this issue: go/types/typeutil: Callee supports generics

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/351832 mentions this issue: go/analysis/passes/printf: warn about verbs that don’t match a type

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/360174 mentions this issue: go/analysis/passes/unsafeptr: add tests using generics

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/360234 mentions this issue: go/analysis/passes/copylock: find locks via type parameters

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/360295 mentions this issue: analysis/internal/facts: Updating facts for generics.

gopherbot pushed a commit to golang/tools that referenced this issue Nov 1, 2021
@findleyr
go/analysis/passes/stringintconv: add support for type parameters
42daa65 
Check for potentially invalid string int conversions via type
parameters.

Updates golang/go#48704

Change-Id: I0269857f3245909cf60c78c6dd624c1c0090c22d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359294
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 1, 2021
@timothy-king
go/types/typeutil: Callee supports generics
ee08195 
Adds support for finding the Callee if it is a generic function.
Generic methods happen to already work without this change.
Updating this so that they are consistent.

For golang/go#48704

Change-Id: I649ec746e350db4a0086ed31535b2e14baa32314
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359974
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 2, 2021
@findleyr
go/analysis/passes/copylock: find locks via type parameters
058ed05 
Add support for finding incorrect usage of locks via type parameters. It
would probably be fine not to do this, since using locks explicitly via
type parameters should be exceedingly rare. However, it was
straightforward to add, and is consistent with other analyzers.

Updates golang/go#48704

Change-Id: I329a2fa9f11c6bbb491d49afde7fabce8299cbdf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/360234
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 5, 2021
@timothy-king
analysis/internal/facts: Updating facts for generics.
b8b8e7f 
Updates importMap() to consider generics.

For golang/go#48704

Change-Id: Ic5dafc644c4e81a64df338a165ee8e20627c6113
Reviewed-on: https://go-review.googlesource.com/c/tools/+/360295
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Tim King <taking@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 6, 2021
@findleyr
go/analysis/passes/printf: warn about verbs that don’t match a type
ddcef59 
parameter’s type set

Pending the acceptance of golang/go#49038, this CL updates the printf
analyzer to warn if any elements of a type parameter's type set do not
match the printf verb being considered. Since this may be a source of
confusion, it also refactors slightly to allow providing a reason why a
match failed.

Updates golang/go#48704
Updates golang/go#49038

Change-Id: I92d4d58dd0e9218ae9d522bf2a2999f4c6f9fd84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/351832
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 15, 2021
@findleyr
go/analysis/passes/unsafeptr: add tests using generics
4392381 
Unlike with some other analyzers, it did not seem worthwhile to consider
a type parameter's type set when looking for incorrect conversions to
unsafe.Pointer. There's probably no reason to have a type parameter with
uintptr structural type.

Add some sanity-check tests for the behavior of this analyzer with
respect to generic code.

Updates golang/go#48704

Change-Id: Ibc3180c6eba9c2c88ea2220b1c84cd27971a6700
Reviewed-on: https://go-review.googlesource.com/c/tools/+/360174
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@findleyr
Copy link
Member Author

I'm pretty sure we looked at everything, and I'm satisfied with the changes we made. It would have been nice to have added more precision to the unsafeptr analyzer (see https://golang.org/cl/360174), but I don't think that's release blocker and we can improve it for 1.19.

Closing this as done. Any additional changes to the analyzers should be against new, more specific issues.

@findleyr findleyr mentioned this issue Jan 11, 2022
Open
@ldez
Copy link

ldez commented Feb 6, 2022 •
edited
Loading

EDIT I opened a dedicated issue #51038

This was referenced Feb 6, 2022
Merged
Closed
@golang golang locked and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@ldez @gopherbot @findleyr