build(deps): bump github.com/go-critic/go-critic from 0.5.6 to 0.6.1

[dependabot]

Bumps github.com/go-critic/go-critic from 0.5.6 to 0.6.1.

go-critic/go-critic@v0.5.6...v0.6.1

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.

---

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 ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
• @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
• @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

[ldez]

$ GO111MODULE=off ./golangci-lint run -v
panic: load embedded ruleguard rules: typechecker error: rules/rules.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.10()
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/checkers/checkers.go:55 +0x611

$ make gocritic 
go build -o gocritic ./cmd/gocritic
$ GO111MODULE=off ./gocritic check
panic: load embedded ruleguard rules: typechecker error: rules/rules.go:4:2: could not import github.com/quasilyte/go-ruleguard/dsl (can't find import: "github.com/quasilyte/go-ruleguard/dsl")

goroutine 1 [running]:
github.com/go-critic/go-critic/checkers.init.10()
        /home/ldez/sources/go/src/github.com/go-critic/go-critic/checkers/checkers.go:55 +0x61f

Sounds related to go-critic/go-critic#1044

Hello @quasilyte, is it the expected behavior?

[quasilyte]

@ldez no, it's not. I'll try to figure it out.

[quasilyte]

I'm working on a fix, but it takes time.
It can be expected in a week or two.
go-critic/go-critic#1060

[quasilyte]

This is fixed in master now. We'll test it for a couple of days, collect a few more minor changes and make a release that could be used as an update target for the golangci-lint.

[quasilyte]

I think that we should use the next release here, 0.5.8; it fixes the unwanted dependency error.
https://github.com/go-critic/go-critic/releases/tag/v0.5.8
It also includes an initial suggestedfix support.

[dependabot]

A newer version of github.com/go-critic/go-critic exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

[ldez]

The new version (v0.5.8) doesn't work:

$ make build
go build -o golangci-lint ./cmd/golangci-lint
$ ./golangci-lint run
ERRO [runner] Panic: gocritic: package "goutil" (isInitialPkg: true, needAnalyzeSource: true): invalid base 175843567 (should be >= 175843620): goroutine 25762 [running]:
runtime/debug.Stack(0x169ce34, 0x3c, 0xc00324ecd0)
        /home/ldez/.gvm/gos/go1.16.9/src/runtime/debug/stack.go:24 +0x9f
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1(0xc0024fcd70)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:101 +0x1be
panic(0x1434ea0, 0xc00818ea90)
        /home/ldez/.gvm/gos/go1.16.9/src/runtime/panic.go:965 +0x1b9
go/token.(*FileSet).AddFile(0xc0002a0000, 0x0, 0x0, 0xa7b28ef, 0x1a, 0x0)
        /home/ldez/.gvm/gos/go1.16.9/src/go/token/position.go:434 +0x425
github.com/quasilyte/go-ruleguard/internal/gogrep.parseDetectingNode(0xc0002a0000, 0xc023170b40, 0x1a, 0x1a, 0x0, 0x0, 0x0, 0x0)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/internal/gogrep/parse.go:132 +0x75
github.com/quasilyte/go-ruleguard/internal/gogrep.parseExpr(0xc0002a0000, 0x16539ed, 0xe, 0xc00324f1b8, 0x8436a5, 0xc00f8e9800, 0xc0005d8900)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/internal/gogrep/parse.go:49 +0x98
github.com/quasilyte/go-ruleguard/internal/gogrep.Compile(0xc0002a0000, 0x16539ed, 0xe, 0x0, 0x3, 0xc00f8e9800, 0x2)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/internal/gogrep/gogrep.go:59 +0x5a
github.com/quasilyte/go-ruleguard/ruleguard.(*irLoader).loadSyntaxRule(0xc00324f850, 0xc01e47dad0, 0x11, 0x0, 0x166d74d, 0x16, 0x0, 0x0, 0x160bf59, 0xa, ...)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/ir_loader.go:300 +0x8d
github.com/quasilyte/go-ruleguard/ruleguard.(*irLoader).loadRule(0xc00324f850, 0x11, 0x16539ed, 0xe, 0x0, 0x0, 0x166d74d, 0x16, 0x160bf59, 0xa, ...)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/ir_loader.go:276 +0x198
github.com/quasilyte/go-ruleguard/ruleguard.(*irLoader).loadRuleGroup(0xc00324f850, 0x1e37e00, 0x0, 0x0)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/ir_loader.go:250 +0x465
github.com/quasilyte/go-ruleguard/ruleguard.(*irLoader).LoadFile(0xc00324f850, 0x16554e3, 0xe, 0x1e22300, 0xc006460c40, 0x2008100, 0x0)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/ir_loader.go:92 +0x294
github.com/quasilyte/go-ruleguard/ruleguard.(*engine).LoadFromIR(0xc0213fbaf0, 0xc01e976ae0, 0x16554e3, 0xe, 0x1e22300, 0xc02084c388, 0x0)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/engine.go:96 +0x212
github.com/quasilyte/go-ruleguard/ruleguard.(*Engine).LoadFromIR(...)
        /home/ldez/sources/go/pkg/mod/github.com/quasilyte/[email protected]/ruleguard/ruleguard.go:44
github.com/go-critic/go-critic/checkers.init.10.func1(0xc006460bc8, 0xc02084c388, 0xc007f826c0, 0xf80000c00324fa20, 0x9050dd)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/checkers/checkers.go:74 +0x158
github.com/go-critic/go-critic/framework/linter.addChecker.func2(0xc02ab472c0, 0xc00019aa80, 0x165a58b, 0xf)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:79 +0xdc
github.com/go-critic/go-critic/framework/linter.newChecker(0xc02ab472c0, 0xc016c207e0, 0x0, 0x0, 0xc0005d4d6e)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:92 +0x71
github.com/go-critic/go-critic/framework/linter.NewChecker(...)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/lintpack.go:169
github.com/golangci/golangci-lint/pkg/golinters.buildEnabledCheckers(0xc0005a1e30, 0xc02ab472c0, 0x80ac65, 0x8013e5, 0x104de35, 0x9f000000a0, 0x5)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/gocritic.go:149 +0x185
github.com/golangci/golangci-lint/pkg/golinters.NewGocritic.func1.1(0xc016c0b6c0, 0x2999b02bd, 0x1fcebc0, 0xc00eea0270, 0x2)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/gocritic.go:46 +0x11f
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc0024fcd70)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:187 +0x9f2
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:105 +0x2a
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc001630230, 0x15f535b, 0x8, 0xc0026bb770)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x4d
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc0024fcd70)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:104 +0x91
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc0004f10f0, 0xc0024fcd70)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0x65
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x316 
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: gocritic: package "goutil" (isInitialPkg: true, needAnalyzeSource: true): invalid base 175843567 (should be >= 175843620) 

