Print error message and exit with non-zero status when ruleguard parse error occurs #1666

sebastien-rosset · 2021-01-25T19:40:51Z

This PR ensures that parsing errors of ruleguard input rules are caught; in that case, golangci-lint will print a human-friendly error message (include file name and line number) and exit with non-zero status. Without this PR, a verbose stack trace is printed.

The ruleguard checker makes it possible to write dynamic rules, which means the rules files may have syntax errors that need to be caught. When a ruleguard parsing error occurs, golangci-lint will print the following message and exit.

ERRO Running error: gocritic: gocritic checker 'ruleguard' initialization failed:
ruleguard init error: parse file error: rules.go:18:1: expected declaration, found FOO

The actual error is raised when the failOnError flag is set to true in the golangci.yaml file, as explained in go-critic/go-critic#1015.

When failOnError is set to false (default), a ruleguard file that contains at least one syntax error is skipped.
When failOnError is set to true, a panic occurs if at least one ruleguard file contains a syntax error.

As of 01/25/2021, the latest goangci-lint release is v1.35.2. It uses gocritic v0.5.3, which does not have the new failOnError flag yet, but the go-critic/go-critic#1015 PR has already been merged to gocritic master. The error handling will be enabled in golangci-lint when 1) a new version of gocritic is released with support for the failOnError flag; and 2) golangci-lint needs to be bumped to the new gocritic version.

Fixes #1565

Without this PR, a verbose stack trace is printed as shown below.

WARN [linters context] Panic: gocritic: package "mos" (isInitialPkg: true, needAnalyzeSource: true): ruleguard init error: parse file error: /home/USER/goroot/src/REPO/gorules/rules.go:18:1: expected declaration, found FOO: goroutine 11646 [running]:
runtime/debug.Stack(0x12e2c7a, 0x3c, 0xc00e3bf558)
	/usr/lib/golang/src/runtime/debug/stack.go:24 +0x9f
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1(0xc00235d950)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:508 +0x1be
panic(0x10e5640, 0xc004bc3ae0)
	/usr/lib/golang/src/runtime/panic.go:969 +0x1b9
log.Panicf(0x12c08a3, 0x19, 0xc00e3bf8f0, 0x1, 0x1)
	/usr/lib/golang/src/log/log.go:358 +0xc5
github.com/go-critic/go-critic/checkers.newRuleguardChecker(0xc0000be090, 0xc000900308, 0xc00b470088)
	/home/USER/goroot/pkg/mod/github.com/go-critic/[email protected]/checkers/ruleguard_checker.go:93 +0x775
github.com/go-critic/go-critic/checkers.init.55.func1(0xc000900308, 0xc00b470088, 0x21)
	/home/USER/goroot/pkg/mod/github.com/go-critic/[email protected]/checkers/ruleguard_checker.go:43 +0x34
github.com/go-critic/go-critic/framework/linter.addChecker.func2(0xc00e1d3c20, 0xc0001a0b70)
	/home/USER/goroot/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:78 +0xd2
github.com/go-critic/go-critic/framework/linter.newChecker(0xc00e1d3c20, 0xc0008f6a20, 0x0)
	/home/USER/goroot/pkg/mod/github.com/go-critic/[email protected]/framework/linter/checkers_db.go:91 +0x71
github.com/go-critic/go-critic/framework/linter.NewChecker(...)
	/home/USER/goroot/pkg/mod/github.com/go-critic/[email protected]/framework/linter/lintpack.go:143
github.com/golangci/golangci-lint/pkg/golinters.buildEnabledCheckers(0xc000268af0, 0xc00e1d3c20, 0x0, 0xc00089a140, 0xc004dba1e0, 0xc0019bdcd0, 0xc0019bdc38)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/gocritic.go:131 +0x23a
github.com/golangci/golangci-lint/pkg/golinters.NewGocritic.func1.1(0xc00e1cef70, 0x102d4f7c0, 0x1c531a0, 0xc00004c388, 0x2)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/gocritic.go:42 +0x11f
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyze(0xc00235d950)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:590 +0xa83
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func2()
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:512 +0x2a
github.com/golangci/golangci-lint/pkg/timeutils.(*Stopwatch).TrackStage(0xc0016e0f00, 0x1246c94, 0x8, 0xc0028a3f70)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/timeutils/stopwatch.go:111 +0x50
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe(0xc00235d950)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:511 +0x91
github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze.func2(0xc00de4aad0, 0xc00235d950)
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:1059 +0x65
created by github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*loadingPackage).analyze
	/home/USER/goroot/src/github.com/golangci-lint/pkg/golinters/goanalysis/runner.go:1054 +0x305 
WARN [runner] Can't run linter goanalysis_metalinter: goanalysis_metalinter: gocritic: package "mos" (isInitialPkg: true, needAnalyzeSource: true): ruleguard init error: parse file error: /home/USER/goroot/src/REPO/gorules/rules.go:18:1: expected declaration, found FOO 
ERRO Running error: goanalysis_metalinter: gocritic: package "mos" (isInitialPkg: true, needAnalyzeSource: true): ruleguard init error: parse file error: /home/USER/goroot/src/REPO/gorules/rules.go:18:1: expected declaration, found FOO

sebastien-rosset · 2021-01-25T20:03:25Z

@quasilyte for review since he is the owner of gocritic and ruleguard.

sebastien-rosset · 2021-01-25T20:11:10Z

To test this feature, I used the following config file:

linters:
  enable:
    - gocritic
