Skip to content

fix(xerrors): do not silently drop foreign collected errors in Collector.Collect#424

Merged
Kybxd merged 2 commits into
masterfrom
fix-err-collector
Jul 7, 2026
Merged

fix(xerrors): do not silently drop foreign collected errors in Collector.Collect#424
Kybxd merged 2 commits into
masterfrom
fix-err-collector

Conversation

@Kybxd

@Kybxd Kybxd commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Problem

xerrors.Collector.Collect short-circuited on any *collected error:

var ce *collected
if errors.As(err, &ce) {
    // record outerWM, return; no append to errs, no counter bump
    return nil
}

This branch was designed for the case where the incoming error comes from one of c's own descendant collectors — that error is already reachable from the root via tree auto-join, so re-counting it on c would produce duplicates.

However, the same branch also matched foreign *collected values: errors produced by an unrelated, parser-local fail-fast collector that is not a child of c. The canonical example in this repo:

  • confgen.NewExtendedSheetParser creates its own xerrors.NewCollector(1) for in-sheet fail-fast diagnostics (internal/confgen/parser.go:333).
  • When protogen.parseEnumTypeSheet (mode MODE_ENUM_TYPE_MULTI) calls into confgen and the sheet parser hits an error, confgen's local collector calls Join() and returns a *collected{origin: parserLocalCol} up the call chain.
  • The outer protogen.go:716 does sheetCollector.Collect(xerrors.WrapKV(blockErr, KeyBookName, ..., KeySheetName, ...)).
  • sheetCollector is built via bookCollector.NewChild(...) and has no parent relationship with parserLocalCol, so the short-circuit was firing on a foreign origin.

Net effect:

  • The error was neither appended to sheetCollector.errs nor counted.
  • outerWM was written onto the foreign collector (which sheetCollector.Join() never visits).
  • sheetCollector.HasErrors() stayed false, Join() returned nil.
  • The "sub-field value not unique" diagnostic (and any other confgen-surfaced error reaching protogen via parseEnumType / parseStructType) was silently swallowed and the tool exited 0 with no output.

Fix

Tighten the short-circuit so it only fires when the collected error's origin is part of c's own collector tree (shares the same root reachable via the parent chain).

if errors.As(err, &ce) && ce.origin != nil && ce.origin.sameTreeAs(c) {
    // same tree → reachable from root via auto-join; only record outerWM
    return nil
}
// otherwise fall through to the ordinary path: append + count

Two small helpers (root, sameTreeAs) are added to Collector. They are pure parent-chain walks, no locks needed.

Semantics after the fix

Collect(err) where err is *collected with origin oc:

Case Behavior
oc is in the same collector tree as c (shares root) Unchanged: record outer WrapKV fields on oc and return; do not double-count.
oc is foreign (different tree, e.g. parser-local fail-fast collector) New: treat as an ordinary error — append to c.errs, bump every ancestor's counter. HasErrors() and Join() now surface it.

Tests

  • Renamed two tests whose assertions encoded the buggy "silently drop foreign collected" behavior:

    • TestCollected_CollectSkipsCollectedErrorTestCollected_CollectForeignCollectedIsNotDropped
    • TestCollected_CollectSkipsWrappedCollectedErrorTestCollected_CollectForeignWrappedCollectedIsNotDropped

    Both now assert that the foreign (and WrapKV-wrapped foreign) collected error survives Collect and shows up in Join().

  • TestCollected_CrossSubtreeCollection keeps its original assertion (same-tree siblings between outerChild / innerChild under docChild): outer must not re-count an inner sibling's *collected. The new same-tree predicate covers this case unchanged.

  • All other internal/x/xerrors tests pass without modification.

Verification

$ go test -count=1 ./internal/x/xerrors/...
ok  	github.com/tableauio/tableau/internal/x/xerrors	0.016s

$ go test -count=1 ./...
ok  	github.com/tableauio/tableau/internal/confgen
ok  	github.com/tableauio/tableau/internal/protogen
ok  	github.com/tableauio/tableau/internal/x/xerrors
... (all packages green)

Compatibility

  • Pure semantic widening: previously-dropped errors now surface. No callers in this repo relied on the dropped behavior; the renamed tests were the only place that asserted it.
  • Same-tree behavior is bit-for-bit unchanged, so existing protogen / confgen / load happy paths and intentional sibling-aggregation patterns are unaffected.

…tor.Collect

Collector.Collect previously short-circuited on any *collected error and returned without storing or counting it. The branch was designed for the case where the collected error originates from one of c's own descendant collectors (so it is reachable via tree auto-join), but it also matched foreign *collected values produced by unrelated, parser-local fail-fast collectors (e.g. confgen sheetParser.sheetCollector created via xerrors.NewCollector(1)).

When such a foreign collected error was passed to an upstream sheetCollector, the error was neither appended to errs nor counted, HasErrors() stayed false, and Join() returned nil — silently swallowing real diagnostics.

Tighten the short-circuit to only fire when the collected error's origin shares the same root as c (i.e. belongs to the same collector tree built via NewChild). Foreign collected errors now fall through to the ordinary path and are stored and counted on c, so they surface through HasErrors()/Join() as expected.

Update tests:

- Replace TestCollected_CollectSkipsCollectedError / TestCollected_CollectSkipsWrappedCollectedError with positive assertions that foreign (wrapped) collected errors are preserved.

- TestCollected_CrossSubtreeCollection still passes: same-tree siblings retain the no-double-count behavior.
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJul 7, 2026, 9:55 AM

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.02%. Comparing base (9c744b8) to head (ddf484c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   74.95%   75.02%   +0.06%     
==========================================
  Files          88       88              
  Lines        9408     9416       +8     
==========================================
+ Hits         7052     7064      +12     
+ Misses       1784     1782       -2     
+ Partials      572      570       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Kybxd Kybxd requested a review from wenchy July 7, 2026 02:45
Add tests covering the previously-uncovered lines in Collector.Collect's
same-tree short-circuit and the sameTreeAs helper:

- CollectSameTreeWrappedCollectedRecordsOuterWM: a WrapKV'd same-tree
  collected error records its outer withMessage on the origin collector
  (lines 87-90) and Join() re-applies the fields.
- CollectSameTreeCollectedWhenFullReturnsJoin: when the receiver is full,
  collecting a same-tree collected marker returns Join() immediately
  instead of nil (line 93).
- SameTreeAs_Nil: sameTreeAs returns false for nil receiver/argument
  (line 147), the branch unreachable from Collect's nil guards.

Patch coverage: 57.1% -> 100% of changed lines.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Kybxd Kybxd merged commit c9a050a into master Jul 7, 2026
9 checks passed
@Kybxd Kybxd deleted the fix-err-collector branch July 7, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants