Conversation
A reference repeated in two sibling branches is not a circular reference,
but refSet never released anything once the walk left a branch. The second
visit found it in the set, warned, and returned false for deeply-equal input:
const shared = [];
isEqual({ errors: shared, warnings: shared }, { errors: [], warnings: [] });
// false, and "Warning: There may be circular references"
A cycle is a value reachable from itself, so the set has to hold the current
path rather than the whole history. Entries are now released as the walk
unwinds. Genuine cycles are still detected, because a self-referencing value
is still its own ancestor when it is reached again.
The tests gain a resetWarned() in beforeEach: warning is warningOnce, so a
message emitted by an earlier case is suppressed in every later one, and
asserting that nothing warned would otherwise prove nothing.
Closes react-component#816
|
@savkaoleg is attempting to deploy a commit to the afc163's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughChangesisEqual 循环引用处理
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The ancestor-path cleanup and accompanying cycle/reference tests align with the intended behavior, with no remaining merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 小兔检查 refSet， Comment |
nrps9909
left a comment
nrps9909
left a comment
There was a problem hiding this comment.
Verified exact head d78c21d59e46ba44288561bb78b46fdec226d81d against its merge base 9f2ff96640a6ab760919a5b5b00fa5d4178c08f4.
An independent 4,840-case corpus covers repeated objects/arrays in both comparison directions, unequal acyclic controls, shallow/identity comparisons, and genuine cycles at 20 depths. The base has 1,600 observations with a false negative or spurious circular warning; the head passes all 4,840, while actual cycles still return false and warn. The finally cleanup also covers short-circuit returns.
Focused formatting/lint, TypeScript, and ESM/CJS/declaration compilation pass. Full exact-head tests report 199 passed, 1 skipped, and the same two failures reproduced on the base (pickAttrs event coverage and React-17 useId test isolation). Those failures are unrelated to this diff: GitHub's current merge preview a3d413d on master 993255e passes all 31 suites / 213 tests, with 1 skipped.
Upstream fork workflows still require maintainer approval; these local results do not mean remote CI has passed. The ancestor-path change looks correct to me.
AI assistance disclosure: Codex assisted with source inspection, independent probes, and validation. The exact revisions and reported outcomes were checked locally.
This branch has not been deployed
2 participants
Closes #816.
The problem
isEqualreturnsfalse, and logsWarning: There may be circular references, for two objects that are deeply equal — whenever one of them holds the same reference in two different keys:Neither object contains a cycle.
Why
refSetrecords every value the walk visits and never releases anything when the walk leaves a branch. A cycle is a value reachable from itself — a value that is its own ancestor on the current path — so the set needs to hold the path, not the history. As written, a reference legitimately reached twice in two sibling branches is indistinguishable from a cycle: the second visit finds it in the set, warns, and returnsfalse.The change
refSet.delete(a)in afinally, so entries are released as the walk unwinds. Genuine cycles are still caught, because a self-referencing value is still on the path when it is reached again.Why it shows up in practice
rc-field-form'sFieldkeeps one shared constant in two places:and
triggerMetaEventcompares the previous meta with the next throughisEqual. Once one field has validated — itserrorsbecoming a fresh[]— the comparison hits exactly the case above. The visible result in any antd app is this warning in the dev console after a programmaticform.setFieldValue(...), pointing at application code that holds no circular data, plus anonMetaChangethat fires when nothing changed.Tests
Four cases added to
src/test/isEqual.test.ts:Every test already in that file passes unchanged, including
should not equal 6, which is the existing cyclic case.false, warnstrue, silentfalse, warnstrue, silenttrue, silenttrue, silenta.self = a)false, warnsfalse, warnsfalse, warnsfalse, warnsSummary by CodeRabbit