Add new linter gomockcontrollerfinish

[hendrywiranto]

Hello,
I'd like to add new linter
https://github.com/hendrywiranto/gomockcontrollerfinish

Closes #3223

Currently, it's only checks whether an unnecessary call to .Finish() on gomock.Controller exists

Note: I'm open to renaming for more general use such as gomocklint to support another linting rule for gomock in the future

[ldez]

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

• The CLA must be signed

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().
• It must not contain log.fatal(), os.exit(), or similar.
• It must not modify the AST.
• 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.
• WithURL() must contain the URL of the repository.

Recommendations

• The linter should not use SSA. (SSA can be a problem with 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.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

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

[Antonboom]

@hendrywiranto, hello!

I don't want to belittle your work, but before a review there is a simple question – what is the profit from such kind of linters?

1.Finish is idempotent and sometimes to see explicit call is more clear for test reader.
2. In the first version of this feature gomock had protection from this, but after this line was deleted because of

Also, removed logging as after thinking about it more it seems a little aggressive to push the feature onto users.

You ignored my comment, but instead of linter I suppose just make contribution in uber-go/mock.

[hendrywiranto]

@hendrywiranto, hello!

I don't want to belittle your work, but before a review there is a simple question – what is the profit from such kind of linters?

Hi @Antonboom , thanks for the comment!

I think the main benefit is we can have less code in the tests, not only just 1 less line, but also we can completely remove the usage of e.g. TearDownTest that only calls this Finish method.

Also a little background, I recently saw lots of my coworker still calling Finish on their unit tests and I think that it would be nice if this can be tackled by a linter so I won't have to nit on this anymore. That's why I'm interested for taking the issue in the first place. For improvement I'm thinking of making an autofix to make it more useful

1.Finish is idempotent and sometimes to see explicit call is more clear for test reader.
2. In the first version of this feature gomock had protection from this, but after this line was deleted because of

Also, removed logging as after thinking about it more it seems a little aggressive to push the feature onto users.

Thank you for the reference, I'm also aware that Finish is idempotent, but seems like calling it explicitly is not necessary anymore since go1.14 as stated in the official docs. Also noted on no. 2.

You ignored my comment, but instead of linter I suppose just make contribution in uber-go/mock.

For your comment I actually saw it, sorry it's just I don't know what to respond to it. For the linter itself, it works as long as the controller object is a gomock.Controller. And sorry for asking, can you elaborate what kind contribution do you recommend for uber-go/mock ?

All in all, thank you for the review, I'm still new at go linters and I'm open for feedbacks on this. It's okay if this ends up to look not really useful, I can still use it on my org codebase. I just think it's better if I'm able to add it into this awesome project :)

[Antonboom]

For the linter itself, it works as long as the controller object is a gomock.Controller

Imho, HasSuffix is not the good solution. I expected the strict compasion with objects from google and uber packages.
Otherwise false positives from forks are possible.

but seems like calling it explicitly is not necessary anymore

As I said before I just affected by different situation, when people used to call Finish explicitly, so as not to think about the internal hidden logic of the package and the Go version. Sorry 🙏

also we can completely remove the usage of e.g. TearDownTest that only calls this Finish method.

Nice point!

And sorry for asking, can you elaborate what kind contribution do you recommend for uber-go/mock?

I meant that you can offer them to return the deleted line with the log, but instead of the log, for example, fail the test.

This is the way of the Go std testing package:

func (t *T) Run(name string, f func(t *T)) bool {
	if t.cleanupStarted.Load() {
		panic("testing: t.Run called during t.Cleanup")
	}
// ...

func (c *common) checkFuzzFn(name string) {
	if c.inFuzzFn {
		panic(fmt.Sprintf("testing: f.%s was called inside the fuzz target, use t.%s instead", name, name))
	}
}
// ...

func (t *T) Parallel() {
	if t.isParallel {
		panic("testing: t.Parallel called multiple times")
	}
// ....

P.S.

Also it is better to remove this filter

if !isTestFile(pass.Fset.Position(callExpr.Pos()).Filename) {

• golangci-lint controls sources to lint
• helpers, base suites, etc. could be not in tests file too

[hendrywiranto]

Imho, HasSuffix is not the good solution. I expected the strict compasion with objects from google and uber packages. Otherwise false positives from forks are possible.

Yes it's still possible, I will look for another way of checking this. thanks

I meant that you can offer them to return the deleted line with the log, but instead of the log, for example, fail the test.

Ah I see, appreciate the explanation
Actually about your suggestion, I'm not sure if that's what the community will agree on.
The other day I saw this deprecation PR on Uber's fork and someone said that eventually removing Finish completely (or in this case making it fail the test if called explicitly) is not preferable.

uber-go/mock#50 (comment)

Please don't remove this function. It's needed to use gomock with certain testing frameworks, and removing the function would break those use cases.
Please also consider reverting the deprecation. If the function is not going to be removed in a future release, it shouldn't be marked deprecated in the first place.
See uber-go/mock#81 for more details.

Also the deprecation of the method itself is reverted uber-go/mock#85.

I'm thinking that the users of the framework that still require to call Finish can just not enable the linter, while users that don't use any framework (or use framework that still works fine without Finish) can still enable it if they wanted to
Quoting the discussion: uber-go/mock#84 (comment)

Given that there are valid use cases of Finish, the function should probably not deprecated at all. Instead, documentation could be added explaining that usually (i.e. when not using any test framework) it's not necessary to call that function.

PS. Also it is better to remove this filter

Noted on the PS. I will adjust it. Thanks

[hendrywiranto]

adjusted based on both of your comments @Antonboom

• isTestFile filter removed
• strict comparison with objects from google and uber packages. Not sure if what I'm doing with the check is preferable, but now it's stricter compared to just using HasSuffix

If the linter is good to go, I will update this PR with the updated version. Thanks.

[alexandear]

I'm not sure if we really need this linter.

A few months ago I pushed changes that deprecated ctrl.Finish. After this staticcheck begins to detect usages of this deprecated function.
But this change breaks some test frameworks and it was reverted by the PR.
So, the ctrl.Finish is still needed.

[hendrywiranto]

Hi @alexandear, yes I noticed your PR and the discussions around it.

I think other than those test frameworks that you mentioned, people can completely omits it.
That's why I propose to just disable the linter if there is still a need for it.

And on my case, it's quite useful e.g. as I mentioned here

I think the main benefit is we can have less code in the tests, not only just 1 less line, but also we can completely remove the usage of e.g. TearDownTest that only calls this Finish method.

What do you think?

[alexandear]

@hendrywiranto
Yes, I find the linter useful, but it's not worth adding the whole linter that detects usages of only one function in one library.

Perhaps we should consider adding a new rule to the existing linters such as go-critic, revive, or staticcheck?

[hendrywiranto]

@hendrywiranto Yes, I find the linter useful, but it's not worth adding the whole linter that detects usages of only one function in one library.

Perhaps we should consider adding a new rule to the existing linters such as go-critic, revive, or staticcheck?

Ah I see...

Thanks for the recommendation, I will take a look on those linters 😃

[hendrywiranto]

also I wonder how many rules does it take for a linter to be considered worth it? @alexandear just curious, no pressure intended

my plan was to make the linter more general (maybe can be about mocking rules (?) ), but still no other rules in mind, so maybe I'll come back later when it has more rules

[ldez]

also I wonder how many rules does it take for a linter to be considered worth it?

This rule doesn't really exist, a linter doesn't have to handle several elements/rules.

Your linter is focused on a tiny rule, it's not a major blocker but this questions the relevance of your linter.

my plan was to make the linter more general (maybe can be about mocking rules (?) ), but still no other rules in mind, so maybe I'll come back later when it has more rules

If you plan that do it now, because the name of your linter is not really open to changes.

[alexandear]

It's possible to prevent the usage of ctrl.Finish with the forbidigo linter. Here is the .golangci-lint.yml configuration:

linters-settings:
    forbid:
      - p: ^gomock.Controller.Finish*
        pkg: ^go.uber.org/mock/gomock$
        msg: "This function is redundant. See https://pkg.go.dev/go.uber.org/mock@v0.2.0/gomock#Controller.Finish"
    analyze-types: true

linters:
  disable-all: true
  enable:
    - forbidigo

exclude-rules:
    - path-except: _test\.go
      linters:
        - forbidigo

[ldez]

so it's already possible with another linter, we will decline this PR.