feat: add uncalled linter

[stevenh]

Add uncalled linter which checks for missing calls based on configured rules and supports generics.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[stevenh]

While this implements the same functionality as rowserrcheck that linter uses buildssa which is currently incompatible with go generics as buildssa.SSA.SrcFuncs doesn't currently include Instantiations.

This is a from scratch implementation that uses inspect.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.

Credit to rowserrcheck for inspiration.

[ldez]

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 use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), 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 with T=<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 and disable 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 the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) 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.

[stevenh]

I've actually refactored the original implementation which was limited to checking for missing database/sql Rows.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.

[stevenh]

@ldez thanks for the checklist, very helpful. I did notice it has more details than new 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.

[stevenh]

@ldez any feedback I need to address?

[stevenh]

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.

[ldez]

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

[stevenh]

It does duplicate the functionality of rowserrcheck as mentioned in previous comment, but it uses buildssa which is currently incompatible with go generics as buildssa.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 uses inspect.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.

[ldez]

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

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

Maybe I miss something here 🤔

[stevenh]

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 support instantiations causing rowserrcheck 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 and rowserrcheck 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 at rowserrcheck 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?

[ccoVeille]

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 replace rowserrcheck 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

[stevenh]

I believe the only concern is that it duplicates the check in rowserrcheck 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 with golangci-lint the PR just needs rebase to resolve the conflicts, which I'm happy to do if it would be accepted.

[ccoVeille]

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.

[ldez]

FYI rowserrcheck will be re-enabled #3691

[ccoVeille]

Thanks for the information.

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

[ldez]

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

Yes because duplicated linters are not accepted, but I have to evaluate pro/cons to replace rowserrcheck with uncalled.

[Antonboom]

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

@ldez, looks like uncalled 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 of uncalled config to prove my list above?

[Antonboom]

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

[stevenh]

But I confused from github.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 has slog I'll likely look to migrate to that.

[stevenh]

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

Thanks :)

@ldez, looks like uncalled 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 of uncalled config to prove my list above?

bodyclose and rowserrcheck are already present in the config here

sqlclose would looks something like:

  # Check for missing sql Rows.Close() calls.
  - name: sql-rows-close
    disabled: false
    category: sql
    packages:
      - database/sql
      - github.com/jmoiron/sqlx
    methods: []
    results:
      - type: .Rows
        pointer: true
        expect:
          call: .Close
          args: []
      - type: error
        pointer: false
  # Check for missing sql Stmt.Close() calls.
  - name: sql-stmt-close
    disabled: false
    category: sql
    packages:
      - database/sql
      - github.com/jmoiron/sqlx
    methods: []
    results:
      - type: .Stmt
        pointer: true
        expect:
          call: .Close
          args: []
      - type: error
        pointer: false
  # Check for missing sql NamedStmt.Close() calls.
  - name: sql-namedstmt-close
    disabled: false
    category: sql
    packages:
      - github.com/jmoiron/sqlx
    methods: []
    results:
      - type: .NamedStmt
        pointer: true
        expect:
          call: .Close
          args: []
      - type: error
        pointer: false

xmlencoderclose would looks something like:

  # Check for missing xml Encoder.Close() calls.
  - name: xml-encoder-close
    disabled: false
    category: xml
    packages:
      - encoding/xml
    methods: []
    results:
      - type: .Encoder
        pointer: true
        expect:
          call: .Close
          args: []
      - type: error
        pointer: 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?

[stevenh]

Looks like Rows.Close is more complex as if Next is called and returns false then Rows is closed implicitly, so will need some more thought to ensure it doesn't give false positives.

[stevenh]

Looking at it https://github.com/ryanrolds/sqlclosecheck suffers from the false positive when you have checked rows.Next() and rows.Err() for example:

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