From f95dd315b720dc149f46821691e482fabb97f938 Mon Sep 17 00:00:00 2001 From: Nodar Date: Mon, 21 Aug 2023 18:00:21 +0200 Subject: [PATCH 1/2] Implement linter detection of non-matching flag and filed name --- linter/koanf/koanf.go | 100 +++++++++++++++++++++++++++++++++++++ linter/koanf/koanf_test.go | 4 +- linter/testdata/src/a/a.go | 27 ++++++++++ 3 files changed, 129 insertions(+), 2 deletions(-) diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index bc94a9c20e..8dbb392cb4 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -51,6 +51,8 @@ func run(dryRun bool, pass *analysis.Pass) (interface{}, error) { switch v := node.(type) { case *ast.StructType: res = checkStruct(pass, v) + case *ast.FuncDecl: + res = checkFlagDefs(pass, v) default: } for _, err := range res.Errors { @@ -70,6 +72,104 @@ func run(dryRun bool, pass *analysis.Pass) (interface{}, error) { return ret, nil } +func containsFlagSet(params []*ast.Field) bool { + for _, p := range params { + se, ok := p.Type.(*ast.StarExpr) + if !ok { + continue + } + sle, ok := se.X.(*ast.SelectorExpr) + if !ok { + continue + } + if sle.Sel.Name == "FlagSet" { + return true + } + } + return false +} + +// checkFlagDefs checks flag definitions in the function. +// Result contains list of errors where flag name doesn't match field name. +func checkFlagDefs(pass *analysis.Pass, f *ast.FuncDecl) Result { + // Ignore functions that does not get flagset as parameter. + if !containsFlagSet(f.Type.Params.List) { + return Result{} + } + var res Result + for _, s := range f.Body.List { + es, ok := s.(*ast.ExprStmt) + if !ok { + continue + } + callE, ok := es.X.(*ast.CallExpr) + if !ok { + continue + } + if len(callE.Args) != 3 { + continue + } + sl, ok := extractStrLit(callE.Args[0]) + if !ok { + continue + } + s, ok := selector(callE.Args[1]) + if !ok { + continue + } + if normSL := normalize(sl); !strings.EqualFold(normSL, s) { + res.Errors = append(res.Errors, koanfError{ + Pos: pass.Fset.Position(f.Pos()), + Message: fmt.Sprintf("koanf tag name: %q doesn't match the field: %q", sl, s), + }) + } + + } + return res +} + +func selector(e ast.Expr) (string, bool) { + n, ok := e.(ast.Node) + if !ok { + return "", false + } + se, ok := n.(*ast.SelectorExpr) + if !ok { + return "", false + } + return se.Sel.Name, true +} + +// Extracts literal from expression that is either: +// - string literal or +// - sum of variable and string literal. +// E.g. +// strLitFromSum(`"max-size"`) = "max-size" +// - strLitFromSum(`prefix + ".enable"“) = ".enable". +func extractStrLit(e ast.Expr) (string, bool) { + if s, ok := strLit(e); ok { + return s, true + } + if be, ok := e.(*ast.BinaryExpr); ok { + if be.Op == token.ADD { + if s, ok := strLit(be.Y); ok { + // Drop the prefix dot. + return s[1:], true + } + } + } + return "", false +} + +func strLit(e ast.Expr) (string, bool) { + if s, ok := e.(*ast.BasicLit); ok { + if s.Kind == token.STRING { + return strings.Trim(s.Value, "\""), true + } + } + return "", false +} + func checkStruct(pass *analysis.Pass, s *ast.StructType) Result { var res Result for _, f := range s.Fields.List { diff --git a/linter/koanf/koanf_test.go b/linter/koanf/koanf_test.go index 2e3e68b0f4..e3ad5e6043 100644 --- a/linter/koanf/koanf_test.go +++ b/linter/koanf/koanf_test.go @@ -15,8 +15,8 @@ func TestAll(t *testing.T) { } testdata := filepath.Join(filepath.Dir(wd), "testdata") res := analysistest.Run(t, testdata, analyzerForTests, "a") - if cnt := countErrors(res); cnt != 1 { - t.Errorf("analysistest.Run() got %v errors, expected 1", cnt) + if cnt := countErrors(res); cnt != 3 { + t.Errorf("analysistest.Run() got %v errors, expected 3", cnt) } } diff --git a/linter/testdata/src/a/a.go b/linter/testdata/src/a/a.go index ddf77b6ed1..86b7739108 100644 --- a/linter/testdata/src/a/a.go +++ b/linter/testdata/src/a/a.go @@ -1,6 +1,11 @@ package a +import ( + "flag" +) + type Config struct { + // Field name doesn't match koanf tag. L2 int `koanf:"chain"` LogLevel int `koanf:"log-level"` LogType int `koanf:"log-type"` @@ -9,3 +14,25 @@ type Config struct { Node int `koanf:"node"` Queue int `koanf:"queue"` } + +type BatchPosterConfig struct { + Enable bool `koanf:"enable"` + MaxSize int `koanf:"max-size" reload:"hot"` +} + +// Flag names don't match field names from default config. +// Contains 2 errors. +func BatchPosterConfigAddOptions(prefix string, f *flag.FlagSet) { + f.Bool(prefix+".enabled", DefaultBatchPosterConfig.Enable, "enable posting batches to l1") + f.Int("max-sz", DefaultBatchPosterConfig.MaxSize, "maximum batch size") +} + +func ConfigAddOptions(prefix string, f *flag.FlagSet) { + f.Bool(prefix+".enable", DefaultBatchPosterConfig.Enable, "enable posting batches to l1") + f.Int("max-size", DefaultBatchPosterConfig.MaxSize, "maximum batch size") +} + +var DefaultBatchPosterConfig = BatchPosterConfig{ + Enable: false, + MaxSize: 100000, +} From 0bf724b80497f821ff31fc36519ffe53b37d9676 Mon Sep 17 00:00:00 2001 From: Nodar Date: Fri, 1 Sep 2023 17:09:28 +0200 Subject: [PATCH 2/2] Fix incorrect merge with base pr --- linter/koanf/koanf.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linter/koanf/koanf.go b/linter/koanf/koanf.go index d483cf7751..c7c38e2571 100644 --- a/linter/koanf/koanf.go +++ b/linter/koanf/koanf.go @@ -116,7 +116,7 @@ func checkFlagDefs(pass *analysis.Pass, f *ast.FuncDecl) Result { if !ok { continue } - if normSL := normalize(sl); !strings.EqualFold(normSL, s) { + if normSL := strings.ReplaceAll(sl, "-", ""); !strings.EqualFold(normSL, s) { res.Errors = append(res.Errors, koanfError{ Pos: pass.Fset.Position(f.Pos()), Message: fmt.Sprintf("koanf tag name: %q doesn't match the field: %q", sl, s),