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.

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

SSA and generics (go1.18) #24

Closed
Tracked by #2649
ldez opened this issue Mar 24, 2022 · 12 comments · Fixed by #27 · May be fixed by #28
Closed
Tracked by #2649

SSA and generics (go1.18) #24

ldez opened this issue Mar 24, 2022 · 12 comments · Fixed by #27 · May be fixed by #28

Comments

@ldez
Copy link

ldez commented Mar 24, 2022

Currently, SSA is not working with generics.

So your linter produces a panic when it is used with generics.

There is an issue open about that in the Go repository: golang/go#48525

Inside golangci-lint, we have disabled your linters: golangci/golangci-lint#2649

You have 2 solutions:

  • waiting for a version of SSA that will support generics
  • dropping the SSA analyzers and using something else to analyze the code.

Related to golang/go#50558

@ldez
Copy link
Author

ldez commented Jul 4, 2022

Updating Update golang.org/x/tools to the newest version is not enough.
Have you tried to test your linter with generics?

@fho
Copy link

fho commented Jul 4, 2022

@ldez I did.
When running it on code containing generics, the execution now succeeds and it also still finds issues, if rows are not closed.

@ldez
Copy link
Author

ldez commented Jul 4, 2022

what I can say: in golangci-lint, this update is not enough to fix the problem of this linter.

There are several syntaxes to validate for the support of generics.

@fho
Copy link

fho commented Jul 4, 2022

@ldez could you provide a codesnippet for that rowserrcheck fails?

@ldez
Copy link
Author

ldez commented Jul 19, 2022

SSA has been updated and it seems to work inside golangci-lint.

@ldez
Copy link
Author

ldez commented Jul 19, 2022

in fact, the problem is still here

@fho
Copy link

fho commented Jul 19, 2022

@ldez what is the problem? How can I reproduce the problem only with rowserrcheck?

@ldez
Copy link
Author

ldez commented Jul 19, 2022

import (
	"database/sql"
)

func RowsErrNotChecked(db *sql.DB, a int) {
	rows, _ := db.Query("select id from tb")
	for rows.Next() {

	}
}

func RowsErrNotCheckedG[T ~int64](db *sql.DB, a T) {
	rows, _ := db.Query("select id from tb")
	for rows.Next() {

	}
}

@fho
Copy link

fho commented Aug 1, 2022

@ldez I can't reproduce an issue with that snippet.
rowserrcheck does not panic and it complains about a missing rows.Err check. It works as expected.

[fho@ltop rowserrcheck]$ git rev-parse HEAD
fa9179529ab817c3359a4f5beeec91546f71d602
[fho@ltop rowserrcheck]$ cat /tmp/main.go
package main

import (
	"database/sql"
)

func RowsErrNotChecked(db *sql.DB, a int) {
	rows, _ := db.Query("select id from tb")
	for rows.Next() {

	}
}

func RowsErrNotCheckedG[T ~int64](db *sql.DB, a T) {
	rows, _ := db.Query("select id from tb")
	for rows.Next() {

	}
}
[fho@ltop rowserrcheck]$ go run main.go -- /tmp/main.go
/tmp/main.go:8:21: rows.Err must be checked
exit status 3

@ldez
Copy link
Author

ldez commented Aug 1, 2022

With the sample, you must have 2 reports, currently, you only have one.
The problem is not a panic.

@fho
Copy link

fho commented Aug 1, 2022

@ldez right, but this is not blocking to enable rowserrcheck in golangci-lint again, is it?
The linter does not panic anymore when it encounters generics.

@ldez
Copy link
Author

ldez commented Aug 1, 2022

It is blocking.
The linter doesn't work as expected with generics, so we will not enable this linter like the others with the same problem.

False negatives are worse than false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants