Skip to content

Commit

Permalink
logcheck: support settings in golangci-lint
Browse files Browse the repository at this point in the history
This implements the new interface from
golangci/golangci-lint#3887 with two fields in the
"settings":

type settings struct {
      Check  map[string]bool `json:"check"`
      Config string          `json:"config"`
}

This is the actual config *data*, not the name of a config file. This is
intentional because
- relative file paths wouldn't work (plugin cannot resolve them)
- loading additional file content wouldn't invalidate cached linter
  results the that embedding the data inside the main config does

The `include` text template method ensures that logcheck.conf gets injected
automatically. Having it as separate file has the advantage that manual
invocations of the stand-alone logcheck linter still work.
  • Loading branch information
pohly committed Jun 12, 2023
1 parent ff0f1e9 commit bbcd3fc
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 16 deletions.
3 changes: 2 additions & 1 deletion logcheck/main.go
Expand Up @@ -23,5 +23,6 @@ import (
)

func main() {
singlechecker.Main(pkg.Analyser())
analyzer, _ := pkg.Analyser()
singlechecker.Main(analyzer)
}
2 changes: 1 addition & 1 deletion logcheck/main_test.go
Expand Up @@ -153,7 +153,7 @@ func TestAnalyzer(t *testing.T) {
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
analyzer := pkg.Analyser()
analyzer, _ := pkg.Analyser()
set := func(flag, value string) {
if value != "" {
if err := analyzer.Flags.Set(flag, value); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions logcheck/pkg/filter.go
Expand Up @@ -20,6 +20,7 @@ import (
"bufio"
"flag"
"fmt"
"io"
"os"
"regexp"
"strings"
Expand Down Expand Up @@ -50,7 +51,10 @@ func (f *RegexpFilter) Set(filename string) error {
return err
}
defer file.Close()
return f.Parse(file, filename)
}

func (f *RegexpFilter) Parse(file io.Reader, filename string) error {
// Reset before parsing.
f.filename = filename
f.lines = nil
Expand Down
36 changes: 25 additions & 11 deletions logcheck/pkg/logcheck.go
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package pkg

import (
"bytes"
"flag"
"fmt"
"go/ast"
Expand Down Expand Up @@ -45,18 +46,31 @@ const (

type checks map[string]*bool

type config struct {
type Config struct {
enabled checks
fileOverrides RegexpFilter
}

func (c config) isEnabled(check string, filename string) bool {
func (c Config) isEnabled(check string, filename string) bool {
return c.fileOverrides.Enabled(check, *c.enabled[check], filename)
}

func (c *Config) SetEnabled(check string, enabled bool) error {
_, ok := c.enabled[check]
if !ok {
return fmt.Errorf("unsupported check %q", check)
}
*c.enabled[check] = enabled
return nil
}

func (c *Config) ParseConfig(configContent string) error {
return c.fileOverrides.Parse(bytes.NewBufferString(configContent), "<buffer>")
}

// Analyser creates a new logcheck analyser.
func Analyser() *analysis.Analyzer {
c := config{
func Analyser() (*analysis.Analyzer, *Config) {
c := Config{
enabled: checks{
structuredCheck: new(bool),
parametersCheck: new(bool),
Expand All @@ -72,7 +86,7 @@ func Analyser() *analysis.Analyzer {
for key := range c.enabled {
c.fileOverrides.validChecks[key] = true
}
logcheckFlags := flag.NewFlagSet("", flag.ExitOnError)
var logcheckFlags flag.FlagSet
prefix := "check-"
logcheckFlags.BoolVar(c.enabled[structuredCheck], prefix+structuredCheck, true, `When true, logcheck will warn about calls to unstructured
klog methods (Info, Infof, Error, Errorf, Warningf, etc).`)
Expand Down Expand Up @@ -110,11 +124,11 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`)
Run: func(pass *analysis.Pass) (interface{}, error) {
return run(pass, &c)
},
Flags: *logcheckFlags,
}
Flags: logcheckFlags,
}, &c
}

func run(pass *analysis.Pass, c *config) (interface{}, error) {
func run(pass *analysis.Pass, c *Config) (interface{}, error) {
for _, file := range pass.Files {
ast.Inspect(file, func(n ast.Node) bool {
switch n := n.(type) {
Expand All @@ -135,7 +149,7 @@ func run(pass *analysis.Pass, c *config) (interface{}, error) {
}

// checkForFunctionExpr checks for unstructured logging function, prints error if found any.
func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *Config) {
fun := fexpr.Fun
args := fexpr.Args

Expand Down Expand Up @@ -414,7 +428,7 @@ func hasFormatSpecifier(fArgs []ast.Expr) (string, bool) {
// context and a logger. That is problematic because it leads to ambiguity:
// does the context already contain the logger? That matters when passing it on
// without the logger.
func checkForContextAndLogger(n ast.Node, params *ast.FieldList, pass *analysis.Pass, c *config) {
func checkForContextAndLogger(n ast.Node, params *ast.FieldList, pass *analysis.Pass, c *Config) {
var haveLogger, haveContext bool

for _, param := range params.List {
Expand Down Expand Up @@ -445,7 +459,7 @@ func checkForContextAndLogger(n ast.Node, params *ast.FieldList, pass *analysis.

// checkForIfEnabled detects `if klog.V(..).Enabled() { ...` and `if
// logger.V(...).Enabled()` and suggests capturing the result of V.
func checkForIfEnabled(i *ast.IfStmt, pass *analysis.Pass, c *config) {
func checkForIfEnabled(i *ast.IfStmt, pass *analysis.Pass, c *Config) {
// if i.Init == nil {
// A more complex if statement, let's assume it's okay.
// return
Expand Down
47 changes: 44 additions & 3 deletions logcheck/plugin/plugin.go
Expand Up @@ -19,17 +19,58 @@ limitations under the License.
package main

import (
"bytes"
"encoding/json"
"fmt"

"golang.org/x/tools/go/analysis"
"sigs.k8s.io/logtools/logcheck/pkg"
)

type analyzerPlugin struct{}

func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
return []*analysis.Analyzer{
pkg.Analyser(),
}
analyzer, _ := pkg.Analyser()
return []*analysis.Analyzer{analyzer}
}

// AnalyzerPlugin is the entry point for golangci-lint.
var AnalyzerPlugin analyzerPlugin

type settings struct {
Check map[string]bool `json:"check"`
Config string `json:"config"`
}

// New API, see https://github.com/golangci/golangci-lint/pull/3887.
func New(pluginSettings interface{}) ([]*analysis.Analyzer, error) {
// We could manually parse the settings. This would involve several
// type assertions. Encoding as JSON and then decoding into our
// settings struct is easier.
//
// The downside is that format errors are less user-friendly.
var buffer bytes.Buffer
if err := json.NewEncoder(&buffer).Encode(pluginSettings); err != nil {
return nil, fmt.Errorf("encoding settings as internal JSON buffer: %v", err)
}
var s settings
decoder := json.NewDecoder(&buffer)
decoder.DisallowUnknownFields()
if err := decoder.Decode(&s); err != nil {
return nil, fmt.Errorf("decoding settings from internal JSON buffer: %v", err)
}

// Now create an analyzer and configure it.
analyzer, config := pkg.Analyser()
for check, enabled := range s.Check {
if err := config.SetEnabled(check, enabled); err != nil {
// No need to wrap, the error is informative.
return nil, err
}
}
if err := config.ParseConfig(s.Config); err != nil {
return nil, fmt.Errorf("parsing config: %v", err)
}

return []*analysis.Analyzer{analyzer}, nil
}

0 comments on commit bbcd3fc

Please sign in to comment.