chore(#784): duplication rescan census, six issues filed - #797
Conversation
Passes 1a/1b (#380/#381) are closed but six duplications surfaced in already-audited code by accident on 2026-08-07. This adds the derived, re-runnable census #784 asks for (Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py, a k-token-shingle clone detector over bridge .mm functions and Swift API funcs) and files six issues (#791-#796) from what it found. No source changes: analysis and artifact only, per this branch's scope constraint against the concurrent deprecation-adjudication pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed. Nothing blocking, and the headline finding corrects my own framing of #784. The six do not share one shape, and that matters#784 asserted the dominant shape was "two entry points onto the same OCCT call that drifted", and
That is worth more than the candidate list. A token-based detector can never find shape B, and Verified independentlyI checked #795's headline pair against the source rather than the score. The tuning is the part I would have got wrongLoose settings reported 3219 of 4152 bridge units as pairs, almost all of them OCCT's own Six tightenings, each validated by reading samples, converging on 38 plus 21. That is a measured The removal matrix11/11, and the row I care about is The nested-Swift-function discovery is a good one. A local function's body is trivially contained in On filing rather than fixingCorrect per the scoping, and the reason still holds: the deprecation agent is editing Six issues, #791 to #796. #795 is the substantial one and should not sit in the backlog long: three |
secondmouseAU-bot
left a comment
secondmouseAU-bot
left a comment
There was a problem hiding this comment.
Automated high-effort code review (8 findings).
| yield name, start, i | ||
|
|
||
|
|
||
| SWIFT_FUNC = re.compile(r"func\s+(\w+)\s*\([^)]*\)(?:\s*(?:async|throws|rethrows))*\s*(?:->\s*[^\{]+?)?\{") |
There was a problem hiding this comment.
correctness (confirmed): SWIFT_FUNC's parameter-list ([^)]*) and return-type ([^\{]+?) regexes aren't nesting-aware, so a Swift func whose signature has a closure-literal default parameter value hijacks the function's body-opening brace. E.g. for func onComplete(handler: () -> Void = { }) { doWork(); doWorkAgain() }, swift_functions() extracts { } (the closure's own empty body) instead of the real two-statement body — verified by running the exact code against this input. Any genuine duplicate logic inside the real body becomes invisible to the whole detector: a silent false negative, exactly the failure mode ("a census that says nothing wrong when it should") this script exists to avoid.
| j = n if j < 0 else j + 2 | ||
| out.append("".join(c if c == "\n" else " " for c in text[i:j])) | ||
| i = j | ||
| elif text[i] == '"': |
There was a problem hiding this comment.
correctness (confirmed): strip_comments() has no special-casing for single-quoted C/C++ char literals, so a char literal containing a double quote (e.g. '"') is misread as opening a string literal, and the scan then swallows everything — including real ////* comment markers — up to the next literal " it finds. For source containing if (c == '"') { return 1; } followed on the next line by // real comment with "quote inside, running this exact function leaves the comment line completely un-blanked (output byte-identical to input). The un-stripped comment prose then gets tokenized as code by tokenize(), either manufacturing a spurious shared-token duplicate pair or diluting a real duplicate's shingle set below the 0.85 containment threshold — with no indication in the report that this happened.
| j = n if j < 0 else j + 2 | ||
| out.append("".join(c if c == "\n" else " " for c in text[i:j])) | ||
| i = j | ||
| elif text[i] == '"': |
There was a problem hiding this comment.
cleanup (confirmed): Separately from the comment-swallowing issue on this same line — strip_comments() excludes double-quoted string contents from brace-scanning but does nothing for single-quoted char literals, so a {/} inside a char literal is left in the stripped text and counted as a real brace. Bridge .mm source with a character literal like '{'/'}' (common in delimiter/format-char comparisons, e.g. the OBJ/PLY exporter code this script itself analyzes) throws off c_functions()'s depth counter, silently truncating or extending the extracted function body into the next function — corrupting the very unit boundaries the duplicate-detection scoring depends on, with no error raised.
| NOT_A_FUNCTION = {"if", "for", "while", "switch", "catch", "return"} | ||
|
|
||
|
|
||
| def c_functions(text): |
There was a problem hiding this comment.
cleanup (confirmed): c_functions() (199-215) and swift_functions() (221-235) copy-paste an identical brace-depth-matching while-loop verbatim, differing only in which regex feeds it. A bug fix to the brace-matching walk (e.g. handling the char-literal gap flagged elsewhere in this review) has to be applied twice by hand in the same file; missing one copy leaves the two extractors silently inconsistent — exactly the copy-paste-with-drift pattern this script exists to catch, now present inside the detector itself.
| Unit = collections.namedtuple("Unit", "name kind file line tokens related") | ||
|
|
||
|
|
||
| def bridge_units(sources): |
There was a problem hiding this comment.
cleanup (confirmed): bridge_units() (273-284) and swift_units() (287-297) duplicate the same source-iteration / strip / extract / tokenize / Unit-construction sequence, differing only in the extractor function called and how kind is computed. Any change to how a Unit is built from a source file (e.g. adding a field, or fixing the strip_comments bugs flagged elsewhere in this review) must be edited in two near-identical 12-line functions instead of one, and the two can silently diverge if only one copy is updated.
| results = [] | ||
| for (i, j), shared in shared_counts.items(): | ||
| if shared < min_shared: | ||
| continue |
There was a problem hiding this comment.
cleanup (confirmed): The delegation exclusion (units[i].name in token_sets[j]) matches the function's name anywhere it appears as a token in the other body — including inside a string literal — not just an actual call. A genuine near-duplicate pair where one function's name happens to be quoted in the other's body (e.g. an NSLog/assert message referencing the sibling function by name) is silently reclassified as "delegation" and dropped from the candidate list — the same under-counting failure mode the script's own docstring says every hand-built census in this repo's history has had (#558/#571/#583/#595/#640), now possibly reproduced inside the tool meant to fix it.
| @@ -0,0 +1,209 @@ | |||
| bridge (Sources/OCCTBridge/src): 38 candidate pair(s) out of 4152 unit(s) | |||
There was a problem hiding this comment.
cleanup (plausible): Committing this captured run snapshot duplicates content the script itself derives and can drift from the tree, in tension with this repo's own Documentation Standards ("No duplicate content — one canonical location per topic. Link, don't copy"; "Code reviews and handoff docs are ephemeral — don't commit them"). A future reader who opens full-report.txt instead of re-running the script (which the README itself warns may be necessary since it "may drift from a fresh run") can act on stale candidate pairs/line numbers that no longer match the current source tree.
|
|
||
| def bridge_sources(): | ||
| paths = sorted(glob.glob(os.path.join(BRIDGE_SRC_DIR, "*.mm"))) | ||
| return [(p, open(p, errors="ignore").read()) for p in paths] |
There was a problem hiding this comment.
cleanup (plausible): bridge_sources()/swift_sources() open every source file without a context manager or explicit close, relying on CPython refcounting to reclaim the file handle. On an interpreter or run where the file object's refcount doesn't drop immediately (e.g. a different Python implementation, or if sources is retained by a caller), file descriptors accumulate across the hundreds of bridge/Swift source files scanned, risking a "too many open files" error on a large enough corpus.
… drop snapshot PR #797 review found eight issues in the duplication-rescan census script, all fixed here, none touching Sources/: Four correctness bugs, all under-reporting: - swift_functions()'s non-nesting parameter-list regex let a closure-typed default parameter hijack the body scan, hiding the real function body entirely. - strip_comments() had no case for single-quoted char literals, so '"' was misread as opening a real string and swallowed real comments looking for a closing quote. - The same gap let a brace inside a char literal corrupt the brace-depth walk, in one measured case swallowing the next function's entire definition. - The delegation exclusion matched a name anywhere in a body, including inside a string literal, so a sibling named in a log message was mistaken for a real call. Bugs 2-4 share one fix: strip_comments() now blanks string/char literal content instead of copying it through. Each bug has a self-test fixture proven to fail against a hybrid build carrying the pre-fix code and pass against the fix (11 bridge, 4 Swift fixtures, 15/15 total). Re-running the fixed detector found the parser fix moved the Swift unit count (3374 to 3471, +97 previously-invisible functions) and the pair list by exactly two: strokeWidth dropped below the size floor (already filed in #795, noted there) and TransformUtils.displacement/transformation appeared as a genuine new pair (added to #796 as a sixth pair). Bridge side unchanged. Also: factored c_functions()/swift_functions()'s duplicated brace-matching loop and bridge_units()/swift_units()'s duplicated construction sequence into shared helpers (the detector contained the duplication it detects); dropped the committed full-report.txt snapshot per this repo's own "no duplicate content" standard, since every filed issue already carries its own complete evidence; closed file handles explicitly instead of relying on refcounting. All six filed issues (#791-#796) and two follow-up comments updated for the TransformUtils/ strokeWidth findings and for writing-style compliance (em-dashes, "--" as a sentence dash). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All eight actioned. One thing needed repairing that the report claimed was done, and the answer The counts did move, and the ratio is the resultI asked whether fixing the parser surfaces units it was dropping. It does: the Swift corpus goes Ninety-seven newly visible functions produced exactly one new duplicate pair, The bridge report is byte-identical, so the C parser was already sound. Shingle counts drop where string literals are now blanked, What I had to repairTwo follow-up comments, on #795 and #796, were posted as the literal text Worth naming because the report said the new pair was "added to #796" and it was not. A shell The restThe four correctness bugs are fixed at one root, Findings 5 and 6, the detector's own duplicated brace walk and unit construction, are factored. That Dropping Also cleared every em-dash from the script, the README, all six issue bodies and the PR body. The |
2 participants
What & why
The duplication-rescan half of #784. Passes 1a (#380) and 1b (#381) are closed at 60 sub-issues,
both built as segmented subsystem reads. On 2026-08-07, six duplications were found in code both
passes had already audited, every one by accident while doing something else. #784 asks for a
derived, re-runnable census of the rest, following
Scripts/derive-bridge-header-split.py'spattern, per
docs/v2.0.0-plan.md's census-once rule.Scope: analysis and artifact, not fixes. A separate agent is doing #784's deprecation half on
chore/784-deprecation-adjudication, editingSources/OCCTSwiftheavily. This PR makes zerochanges to
Sources/OCCTBridgeorSources/OCCTSwift: every finding is filed, not fixed, per theexplicit instruction not to conflict with that concurrent pass. None of the findings needed a
breaking change to fix, so this constraint cost nothing extra.
Closes #784 (the rescan half only; the deprecation-adjudication half is tracked and closed
separately on the other branch)
Updated after review (see "Review round" below): four correctness bugs in the detector itself
fixed, two internal duplications in the detector fixed, a committed report snapshot removed, a
resource-cleanup nit fixed. All findings and issue numbers below are current as of that round.
What shape the six share
Not one shape, three, two each (full detail with citations in
Scripts/repro/784-duplication-rescan/README.md):inline-block half is recognised as a function containing a copy-pasted block). This is the shape
this rescan's artifact targets, and is NOT the majority of the six despite Merge-to-main condition: adjudicate all 61 deprecations, and rescan Pass 1a/1b for duplication those passes missed #784's own framing
suggesting it: 2 of 6 are strictly bridge, 1 more is Swift-app-level.
sequences; named as a scope boundary, not chased.
(PR#774, PR#773). A different detection problem and a different population; out of scope here,
not silently dropped.
The artifact
Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py: a MOSS-style k-token-shingleclone detector over every bridge C/Objective-C++ function (
Sources/OCCTBridge/src/*.mm) and everySwift
func(Sources/OCCTSwift/*.swift), scored by containment (not Jaccard, so a smallduplicated function embedded inside a much bigger caller, the PR#778 shape, still registers), with
a document-frequency boilerplate exclusion (the bridge's own necessarily-repetitive C-wrapper
preamble is expected, not a finding) and a delegation exclusion (A calling B by name is
composition, not duplication, including the accidentally-discovered case of a Swift function
nested inside another, where containment is approximately 1.0 by pure syntax).
Tuning was measured, not guessed: the first pass at loose settings reported 3219 of the
bridge's 4152 units as candidate pairs, almost entirely OCCT's own deliberately parallel 3D/2D
class hierarchies (
Geom_Curve/Geom2d_Curve) and already-deduplicated forwarding shims. Sixsuccessive tightenings, each validated by reading a sample, converge on 38 bridge candidates and 21
Swift candidates. Full trace in the README.
Review round: 8 findings addressed, none in
Sources/Code review found 8 issues, all in the census script itself; nothing it touched required a
Sources/change.Four confirmed correctness bugs, all under-reporting (the failure mode that matters most for a
script whose whole claim is "the tree is clean of shape A", since a clean run cannot be told apart
from a blind one):
swift_functions()'s parameter-list regex ([^)]*) was not nesting-aware, so a closure-typeddefault parameter (
handler: () -> Void = { }) hijacked the body scan and extracted theclosure's own empty
{ }instead of the real body. Fixed with paren-depth tracking.strip_comments()had no case for single-quoted char literals, so'"'was misread as openinga real string and the scan swallowed real comments looking for a closing
".{/}inside a char literal corruptc_functions()'s brace-depth walk;measured case: one phantom brace was enough to swallow the next function's entire definition.
literal, so a sibling's name quoted in a log message was mistaken for a real call.
Bugs 2-4 share one fix:
strip_comments()now blanks the content of string AND char literals(previously it copied double-quoted string content through unchanged and had no char-literal
handling at all). Each of the four has a dedicated
--self-testfixture, confirmed to fail againsta hybrid build carrying the pre-fix extraction code and pass against the fix, per
okf/policies/prove-the-test-fails.md.Did the fix move the candidate counts? Checked, not assumed. Bridge: unchanged (38 pairs, 4152
units, byte-identical report). Swift: unit count rose from 3374 to 3471 (+97 previously-invisible
functions, mostly tuple-typed parameter lists hitting bug 1's mechanism), and the pair list changed
by exactly two:
strokeWidth(PDFExporter/SVGExporter) dropped below the size floor once itsstring-literal case labels stopped inflating its token count (already filed in #795, noted there
as a comment so a fresh run isn't confusing);
TransformUtils.displacement/transformationappeared as a genuine new pair (the old parser found zero functions in that file at all, confirmed
directly), added to #796 as a sixth pair.
Two internal duplications, found in the duplication detector itself:
c_functions()andswift_functions()carried an identical brace-matching loop;bridge_units()/swift_units()duplicated the same iterate/strip/extract/tokenize/construct sequence. Both factored into one
shared helper each (
_brace_match(),_units_from_sources()), no behavior change.One committed artifact dropped. An earlier revision committed
full-report.txt, a capturedrun's output. Review cited this repo's own Documentation Standards ("no duplicate content", "code
reviews and handoff docs are ephemeral, don't commit them") against it, correctly: the file had
already drifted from a fresh run by the time of the review. Dropped rather than kept; the six filed
issues each carry their own complete evidence independent of it (see README's "On not committing a
captured report").
One resource-cleanup nit:
bridge_sources()/swift_sources()now close each file handleexplicitly (
with open(...)) instead of relying on refcounting.--self-testis now 15/15 (11 bridge, 4 Swift, up from 11/11), with a removal-matrix row addedfor the four parser fixes (revert to pre-fix extraction code, confirm exactly those four fail and
nothing else changes). Full table in the README.
Findings: 6 issues filed, 0 fixed here
Convert*ToBSpline*entry points independently reimplement an array-building helper their siblings already call.
Convert_SphereToBSplineSurfacegenuinely inheritsConvert_ElementarySurfaceToBSplineSurface(verified against the OCCT refman), confirming the fix is a drop-in call, not a redesign.
added in a later release, zero cross-reference.
oriFromInt/intToOrientation, the int-to-TopAbs_Orientationdecodereimplemented in two files, the same class of bug Consolidate the bridge's ~8 duplicate int→GeomAbs_Shape continuity mappers — divergent numbering already shipped one bug (#433) and still causes bsplineRestriction vs bsplineRestrictionAdvanced to silently disagree #490 already fixed for continuity enums.
scaffolding around one differing OCCT call, filed as one issue per Merge-to-main condition: adjudicate all 61 deprecations, and rescan Pass 1a/1b for duplication those passes missed #784's "cheap and targeted"
instruction rather than eleven. Explicitly excludes
OCCTShapeCreateMesh/WithParams, whichalready has an in-place comment acknowledging the duplication as a deliberate decision.
PDFExporter/SVGExporterduplicatedrawing-collection logic verbatim despite sharing no base class;
DXFExporter's ownformatTolerance/TolerancedLabelis byte-identical toDrawingDispatch.swift's own privatefunction of the same name, the shared-dispatch file duplicating itself in a sibling file that
doesn't use it.
parser fix surfaced it) sharing marshaling scaffolding around distinct bridge calls, with three
measured-and-excluded near-misses documented in the issue (already acknowledged in a comment,
already-deprecated forwarding shims, or legitimate overloads).
All labelled
type:chore, cluster:kernel, off the v2.0.0 milestone (matches #761's own precedent:internal consolidation changes no public behavior, so it can ship in any release).
What the census does NOT find (read before trusting a small number)
Documented in the README's own section: shape B (#777, no shared text), shape C (intra-function
branch duplication, dev-tooling population), any unit below
min_distinctive(two real one-lineduplicates in #792 were found by reading the file directly, not by the script, noted rather than
hidden), and generic/computed-property Swift units
swift_functions()doesn't extract standalone.Gates and tests
--self-test: clean. (check-bridge-index,check-null-handle-guards,check-docs-defaults,derive-bridge-header-split --verify,count-operations,census-unmeasured-values,check-changelog-transcription,derive-shape-domain-split,derive-swift-file-split.)swift test: 5510 tests, 1440 suites, 0 failures (unchanged from before the reviewround, since nothing under
Sources/changed).swift build: clean (pre-existing unrelated deprecation warnings only).Scripts/tsan-stress.shnot run: this PR touches no concurrency-relevant code (a standalonePython script; zero changes under
Sources/).CHANGELOG entry
#784 duplication rescan: a committed census artifact, six issues filed, no source changes
Added
Scripts/repro/784-duplication-rescan/detect-duplicate-logic.py, a re-runnable k-token-shingleclone detector over the bridge's C functions and the Swift API's
funcs, built to answer whetherPass 1a/1b (#380/#381) missed more of the duplication shape six accidentally-found instances
revealed on 2026-08-07. It found 38 bridge and 21 Swift candidate pairs at its tuned thresholds; 6
issues (#791-#796) were filed from what measured as genuine, none fixed in this PR. No public API
changed.
SemVer impact
NONE. Zero changes under
Sources/OCCTBridgeorSources/OCCTSwift; this PR adds a script and itsREADME under
Scripts/repro/, outside the public Swift API surfacecount-operations.pytracks,and files GitHub issues. No consumer-visible effect, no migration.
Checklist
--self-test(15/15), which is the "behavior" a census script has.(15 cases across 7 mechanism rows plus the parser-fix reversion row) is in the README,
including one row's flip explained structurally rather than left as an unexplained "adds
nothing."
docs/CHANGELOG.mdis not in this diff.docs/SEMVER.mdis not in this diff.Notes for the reviewer
refactor/381-pass1b, notmain, per Merge-to-main condition: adjudicate all 61 deprecations, and rescan Pass 1a/1b for duplication those passes missed #784/Merge refactor/381-pass1b to main and ship v2.0.0: the plan #786.answer. See README's "On not committing a captured report" for the reasoning.
type:chore, cluster:kernel.None were fixed here; Merge-to-main condition: adjudicate all 61 deprecations, and rescan Pass 1a/1b for duplication those passes missed #784's own "either fixed or filed" bar is met by "filed" for all six, since
none required a breaking change that couldn't wait for the major. Issue bodies for all six, plus
two follow-up comments on PDF/SVG/DXF exporters duplicate drawing-collection logic instead of sharing DrawingDispatch.swift; DXF duplicates its own formatTolerance #795/Census: 5 Swift API sibling pairs share marshaling scaffolding around distinct bridge calls #796, were updated during this review round for the two findings
surfaced by the parser fix and for writing-style compliance.
the real corpus's
exclude_delegation=Falsedelta (0 bridge / 21 Swift), not assumed, and turnedout to matter directly: the same measurement, re-run after the parser fix, is what surfaced the
TransformUtilspair above.