the code has changed

[quasilyte]

I'll take a look.
Upd: I rolled a fix, but we'll have to wait for ~1 day until we merge upcoming changed and do a new rc release.

[quasilyte]

https://github.com/go-critic/go-critic/releases/tag/v0.6.0

[ldez]

There are still issues but for now, I don't know the root cause.

.
├── cmd
│   └── main.go
├── .gitignore
├── golangci-lint
├── .golangci.yml
├── go.mod
├── go.sum
└── rules.go

.golangci.yml

linters:
  disable-all: true
  enable:
    - "gocritic"

linters-settings:
  gocritic:
    enabled-checks:
      - ruleguard
    settings:
      ruleguard:
        rules: "./rules.go"
        failOnError: true

rules.go

package gorules

import "github.com/quasilyte/go-ruleguard/dsl"

func dupSubExpr(m dsl.Matcher) {
	m.Match(`$x || $x`,
		`$x && $x`,
		`$x | $x`,
		`$x & $x`).
		Where(m["x"].Pure).
		Report(`suspicious identical LHS and RHS`)
}

func boolExprSimplify(m dsl.Matcher) {
	m.Match(`!($x != $y)`).Suggest(`$x == $y`)
	m.Match(`!($x == $y)`).Suggest(`$x != $y`)
}

cmd/main.go

package main

func main() {
	var v1, v2 int
	println(!(v1 != v2))
	println(!(v1 == v2))
	if v1 == 0 && v1 == 0 {
		println("hello, world!")
	}
}

go.mod

module github.com/golangci/sandbox

go 1.15

require github.com/quasilyte/go-ruleguard/dsl v0.3.10

