Hey, thank you for opening your first Pull Request !

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign ourContributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let usrecheck it.}

While this implements the same functionality asrowserrcheck that linter usesbuildssa which is currently incompatible withgo generics asbuildssa.SSA.SrcFuncs doesn't currently includeInstantiations.

This is a from scratch implementation that usesinspect.Analyzer which doesn't have such problems. It also doesn't suffer from the false positives when the code has multiple reassignedRow variables that don't result in a real check.

Credit torowserrcheck for inspiration.

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must usego/analysis.
• It must have a valid tag, ex:v1.0.0,v0.1.0.
• It must not containinit().
• It must not containpanic(),log.fatal(),os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work withT=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable anddisable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The.golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in theManager.GetAllSupportedLinterConfigs(...) and.golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types:goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo requiredWithLoadForGoAnalysis() in theManager.GetAllSupportedLinterConfigs(...)
• The version inWithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

• The linter should not use SSA. (currently, SSA does not support generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

This checklist does not imply that we will accept the linter.

I've actually refactored the original implementation which was limited to checking for missingdatabase/sqlRows.Err() to use a configuration that will allow it to check for any number of missing calls.

Working my way though validating your check list.

@ldez thanks for the checklist, very helpful. I did notice it has more details thannew linters guide so I'll try to find some time to update that if it would help.

I believe all the items on your list are good, but look forward to your feedback.

@ldez any feedback I need to address?

I see this is now tagged at blocked@ldez anything that I need to address?

I just updated to the latest version and rebased so it's in line with current master.

I blocked the PR because I think this linter is a kind of duplicate but I need to investigate.

It does duplicate the functionality ofrowserrcheck as mentioned in previous comment, but it usesbuildssa which is currently incompatible withgo generics asbuildssa.SSA.SrcFuncs so users currently have no way to check for that issue if they are using go 1.18+

This is a from scratch implementation that usesinspect.Analyzer which doesn't have such problems. It also doesn't suffer from the false positives when the code has multiple reassigned Row variables that don't result in a real check.

Finally this uses an extensible model based on configuration, providing three separate checks currently:

• sql-rows-err - Check for missing sql Rows.Err() calls.
• http-response-body-close - Check for missing http Reponse.Body.Close() calls.
• context-cancel - Check for missing context CancelFunc() calls.

SSA supports generics for about 1 year#2649 (comment)

And since go1.20, go1.18 is unmaintained.

Maybe I miss something here 🤔

SSA supports generics for about 1 year#2649 (comment)

And since go1.20, go1.18 is unmaintained.

Maybe I miss something here 🤔

Thanks for checking back in on this.

When I wrote this SSA didn't have full generics support specifically it didn't supportinstantiations causingrowserrcheck to be disabled in go 1.18+ which have generics. I only mentioned 1.18 as that's when generics was introduced.

I just did a check with golangci-lint v1.51.2 androwserrcheck is still disabled when running with go 1.20:

golangci-lint run
WARN [linters_context] rowserrcheck is disabled because of generics. You can track the evolution of the generics support by following the#2649.

Looking atrowserrcheck it hasn't been updated since 2021, so unfortunately still isn't working with generics, although should be easier to fix now.

uncalled, also has the ability to be extended with additional user checks via extra configuration so I think it still has value to the community, thoughts?

Hi everyone

I'm interested in the feature that uncalled linter would provide

I'm thinking about a lot of use cases in my codebase.

But I'm a bit lost in the current discussion, so bare with me, please.

From what I understood,@ldez think it might be a duplicate, and is also worried about the fact that uncalled is coded in a way that prevents it to be added easily to golangci. Here is the point where you lost me guys 📦

I'm able to use uncalled via precommit hook, but having them in golangci would be easier.

With my newbie point of view, I understand that uncalled might be a solution to replacerowserrcheck and provides a way to make people able to detect again what rowserrcheck was detecting, but that is likely to never be available as the project seems dead.

So now, I said that...

what is the current status ?

@ldez : does you only need time to look at this project ? or does it mean@stevenh needs to write it totally ? because right now it's not compatible or may cause issues.

Thank you both for your time on working on golangci-lint

I believe the only concern is that it duplicates the check inrowserrcheck however as explained above that's currently unavailable if you're using golang v1.18 or above due to generics.

uncalled is already fully integrated withgolangci-lint the PR just needs rebase to resolve the conflicts, which I'm happy to do if it would be accepted.

Thanks for the explanation.

From my understanding, it doesn't duplicate it. It supersedes it.

I would be able not only to restore the feature of rowserrcheck on version above go1. 18

It will allow to provide way to define custom rules.

rowserrcheck is only one possible things it can do.

FYI rowserrcheck will be re-enabled#3691

Thanks for the information.

Does it reduce the chance to getuncalled integrated into golangci-lint ?

Does it reduce the chance to getuncalled integrated into golangci-lint?

Yes because duplicated linters are not accepted, but I have to evaluate pro/cons to replacerowserrcheck withuncalled.

@stevenh, hi!
For meuncalled seems very cool generic linter, thank you for job.

@ldez, looks likeuncalled could make deprecated:

1. https://github.com/timakin/bodyclose
2. https://github.com/jingyugao/rowserrcheck
3. https://github.com/ryanrolds/sqlclosecheck
4. xmlencoderclose: linter that checks xml.Encoder is closed #3895
5. https://github.com/ykadowak/zerologlint (probably)

The killer feature is support not only std types 😎

P.S.@stevenh could you provide an example ofuncalled config to prove my list above?

But I confused fromgithub.com/rs/zerolog dependency in the linter 😔

But I confused fromgithub.com/rs/zerolog dependency in the linter 😔

It's there to add structured logging with control over the level when running checks. Now std lib hasslog I'll likely look to migrate to that.

@stevenh, hi! For meuncalled seems very cool generic linter, thank you for job.

Thanks :)

