build(deps): bump github.com/go-critic/go-critic from 0.4.1 to 0.4.3

[dependabot-preview]

Bumps github.com/go-critic/go-critic from 0.4.1 to 0.4.3.

Release notes

Sourced from github.com/go-critic/go-critic's releases.

v0.4.3

No release notes provided.

v0.4.2

Changes

• checkers: add ruleguard checker @quasilyte (#907)
• add draw.Draw to draw.DrawMask checker @seregayoga (#904)
• checkers: rename deferAtTheEnd to unnecessaryDefer @quasilyte (#902)
• checker: add defer at the end of function checker @janisz (#901)

Commits

• 8c5f841 checkers: add ruleguard checker (#907)
• 0d5eff4 checkers: add draw.Draw to draw.DrawMask checker (#904)
• 5714434 checkers: rename deferAtTheEnd to unnecessaryDefer (#902)
• 3062653 checker: add defer at the end of function checker (#901)
• See full diff in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

---

Note: This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking Bump now in your Dependabot dashboard.

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

• @dependabot rebase will rebase this PR
• @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
• @dependabot merge will merge this PR after your CI passes on it
• @dependabot squash and merge will squash and merge this PR after your CI passes on it
• @dependabot cancel merge will cancel a previously requested merge and block automerging
• @dependabot reopen will reopen this PR if it is closed
• @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
• @dependabot badge me will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the .dependabot/config.yml file in this repo:

• Update frequency
• Out-of-range updates (receive only lockfile updates, if desired)
• Security updates (receive only security updates, if desired)

[jirfag]

we need to manually fix newly found issues

[bombsimon]

Do we just fix the issues in this PR and push on this branch or should I create a new branch and merge to master before re-running these tests?

Also, isn't all those errors false positives? It seems like defer statements in the end of a block (even if it's an if-statement) is seen as last before return. Example for line 138 in testsahred.go

golangci-lint/test/testshared/testshared.go

Lines 137 to 148 in 09df97a

	if os.Getenv("GL_KEEP_TEMP_FILES") != "1" {
	defer os.Remove(cfgPath)
	}
	cfg = strings.TrimSpace(cfg)
	cfg = strings.Replace(cfg, "\t", " ", -1)
	err = ioutil.WriteFile(cfgPath, []byte(cfg), os.ModePerm)
	assert.NoError(r.t, err)
	pargs := append([]string{"-c", cfgPath}, args...)
	return r.RunCommand(command, pargs...)

Maybe remove the check from the CI linting before the false positive is fixed?

[jirfag]

thank you for the related issue in go-critic.
You can fix in a new pull request or here, which would be more comfortable for you.

[bombsimon]

I did what I thought was right and resolved the bug upstream instead. 😄

I'll ignore this check until a new go-critic version is released.

[bombsimon]

Uhh, it seems like the task fails because I'm disabling the check that was failing by saying it's disabled by default. It still proceeds but fails since it doesn't recognize the linter being disabled.

 level=warning msg="Gocritic check \"unnecessaryDefer\" was explicitly disabled via config. However, as this checkis disabled by default, there is no need to explicitly disable it via config."
 level=error msg="Invalid gocritic settings: validation failed: disabled checker unnecessaryDefer doesn't exist[...]"

I'll see what I can find!

EDIT: Oh, it's in the current golangci-lint version the check doesn't exist which is obvious since this is the PR that will introduce it. Haven't thought about where the linting comes from while running tests since it's make test is what fails.

As far as I can tell the test target builds a new version of golangci-lint and executes that one so thew newly ignored check should not fail. 🤔

This is the result of me running this locally (but the steps manually).

# make golangci-lint
$ go build -o golangci-lint ./cmd/golangci-lint && echo "Ok" 
Ok

# make test (first step without `-v` flag)
$ GL_TEST_RUN=1 ./golangci-lint run && echo "Ok"
Ok

Had some issues with make test before but got that to work locally now as well.

EDIT2: I should read the code before I write... I see that the step failing is in the action where we run golangci-lint v1.26. So for this to work we want the action to run the current config without disabling unnecessaryDefer but when running make test we want to disable it untill it's fixed.

What's the preferred way forward here? Do we ever want to be able to run those steps with different configurations? This time it's a bug but I guess similar things would be of interest for actual changes too?

[ernado]

Probably we can exclude this check by exclude-rules and remove that exclude on nextgocritic release.

[bombsimon]

@ernado Yeah I thought that was what I did in my last commit (8a749af) but the build still fails. This is how I tried to exclude the check.

I think the reason is because the action runs an older version of golangci-lint (v.1.26) which doesn't have this check an fails because the configuration is invalid. Without it however the make test step fails because this check reports faulty errors.

[ernado]

@bombsimon I was talking about issues.exclude-rules. We can specify a linter name and a regex to ignore here :)

[bombsimon]

@ernado Oh, of course, that's would solve the issue! I'll push a new change!

[jirfag]

I think the reason is because the action runs an older version of golangci-lint (v.1.26) which doesn't have this check

let's update it to v1.27?

[ernado]

Does v1.27 include unnecessaryDefer?

It looks like not, because it was introduced in gocritic update (which is in current PR)

[jirfag]

ok, got it