$ ./golangci-lint run
ERRO [runner] Panic: gocritic: package "gorules" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is bool, not string: goroutine 465 [running]:
runtime/debug.Stack()
        /home/ldez/.gvm/gos/go1.17.2/src/runtime/debug/stack.go:24 +0x65
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1()
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:101 +0x155
panic({0x12c28a0, 0xc00106f3b0})
        /home/ldez/.gvm/gos/go1.17.2/src/runtime/panic.go:1038 +0x215
github.com/go-critic/go-critic/framework/linter.CheckerParams.String(...)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/lintpack.go:60
github.com/go-critic/go-critic/checkers.newRuleguardChecker(0xc0000d9200, 0xc0003cdbc8)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/checkers/ruleguard_checker.go:108 +0x85f
github.com/go-critic/go-critic/checkers.init.44.func1(0x750000c000538e00)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/checkers/ruleguard_checker.go:49 +0x25
github.com/go-critic/go-critic/framework/linter.addChecker.func2(0xc000705200)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:79 +0x118
github.com/go-critic/go-critic/framework/linter.newChecker(0xc0000290e0, 0xc0000290e0)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:92 +0x51
github.com/go-critic/go-critic/framework/linter.NewChecker(...)
        /home/ldez/sources/go/pkg/mod/github.com/go-critic/[email protected]/framework/linter/lintpack.go:169
github.com/golangci/golangci-lint/pkg/golinters.buildEnabledCheckers(0x1007f01f7f1c018, 0x167)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/gocritic.go:149 +0x145
github.com/golangci/golangci-lint/pkg/golinters.NewGocritic.func1.1(0xc000f9eea0)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/gocritic.go:46 +0x125
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc000982120)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:187 +0x9c4
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:105 +0x1d
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc00089ad70, {0x13de26e, 0x8}, 0xc000069f60)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x4a
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc000982120)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_action.go:104 +0x85
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc000069ea0)
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:80 +0x67
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
        /home/ldez/sources/go/src/github.com/golangci/golangci-lint/pkg/golinters/goanalysis/runner_loadingpackage.go:75 +0x1fd 
WARN [runner] Can't run linter gocritic: gocritic: gocritic: package "gorules" (isInitialPkg: true, needAnalyzeSource: true): interface conversion: interface {} is bool, not string 

[quasilyte]

Interesting, to say the least.
I'll try to figure things out.
There is also this: https://groups.google.com/g/golang-tools/c/yQU-rDYo4aY/m/yBb5FeMIAgAJ?utm_medium=email&utm_source=footer
I'm hoping that someone could land me a hand in there.

[quasilyte]

Aha, I see.
It's a param type error.
The recent change in failOnError is that it's not a string, not bool.
Valid values:

"all" -- like failOnError:true previously
"dsl" -- DSL type checking
"import" -- import errors

"all" implies "dsl"+"import".
@sebastien-rosset can give more info on that.

As for the issue I referenced above, it was nasty, but I think we have a solution.
It'll require a version bump though.
I'll release v0.6.1 when we're sure that it fixes that problem.

[ldez]

👍 the problem was related to the type of failOnError

[sebastien-rosset]

Maybe we can split it into 2 options:

• failOnError remains a bool; implies "all" if set to true
• failOn becomes a new string that lists failing conditions (like "dsl" and "import")

If failOn is used (it's non-empty), failOnError is ignored.

Sure, I can submit a PR for this.

[quasilyte]

I've made it 2 separate flags as described above.
https://github.com/go-critic/go-critic/releases/tag/v0.6.1
I hope that everything will work this time...
Fingers crossed.

[ldez]

Everything seems to work now! 🎉

Thank you @quasilyte !

[ldez]

Then the story is not finished... Now we have an issue because of go1.15 and os.ReadFile.
I will fix that.

EDIT1: I still have an issue with CodeQL steps
EDIT2: Problem fixed

[quasilyte]

It looks like CI actually passed!
Wooo-ho. 🎉

[ernado]

LGTM now, thanks everyone ❤️

[sebastien-rosset]

I updated the PR title and description to show the upgrade is to gocritic 0.6.1

[ldez]

@quasilyte I did a quick performance test, and the new version (v0.6.1) seems to be at least 2 times slower than the previous version (v0.5.6)

I will try to investigate.