@ldez, looks likeuncalled could make deprecated:

1. https://github.com/timakin/bodyclose
2. https://github.com/jingyugao/rowserrcheck
3. https://github.com/ryanrolds/sqlclosecheck
4. xmlencoderclose: linter that checks xml.Encoder is closed #3895
5. https://github.com/ykadowak/zerologlint (probably)

The killer feature is support not only std types 😎

P.S.@stevenh could you provide an example ofuncalled config to prove my list above?

bodyclose and rowserrcheck are already present in theconfig here

sqlclose would looks something like:

# Check for missing sql Rows.Close() calls.  -name:sql-rows-closedisabled:falsecategory:sqlpackages:      -database/sql      -github.com/jmoiron/sqlxmethods:[]results:      -type:.Rowspointer:trueexpect:call:.Closeargs:[]      -type:errorpointer:false# Check for missing sql Stmt.Close() calls.  -name:sql-stmt-closedisabled:falsecategory:sqlpackages:      -database/sql      -github.com/jmoiron/sqlxmethods:[]results:      -type:.Stmtpointer:trueexpect:call:.Closeargs:[]      -type:errorpointer:false# Check for missing sql NamedStmt.Close() calls.  -name:sql-namedstmt-closedisabled:falsecategory:sqlpackages:      -github.com/jmoiron/sqlxmethods:[]results:      -type:.NamedStmtpointer:trueexpect:call:.Closeargs:[]      -type:errorpointer:false

xmlencoderclose would looks something like:

# Check for missing xml Encoder.Close() calls.  -name:xml-encoder-closedisabled:falsecategory:xmlpackages:      -encoding/xmlmethods:[]results:      -type:.Encoderpointer:trueexpect:call:.Closeargs:[]      -type:errorpointer:false

zerologlint looks a little more complex as there are multiple ways to trigger the dispatch, but this should also be possible with some enhancements.

If this is of interest, I can look at creating tests to confirm the above?

Looks likeRows.Close is more complex as ifNext is called and returns false thenRows is closed implicitly, so will need some more thought to ensure it doesn't give false positives.

Looking at ithttps://github.com/ryanrolds/sqlclosecheck suffers from the false positive when you have checkedrows.Next() androws.Err() for example:

funcCalled(db*sql.DB) {rows,_:=db.Query("select id from tb")forrows.Next() {// Handle row.}ifrows.Err()!=nil {// Handle error.fmt.Fprintln(ioutil.Discard,"error")}}