linters-settings:
  gocritic:
    settings:
      ruleguard:
        failOnError: true
        rules: rules.go

Commands for test purpose:

git clone https://github.com/golangci/golangci-lint.git
cd golangci-lint
# Update go mod to use the go-critic@master, which has support for the 'failOnError' flag.
go get -v github.com/go-critic/go-critic@master
make
# Create rules.go file containing ruleguard rules.
# Try with and without syntax errors in the rules.go file.

ldez · 2021-01-25T23:04:38Z

Converted to draft because of:

a new version of gocritic is released with support for the failOnError flag; and 2) golangci-lint needs to be bumped to the new gocritic version.

ldez · 2021-01-25T23:08:13Z

A lib that throws a panic is not a good thing, seems to be a design flaw.

sebastien-rosset · 2021-01-25T23:39:05Z

A lib that throws a panic is not a good thing, seems to be a design flaw.

Sure, that makes sense. It seems this is because the signature of gocriticlinter.NewChecker(linterCtx, info) does not return an error, hence it's not currently possible to handle initialization issues gracefully. Maybe the NewChecker() function should return (*Checker, error) instead of today *Checker.

The constructor in https://github.com/go-critic/go-critic/blob/master/framework/linter/checkers_db.go#L14 should return (*Checker, error). With this, all checkers under https://github.com/go-critic/go-critic/tree/master/checkers need to be changed to return an error. In particular the ruleguard checker can be changed here to return an error instead of panic: https://github.com/go-critic/go-critic/blob/master/checkers/ruleguard_checker.go#L84

One potential issue is that gocriticlinter.NewChecker is exported. Is it ok to change the signature, or do we need a new function?

@quasilyte , are you ok with these proposed changes in go-critic/go-critic#1020?

go.mod

sebastien-rosset · 2021-01-31T20:55:55Z

A lib that throws a panic is not a good thing, seems to be a design flaw.

go-critic is now returning errors instead of invoking panic.

ldez · 2021-01-31T21:13:25Z

@sebastien-rosset could you execute go mod tidy?

sebastien-rosset · 2021-02-01T01:26:30Z

@sebastien-rosset could you execute go mod tidy?

done

ldez

LGTM

…e error occurs (golangci#1666)

Add defer function to recover from ruleguard parse errors

9e35ec4

sebastien-rosset changed the title ~~Add defer function to recover from ruleguard parse errors~~ Print error message and exit with non-zero status when ruleguard parse error occurs Jan 25, 2021

sebastien-rosset added 2 commits January 25, 2021 11:49

Add defer function to recover from ruleguard parse errors

7c639ab

Add defer function to recover from ruleguard parse errors

2503ca7

ldez marked this pull request as draft January 25, 2021 23:03

ldez added the linter: update version Update version of linter label Jan 25, 2021

sebastien-rosset mentioned this pull request Jan 26, 2021

Checker constructor may return error go-critic/go-critic#1020

Merged

sebastien-rosset added 3 commits January 31, 2021 20:49

Error handling when go-critic checkers are initialized

0e6d99d

Error handling when go-critic checkers are initialized

d9b263a

Error handling when go-critic checkers are initialized

df0fe3a

sebastien-rosset commented Jan 31, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Error handling when go-critic checkers are initialized

7cac8d4

ldez marked this pull request as ready for review January 31, 2021 21:11

Error handling when go-critic checkers are initialized

57926d3

ldez mentioned this pull request Feb 1, 2021

build(deps): bump github.com/go-critic/go-critic from 0.5.3 to 0.5.4 #1681

Closed

ldez approved these changes Feb 1, 2021

View reviewed changes

ldez merged commit 443e5b6 into golangci:master Feb 1, 2021

ashanbrown pushed a commit to ashanbrown/golangci-lint that referenced this pull request Feb 20, 2021

Print error message and exit with non-zero status when ruleguard pars…

d25f18d

…e error occurs (golangci#1666)

ldez added this to the v1.37 milestone Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print error message and exit with non-zero status when ruleguard parse error occurs #1666

Print error message and exit with non-zero status when ruleguard parse error occurs #1666

sebastien-rosset commented Jan 25, 2021 •

edited by ldez

Loading

sebastien-rosset commented Jan 25, 2021

sebastien-rosset commented Jan 25, 2021 •

edited

Loading

ldez commented Jan 25, 2021

ldez commented Jan 25, 2021

sebastien-rosset commented Jan 25, 2021 •

edited

Loading

sebastien-rosset commented Jan 31, 2021

ldez commented Jan 31, 2021

sebastien-rosset commented Feb 1, 2021

ldez left a comment

Print error message and exit with non-zero status when ruleguard parse error occurs #1666

Print error message and exit with non-zero status when ruleguard parse error occurs #1666

Conversation

sebastien-rosset commented Jan 25, 2021 • edited by ldez Loading

sebastien-rosset commented Jan 25, 2021

sebastien-rosset commented Jan 25, 2021 • edited Loading

ldez commented Jan 25, 2021

ldez commented Jan 25, 2021

sebastien-rosset commented Jan 25, 2021 • edited Loading

sebastien-rosset commented Jan 31, 2021

ldez commented Jan 31, 2021

sebastien-rosset commented Feb 1, 2021

ldez left a comment

Choose a reason for hiding this comment

sebastien-rosset commented Jan 25, 2021 •

edited by ldez

Loading

sebastien-rosset commented Jan 25, 2021 •

edited

Loading

sebastien-rosset commented Jan 25, 2021 •

edited

Loading