From dfce8a98399b4d1612054a0080a72925efab3984 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Thu, 26 Feb 2026 02:34:35 +0200 Subject: [PATCH] fix(go): reduce false positives from comment lines and multi-return discards Tested against gin, echo, cobra, zap - reduced FPs from 78 to 40 (-49%). - Add ^(?!\s*\/\/) comment-line guard to go_fmt_debugging, go_log_debugging, go_spew_debugging, go_bare_os_exit patterns - Change go_discarded_error to ^\s*_ anchor - only matches when _ is the first token on the line, excluding multi-return assignments like value, _ = func() and _, _ = fmt.Fprintf(...) - Add false positive tests for all tuned patterns --- __tests__/slop-patterns-go.test.js | 41 +++++++++++++++++++++++++++--- lib/patterns/slop-patterns.js | 10 ++++---- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/__tests__/slop-patterns-go.test.js b/__tests__/slop-patterns-go.test.js index 9993a85..111b66e 100644 --- a/__tests__/slop-patterns-go.test.js +++ b/__tests__/slop-patterns-go.test.js @@ -109,6 +109,11 @@ describe('go_fmt_debugging', () => { expect(pattern.test('fmt.Print("hello")')).toBe(true); }); + test('does not match commented-out fmt.Println', () => { + expect(pattern.test('// fmt.Println("debug")')).toBe(false); + expect(pattern.test(' // fmt.Printf("value: %v", x)')).toBe(false); + }); + test('does not match fmt.Sprintf()', () => { expect(pattern.test('s := fmt.Sprintf("value: %d", x)')).toBe(false); }); @@ -170,6 +175,11 @@ describe('go_log_debugging', () => { expect(pattern.test('log.Panicf("critical: %v", err)')).toBe(true); }); + test('does not match commented-out log calls', () => { + expect(pattern.test('// log.Println("debug")')).toBe(false); + expect(pattern.test(' // log.Fatal(err)')).toBe(false); + }); + test('does not match slog.Info()', () => { expect(pattern.test('slog.Info("starting server")')).toBe(false); }); @@ -223,8 +233,9 @@ describe('go_spew_debugging', () => { expect(pattern.test('pp.Print(data)')).toBe(true); }); - test('does not match spew in comments', () => { - expect(pattern.test('// use spew for debugging')).toBe(false); + test('does not match commented-out spew calls', () => { + expect(pattern.test('// spew.Dump(config)')).toBe(false); + expect(pattern.test(' // pp.Println(result)')).toBe(false); }); test('excludes test files', () => { @@ -295,13 +306,29 @@ describe('go_discarded_error', () => { expect(pattern.test('_ = file.Close()')).toBe(true); }); + test('matches indented _ = file.Close()', () => { + expect(pattern.test('\t_ = file.Close()')).toBe(true); + expect(pattern.test(' _ = conn.Close()')).toBe(true); + }); + + test('does not match multi-return value, _ = func()', () => { + // Multi-return where _ discards a bool or non-error value + expect(pattern.test('value, _ = c.GetQuery(key)')).toBe(false); + expect(pattern.test('head, tail, _ = strings.Cut(str, sep)')).toBe(false); + expect(pattern.test('resp, _ = UnwrapResponse(rw)')).toBe(false); + }); + + test('does not match double-discard _, _ = func()', () => { + // Both returns discarded deliberately + expect(pattern.test('_, _ = fmt.Fprintf(w, "response")')).toBe(false); + expect(pattern.test('_, _ = io.Copy(io.Discard, r.Body)')).toBe(false); + }); + test('does not match for _, v := range', () => { - // This is a critical false positive guard - range iteration is not error discarding expect(pattern.test('for _, v := range items {')).toBe(false); }); test('does not match _, ok := m[key]', () => { - // Map existence check - the _ discards value, not an error expect(pattern.test('_, ok := m[key]')).toBe(false); }); @@ -329,6 +356,12 @@ describe('go_bare_os_exit', () => { expect(pattern.test('os.Exit(0)')).toBe(true); }); + test('does not match os.Exit in comments', () => { + expect(pattern.test('// os.Exit(1).')).toBe(false); + expect(pattern.test('// The logger then calls os.Exit(1).')).toBe(false); + expect(pattern.test(' // FatalLevel logs a message, then calls os.Exit(1).')).toBe(false); + }); + test('does not match os.Exitcode', () => { expect(pattern.test('code := os.Exitcode')).toBe(false); }); diff --git a/lib/patterns/slop-patterns.js b/lib/patterns/slop-patterns.js index 385583a..4be49c8 100644 --- a/lib/patterns/slop-patterns.js +++ b/lib/patterns/slop-patterns.js @@ -584,7 +584,7 @@ const slopPatterns = { * Debug print statements left in production code - use a logging package instead */ go_fmt_debugging: { - pattern: /\bfmt\.Print(?:ln|f)?\s*\(/, + pattern: /^(?!\s*\/\/).*\bfmt\.Print(?:ln|f)?\s*\(/, exclude: ['*_test.go', '**/testdata/**', 'cmd/**', '**/cmd/**', 'main.go'], severity: 'medium', autoFix: 'remove', @@ -597,7 +597,7 @@ const slopPatterns = { * Standard library log calls often indicate leftover debugging */ go_log_debugging: { - pattern: /\blog\.(?:Print|Fatal|Panic)(?:ln|f)?\s*\(/, + pattern: /^(?!\s*\/\/).*\blog\.(?:Print|Fatal|Panic)(?:ln|f)?\s*\(/, exclude: ['*_test.go', '**/testdata/**', 'cmd/**', '**/cmd/**', 'main.go'], severity: 'low', autoFix: 'flag', @@ -610,7 +610,7 @@ const slopPatterns = { * Third-party debug dump libraries left in production code */ go_spew_debugging: { - pattern: /\b(?:spew\.(?:Dump|Printf|Sdump)|pp\.Print(?:ln)?)\s*\(/, + pattern: /^(?!\s*\/\/).*\b(?:spew\.(?:Dump|Printf|Sdump)|pp\.Print(?:ln)?)\s*\(/, exclude: ['*_test.go', '**/testdata/**'], severity: 'medium', autoFix: 'remove', @@ -636,7 +636,7 @@ const slopPatterns = { * Assigning function return to _ discards potential errors */ go_discarded_error: { - pattern: /\b_\s*=\s*\w+(?:\.\w+){0,10}\s*\(/, + pattern: /^\s*_\s*=\s*\w+(?:\.\w+){0,10}\s*\(/, exclude: ['*_test.go', '**/testdata/**'], severity: 'medium', autoFix: 'flag', @@ -649,7 +649,7 @@ const slopPatterns = { * os.Exit() skips deferred functions and should only be in main() */ go_bare_os_exit: { - pattern: /\bos\.Exit\s*\(/, + pattern: /^(?!\s*\/\/).*\bos\.Exit\s*\(/, exclude: ['*_test.go', '**/testdata/**', 'cmd/**', '**/cmd/**', 'main.go'], severity: 'medium', autoFix: 'flag',