From d3af61c5a2cb13017d72fd35e84667963a47777f Mon Sep 17 00:00:00 2001 From: Lucas Machado Date: Sun, 19 Jul 2026 15:47:12 +0200 Subject: [PATCH 1/2] fix: stop table-driven tests from being reported as clones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A table-driven test matched its own rows: the "instances" were overlapping sliding windows over one composite literal, not duplicated logic. Two independent defects. 1. Overlapping instances. Seeds inside a file are spaced minTokens apart, but greedy extension grows each block to totalTokens, so neighbouring instances overlapped — the same text counted several times. Instances are now kept only when they stay clear of the previous one. 2. Data tables. The rows of a collection literal are structurally identical by design. Literals are recorded as token spans, and a clone whose every instance lies inside ONE span is dropped as data. Masking literal tokens out of detection was tried first and rejected: a short inline `[]T{a, b}` in the middle of a duplicated block split that block and lost the finding. On this repo's own tests that approach dropped ~50 genuine clones; the span rule drops none. Logic still counts — a callback column, two identical tables in different places, and blocks containing inline literals are all still reported. Covers Go, Python, JS/TS, Ruby, Rust, PHP, Elixir, Swift, Dart and Lua. Java, C, C++, C# and Kotlin are excluded: there `[` is an index or array type and `{` is also a block, so a literal can't be told from code. Two bugs found while building this, fixed here: - markPythonFunctions resumed scanning *after* the `def` that ended a body, stepping over it, so every function after the first was never in scope and its duplication was invisible. - Bodies in languages without a `func` keyword come from a brace-depth heuristic that read a `{…}` map literal as a function body; a real body follows `)` or an arrow, a literal follows `=`, `,` or `[`. - TokenizeFile dereferenced a nil language while markFunctionBodies documents nil as "no syntax knowledge". Closes #31 --- integration/table_literal_test.go | 208 +++++++++++ services/composite_literal_test.go | 516 ++++++++++++++++++++++++++++ services/data_literal_langs_test.go | 303 ++++++++++++++++ services/detector.go | 163 ++++++++- services/overlap_test.go | 150 ++++++++ services/pyfuncboundary_test.go | 123 +++++++ services/table_literal_test.go | 278 +++++++++++++++ services/testhelpers_test.go | 9 +- services/tokenizer.go | 398 ++++++++++++++++++++- 9 files changed, 2124 insertions(+), 24 deletions(-) create mode 100644 integration/table_literal_test.go create mode 100644 services/composite_literal_test.go create mode 100644 services/data_literal_langs_test.go create mode 100644 services/overlap_test.go create mode 100644 services/pyfuncboundary_test.go create mode 100644 services/table_literal_test.go diff --git a/integration/table_literal_test.go b/integration/table_literal_test.go new file mode 100644 index 0000000..9ecff2a --- /dev/null +++ b/integration/table_literal_test.go @@ -0,0 +1,208 @@ +package integration + +import ( + "encoding/json" + "os/exec" + "testing" + + "github.com/AxeForging/dupehound/domain" +) + +// End-to-end coverage for issue #31, driven through the built binary at its +// real defaults. The unit tests pin the mask and the overlap guard; these pin +// what a user actually sees when they run `dupehound scan` on a Go package with +// table-driven tests. + +// issue31Repro is the fixture from the issue report. +const issue31Repro = `package repro + +import ( + "strings" + "testing" +) + +func TestRedact(t *testing.T) { + tests := []struct { + name string + input string + absent string + present string + }{ + {name: "openai preset case", input: "token openai_AAAA0\n", absent: "openai_AAAA", present: "openai_***"}, + {name: "anthropic preset case", input: "token anthropic_AAAA1\n", absent: "anthropic_AAAA", present: "anthropic_***"}, + {name: "github preset case", input: "token github_AAAA2\n", absent: "github_AAAA", present: "github_***"}, + {name: "awskey preset case", input: "token awskey_AAAA3\n", absent: "awskey_AAAA", present: "awskey_***"}, + {name: "bearer preset case", input: "token bearer_AAAA4\n", absent: "bearer_AAAA", present: "bearer_***"}, + {name: "stripe preset case", input: "token stripe_AAAA5\n", absent: "stripe_AAAA", present: "stripe_***"}, + {name: "gcp preset case", input: "token gcp_AAAA6\n", absent: "gcp_AAAA", present: "gcp_***"}, + {name: "azure preset case", input: "token azure_AAAA7\n", absent: "azure_AAAA", present: "azure_***"}, + {name: "npm preset case", input: "token npm_AAAA8\n", absent: "npm_AAAA", present: "npm_***"}, + {name: "slack preset case", input: "token slack_AAAA9\n", absent: "slack_AAAA", present: "slack_***"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if strings.Contains(tc.present, tc.absent) { + t.Errorf("%s", tc.name) + } + }) + } +} +` + +// neutralConfig writes a config with no excludes into dir and returns its path. +// +// Config discovery walks up from the working directory, so a scan launched from +// the test binary would otherwise inherit dupehound's own .dupehound.yml — which +// excludes *_test.go and would silently skip every fixture here. Passing an +// explicit config pins these tests to default scanning behaviour, which is the +// behaviour issue #31 is about. +func neutralConfig(t *testing.T, dir string) string { + t.Helper() + return writeFile(t, dir, "neutral.yml", "scan:\n exclude: []\n") +} + +// scanJSON runs a scan over dir and decodes the report. +func scanJSON(t *testing.T, bin, dir string, args ...string) domain.Report { + t.Helper() + full := append([]string{ + "scan", "--path", dir, + "--config", neutralConfig(t, dir), + "--format", "json", "--exit-zero", + }, args...) + out, err := exec.Command(bin, full...).Output() // stdout only — logs go to stderr + if err != nil { + t.Fatalf("scan failed: %v", err) + } + var report domain.Report + if err := json.Unmarshal(out, &report); err != nil { + t.Fatalf("invalid JSON output: %v\n%s", err, string(out)) + } + return report +} + +// scanExitCode runs a scan for its exit status — the signal CI and pre-commit +// hooks actually gate on. +func scanExitCode(t *testing.T, bin, dir string) error { + t.Helper() + return exec.Command(bin, "scan", "--path", dir, + "--config", neutralConfig(t, dir), "--quiet").Run() +} + +func TestScan_Issue31_TableDrivenTestIsClean(t *testing.T) { + bin := buildBinary(t) + dir := t.TempDir() + writeFile(t, dir, "table_test.go", issue31Repro) + + report := scanJSON(t, bin, dir) + + if len(report.Clones) != 0 { + t.Errorf("table-driven test reported %d clone(s); expected a clean scan", len(report.Clones)) + for _, c := range report.Clones { + t.Logf(" %s similarity=%.2f instances=%d", c.Type, c.Similarity, len(c.Instances)) + for _, in := range c.Instances { + t.Logf(" %s:%d-%d", in.File, in.StartLine, in.EndLine) + } + } + } +} + +// The scan must exit 0 for a package whose only "duplication" was a data table +// — that exit code is what gates CI and pre-commit hooks. +func TestScan_Issue31_TableDrivenTestExitsZeroWithoutExitZeroFlag(t *testing.T) { + bin := buildBinary(t) + dir := t.TempDir() + writeFile(t, dir, "table_test.go", issue31Repro) + + // Deliberately without --exit-zero: a clean scan must exit 0 on its own. + if err := scanExitCode(t, bin, dir); err != nil { + t.Errorf("scan exited non-zero on a package with only a data table: %v", err) + } +} + +// The counterpart guard: genuinely duplicated logic in the same package is +// still found, and the scan still fails. Without this, "no clones" could just +// mean detection is broken. +func TestScan_Issue31_RealDuplicationStillFailsTheScan(t *testing.T) { + bin := buildBinary(t) + dir := t.TempDir() + writeFile(t, dir, "table_test.go", issue31Repro) + + dup := ` + server := newServer() + defer server.Close() + client := server.Client() + req := buildRequest(server.URL) + resp, err := client.Do(req) + if err != nil { + panic(err) + } + defer resp.Body.Close() + body := readAll(resp.Body) + if resp.StatusCode != 200 { + panic(body) + } + record(body) +` + writeFile(t, dir, "handlers.go", + "package repro\n\nfunc alpha() {"+dup+"}\n\nfunc bravo() {"+dup+"}\n") + + report := scanJSON(t, bin, dir) + if len(report.Clones) == 0 { + t.Fatal("copy-pasted function bodies must still be reported") + } + + // Every reported instance must come from the real duplication, not the table. + for _, c := range report.Clones { + for _, in := range c.Instances { + if in.File == "table_test.go" || filepathBase(in.File) == "table_test.go" { + t.Errorf("clone instance points at the data table: %s:%d-%d", in.File, in.StartLine, in.EndLine) + } + } + } + + // And a scan with real duplication must fail (exit non-zero) so CI blocks it. + if err := scanExitCode(t, bin, dir); err == nil { + t.Error("scan exited 0 despite genuine duplication; CI would not block it") + } +} + +// No clone group may ever report overlapping instances within one file. +func TestScan_Issue31_NoOverlappingInstancesInReport(t *testing.T) { + bin := buildBinary(t) + dir := t.TempDir() + + // A long, highly periodic body — the shape that produced sliding-window + // "instances" of the same text before the overlap guard. + src := "package repro\n\nfunc run() {\n" + for i := 0; i < 40; i++ { + src += "\tstep(a, b)\n\tcheck(a, b)\n\temit(a, b)\n" + } + src += "}\n" + writeFile(t, dir, "periodic.go", src) + + report := scanJSON(t, bin, dir) + for _, c := range report.Clones { + for i := range c.Instances { + for j := i + 1; j < len(c.Instances); j++ { + a, b := c.Instances[i], c.Instances[j] + if a.File != b.File { + continue + } + if a.StartLine <= b.EndLine && b.StartLine <= a.EndLine { + t.Errorf("clone %s reports overlapping instances in %s: %d-%d and %d-%d", + c.Hash, a.File, a.StartLine, a.EndLine, b.StartLine, b.EndLine) + } + } + } + } +} + +// filepathBase avoids importing path/filepath just for one call in assertions. +func filepathBase(p string) string { + for i := len(p) - 1; i >= 0; i-- { + if p[i] == '/' || p[i] == '\\' { + return p[i+1:] + } + } + return p +} diff --git a/services/composite_literal_test.go b/services/composite_literal_test.go new file mode 100644 index 0000000..bc2606f --- /dev/null +++ b/services/composite_literal_test.go @@ -0,0 +1,516 @@ +package services + +import ( + "testing" + + "github.com/AxeForging/dupehound/domain" +) + +// Tests for the composite-literal mask (issue #31). The mask decides which +// tokens are data rather than logic, so both directions matter: masking a +// control-flow block would blind the detector, and failing to mask a data table +// brings the false positives back. + +// maskProbe pairs a tokenized fixture with its data-literal spans so a test can +// ask whether a specific, distinctive token was classified as table data. +type maskProbe struct { + tokens []Token + mask []bool +} + +// spansToMask expands token spans into a per-token bool, which makes the +// assertions below read in terms of individual tokens. +func spansToMask(n int, spans []TokenSpan) []bool { + mask := make([]bool, n) + for _, s := range spans { + for i := s.Start; i <= s.End && i < n; i++ { + mask[i] = true + } + } + return mask +} + +// probeLiterals reports the raw data-literal spans, before function literals +// nested inside a table are carved back out. +func probeLiterals(src string) maskProbe { + tokens := TokenizeFile(src, goL()) + return maskProbe{tokens: tokens, mask: spansToMask(len(tokens), findDataLiteralSpans(tokens, goL()))} +} + +// probeDetectionMask reports what the detector actually treats as table data: +// inside a data literal, but not inside a function literal nested in one. +func probeDetectionMask(src string) maskProbe { + return probeDetectionMaskLang(src, goL()) +} + +func probeDetectionMaskLang(src string, lang *domain.Language) maskProbe { + tokens := TokenizeFile(src, lang) + _, funcs := markFunctionBodies(tokens, lang) + literals := findDataLiteralSpans(tokens, lang) + data := spansToMask(len(tokens), literals) + for _, fl := range nestedFuncLiterals(findFuncLiteralBodies(tokens, lang, funcs), literals) { + for i := fl.Start; i <= fl.End && i < len(data); i++ { + data[i] = false + } + } + return maskProbe{tokens: tokens, mask: data} +} + +// wantMasked asserts that every occurrence of a source token is classified as +// table data (or, for want=false, that none of them are). The fixtures use +// distinctive names so a probe identifies exactly one region of interest. +func (p maskProbe) wantMasked(t *testing.T, text string, want bool) { + t.Helper() + found := 0 + for i, tok := range p.tokens { + s := tok.Text + if s == "" { + s = tok.OrigText + } + if s != text { + continue + } + found++ + if p.mask[i] != want { + t.Errorf("token %q (occurrence %d, line %d): data=%v, want data=%v", + text, found, tok.Line, p.mask[i], want) + } + } + if found == 0 { + t.Fatalf("probe token %q never appears in the fixture — the test is not asserting anything", text) + } +} + +func TestMarkCompositeLiterals_DataLiteralsAreMasked(t *testing.T) { + tests := []struct { + name string + src string + probe string + }{ + { + name: "slice of named type", + src: "package p\nfunc f() {\n\tx := []string{ALPHA, BRAVO}\n}\n", + probe: "ALPHA", + }, + { + name: "slice of anonymous struct (table-driven test shape)", + src: "package p\nfunc f() {\n\tx := []struct{ n int }{{n: ALPHA}, {n: BRAVO}}\n}\n", + probe: "ALPHA", + }, + { + name: "map literal", + src: "package p\nfunc f() {\n\tx := map[string]int{ALPHA: 1, BRAVO: 2}\n}\n", + probe: "ALPHA", + }, + { + name: "map of slices", + src: "package p\nfunc f() {\n\tx := map[string][]int{ALPHA: {1, 2}}\n}\n", + probe: "ALPHA", + }, + { + name: "slice of pointers", + src: "package p\nfunc f() {\n\tx := []*Thing{{Name: ALPHA}}\n}\n", + probe: "ALPHA", + }, + { + name: "slice of slices", + src: "package p\nfunc f() {\n\tx := [][]int{{ALPHA}, {BRAVO}}\n}\n", + probe: "ALPHA", + }, + { + name: "fixed-size array", + src: "package p\nfunc f() {\n\tx := [3]int{ALPHA, BRAVO, CHARLIE}\n}\n", + probe: "ALPHA", + }, + { + name: "ellipsis array", + src: "package p\nfunc f() {\n\tx := [...]int{ALPHA, BRAVO}\n}\n", + probe: "ALPHA", + }, + { + name: "qualified element type", + src: "package p\nfunc f() {\n\tx := []time.Duration{ALPHA, BRAVO}\n}\n", + probe: "ALPHA", + }, + { + name: "generic element type", + src: "package p\nfunc f() {\n\tx := []Set[int]{{ALPHA}, {BRAVO}}\n}\n", + probe: "ALPHA", + }, + { + name: "slice of interface", + src: "package p\nfunc f() {\n\tx := []interface{}{ALPHA, BRAVO}\n}\n", + probe: "ALPHA", + }, + { + name: "literal passed as a call argument", + src: "package p\nfunc f() {\n\tprocess([]int{ALPHA, BRAVO})\n}\n", + probe: "ALPHA", + }, + { + name: "address-of literal", + src: "package p\nfunc f() {\n\tx := &[]int{ALPHA}\n}\n", + probe: "ALPHA", + }, + { + name: "literal returned directly", + src: "package p\nfunc f() []int {\n\treturn []int{ALPHA, BRAVO}\n}\n", + probe: "ALPHA", + }, + { + name: "nested literal inside an outer literal", + src: "package p\nfunc f() {\n\tx := []Row{{Vals: []int{ALPHA}}}\n}\n", + probe: "ALPHA", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + probeLiterals(tc.src).wantMasked(t, tc.probe, true) + }) + } +} + +func TestMarkCompositeLiterals_LogicIsNotMasked(t *testing.T) { + tests := []struct { + name string + src string + probe string + }{ + { + name: "map index guarding an if block", + src: "package p\nfunc f() {\n\tif m[k] {\n\t\tALPHA()\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "slice index in a range statement", + src: "package p\nfunc f() {\n\tfor _, v := range m[k] {\n\t\tALPHA(v)\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "index expression in a comparison", + src: "package p\nfunc f() {\n\tif arr[i] > 0 {\n\t\tALPHA()\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "index on a call result", + src: "package p\nfunc f() {\n\tif rows()[0] {\n\t\tALPHA()\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "plain struct literal is left alone", + src: "package p\nfunc f() {\n\tx := Config{Name: ALPHA, Port: 8080}\n}\n", + probe: "ALPHA", + }, + { + name: "for loop body", + src: "package p\nfunc f() {\n\tfor i := 0; i < 10; i++ {\n\t\tALPHA(i)\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "switch body", + src: "package p\nfunc f() {\n\tswitch v {\n\tcase 1:\n\t\tALPHA()\n\t}\n}\n", + probe: "ALPHA", + }, + { + name: "func literal assigned to a variable", + src: "package p\nfunc f() {\n\tg := func() {\n\t\tALPHA()\n\t}\n\tg()\n}\n", + probe: "ALPHA", + }, + { + name: "statement following a literal", + src: "package p\nfunc f() {\n\tx := []int{1, 2, 3}\n\tALPHA(x)\n}\n", + probe: "ALPHA", + }, + { + name: "brace inside a string literal", + src: "package p\nfunc f() {\n\ts := \"{not a brace\"\n\tALPHA(s)\n}\n", + probe: "ALPHA", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + probeLiterals(tc.src).wantMasked(t, tc.probe, false) + }) + } +} + +// Barrier cases: the mask must survive degenerate and malformed input without +// panicking and without swallowing the rest of the file. +func TestMarkCompositeLiterals_BarrierCases(t *testing.T) { + t.Run("empty literal", func(t *testing.T) { + probeLiterals("package p\nfunc f() {\n\tx := []int{}\n\tALPHA(x)\n}\n"). + wantMasked(t, "ALPHA", false) + }) + + t.Run("empty struct literal", func(t *testing.T) { + probeLiterals("package p\nfunc f() {\n\tx := struct{}{}\n\tALPHA(x)\n}\n"). + wantMasked(t, "ALPHA", false) + }) + + t.Run("unterminated literal does not mask trailing code", func(t *testing.T) { + // A truncated file: the literal never closes, so nothing balances it. + // The mask must decline rather than run to the end of the stream. + p := probeLiterals("package p\nfunc f() {\n\tx := []int{1, 2\n") + for i, m := range p.mask { + if m { + t.Fatalf("unbalanced literal masked token %d; expected no mask at all", i) + } + } + }) + + t.Run("literal at the very start of the stream", func(t *testing.T) { + // No preceding token, so the "is this an index expression" check has + // nothing to look back at. + probeLiterals("[]int{ALPHA}").wantMasked(t, "ALPHA", true) + }) + + t.Run("empty token stream", func(t *testing.T) { + if got := findDataLiteralSpans(nil, goL()); len(got) != 0 { + t.Errorf("spans for empty stream = %d, want 0", len(got)) + } + }) + + t.Run("nil language is a no-op", func(t *testing.T) { + // Scanning tokenizes with a resolved language, but markFunctionBodies + // treats a nil language as "no syntax knowledge, mark everything"; + // literal discovery must be equally defensive rather than guessing. + tokens := TokenizeFile("package p\nfunc f() {\n\tx := []int{1}\n}\n", goL()) + if got := findDataLiteralSpans(tokens, nil); len(got) != 0 { + t.Errorf("nil language produced %d spans; discovery requires known syntax", len(got)) + } + }) + + t.Run("nil language lexes instead of panicking", func(t *testing.T) { + // markFunctionBodies documents nil as "no syntax knowledge, mark + // everything", but TokenizeFile used to dereference it and panic on the + // keyword lookup. The scanner drops unknown-language files before + // reaching here, so this is about the contract being coherent. + src := "package p\nfunc f() {\n\tx := 1\n}\n" + tokens := TokenizeFile(src, nil) + if len(tokens) == 0 { + t.Fatal("nil language produced no tokens") + } + tf := BuildTokenizedFile("unknown.xyz", src, nil) + if len(tf.Tokens) != len(tokens) { + t.Errorf("BuildTokenizedFile produced %d tokens, TokenizeFile %d", len(tf.Tokens), len(tokens)) + } + }) + + t.Run("opted-out language is a no-op", func(t *testing.T) { + // Java is deliberately excluded: `[` there is an index or an array type, + // and its array initializers use `{`, which is also a block. + src := "class C {\n void f() {\n int[] xs = {1, 2, 3};\n if (xs[0] > 0) { g(); }\n }\n}\n" + lang := LangForName("java") + tokens := TokenizeFile(src, lang) + if got := findDataLiteralSpans(tokens, lang); len(got) != 0 { + t.Errorf("java source produced %d spans; the language is opted out", len(got)) + } + }) +} + +// A table whose fields hold real logic must keep that logic visible: only the +// surrounding data rows are excluded. +func TestMaskDataLiterals_FuncLiteralsInsideTablesStayVisible(t *testing.T) { + src := `package p + +func f() { + tests := []struct { + name string + setup func() + }{ + { + name: ROWNAME, + setup: func() { + ALPHA() + BRAVO() + }, + }, + } + _ = tests +} +` + p := probeDetectionMask(src) + // The data column is excluded... + p.wantMasked(t, "ROWNAME", true) + // ...but the logic carried in the func-literal field is not. + p.wantMasked(t, "ALPHA", false) + p.wantMasked(t, "BRAVO", false) +} + +// A `func` inside a table is either a field's TYPE (data — stays masked) or a +// literal supplying a value (logic — its body re-opens). Getting this backwards +// either hides real duplication or lets a whole table back through, so each +// signature shape is pinned down. +func TestMaskDataLiterals_FuncTypeVersusFuncLiteral(t *testing.T) { + tests := []struct { + name string + src string + // probe sits inside the func region; wantMasked says whether that + // region is data (true) or logic that must stay visible (false). + probe string + wantMasked bool + }{ + { + name: "field type with no body stays data", + src: "package p\nfunc f() {\n\tx := []struct{ setup func(PROBE int) }{{}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: true, + }, + { + name: "field type with a result stays data", + src: "package p\nfunc f() {\n\tx := []struct{ run func() PROBE }{{}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: true, + }, + { + name: "literal with no result re-opens", + src: "package p\nfunc f() {\n\tx := []Case{{fn: func() { PROBE() }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: false, + }, + { + name: "literal with a single result re-opens", + src: "package p\nfunc f() {\n\tx := []Case{{fn: func() error { PROBE(); return nil }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: false, + }, + { + name: "literal with parenthesized results re-opens", + src: "package p\nfunc f() {\n\tx := []Case{{fn: func() (int, error) { PROBE(); return 0, nil }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: false, + }, + { + name: "literal taking arguments re-opens", + src: "package p\nfunc f() {\n\tx := []Case{{fn: func(t *testing.T) { PROBE(t) }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: false, + }, + { + name: "nested literal inside a re-opened body stays visible", + src: "package p\nfunc f() {\n\tx := []Case{{fn: func() { g(func() { PROBE() }) }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: false, + }, + { + name: "the row data around a re-opened body stays masked", + src: "package p\nfunc f() {\n\tx := []Case{{name: PROBE, fn: func() { g() }}}\n\t_ = x\n}\n", + probe: "PROBE", + wantMasked: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + probeDetectionMask(tc.src).wantMasked(t, tc.probe, tc.wantMasked) + }) + } +} + +// End-to-end at the detector level: duplicated logic carried in table func +// fields is still real duplication and must be reported. +func TestDetect_DuplicatedLogicInTableFuncFieldsStillReported(t *testing.T) { + body := ` + srv := newServer(t) + defer srv.Close() + req := build(t, srv.URL) + resp, err := srv.Client().Do(req) + if err != nil { + t.Fatalf("do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != 200 { + t.Fatalf("status %d", resp.StatusCode) + } + checkHeaders(t, resp) + checkTrailers(t, resp) +` + src := `package p + +import "testing" + +func TestCases(t *testing.T) { + cases := []struct { + name string + run func(t *testing.T) + }{ + { + name: "alpha", + run: func(t *testing.T) {` + body + `}, + }, + { + name: "bravo", + run: func(t *testing.T) {` + body + `}, + }, + } + _ = cases +} +` + clones := Detect([]TokenizedFile{makeFile("cases_test.go", src)}, 50, 1.0) + if len(clones) == 0 { + t.Fatal("copy-pasted logic inside table func fields must still be reported") + } + assertNoOverlappingInstances(t, clones) +} + +// Data literals must stay INSIDE function scope. Excluding their tokens from +// InFunc was the tempting fix and it is wrong: a small inline literal sitting in +// the middle of a duplicated block would split that block in two and lose the +// finding. The literal is recorded as a span instead, and the detector uses the +// span to judge whole clone groups. +func TestBuildTokenizedFile_LiteralTokensStayInFunctionScope(t *testing.T) { + src := "package p\nfunc f() {\n\tx := []int{ALPHA, BRAVO}\n\tCHARLIE(x)\n}\n" + tf := BuildTokenizedFile("a.go", src, goL()) + + for _, probe := range []string{"ALPHA", "CHARLIE"} { + found := false + for i, tok := range tf.Tokens { + s := tok.Text + if s == "" { + s = tok.OrigText + } + if s != probe { + continue + } + found = true + if !tf.InFunc[i] { + t.Errorf("token %q left function scope; literals must stay visible to detection", probe) + } + } + if !found { + t.Fatalf("probe token %q not found in fixture", probe) + } + } + + if len(tf.DataLiterals) != 1 { + t.Fatalf("recorded %d data literal spans, want 1", len(tf.DataLiterals)) + } +} + +// The regression that drove the span-based design: a short inline literal in the +// middle of two otherwise-identical blocks must not break them apart. +func TestDetect_InlineLiteralDoesNotSplitDuplicatedBlock(t *testing.T) { + body := ` + dir := t.TempDir() + a := makeThing(t, dir, "a.go", "package main") + b := makeThing(t, dir, "b.go", "package main") + results := Combine([]Thing{a, b}, 10, 0.5) + if len(results) == 0 { + t.Fatal("expected results") + } + for _, r := range results { + if r.Score < 0 { + t.Errorf("negative score: %v", r) + } + } +` + src := "package p\n\nimport \"testing\"\n\nfunc TestAlpha(t *testing.T) {" + body + "}\n\nfunc TestBravo(t *testing.T) {" + body + "}\n" + + clones := Detect([]TokenizedFile{makeFile("split_test.go", src)}, 50, 1.0) + if len(clones) == 0 { + t.Fatal("duplicated block containing an inline []Thing{a, b} literal must still be reported") + } + assertNoOverlappingInstances(t, clones) +} diff --git a/services/data_literal_langs_test.go b/services/data_literal_langs_test.go new file mode 100644 index 0000000..a3f98aa --- /dev/null +++ b/services/data_literal_langs_test.go @@ -0,0 +1,303 @@ +package services + +import "testing" + +// Table-driven tests are idiomatic well beyond Go, so the data-literal rule +// covers every language whose collection literals can be identified from tokens +// alone. These tests pin down, per language, both directions: the table is +// recognised as data, and the code around it is not. + +// Each fixture wraps a probe token in a collection literal, and another probe in +// ordinary logic, so one source exercises both classifications. +func TestDataLiteralSpans_PerLanguage(t *testing.T) { + tests := []struct { + name string + lang string + src string + // data must be classified as table data; logic must not. + data string + logic string + }{ + { + name: "python list of dicts", + lang: "python", + src: "def test():\n cases = [\n {'name': DATA, 'want': 1},\n {'name': 'b', 'want': 2},\n ]\n for c in cases:\n LOGIC(c)\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "python dict literal", + lang: "python", + src: "def test():\n m = {'a': DATA, 'b': 2}\n LOGIC(m)\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "javascript array of objects", + lang: "javascript", + src: "function test() {\n const cases = [\n {name: DATA, want: 1},\n {name: 'b', want: 2},\n ];\n cases.forEach(c => LOGIC(c));\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "typescript array of objects", + lang: "typescript", + src: "function test() {\n const cases = [\n {name: DATA, want: 1},\n {name: 'b', want: 2},\n ];\n for (const c of cases) { LOGIC(c); }\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "ruby array of hashes", + lang: "ruby", + src: "def test\n cases = [\n {name: DATA, want: 1},\n {name: 'b', want: 2},\n ]\n cases.each { |c| LOGIC(c) }\nend\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "rust vec macro", + lang: "rust", + src: "fn test() {\n let cases = vec![\n (DATA, 1),\n (\"b\", 2),\n ];\n for c in cases {\n LOGIC(c);\n }\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "php array literal", + lang: "php", + src: "function test() {\n $cases = [\n ['name' => DATA, 'want' => 1],\n ['name' => 'b', 'want' => 2],\n ];\n foreach ($cases as $c) {\n LOGIC($c);\n }\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "elixir list of tuples", + lang: "elixir", + src: "def test do\n cases = [\n {DATA, 1},\n {:b, 2}\n ]\n Enum.each(cases, fn c -> LOGIC(c) end)\nend\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "swift array of tuples", + lang: "swift", + src: "func test() {\n let cases = [\n (name: DATA, want: 1),\n (name: \"b\", want: 2),\n ]\n for c in cases {\n LOGIC(c)\n }\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "dart list of maps", + lang: "dart", + src: "void test() {\n var cases = [\n {'name': DATA, 'want': 1},\n {'name': 'b', 'want': 2},\n ];\n for (var c in cases) {\n LOGIC(c);\n }\n}\n", + data: "DATA", + logic: "LOGIC", + }, + { + name: "lua table constructor", + lang: "lua", + src: "function test()\n local cases = {\n {name = DATA, want = 1},\n {name = \"b\", want = 2},\n }\n for _, c in ipairs(cases) do\n LOGIC(c)\n end\nend\n", + data: "DATA", + logic: "LOGIC", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lang := LangForName(tc.lang) + if lang == nil { + t.Fatalf("language %q is not supported by the scanner", tc.lang) + } + p := probeDetectionMaskLang(tc.src, lang) + p.wantMasked(t, tc.data, true) + p.wantMasked(t, tc.logic, false) + }) + } +} + +// Indexing uses the same bracket a list literal does. Mistaking `m[k]` for a +// literal would let the rule silence real code, so every bracket language is +// checked against its own indexing syntax. +func TestDataLiteralSpans_IndexingIsNotALiteral(t *testing.T) { + tests := []struct { + name string + lang string + src string + }{ + { + name: "python subscript", + lang: "python", + src: "def f(m, k):\n if m[k]:\n LOGIC()\n", + }, + { + name: "javascript index", + lang: "javascript", + src: "function f(m, k) {\n if (m[k]) {\n LOGIC();\n }\n}\n", + }, + { + name: "javascript index on call result", + lang: "javascript", + src: "function f() {\n if (rows()[0]) {\n LOGIC();\n }\n}\n", + }, + { + name: "ruby index", + lang: "ruby", + src: "def f(m, k)\n if m[k]\n LOGIC()\n end\nend\n", + }, + { + name: "rust index", + lang: "rust", + src: "fn f(m: &[i32]) {\n if m[0] > 0 {\n LOGIC();\n }\n}\n", + }, + { + name: "php index", + lang: "php", + src: "function f($m, $k) {\n if ($m[$k]) {\n LOGIC();\n }\n}\n", + }, + { + name: "dart index", + lang: "dart", + src: "void f(m, k) {\n if (m[k]) {\n LOGIC();\n }\n}\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lang := LangForName(tc.lang) + if lang == nil { + t.Fatalf("language %q is not supported by the scanner", tc.lang) + } + probeDetectionMaskLang(tc.src, lang).wantMasked(t, "LOGIC", false) + }) + } +} + +// Logic written inside a table — a callback column — is still logic. The +// exemption has to survive each language's own closure syntax. +func TestDataLiteralSpans_CallbacksInsideTablesStayVisible(t *testing.T) { + tests := []struct { + name string + lang string + src string + }{ + { + name: "javascript arrow function in a table row", + lang: "javascript", + src: "function test() {\n const cases = [\n {name: 'a', run: () => { LOGIC(); }},\n {name: 'b', run: () => { other(); }},\n ];\n}\n", + }, + { + name: "javascript function expression in a table row", + lang: "javascript", + src: "function test() {\n const cases = [\n {name: 'a', run: function () { LOGIC(); }},\n ];\n}\n", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lang := LangForName(tc.lang) + if lang == nil { + t.Fatalf("language %q is not supported by the scanner", tc.lang) + } + probeDetectionMaskLang(tc.src, lang).wantMasked(t, "LOGIC", false) + }) + } +} + +// End-to-end per language: the table is not reported, and genuine duplication in +// the same file still is. Without the second half, "no clones" could just mean +// detection broke for that language. +func TestDetect_TableDrivenTestsAcrossLanguages(t *testing.T) { + tests := []struct { + name string + lang string + file string + table string + dup string + }{ + { + name: "python", + lang: "python", + file: "test_things.py", + table: `def test_presets(): + cases = [ + {'name': 'openai', 'token': 'openai_AAAA0', 'want': 'openai_***'}, + {'name': 'anthropic', 'token': 'anthropic_AAAA1', 'want': 'anthropic_***'}, + {'name': 'github', 'token': 'github_AAAA2', 'want': 'github_***'}, + {'name': 'awskey', 'token': 'awskey_AAAA3', 'want': 'awskey_***'}, + {'name': 'bearer', 'token': 'bearer_AAAA4', 'want': 'bearer_***'}, + {'name': 'stripe', 'token': 'stripe_AAAA5', 'want': 'stripe_***'}, + {'name': 'gcp', 'token': 'gcp_AAAA6', 'want': 'gcp_***'}, + {'name': 'azure', 'token': 'azure_AAAA7', 'want': 'azure_***'}, + ] + for c in cases: + assert redact(c['token']) == c['want'] +`, + dup: ` server = start_server() + client = server.client() + request = build_request(server.url) + response = client.send(request) + assert response.status == 200 + body = response.read_all() + assert body is not None + server.close() + log_result(body) +`, + }, + { + name: "javascript", + lang: "javascript", + file: "things.test.js", + table: `function testPresets() { + const cases = [ + {name: 'openai', token: 'openai_AAAA0', want: 'openai_***'}, + {name: 'anthropic', token: 'anthropic_AAAA1', want: 'anthropic_***'}, + {name: 'github', token: 'github_AAAA2', want: 'github_***'}, + {name: 'awskey', token: 'awskey_AAAA3', want: 'awskey_***'}, + {name: 'bearer', token: 'bearer_AAAA4', want: 'bearer_***'}, + {name: 'stripe', token: 'stripe_AAAA5', want: 'stripe_***'}, + {name: 'gcp', token: 'gcp_AAAA6', want: 'gcp_***'}, + {name: 'azure', token: 'azure_AAAA7', want: 'azure_***'}, + ]; + cases.forEach(c => expect(redact(c.token)).toBe(c.want)); +} +`, + dup: ` const server = startServer(); + const client = server.client(); + const request = buildRequest(server.url); + const response = client.send(request); + expect(response.status).toBe(200); + const body = response.readAll(); + expect(body).toBeDefined(); + server.close(); + logResult(body); +`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lang := LangForName(tc.lang) + + // The table alone must produce nothing. + tableOnly := BuildTokenizedFile(tc.file, tc.table, lang) + if clones := Detect([]TokenizedFile{tableOnly}, 50, 0.70); len(clones) != 0 { + t.Errorf("%s table-driven test reported %d clone(s); want a clean scan", tc.lang, len(clones)) + for _, c := range clones { + for _, in := range c.Instances { + t.Logf(" %s:%d-%d", in.File, in.StartLine, in.EndLine) + } + } + } + + // Genuine duplication in the same language must still be reported. + var withDup string + switch tc.lang { + case "python": + withDup = tc.table + "\ndef test_alpha():\n" + tc.dup + "\ndef test_bravo():\n" + tc.dup + case "javascript": + withDup = tc.table + "\nfunction testAlpha() {\n" + tc.dup + "}\n\nfunction testBravo() {\n" + tc.dup + "}\n" + } + dupFile := BuildTokenizedFile(tc.file, withDup, lang) + clones := Detect([]TokenizedFile{dupFile}, 50, 0.70) + if len(clones) == 0 { + t.Fatalf("%s: copy-pasted logic alongside a table must still be reported", tc.lang) + } + assertNoOverlappingInstances(t, clones) + }) + } +} diff --git a/services/detector.go b/services/detector.go index 052a349..ad5ebe8 100644 --- a/services/detector.go +++ b/services/detector.go @@ -22,27 +22,121 @@ type TokenizedFile struct { InFunc []bool // per-token: true if inside a function/method body Ignored []bool // per-token: true if in a dupehound:ignore annotated block Funcs []FuncSpan // function body spans with best-effort names, for clone attribution + // DataLiterals are the token spans of composite data literals written inside + // function bodies — `[]T{…}`, `map[K]V{…}`, and the `[]struct{…}{…}` of a + // table-driven test. They are not excluded from detection; they are used to + // recognise a clone whose every instance sits inside ONE such literal, which + // describes a data table rather than duplicated logic. + DataLiterals []TokenSpan + // FuncLiterals are the bodies of function literals. A body nested inside a + // DataLiterals span is logic that merely lives in a table, so it is exempt + // from the rule above. + FuncLiterals []TokenSpan } // BuildTokenizedFileWithIgnore tokenizes a source file, applying inline suppression // markers from both the source (dupehound:ignore comments) and the ignore rules. func BuildTokenizedFileWithIgnore(path, content string, lang *domain.Language, rules []IgnoreRule) TokenizedFile { - tokens := TokenizeFileWithIgnore(content, lang) + return buildTokenizedFile(path, TokenizeFileWithIgnore(content, lang), lang) +} + +// buildTokenizedFile assembles a TokenizedFile from an already-lexed token +// stream. It is the single place the per-token masks and spans are derived, so +// the ignore-aware and plain constructors cannot drift apart. +func buildTokenizedFile(path string, tokens []Token, lang *domain.Language) TokenizedFile { inFunc, funcs := markFunctionBodies(tokens, lang) ignored := markIgnoredBlocks(tokens, inFunc) - // Zero out InFunc for ignored tokens so detection skips them. + + // Detection only ever looks at InFunc, so clearing it is how a suppression + // takes effect. for i, ign := range ignored { if ign { inFunc[i] = false } } + + dataLiterals := findDataLiteralSpans(tokens, lang) + return TokenizedFile{ - Path: path, - Tokens: tokens, - InFunc: inFunc, - Ignored: ignored, - Funcs: funcs, + Path: path, + Tokens: tokens, + InFunc: inFunc, + Ignored: ignored, + Funcs: funcs, + DataLiterals: dataLiterals, + FuncLiterals: nestedFuncLiterals(findFuncLiteralBodies(tokens, lang, funcs), dataLiterals), + } +} + +// nestedFuncLiterals keeps only the function bodies that sit inside a data +// literal — the `run func(t *testing.T)` column of a table. +// +// The narrowing is what makes the exemption safe outside Go, where the spans +// come from markFunctionBodies and therefore include the enclosing test function +// itself. Left unfiltered, that outer span would cover every table in the file +// and exempt all of them, disabling the rule entirely. +func nestedFuncLiterals(funcSpans, dataLiterals []TokenSpan) []TokenSpan { + if len(funcSpans) == 0 || len(dataLiterals) == 0 { + return nil + } + nested := make([]TokenSpan, 0, len(funcSpans)) + for _, fs := range funcSpans { + for _, dl := range dataLiterals { + if fs.Start >= dl.Start && fs.End <= dl.End { + nested = append(nested, fs) + break + } + } + } + return nested +} + +// dataLiteralAt returns the index of the data literal wholly containing the +// token window [pos, pos+length), or -1 when the window is not pure table data. +// +// Touching a function literal at all disqualifies the window. Logic written in +// a table — a `run func(t *testing.T)` column — is still logic, and a window +// that straddles a func body and the rows around it is not something the "this +// is just a data table" rule should ever silence. +func (tf TokenizedFile) dataLiteralAt(pos, length int) int { + if len(tf.DataLiterals) == 0 || length <= 0 { + return -1 + } + end := pos + length - 1 + for _, fl := range tf.FuncLiterals { + if pos <= fl.End && fl.Start <= end { + return -1 + } + } + for i, s := range tf.DataLiterals { + if pos >= s.Start && end <= s.End { + return i + } + } + return -1 +} + +// allInsideOneDataLiteral reports whether every instance of a clone group lands +// in the same composite data literal. The rows of a table are structurally +// identical by construction, so a "clone" confined to one literal describes a +// single data table rather than duplicated logic (issue #31). Instances spread +// across two literals, two functions, or two files are left alone — repeating +// the same table twice is duplication worth reporting. +func allInsideOneDataLiteral(files []TokenizedFile, starts []globalPos, totalTokens int) bool { + first := starts[0] + litIdx := files[first.FileIdx].dataLiteralAt(first.Pos, totalTokens) + if litIdx < 0 { + return false + } + for _, s := range starts[1:] { + if s.FileIdx != first.FileIdx { + return false + } + if files[s.FileIdx].dataLiteralAt(s.Pos, totalTokens) != litIdx { + return false + } } + return true } // globalPos identifies a window position across all files. @@ -300,12 +394,30 @@ func detectExact(files []TokenizedFile, minTokens int, inScopeFiles []bool) []do totalTokens := minTokens + extLen + // Every seed position is marked covered — including ones dropped just + // below — so the same region is never reseeded as a second group. for _, s := range starts { for k := 0; k <= extLen; k++ { covered[globalPos{s.FileIdx, s.Pos + k}] = true } } + // Seeds within one file were spaced minTokens apart, but greedy + // extension grew every block to totalTokens, so neighbouring instances + // can now overlap. Overlapping windows are one stretch of text counted + // several times rather than distinct duplicates, so keep only instances + // that stay clear of the previous one. + starts = dropOverlappingStarts(starts, totalTokens) + if len(starts) < 2 { + continue + } + + // A group confined to one composite data literal is a table matching + // its own rows, not duplicated logic. + if allInsideOneDataLiteral(files, starts, totalTokens) { + continue + } + instances := buildInstances(files, starts, totalTokens) lineCount := 0 @@ -336,6 +448,24 @@ func detectExact(files []TokenizedFile, minTokens int, inScopeFiles []bool) []do return clones } +// dropOverlappingStarts keeps only the instances of one clone group that do not +// overlap an instance already kept. starts must be sorted by (FileIdx, Pos); +// instances in different files can never overlap, so the check is per-file. +// The first instance is always kept, which keeps the group's fingerprint and +// classification (both derived from starts[0]) stable. +func dropOverlappingStarts(starts []globalPos, totalTokens int) []globalPos { + kept := make([]globalPos, 0, len(starts)) + lastFile, lastEnd := -1, 0 + for _, s := range starts { + if s.FileIdx == lastFile && s.Pos < lastEnd { + continue + } + kept = append(kept, s) + lastFile, lastEnd = s.FileIdx, s.Pos+totalTokens + } + return kept +} + // detectFuzzy finds type-3 near-miss clones using mini-window Jaccard similarity. // It skips blocks already covered by exact clones. // Requires minTokens >= 10 to produce meaningful mini-windows; returns nil otherwise. @@ -387,6 +517,10 @@ func detectFuzzy(files []TokenizedFile, minTokens int, threshold float64, maxBuc miniSet []uint64 // sorted, deduped mini-window hashes startLine int endLine int + // dataLiteral is the index of the composite data literal wholly + // containing this block, or -1. Resolved once here rather than per + // candidate pair, since the pair loop runs millions of times. + dataLiteral int } var blocks []blockInfo @@ -434,10 +568,11 @@ func detectFuzzy(files []TokenizedFile, minTokens int, threshold float64, maxBuc idx := len(blocks) blocks = append(blocks, blockInfo{ - key: blockKey{fi, pos}, - miniSet: miniSet, - startLine: startLine, - endLine: endLine, + key: blockKey{fi, pos}, + miniSet: miniSet, + startLine: startLine, + endLine: endLine, + dataLiteral: tf.dataLiteralAt(pos, minTokens), }) for _, h := range miniSet { @@ -532,7 +667,8 @@ func detectFuzzy(files []TokenizedFile, minTokens int, threshold float64, maxBuc !fileInScope(inScopeFiles, bb.key.fileIdx) { continue } - // Skip same-file overlapping blocks. + // Skip same-file overlapping blocks, and blocks that are two + // windows over the rows of a single data table (issue #31). if ba.key.fileIdx == bb.key.fileIdx { dist := ba.key.pos - bb.key.pos if dist < 0 { @@ -541,6 +677,9 @@ func detectFuzzy(files []TokenizedFile, minTokens int, threshold float64, maxBuc if dist < minTokens { continue } + if ba.dataLiteral >= 0 && ba.dataLiteral == bb.dataLiteral { + continue + } } evaluated++ diff --git a/services/overlap_test.go b/services/overlap_test.go new file mode 100644 index 0000000..90930cc --- /dev/null +++ b/services/overlap_test.go @@ -0,0 +1,150 @@ +package services + +import "testing" + +// Tests for the instance-overlap guard (issue #31). Seed windows inside one +// file are spaced minTokens apart, but greedy extension grows every block to +// totalTokens — so without this guard a clone group reports the same stretch of +// text several times as if it were several duplicates. + +func TestDropOverlappingStarts(t *testing.T) { + tests := []struct { + name string + starts []globalPos + totalTokens int + want []globalPos + }{ + { + name: "already disjoint instances are all kept", + starts: []globalPos{{0, 0}, {0, 100}, {0, 200}}, + totalTokens: 50, + want: []globalPos{{0, 0}, {0, 100}, {0, 200}}, + }, + { + name: "instances exactly touching are disjoint", + starts: []globalPos{{0, 0}, {0, 50}, {0, 100}}, + totalTokens: 50, + want: []globalPos{{0, 0}, {0, 50}, {0, 100}}, + }, + { + name: "one token of overlap drops the instance", + starts: []globalPos{{0, 0}, {0, 49}}, + totalTokens: 50, + want: []globalPos{{0, 0}}, + }, + { + name: "a run of overlapping instances collapses to the survivors", + starts: []globalPos{{0, 0}, {0, 30}, {0, 60}, {0, 90}}, + totalTokens: 50, + want: []globalPos{{0, 0}, {0, 60}}, + }, + { + name: "instances in different files never overlap", + starts: []globalPos{{0, 0}, {1, 10}, {2, 20}}, + totalTokens: 50, + want: []globalPos{{0, 0}, {1, 10}, {2, 20}}, + }, + { + name: "overlap is judged per file", + starts: []globalPos{{0, 0}, {0, 10}, {1, 0}, {1, 10}}, + totalTokens: 50, + want: []globalPos{{0, 0}, {1, 0}}, + }, + { + name: "the first instance is always kept", + starts: []globalPos{{0, 7}, {0, 8}, {0, 9}}, + totalTokens: 50, + want: []globalPos{{0, 7}}, + }, + { + name: "single instance passes through", + starts: []globalPos{{0, 0}}, + totalTokens: 50, + want: []globalPos{{0, 0}}, + }, + { + name: "empty input yields empty output", + starts: []globalPos{}, + totalTokens: 50, + want: []globalPos{}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := dropOverlappingStarts(tc.starts, tc.totalTokens) + if len(got) != len(tc.want) { + t.Fatalf("kept %d instances %v, want %d %v", len(got), got, len(tc.want), tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("instance %d = %+v, want %+v", i, got[i], tc.want[i]) + } + } + }) + } +} + +// The guard must not weaken genuine detection: a file with several truly +// separate copies of a block still reports all of them. +func TestDetect_RepeatedDisjointBlocksAllReported(t *testing.T) { + block := ` + conn := open(addr) + if conn == nil { + panic("no conn") + } + rows := conn.Query(stmt) + for rows.Next() { + var id int + var name string + rows.Scan(&id, &name) + emit(id, name) + } + rows.Close() + conn.Close() +` + // Three separate functions, each holding one copy — unambiguously disjoint. + src := "package main\n" + + "func alpha() {" + block + "}\n" + + "func bravo() {" + block + "}\n" + + "func charlie() {" + block + "}\n" + + clones := Detect([]TokenizedFile{makeFile("a.go", src)}, 50, 1.0) + if len(clones) == 0 { + t.Fatal("three copies of the same block must be reported") + } + assertNoOverlappingInstances(t, clones) + + best := 0 + for _, c := range clones { + if len(c.Instances) > best { + best = len(c.Instances) + } + } + if best < 3 { + t.Errorf("largest clone group has %d instances, want 3 (one per copy)", best) + } +} + +// Cross-file groups have one instance per file and so can never overlap; the +// guard must leave them untouched. +func TestDetect_CrossFileInstancesUnaffectedByOverlapGuard(t *testing.T) { + block := "func work() {\n\tx := compute(a, b)\n\ty := refine(x)\n\tz := combine(x, y)\n\treport(z)\n\tflush(z)\n\tclose(z)\n}\n" + files := []TokenizedFile{ + makeFile("a.go", "package main\n"+block), + makeFile("b.go", "package main\n"+block), + makeFile("c.go", "package main\n"+block), + } + + clones := Detect(files, 20, 1.0) + if len(clones) == 0 { + t.Fatal("identical block across three files must be reported") + } + assertNoOverlappingInstances(t, clones) + + for _, c := range clones { + if len(c.Instances) != 3 { + t.Errorf("clone %s has %d instances, want 3 (one per file)", c.Hash, len(c.Instances)) + } + } +} diff --git a/services/pyfuncboundary_test.go b/services/pyfuncboundary_test.go new file mode 100644 index 0000000..ac7ad68 --- /dev/null +++ b/services/pyfuncboundary_test.go @@ -0,0 +1,123 @@ +package services + +import "testing" + +// Regression tests for Python function-boundary detection, found while adding +// multi-language coverage for issue #31. +// +// markPythonFunctions ends a body when it meets the next blank-line-separated +// `def`, then used to resume the outer scan AFTER that keyword — stepping over +// it. Every definition following the first was therefore never marked as a +// function body, and since detection only looks inside function bodies, all +// duplication in those functions was invisible. + +// funcNames returns the names of the function spans found in src. +func pyFuncNames(t *testing.T, src string) []string { + t.Helper() + lang := LangForName("python") + tokens := TokenizeFile(src, lang) + _, funcs := markFunctionBodies(tokens, lang) + names := make([]string, 0, len(funcs)) + for _, f := range funcs { + names = append(names, f.Name) + } + return names +} + +func TestMarkPythonFunctions_FindsEveryDefinition(t *testing.T) { + tests := []struct { + name string + src string + want []string + }{ + { + name: "two blank-line separated defs", + src: "def alpha():\n a = 1\n return a\n\ndef bravo():\n b = 2\n return b\n", + want: []string{"alpha", "bravo"}, + }, + { + name: "four defs in a row", + src: "def alpha():\n return 1\n\ndef bravo():\n return 2\n\n" + + "def charlie():\n return 3\n\ndef delta():\n return 4\n", + want: []string{"alpha", "bravo", "charlie", "delta"}, + }, + { + name: "async def between sync defs", + src: "def alpha():\n return 1\n\nasync def bravo():\n return 2\n\ndef charlie():\n return 3\n", + want: []string{"alpha", "bravo", "charlie"}, + }, + { + name: "def following a class", + src: "class Thing:\n pass\n\ndef alpha():\n return 1\n\ndef bravo():\n return 2\n", + want: []string{"alpha", "bravo"}, + }, + { + name: "single def is unaffected", + src: "def alpha():\n a = 1\n return a\n", + want: []string{"alpha"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := pyFuncNames(t, tc.src) + if len(got) != len(tc.want) { + t.Fatalf("found %d functions %v, want %d %v", len(got), got, len(tc.want), tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("function %d = %q, want %q", i, got[i], tc.want[i]) + } + } + }) + } +} + +// The behaviour that actually matters: duplication in the second function is +// reported. Before the fix its body was never in scope, so this found nothing. +func TestDetect_PythonDuplicationInLaterFunctions(t *testing.T) { + body := ` server = start_server() + client = server.client() + request = build_request(server.url) + response = client.send(request) + assert response.status == 200 + body = response.read_all() + assert body is not None + server.close() + log_result(body) +` + src := "def test_alpha():\n" + body + "\ndef test_bravo():\n" + body + + tf := BuildTokenizedFile("things_test.py", src, LangForName("python")) + clones := Detect([]TokenizedFile{tf}, 50, 1.0) + if len(clones) == 0 { + t.Fatal("copy-pasted body in the second Python function must be reported") + } + assertNoOverlappingInstances(t, clones) +} + +// A whole-file check: with several duplicated functions, every one of them is in +// scope, not just the first. +func TestMarkPythonFunctions_AllBodiesEnterScope(t *testing.T) { + src := "def alpha():\n x = compute()\n return x\n\n" + + "def bravo():\n y = compute()\n return y\n\n" + + "def charlie():\n z = compute()\n return z\n" + + lang := LangForName("python") + tf := BuildTokenizedFile("m.py", src, lang) + + // Each body contains a `compute` call; all three must be inside a function. + seen := 0 + for i, tok := range tf.Tokens { + if tok.OrigText != "compute" { + continue + } + seen++ + if !tf.InFunc[i] { + t.Errorf("compute call on line %d is not inside a function body", tok.Line) + } + } + if seen != 3 { + t.Fatalf("found %d compute calls, want 3", seen) + } +} diff --git a/services/table_literal_test.go b/services/table_literal_test.go new file mode 100644 index 0000000..e83b218 --- /dev/null +++ b/services/table_literal_test.go @@ -0,0 +1,278 @@ +package services + +import ( + "testing" + + "github.com/AxeForging/dupehound/domain" +) + +// Regression tests for issue #31: idiomatic Go table-driven tests were reported +// as type-2 clones because (a) composite data literals inside function bodies +// were treated as logic, and (b) same-file clone instances were allowed to +// overlap each other after greedy extension. +// +// The fixtures here are deliberately shaped like real code: gofmt-style +// multi-line rows, one-line rows, nested literals, and — for the negative +// cases — genuine copy-pasted logic that MUST still be reported. + +// issue31Repro is the reproduction from the issue report, verbatim in shape: +// a ten-row table-driven test with one-line rows. +const issue31Repro = `package repro + +import ( + "strings" + "testing" +) + +func TestRedact(t *testing.T) { + tests := []struct { + name string + input string + absent string + present string + }{ + {name: "openai preset case", input: "token openai_AAAA0\n", absent: "openai_AAAA", present: "openai_***"}, + {name: "anthropic preset case", input: "token anthropic_AAAA1\n", absent: "anthropic_AAAA", present: "anthropic_***"}, + {name: "github preset case", input: "token github_AAAA2\n", absent: "github_AAAA", present: "github_***"}, + {name: "awskey preset case", input: "token awskey_AAAA3\n", absent: "awskey_AAAA", present: "awskey_***"}, + {name: "bearer preset case", input: "token bearer_AAAA4\n", absent: "bearer_AAAA", present: "bearer_***"}, + {name: "stripe preset case", input: "token stripe_AAAA5\n", absent: "stripe_AAAA", present: "stripe_***"}, + {name: "gcp preset case", input: "token gcp_AAAA6\n", absent: "gcp_AAAA", present: "gcp_***"}, + {name: "azure preset case", input: "token azure_AAAA7\n", absent: "azure_AAAA", present: "azure_***"}, + {name: "npm preset case", input: "token npm_AAAA8\n", absent: "npm_AAAA", present: "npm_***"}, + {name: "slack preset case", input: "token slack_AAAA9\n", absent: "slack_AAAA", present: "slack_***"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if strings.Contains(tc.present, tc.absent) { + t.Errorf("%s", tc.name) + } + }) + } +} +` + +// gofmtStyleTable uses one field per line, which is what gofmt produces for +// larger literals. The issue notes the effect is stronger in this shape +// because each row spans many more tokens and lines. +const gofmtStyleTable = `package repro + +import "testing" + +func TestValidate(t *testing.T) { + cases := []struct { + name string + input string + want int + wantErr bool + }{ + { + name: "empty input is rejected", + input: "", + want: 0, + wantErr: true, + }, + { + name: "single element parses", + input: "a", + want: 1, + wantErr: false, + }, + { + name: "two elements parse", + input: "a,b", + want: 2, + wantErr: false, + }, + { + name: "three elements parse", + input: "a,b,c", + want: 3, + wantErr: false, + }, + { + name: "trailing comma is rejected", + input: "a,", + want: 0, + wantErr: true, + }, + { + name: "leading comma is rejected", + input: ",a", + want: 0, + wantErr: true, + }, + } + for _, tc := range cases { + got, err := Validate(tc.input) + if (err != nil) != tc.wantErr { + t.Fatalf("%s: err = %v", tc.name, err) + } + if got != tc.want { + t.Errorf("%s: got %d want %d", tc.name, got, tc.want) + } + } +} +` + +func TestDetect_Issue31_TableDrivenTestNotReported(t *testing.T) { + tests := []struct { + name string + src string + }{ + {name: "one-line rows (issue #31 repro)", src: issue31Repro}, + {name: "gofmt multi-line rows", src: gofmtStyleTable}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + files := []TokenizedFile{makeFile("table_test.go", tc.src)} + clones := Detect(files, 50, 0.70) + if len(clones) != 0 { + t.Errorf("table-driven test data reported as %d clone(s); a single data table is not duplicated logic", len(clones)) + for _, c := range clones { + t.Logf(" %s similarity=%.2f tokens=%d instances=%d", c.Type, c.Similarity, c.TokenCount, len(c.Instances)) + for _, in := range c.Instances { + t.Logf(" %s:%d-%d", in.File, in.StartLine, in.EndLine) + } + } + } + }) + } +} + +// A clone's instances describe distinct occurrences of the same block. If two +// instances of one group overlap, they are the same text counted twice — never +// a real duplicate. This invariant is asserted broadly across the suite via +// assertNoOverlappingInstances. +func TestDetect_Issue31_InstancesNeverOverlapWithinAFile(t *testing.T) { + // A long, highly periodic function body: greedy extension grows blocks + // past the seed window spacing, which is exactly how overlapping + // instances were produced. + src := "package main\n\nfunc run() {\n" + for i := 0; i < 40; i++ { + src += "\tstep(a, b)\n\tcheck(a, b)\n\temit(a, b)\n" + } + src += "}\n" + + files := []TokenizedFile{makeFile("periodic.go", src)} + clones := Detect(files, 50, 0.70) + assertNoOverlappingInstances(t, clones) +} + +// Negative case — the fix must not blind the detector to real duplication that +// happens to live in a test file next to a table. +func TestDetect_Issue31_RealDuplicationInTestFilesStillReported(t *testing.T) { + dup := ` + server := newServer(t) + defer server.Close() + client := server.Client() + req := buildRequest(t, server.URL) + resp, err := client.Do(req) + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer resp.Body.Close() + body := readAll(t, resp.Body) + if resp.StatusCode != 200 { + t.Fatalf("status = %d, body = %s", resp.StatusCode, body) + } +` + src := "package repro\n\nimport \"testing\"\n\nfunc TestAlpha(t *testing.T) {" + dup + "}\n\nfunc TestBeta(t *testing.T) {" + dup + "}\n" + + files := []TokenizedFile{makeFile("dup_test.go", src)} + clones := Detect(files, 50, 0.70) + if len(clones) == 0 { + t.Fatal("genuine copy-pasted test setup logic must still be reported") + } + assertNoOverlappingInstances(t, clones) +} + +// A table followed by genuinely duplicated logic in the same file: excluding +// the literal must not swallow the logic that follows it. +func TestDetect_Issue31_LogicAfterTableStillReported(t *testing.T) { + body := ` + cfg := load(path) + if cfg == nil { + t.Fatal("nil config") + } + conn, err := dial(cfg.Addr, cfg.Timeout, cfg.Retries) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.Ping(); err != nil { + t.Fatalf("ping: %v", err) + } +` + src := `package repro + +import "testing" + +func TestWithTable(t *testing.T) { + cases := []struct { + name string + want int + }{ + {name: "alpha", want: 1}, + {name: "bravo", want: 2}, + {name: "charlie", want: 3}, + {name: "delta", want: 4}, + {name: "echo", want: 5}, + {name: "foxtrot", want: 6}, + } + _ = cases +` + body + `} + +func TestPlain(t *testing.T) {` + body + `} +` + + files := []TokenizedFile{makeFile("mixed_test.go", src)} + clones := Detect(files, 50, 0.70) + if len(clones) == 0 { + t.Fatal("duplicated logic following a table literal must still be reported") + } + // And the reported clone must be the logic, not the table. + for _, c := range clones { + for _, in := range c.Instances { + for _, line := range in.Lines { + if containsAny(line, `{name: "alpha"`, `{name: "bravo"`) { + t.Errorf("clone instance %s:%d-%d covers table literal rows: %q", in.File, in.StartLine, in.EndLine, line) + } + } + } + } +} + +func containsAny(s string, subs ...string) bool { + for _, sub := range subs { + if len(sub) > 0 && len(s) >= len(sub) { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + } + } + return false +} + +// assertNoOverlappingInstances enforces the core invariant: within one clone +// group, no two instances in the same file may overlap. +func assertNoOverlappingInstances(t *testing.T, clones []domain.Clone) { + t.Helper() + for _, c := range clones { + for i := range c.Instances { + for j := i + 1; j < len(c.Instances); j++ { + a, b := c.Instances[i], c.Instances[j] + if a.File != b.File { + continue + } + if a.StartLine <= b.EndLine && b.StartLine <= a.EndLine { + t.Errorf("clone %s has overlapping instances in %s: %d-%d and %d-%d", + c.Hash, a.File, a.StartLine, a.EndLine, b.StartLine, b.EndLine) + } + } + } + } +} diff --git a/services/testhelpers_test.go b/services/testhelpers_test.go index ba9bdcd..a84733f 100644 --- a/services/testhelpers_test.go +++ b/services/testhelpers_test.go @@ -7,14 +7,7 @@ import ( // BuildTokenizedFile is a test helper that tokenizes a source file without // applying any inline-ignore rules. func BuildTokenizedFile(path, content string, lang *domain.Language) TokenizedFile { - tokens := TokenizeFile(content, lang) - inFunc, funcs := markFunctionBodies(tokens, lang) - return TokenizedFile{ - Path: path, - Tokens: tokens, - InFunc: inFunc, - Funcs: funcs, - } + return buildTokenizedFile(path, TokenizeFile(content, lang), lang) } // Detect is a test helper around DetectWithOptions for tests that only need diff --git a/services/tokenizer.go b/services/tokenizer.go index 459866a..07610e0 100644 --- a/services/tokenizer.go +++ b/services/tokenizer.go @@ -134,11 +134,391 @@ func markIgnoredBlocks(tokens []Token, inFunc []bool) []bool { return ignored } +// Data-literal syntax per language. A table-driven test is idiomatic well +// beyond Go, so the rule applies wherever a collection literal can be told from +// surrounding code by tokens alone. Each language is opted in only where that +// distinction is unambiguous; everything else is left out, because a wrong span +// silences findings while a missing one merely leaves noise. +// +// bracketDataLiteralLangs: `[` in expression position opens a list or array +// literal that closes at the matching `]` — Python lists of tuples, JS arrays of +// objects, Rust `vec![…]`, PHP and Elixir lists. An index expression (`m[k]`) +// uses the same bracket, so position is what separates them; see +// startsBracketLiteral. +// +// Java, C, C++, C# and Kotlin are absent on purpose: there `[` is virtually +// always an index or an array type, and their array initializers use `{`, which +// is indistinguishable from a block. Scala is absent because `[` is type +// parameters. +var bracketDataLiteralLangs = strSet( + "python", "javascript", "typescript", "ruby", "rust", "php", "elixir", "swift", "dart", +) + +// braceDataLiteralLangs: `{` is unambiguously a data constructor rather than a +// block, because these languages delimit blocks some other way — indentation in +// Python, `do`/`then` … `end` in Lua and Elixir. +// +// Ruby is absent: `{` there is either a hash literal or a block argument +// (`each { |x| … }`), and the two cannot be told apart by tokens alone. +var braceDataLiteralLangs = strSet("python", "lua", "elixir") + +// goDataLiteralLang gates Go's own form, which is neither of the above: the +// literal is introduced by a TYPE prefix (`[]T{…}`, `map[K]V{…}`) and its body +// starts at the `{` that follows the type. +const goDataLiteralLang = "go" + +// TokenSpan is an inclusive token index range. +type TokenSpan struct { + Start int + End int +} + +// findDataLiteralSpans locates composite DATA literals and returns their token +// spans: Go's `[]T{…}` / `map[K]V{…}` / `[]struct{…}{…}`, and the bracket- or +// brace-delimited collection literals of the other opted-in languages. +// +// Detection is restricted to function bodies so that data and config +// declarations never register as duplicated logic. That holds for anything +// declared at the top level, but a data table written INSIDE a function slipped +// through, and the rows of a table are structurally identical by construction — +// so idiomatic table-driven tests were reported as clones of themselves +// (issue #31). +// +// The spans are not excluded from detection. Excluding them would punch holes +// through surrounding code: a small inline `[]T{a, b}` in the middle of a +// genuinely duplicated block would split that block in two and lose the finding. +// The detector instead uses these spans to drop a clone whose every instance +// lies within ONE of them, which is the shape a data table produces and real +// duplication does not. +// +// Only outermost literals are returned; a literal nested in another is already +// covered by its parent's span. +func findDataLiteralSpans(tokens []Token, lang *domain.Language) []TokenSpan { + if lang == nil { + return nil + } + if lang.Name == goDataLiteralLang { + return goDataLiteralSpans(tokens) + } + brackets := bracketDataLiteralLangs[lang.Name] + braces := braceDataLiteralLangs[lang.Name] + if !brackets && !braces { + return nil + } + return delimitedDataLiteralSpans(tokens, brackets, braces) +} + +// goDataLiteralSpans finds Go composite literals, which are introduced by a type +// prefix rather than by the brace itself. +// +// The scan is deliberately conservative: only literals carrying an explicit +// slice, array, or map type prefix are reported. A bare `T{…}` struct literal is +// left out, since it is far more often a single value than a data table. +func goDataLiteralSpans(tokens []Token) []TokenSpan { + var spans []TokenSpan + for i := 0; i < len(tokens); i++ { + if !startsGoTypePrefix(tokens, i) { + continue + } + braceIdx := goTypeEnd(tokens, i) + if braceIdx < 0 || braceIdx >= len(tokens) || !isOpenBrace(tokens[braceIdx]) { + continue + } + end := matchDelimiter(tokens, braceIdx, "{", "}") + if end < 0 { + continue + } + spans = append(spans, TokenSpan{Start: i, End: end}) + i = end + } + return spans +} + +// delimitedDataLiteralSpans finds collection literals that their opening +// delimiter alone identifies: `[…]` in expression position, and — for languages +// that do not use braces for blocks — `{…}` anywhere. +func delimitedDataLiteralSpans(tokens []Token, brackets, braces bool) []TokenSpan { + var spans []TokenSpan + for i := 0; i < len(tokens); i++ { + t := tokens[i] + if t.Kind != TokOperator { + continue + } + + var end int + switch { + case brackets && t.Text == "[" && startsBracketLiteral(tokens, i): + end = matchDelimiter(tokens, i, "[", "]") + case braces && t.Text == "{": + end = matchDelimiter(tokens, i, "{", "}") + default: + continue + } + if end < 0 { + continue + } + + spans = append(spans, TokenSpan{Start: i, End: end}) + i = end + } + return spans +} + +// findFuncLiteralBodies returns the body spans of function literals, so the +// detector can tell logic that lives inside a table from the table's own rows. +// +// Go gets a dedicated scan because markFunctionBodies keys off the `func` +// keyword alone and cannot tell a struct field's func TYPE (`setup func()`) from +// an actual literal (`setup: func() { … }`). Other languages reuse the spans +// that pass already computed; callers narrow them to the ones nested inside a +// data literal. +func findFuncLiteralBodies(tokens []Token, lang *domain.Language, funcs []FuncSpan) []TokenSpan { + if lang == nil { + return nil + } + if lang.Name != goDataLiteralLang { + spans := make([]TokenSpan, 0, len(funcs)) + for _, f := range funcs { + if !isRealFunctionBody(tokens, f.Start) { + continue + } + spans = append(spans, TokenSpan{Start: f.Start, End: f.End}) + } + return spans + } + + var spans []TokenSpan + for i, t := range tokens { + if t.Kind != TokKeyword || t.Text != "func" { + continue + } + brace := goFuncLiteralBody(tokens, i) + if brace < 0 { + continue + } + end := matchDelimiter(tokens, brace, "{", "}") + if end < 0 { + continue + } + // The body only — the signature belongs to the row that declares it. + spans = append(spans, TokenSpan{Start: brace + 1, End: end - 1}) + } + return spans +} + +// isRealFunctionBody reports whether the span starting at bodyStart is genuinely +// a function body, given the token that opens it. +// +// Languages without a `func` keyword (Dart, Java, C#, C, C++) get their bodies +// from a brace-depth heuristic that cannot tell a function body from a `{…}` map +// or array literal sitting at the same depth. Left unchecked, that reads every +// row of a Dart table as a function body and exempts the whole table from the +// data-literal rule. +// +// A brace-delimited body always follows a parameter list or an arrow; a data +// literal follows an assignment, a comma, or an opening bracket. Bodies that are +// not brace-delimited at all (Python's `:`, Ruby and Lua's `def … end`) are +// accepted as-is, since no literal shares that shape. +func isRealFunctionBody(tokens []Token, bodyStart int) bool { + braceIdx := bodyStart - 1 + if braceIdx < 0 || braceIdx >= len(tokens) || !isOpenBrace(tokens[braceIdx]) { + return true + } + prev := braceIdx - 1 + if prev < 0 { + return false + } + if tokens[prev].Kind != TokOperator { + // A keyword or identifier before the brace — `try {`, `else {`, or a + // language whose bodies are named rather than parenthesized. + return true + } + switch tokens[prev].Text { + case ")", "=>", "->": + return true + } + return false +} + +// startsBracketLiteral reports whether the `[` at i opens a collection literal +// rather than an index or slice expression. Both spell the same bracket, so what +// precedes it decides: indexing always follows the thing being indexed — an +// identifier, a literal, or a closing bracket, brace, or paren. +func startsBracketLiteral(tokens []Token, i int) bool { + if i == 0 { + return true + } + prev := tokens[i-1] + switch prev.Kind { + case TokIdent, TokNumber, TokString: + return false + case TokOperator: + switch prev.Text { + case ")", "]", "}": + return false + } + } + return true +} + +// startsGoTypePrefix reports whether tokens[i] can open the slice, array, or map +// type of a composite literal rather than an index expression. From the bracket +// onward `m[k] {` in `if m[k] { … }` is indistinguishable from a type, so the +// token BEFORE the bracket is what settles it: an index expression always +// follows the thing being indexed — an identifier, a literal, or a closing +// bracket, brace, or paren. +func startsGoTypePrefix(tokens []Token, i int) bool { + t := tokens[i] + if t.Kind == TokKeyword && t.Text == "map" { + // `map` is reserved, so it can only begin a type. + return true + } + if t.Kind != TokOperator || t.Text != "[" { + return false + } + return startsBracketLiteral(tokens, i) +} + +// goTypeEnd parses the Go type expression starting at tokens[i] and returns the +// index just past it, or -1 for anything this scan does not model. Only the +// forms that can precede a composite literal are handled; func and channel +// types return -1, which leaves their literals unmasked. +func goTypeEnd(tokens []Token, i int) int { + n := len(tokens) + if i >= n { + return -1 + } + t := tokens[i] + switch { + case t.Kind == TokOperator && t.Text == "[": + // Slice `[]T`, array `[N]T`, or `[...]T` — whatever sits between the + // brackets is a length expression we never need to interpret. + closeIdx := matchDelimiter(tokens, i, "[", "]") + if closeIdx < 0 { + return -1 + } + return goTypeEnd(tokens, closeIdx+1) + + case t.Kind == TokKeyword && t.Text == "map": + if i+1 >= n || tokens[i+1].Kind != TokOperator || tokens[i+1].Text != "[" { + return -1 + } + closeIdx := matchDelimiter(tokens, i+1, "[", "]") + if closeIdx < 0 { + return -1 + } + return goTypeEnd(tokens, closeIdx+1) + + case t.Kind == TokOperator && t.Text == "*": + return goTypeEnd(tokens, i+1) + + case t.Kind == TokKeyword && (t.Text == "struct" || t.Text == "interface"): + if i+1 >= n || !isOpenBrace(tokens[i+1]) { + return -1 + } + closeIdx := matchDelimiter(tokens, i+1, "{", "}") + if closeIdx < 0 { + return -1 + } + return closeIdx + 1 + + case t.Kind == TokIdent: + j := i + 1 + // Qualified name: pkg.Type + if j+1 < n && tokens[j].Kind == TokOperator && tokens[j].Text == "." && tokens[j+1].Kind == TokIdent { + j += 2 + } + // Generic instantiation: Type[int] + if j < n && tokens[j].Kind == TokOperator && tokens[j].Text == "[" { + closeIdx := matchDelimiter(tokens, j, "[", "]") + if closeIdx < 0 { + return -1 + } + j = closeIdx + 1 + } + return j + } + return -1 +} + +// goFuncLiteralBody returns the index of the body brace of the function literal +// whose `func` keyword sits at i, or -1 when that `func` opens a TYPE rather +// than a literal. Both forms appear inside a table: `setup func()` declares a +// struct field's type, while `setup: func() { … }` supplies a value. What +// follows the signature tells them apart — only a literal has a body. +func goFuncLiteralBody(tokens []Token, i int) int { + n := len(tokens) + if i+1 >= n || tokens[i+1].Kind != TokOperator || tokens[i+1].Text != "(" { + return -1 + } + closeParen := matchDelimiter(tokens, i+1, "(", ")") + if closeParen < 0 { + return -1 + } + + j := closeParen + 1 + if j >= n { + return -1 + } + switch { + case isOpenBrace(tokens[j]): + return j + case tokens[j].Kind == TokOperator && tokens[j].Text == "(": + // Parenthesized results: `func() (int, error) {`. + closeResults := matchDelimiter(tokens, j, "(", ")") + if closeResults < 0 { + return -1 + } + j = closeResults + 1 + default: + // A single result type: `func() error {`. + end := goTypeEnd(tokens, j) + if end < 0 { + return -1 + } + j = end + } + + if j < n && isOpenBrace(tokens[j]) { + return j + } + return -1 +} + +// matchDelimiter returns the index of the closing delimiter balancing the +// opening one at open, or -1 when the pair is unbalanced. +func matchDelimiter(tokens []Token, open int, openText, closeText string) int { + depth := 0 + for j := open; j < len(tokens); j++ { + if tokens[j].Kind != TokOperator { + continue + } + switch tokens[j].Text { + case openText: + depth++ + case closeText: + depth-- + if depth == 0 { + return j + } + } + } + return -1 +} + // TokenizeFile lexes content into a normalized token sequence for the given language. // Comments are consumed (no token emitted). Identifiers become TokIdent, keywords become // TokKeyword, number literals become TokNumber, string literals become TokString. // Operators and punctuation are emitted as TokOperator. func TokenizeFile(content string, lang *domain.Language) []Token { + // A nil language means "no syntax knowledge": no keywords, no comment + // markers, everything lexed as plain tokens. Scanning never reaches here + // with one — collectFiles drops files whose language is unknown — but + // markFunctionBodies handles nil explicitly, so lexing must too rather than + // panicking on the first field access. + if lang == nil { + lang = &domain.Language{} + } kws := getKeywords(lang.Name) src := content n := len(src) @@ -1051,6 +1431,12 @@ func markPythonFunctions(tokens []Token, inFunc []bool) []FuncSpan { } // Mark everything after the colon until next def/class at same or lesser indentation. end := colonIdx + // resume is the index of the definition that ended this body, so the + // outer scan can restart ON it. Restarting AFTER it would step over + // the keyword and drop that function entirely — which is what used + // to happen, leaving every def after the first one unmarked and its + // duplication invisible. + resume := -1 for j := colonIdx + 1; j < n; j++ { // Stop at next top-level def or class (heuristic: if the def/class // is on a line that's <= the original def line's indent, we stop). @@ -1065,19 +1451,23 @@ func markPythonFunctions(tokens []Token, inFunc []bool) []FuncSpan { // if there's a blank-line gap (line difference > 1 from previous token), // this might be a new top-level definition. if j > 0 && tokens[j].Line > tokens[j-1].Line+1 { - i = j + resume = j break } } inFunc[j] = true end = j - if j == n-1 { - i = n - } } if end > colonIdx { funcs = append(funcs, FuncSpan{Start: colonIdx + 1, End: end, Name: name}) } + if resume >= 0 { + i = resume + } else { + // The body ran to the end of the file. + i = n + } + continue } i++ } From 7a03e7e8c01d8c322fc688622599bc093bb06df1 Mon Sep 17 00:00:00 2001 From: Lucas Machado Date: Sun, 19 Jul 2026 15:47:17 +0200 Subject: [PATCH 2/2] docs: document the data-table exclusion README now states which literals are excluded, in which languages, and what still counts as duplication. Adds the implementation plan for #31, including the approach that was tried and rejected. --- README.md | 6 + .../001-table-literal-false-positive.plan.md | 138 ++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 docs/specs/001-table-literal-false-positive.plan.md diff --git a/README.md b/README.md index 74f677d..82075ee 100644 --- a/README.md +++ b/README.md @@ -295,6 +295,12 @@ dupehound tokenizes source files into a normalized token stream (stripping comme Detection is restricted to **function and method bodies** — imports, top-level declarations, struct definitions, config blocks, and keyword maps are automatically excluded. This dramatically reduces false positives and focuses on actionable logic duplication. +**Data tables inside functions are excluded too.** The rows of a collection literal are structurally identical by design, so a clone whose every instance falls inside a single literal is reported as data, not duplication — the table-driven test pattern no longer flags itself. + +This covers Go (`[]T{…}`, `map[K]V{…}`, `[]struct{…}{…}`), Python (lists and dicts), JavaScript/TypeScript, Ruby, Rust, PHP, Elixir, Swift, Dart, and Lua. Java, C, C++, C# and Kotlin are deliberately left out: there `[` is an index or an array type and `{` is also a block, so a literal cannot be told from code by tokens alone. + +Logic still counts. A callback column (`run func(t *testing.T)`, `run: () => {…}`) is checked normally, two identical tables in different places are still flagged, and a short inline `[]T{a, b}` never breaks up the block around it. + ## Output formats - **text** — human-readable with hotspots, top clones by impact, and full clone list diff --git a/docs/specs/001-table-literal-false-positive.plan.md b/docs/specs/001-table-literal-false-positive.plan.md new file mode 100644 index 0000000..52a44c8 --- /dev/null +++ b/docs/specs/001-table-literal-false-positive.plan.md @@ -0,0 +1,138 @@ +# Plan: Fix table-driven test literal false positives (issue #31) + +**Spec of record:** [GitHub issue #31](https://github.com/AxeForging/dupehound/issues/31) — +no `docs/specs/` spec exists for this; the issue contains the full repro, expected +behavior, and suggested fixes, so it serves as the spec (spec review waived as a bugfix). + +**Problem:** Idiomatic Go table-driven tests (`tests := []struct{…}{…}` inside a test +function) are reported as type-2 clones. The reported "instances" are overlapping +sliding windows over the rows of a single composite literal. This contradicts the +README's documented exclusion of data/config literals — the exclusion only works +today because struct/config blocks usually sit *outside* function bodies, and +`markFunctionBodies` masking is the only data filter. + +**Root causes (two independent defects):** + +1. `services/detector.go` `detectExact`: seed windows are spaced `>= minTokens` + apart, but greedy extension grows each block to `totalTokens > minTokens`, so + emitted instances of one clone group overlap each other. Overlapping windows are + never distinct duplicates. +2. There is no notion of composite literals *inside* function bodies, so a data + table is treated as logic. Even with defect 1 fixed, a uniform 10-row table still + yields disjoint identical windows and gets flagged. + +## Design change during implementation + +The plan originally chose the issue's suggestion **1** (exclude literal tokens from +detection, by clearing `InFunc`). That was implemented, then **rejected on evidence** +and replaced with the issue's suggestion **2** (suppress clone groups confined to one +literal). + +Why: masking tokens punches a hole through the middle of surrounding code. A short +inline `[]TokenizedFile{a, b}` sitting inside an otherwise-duplicated block split that +block in two and the finding was lost. Measured on this repo's own test suite, the +masking approach removed **58 clones, ~50 of them genuine** duplication. The +span-based rule removes **5, of which 4 are overlapping-window artifacts and 1 is a +real data table** — with zero new findings and no real duplication lost. + +Final design: + +- **Overlap guard** (`dropOverlappingStarts`): after extension, same-file instances + must be spaced `>= totalTokens`; groups left with < 2 instances are dropped. +- **Data-literal spans** (`findDataLiteralSpans`): composite literals are recorded as + token spans on `TokenizedFile`, *not* excluded from detection. `detectExact` drops a + clone group when every instance lies inside one span; `detectFuzzy` skips candidate + pairs whose blocks share a span (resolved once per block, not per pair). +- **Func-literal exemption** (`findFuncLiteralBodies`): a window touching a function + literal body is logic, never data — so a `run func(t *testing.T)` column is still + checked. Distinguishing a func literal from a struct field's func *type* needed its + own parse (`goFuncLiteralBody`); `markFunctionBodies` keys off the `func` keyword + alone and cannot tell them apart. + +## Steps + +- [x] **TDD repro first (must fail):** detector-level test reproducing issue #31 + verbatim, plus a gofmt-style multi-line-row table. + Files: `services/table_literal_test.go`. +- [x] **Overlap guard in `detectExact`.** Files: `services/detector.go`, + `services/overlap_test.go`. +- [x] **Data-literal span discovery + suppression rule**, wired into both the exact + and fuzzy detectors. Files: `services/tokenizer.go`, `services/detector.go`, + `services/composite_literal_test.go`, `services/testhelpers_test.go`. +- [x] **Extend beyond Go** to every language whose collection literals are + unambiguous, with per-language positive, negative (indexing) and callback tests. + Files: `services/tokenizer.go`, `services/data_literal_langs_test.go`. +- [x] **Fix the bugs surfaced along the way** (see below): Python function + boundaries, Dart body misclassification, nil-language panic. + Files: `services/tokenizer.go`, `services/pyfuncboundary_test.go`. +- [x] **Repro green + full regression:** `go test -race ./...` (469 pass), + `make lint` (0 issues), `gofumpt -w`. No golden-file churn. +- [x] **Integration tests via the built binary.** Files: + `integration/table_literal_test.go`. All four fail on `main`, pass on the branch. +- [x] **Docs:** README "How it works" documents the data-table exclusion and its + limits. Files: `README.md`. +- [x] **Branch + PR:** `fix/table-literal-false-positive`, `Closes #31`. + +## Verification performed + +- Every new test was run against pristine `main` and **fails** there (4/4 integration, + 4/6 unit — the 2 that pass on main are the negative "real duplication is still + reported" guards, which must pass in both). +- A/B scan of identical pristine `main` source with the pre-fix and post-fix binaries: + 157 → 152 clones, 0 new, and the only non-overlap removal is a genuine + `[]domain.Clone{…}` fixture table. +- Self-scan under the repo's own `.dupehound.yml` is byte-identical before and after + (13 clones, 5.1%), so the CI quality gate is unaffected. + +## Risks + +- **Heuristic misclassification:** token-level Go brace disambiguation is heuristic. + Mitigated by a conservative trigger (only `]T{` / `map[…]V{` / `]struct{…}{`), + Go-only gating, and negative tests covering `if m[k] {`, range statements, index + expressions on call results, func literals, and plain `T{…}` struct literals. +- **Under-suppression is the deliberate failure mode:** anything ambiguous stays + reported. A missed false positive is noise; a swallowed real clone is a silent + correctness loss. +- **Baselines:** users with committed baselines may see previously reported table + clones disappear. Stale entries are inert, not breaking. +- **Rollback:** revert the PR — no schema, config, or flag changes; behavior-only. + +## Done means + +- The exact repro from issue #31 scans to `Clones found: 0` (unit + binary-level). +- No clone group ever reports overlapping same-file instances (unit-tested invariant). +- Genuine duplication is still detected: in test files, in table func-literal columns, + and in blocks containing short inline literals — each covered by a dedicated test. +- `go test -race ./...` and `make lint` clean; PR open referencing issue #31. + +## Bugs found while working — all fixed in this PR + +1. **Python function boundaries (`markPythonFunctions`)** — the body scan ended at the + next blank-line-separated `def`, then resumed the outer scan *after* that keyword, + stepping over it. Every definition following the first was never marked as a + function body, and since detection only looks inside function bodies, all + duplication in those functions was invisible. In a file of `n` blank-line separated + defs, only the first was ever scanned. Fixed by resuming *on* the boundary keyword. + Regression tests: `services/pyfuncboundary_test.go` (7 of 8 fail on `main`). +2. **Dart function bodies misread as literals** — bodies in languages without a `func` + keyword come from a brace-depth heuristic that cannot tell a function body from a + `{…}` map literal at the same depth, so every row of a Dart table looked like a + function body. Fixed with `isRealFunctionBody`: a brace-delimited body follows `)` + or an arrow, a data literal follows `=`, `,` or `[`. +3. **`TokenizeFile` nil-language panic** — it dereferenced `lang.Name` while + `markFunctionBodies` explicitly documents nil as "no syntax knowledge". Unreachable + today (`collectFiles` drops unknown-language files first), but the contract was + incoherent. Fixed by substituting a zero-value language. + +## Language coverage + +The rule applies wherever a collection literal can be told from code by tokens alone: + +- **Go** — typed composite literals: `[]T{…}`, `map[K]V{…}`, `[]struct{…}{…}`. +- **Bracket literals** (`[…]` in expression position) — Python, JavaScript, TypeScript, + Ruby, Rust, PHP, Elixir, Swift, Dart. +- **Brace literals** (`{…}` where braces are never blocks) — Python, Lua, Elixir. + +Deliberately excluded: Java, C, C++, C#, Kotlin (there `[` is an index or array type +and `{` is also a block) and Scala (`[` is type parameters). Ruby takes brackets only, +since `{` is either a hash or a block argument.