fix: make clone() copy its rects instead of sharing them - #79
Conversation
`MaxRectsBin.clone()` and `OversizedElementBin.clone()` returned a bin holding the **same** rect
objects as the source, so the two bins were never really separate: `clone.rects[0].width = 999`
changed the source bin's rect too, and the source's `rects` then described a rect the bin had not
placed. Measured on both classes before this change:
clone.rects[i] === bin.rects[i] [true, true, true, true]
clone.rects[0].width = 999 -> source width 999
clone.tag (source tag "one") undefined
`MaxRectsBin` also dropped the source's `tag` and `data`, so a clone was not even a faithful copy of
the bin's metadata. A copy operation that hands out aliases is what made this surprising; the only
in-library caller is `addArray()`'s tag-grouping probe, which itself wants a bin that cannot touch
the one it is probing.
- `Rectangle.Clone(rect)` (new static) copies a rect without going through its setters: the prototype
and every own property are kept, so a `Rectangle` stays a `Rectangle`, a custom rect class keeps its
identity and extra fields, and a plain `{width, height}` object stays plain. The copy is shallow, so
the object in `rect.data` is shared — documented on the static, on both `clone()`s and in
`AGENTS.md` (invariant 8).
- `MaxRectsBin.clone()` re-packs the copies and carries `tag`/`data` over;
`OversizedElementBin.clone()` copies its rect and carries `tag` over.
- The published declaration of the new static is pinned by the type fixture in
`scripts/verify-package.mjs`; narrowing `Clone` back to `(rect: IRectangle): IRectangle` fails the
gate with the fixture line as the only error.
The copy is still produced by re-packing rather than by a state copy, and that replay had to stay
faithful: with the rects now being copies, a replayed input that differed from the original — a rect
whose dimensions rotation had already swapped, say — would land elsewhere. Measured before writing
the specs: identical placements on a rotation-heavy fixture and on 80 seeded bins (30 rects each, both
rotation modes). `test/maxrects-bin.spec.js` now pins one rotated fixture and sweeps the 80 seeded
bins, so the property is a gate rather than a measurement.
Behaviour change to call out: code that relied on mutating a rect through one bin to reach the other
stops working. Read-only uses (cloning a bin to test whether something still fits) see the same
placements as before. Verified against the old implementation: the six new/rewritten assertions fail
there — identity on both classes, cross-bin mutation, custom-class identity, and the missing `tag`.
Tests 98 -> 105 (2 skipped), coverage still 100% (441/441 statements, 340/340 branches, 76/76
functions, 397/397 lines).
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comments Outside DiffThese findings could not be posted inline.
|
Two failures from the review of the previous commit, both reproduced first.
**A tagged bin cloned empty.** With `exclusiveTag` in play, `place()` refuses a rect whose tag does
not match the bin's, and the copy was built before it had the source's tag — so cloning a tagged bin
handed back a bin with no rects at all. Measured on the previous commit: a 1024x1024 bin holding one
tagged rect reports `source rects 1, clone rects 0`. Also measured on `HEAD~1`, i.e. before this
branch: the same, so the ordering has been wrong for as long as the tag gate has been in `place()`.
The obvious repair — put the tag on the copy first — breaks the mirror case, which the existing
"copies the tag and the data" spec covers: a bin tagged *after* it was filled holds untagged rects,
and the gate then refuses those. So the replay now runs with the tag gate **off**
(`{...this.options, exclusiveTag: false}`) and `options`, `tag` and `data` are restored from the
source afterwards. The source already settled which rects it accepts; re-deciding that while copying
is what dropped them. Both orderings are pinned by specs.
**A rect class with ECMAScript `#private` fields could not be copied.** Such a rect can be packed —
`add()` stores `x`/`y`/`rot` as own properties and reads `width`/`height` through getters — but
`Object.assign` cannot reach a private slot, so the copy threw
`TypeError: Cannot read private member #width from an object whose class did not declare it` deep
inside the replay. `Rectangle.Clone` now uses the rect's own `clone()` method when its class provides
one, and otherwise reports the limitation where the copy is made, naming the fix, instead of leaving
a TypeError to surface from a bin that is only trying to place it. Sharing the rect instead of
copying it was the other option and is exactly the aliasing this branch removed.
**Third finding, from measuring the above:** the replay silently dropped a rect it could not place.
A placed rect the caller resizes out of its bin (a 100x100 rect taken to 4000 wide in a 256x128 bin)
made `clone()` hand back a bin holding fewer rects than the original, with nothing said. That now
throws and asks for a repack.
All four new assertions fail on the previous commit with the symptoms above (`expected [] to have a
length of 1 but got +0`, the private-member TypeError, the unmatched error message, and the missing
throw); all pass here. Tests 105 -> 109 (2 skipped), coverage still 100% (452/452 statements, 344/344
branches, 76/76 functions, 407/407 lines).
soimy
left a comment
soimy
left a comment
There was a problem hiding this comment.
复核最新 HEAD e0264d98a4c02b5d90310e1f790a1d5e3fcbd6d7 后，我认为前一轮两个已知 P1 都已经针对其具体 reproducer 修复，但目前仍有 1 个复制语义上的阻塞点，建议修复后再合并。
Blocking: Rectangle.Clone() 还没有完整兑现“保留 every own property / custom rect extra fields”的契约
当前实现：
const copy = Object.assign(Object.create(Object.getPrototypeOf(rect)), rect) as T;只会复制 enumerable own properties，并不会复制所有 own properties。这样会产生两个问题：
-
non-enumerable own state 会被静默丢失。
一个合法的自定义IRectangle如果用 non-enumerable own property 保存额外字段，clone 后这些字段不会存在；如果 width/height 的 backing fields 本身是 non-enumerable，copy.width甚至可能变成undefined，随后被误报成 “rect can no longer place”。 -
当前 private-state 防护只探测
width/height。
Rectangle.Clone()只在这里主动读取：void copy.width; void copy.height;
因此一个可以正常
add()的类，如果 width/height 是普通字段，但x/y、rot或data由 ECMAScript#private字段支持，就会通过这里，然后在MaxRectsBin.clone()replay 时以原生 private-brandTypeError崩掉，而不是得到这里承诺的 actionable cloning-contract error。
也就是说，Greptile 原来的 private-state finding 对 #width/#height 这个具体 reproducer 已经修复，但一般性 private-state / own-property 复制问题还没有完全封住。
建议的最小修复：
- 用
Object.getOwnPropertyDescriptors()+Object.defineProperties()真正复制全部 own properties，而不是Object.assign()。 - 增加一个 non-enumerable backing/extra-field 的回归测试。
- 增加一个 private
x/y（无自定义clone()）的回归测试，确保失败统一变成清晰的 “give its class a clone() method” 语义，而不是底层TypeError。
已确认通过
exclusiveTagreplay 现在关闭 gate，随后恢复 source 的options/tag/data，覆盖了“先打 tag”和“后打 tag”两种场景。- replay 无法重新放置被外部修改到不合法尺寸的 rect 时会显式 throw，不再静默返回不完整 clone。
- rotated fixture + 80 seeded bins 覆盖了 re-pack clone 的 placement 一致性。
- 最新 CI 在 Node 20 / 22 / 24 全绿；每组都执行并通过了 lint、format、typecheck、coverage 和
verify:package。 verify:package实际覆盖 bundler / node16 / nodenext 三种 TypeScript resolution mode。
Non-blocking notes
- Coverage workflow 的 threshold 是 99/98/99/99；绿色 CI 能证明 coverage gate 通过，但本身并不机械证明 PR 描述中的精确 100% 计数。
verify:package目前验证了Rectangle.Clone(new Rectangle())的发布类型，但没有用带额外字段的 subtype 明确钉住Clone<T>(rect: T): T的泛型保留；可作为后续加强，不必单独阻塞。
修掉上面的 own-property/private-state 边界并补两条小测试后，我没有看到其他真实的合并阻塞项。
Review of e0264d9 found the copy contract was only half kept: `Object.assign` copies *enumerable* own properties, so anything a rect keeps non-enumerable was silently dropped. Measured on that commit, a rect class with non-enumerable `width`/`height` did not merely lose fields — the copy read `width: undefined`, and cloning its bin blew up with `RangeError: Maximum call stack size exceeded` after 162s of growth probing, i.e. the review's "misreported as a rect it can no longer place" case was in practice a stack overflow. The private-state guard was also size-only: a class with plain `width`/`height` and `#private` `x`/`y` passed the guard and then died inside the replay with `Cannot write private member #x to an object whose class did not declare it`, instead of the actionable error this branch promises. - The copy now takes `Object.getOwnPropertyDescriptors()` and applies them with `Object.defineProperties()`, so non-enumerable fields — extra ones and the backing fields behind `width`/`height` — arrive intact. - The guard reads every field the replay touches (`width`, `height`, `x`, `y`, `rot`, `data`), not just the size, so the private-state failure is the documented "give its class a clone() method" one. - A class with its own `clone()` is now only the *base* of the copy: the original's own property descriptors are applied on top. Review also flagged that a `clone()` rebuilding just the dimensions (the tempting implementation) dropped the payload, the tag and everything added since construction — `clone.rects[0].data` came back `undefined`, and the same applied to an `OversizedElementBin`'s payload. That is fixed by the same descriptor pass. Verified against e0264d9 with the three new specs, which fail there with exactly the symptoms above: `copies non-enumerable own properties` (RangeError), `reports a rect whose placement is out of reach` (private-brand TypeError instead of the message), `keeps what a narrow custom clone() leaves out` (`expected undefined to be { sheet: 'atlas-1' }`). Also pinned the generic in the published types, as the review suggested: the `verify:package` fixture now copies a `SheetRect extends Rectangle` carrying an extra field, and narrowing `Clone` to `(rect: IRectangle): IRectangle` fails the gate with two errors before the fix is restored. Tests 109 -> 112 (2 skipped), coverage still 100% (455/455 statements, 344/344 branches, 76/76 functions, 411/411 lines).
|
Blocking item fixed in c8decb2, both parts, plus the two non-blocking notes. Own-property contract. Private-state guard. It now reads every field the replay touches — Non-blocking note 2 (generic preservation in the published types) is done too: the On note 1 (coverage thresholds) you are right that green CI does not prove the exact numbers. They come from a local One extra case came out of this round, from Greptile's second finding rather than from yours: with a placed rect resized beyond its bin, |
soimy
left a comment
soimy
left a comment
There was a problem hiding this comment.
复审最新 HEAD c8decb258bd145fc8af04a76c62f85367ea61257 后，上次 review 提出的阻塞项已经修复到位：
Rectangle.Clone()已改为用Object.getOwnPropertyDescriptors()+Object.defineProperties()复制全部 own properties，包括 non-enumerable state。- 新增测试覆盖了 non-enumerable backing/extra fields。
- private
x/y场景现在会得到明确的give its class a clone() method错误，而不是底层 private-brandTypeError。 - 自定义
clone()现在只负责提供具有正确 private brand 的基础对象，原 rect 的 payload / placement / extra fields 会重新覆盖，因此 Greptile 的 “custom clones lose metadata” finding 已解决。 verify:package已用SheetRect extends Rectangle的额外字段真正钉住Clone<T>(rect: T): T的泛型保持。- 最新 Node 20 / 22 / 24 CI 全部通过 lint、format、typecheck、coverage 和
verify:package。
不过当前仍有 1 个真实阻塞风险：
Blocking: addArray() 的 clone probe 会把单个不可克隆 bin 的错误升级成整个 packing 操作失败
在 non-exclusive tag 路径中：
let targetBin = this.bins.slice(this._currentBinIndex).find((bin) => {
let testBin = bin.clone();
...
});现在 MaxRectsBin.clone() 有意会在无法得到完整副本时 throw。这对显式调用 bin.clone() 是合理的，但这里的 clone 只是 addArray() 用来判断“当前 tag group 是否适合这个 bin”的内部探针。
因此只要某个候选 bin 无法 clone，异常就会直接中断整个 addArray()，后面的 bin 不再被尝试，即使其中某个 bin 完全能够容纳当前 group。
这不只覆盖“调用者把已放置 rect resize 到超出 bin”的不一致状态。此次 PR 新增的 clone contract 本身也能触发：一个带 #private 状态、没有自定义 clone() 的 rect 仍可以正常被 add() 接受，但其所在 bin 无法 clone；之后走 non-exclusive-tag addArray() 时，这个 bin 就会把 clone API 的限制泄漏成整个 addArray() 的限制。
建议的最小修复： 在 probe 位置捕获 bin.clone() 的失败，并把该 bin 当作“当前 group 不适合这个候选 bin”，返回 false 让 .find() 继续检查后面的 bin；如果没有现有 bin 可用，再沿现有逻辑创建新 bin。
建议同时补一条回归测试：
- 第一个候选 bin 的
clone()会失败； - 第二个 bin 能容纳当前 tag group；
addArray()不 throw；- group 最终进入第二个 bin。
修掉这一点并让 CI 再绿一次后，我没有看到其他需要继续阻塞合并的问题。
The grouping probe in non-exclusive tag mode (`addArray()`) copies each candidate bin. `clone()` now
throws rather than returning an incomplete copy, and the probe had no handler, so one uncopyable bin
aborted the entire call: the later bins were never tried even when one could hold the group, and the
rects after the current group were left unpacked.
Reachable in two ways, both real:
- a placed rect the caller resized out of its bin, which `clone()` reports on purpose;
- a rect class whose state a copy cannot reach (an ECMAScript `#private` field, no `clone()` of its
own): such a rect packs fine through `add()`, but its bin cannot be copied, so the clone API's
limitation leaked into the packing call.
The probe now treats a bin that cannot be copied as one the group does not fit — `catch { return
false }` — so the search continues and the existing "no bin fits, open a new one" path handles the
rest. Calling `clone()` directly still throws, with its own message.
Spec `does not let an uncopyable bin abort the tag grouping` covers the reported shape: the first bin
holds a rect whose class hides its size behind `#private` fields, the second bin has room, and the
group has to land there. On the previous commit it fails with
`Error: Rectangle.Clone(): the rect keeps its state out of reach of a copy …` escaping `addArray()`;
here the call completes, the packer keeps two bins and the group's two rects join the second one.
Tests 112 -> 113 (2 skipped), coverage still 100% (457/457 statements, 344/344 branches, 76/76
functions, 413/413 lines).
|
Fixed in 9d6ad95, exactly as suggested: the probe catches the Your second reachability condition was the one I had underweighted when I asked about this last round: a rect class with On 9d6ad95's parent it fails with the clone error escaping Tests 113 passed / 2 skipped, coverage still 100% (457/457 statements, 344/344 branches, 76/76 functions, 413/413 lines), and |
soimy
left a comment
soimy
left a comment
There was a problem hiding this comment.
复审最新 HEAD 9d6ad9525287e1f137ecdb002205edb7ae4a8188 后，上次剩余的阻塞项已经正确修复，目前未发现新的合并阻塞风险。
最终结论：建议合并
本轮增量只涉及 addArray() 的 speculative clone probe，处理方式是正确的：
let testBin: Bin<T>;
try {
testBin = bin.clone();
} catch {
return false;
}这个边界保持得很好：
- 直接调用
bin.clone()时，无法产生完整副本仍然会明确 throw，不会重新引入静默丢 rect 的问题。 - 只有当
addArray()把 clone 用作候选 bin 的内部 fit probe 时，clone failure 才被解释为“这个 bin 不适合当前 group”，随后继续搜索后面的 bin。 - catch 范围只包住
bin.clone()，不会吞掉后续真实 packing 路径中的异常。
新增回归测试也覆盖了上一轮要求的关键场景：
- 第一个 bin 持有
#privatestate、无自定义clone()的 rect，直接bin.clone()会失败； - 第二个 bin 有足够空间；
- non-exclusive-tag
addArray()不再 throw； - 当前 group 最终进入第二个 bin；
- 第一个 bin 保持原状态。
这个测试比只验证“失败后新开一个 bin”更强，因为它实际证明了候选搜索会继续到后续 bin。
CI / 回归状态
最新 HEAD 的 GitHub Actions 已全部完成，Node 20.x / 22.x / 24.x 三组均为 success，每组都通过：
npm cinpm run lintnpm run format:checknpm run typechecknpm run covernpm run verify:package
此前几轮确认的其他风险也没有被本轮重新打开：own-property descriptor copy、non-enumerable state、private-state cloning contract、custom clone() metadata preservation、exclusiveTag replay、rotation placement replay，以及 bundler / node16 / nodenext 发布类型验证都保持成立。
基于当前 9d6ad95，我没有看到还需要继续阻塞合并的问题。
| typeof copier === "function" ? copier.call(rect) : (Object.create(Object.getPrototypeOf(rect)) as T); | ||
| // Descriptors rather than `Object.assign`: a rect may keep extra fields, or the backing fields | ||
| // behind `width`/`height`, non-enumerable, and assignment would drop those silently. | ||
| Object.defineProperties(copy, Object.getOwnPropertyDescriptors(rect)); |
There was a problem hiding this comment.
If a custom rectangle’s clone() returns independent coordinate accessors, this line replaces them with accessors tied to the original rectangle. Repacking the cloned bin can then move a rectangle in the source bin, breaking clone isolation. A valid custom clone with non-configurable accessors instead throws when those properties are redefined.
Artifacts
Custom rectangle and bin clone reproduction script
- The executed script loads Rectangle.Clone from either revision and runs identical bin-clone, repack, and nonconfigurable-accessor checks, showing the exact test input.
Bin clone behavior before the change
- Running the script with Rectangle.Clone from e0264d9 left the original at y=0 and cloned successfully with nonconfigurable accessors, establishing the baseline.
Bin clone behavior with the current source
- Running the same script with the current Rectangle.Clone moved the original to y=3 and threw on nonconfigurable accessors, confirming the defect.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/geom/Rectangle.ts
Line: 86
Comment:
**Cloned rectangles change originals**
If a custom rectangle’s `clone()` returns independent coordinate accessors, this line replaces them with accessors tied to the original rectangle. Repacking the cloned bin can then move a rectangle in the source bin, breaking clone isolation. A valid custom clone with non-configurable accessors instead throws when those properties are redefined.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
The Reproduced against that commit before touching anything:
Three specs pin it, all failing on |
1 participant
Closes the last code item in
DEFERRED_WORK.md:clone()promised a copy and handed out aliases.The defect, measured on both classes
Mutating a rect through a clone changed the source bin's rect, after which the source's own
rectsdescribed a rect the bin had never placed.MaxRectsBin.clone()additionally droppedtaganddata, so the copy was not even faithful on metadata. The only caller inside the library isaddArray()'s tag-grouping probe, which specifically wants a throwaway bin that cannot reach the one it probes.The fix
Rectangle.Clone(rect)(new static) copies a rect without going through its setters: prototype and every own property are kept, so aRectanglestays aRectangle, a custom rect class keeps its identity and its extra fields, and a plain{width, height}object stays plain. The copy is shallow — the object inrect.datais shared. Documented on the static, on bothclone()s and inAGENTS.md.MaxRectsBin.clone()re-packs copies and carriestag/data;OversizedElementBin.clone()copies its rect and carriestag.verify:packagetype fixture: narrowing it back to(rect: IRectangle): IRectanglefails the gate, with the fixture line as the only error.Making sure the replay stayed faithful
The copy is still built by re-packing rather than by copying internal state, and that replay now feeds the copies. A replayed input that differed from the original — a rect whose dimensions rotation had already swapped, for instance — would land somewhere else. Measured before writing any specs: identical placements on a rotation-heavy fixture and on 80 seeded bins (30 rects each,
allowRotationoff and on, 0 divergences). That is not left as a claim:test/maxrects-bin.spec.jspins one rotated fixture and sweeps the 80 seeded bins.Review round 1 — two P1s, both real, both fixed (
e0264d9)A tagged bin cloned empty. With
exclusiveTagin play the copy was built before it carried the source's tag, soplace()refused every rect. Measured on the previous commit:source rects 1, clone rects 0; measured onHEAD~1too, so it predates this branch. Setting the tag first breaks the mirror case (a bin tagged after it was filled holds untagged rects, and the gate refuses those — an existing spec caught it), so the replay now runs with the gate off andoptions/tag/dataare restored from the source afterwards.A rect class with ECMAScript
#privatefields could not be copied.Object.assigncannot reach a private slot:TypeError: Cannot read private member #width from an object whose class did not declare it.Rectangle.Clonenow uses the rect's ownclone()when its class has one, and otherwise reports the limitation where the copy is made, naming the fix. Copying genuinely cannot work for unreachable state (structuredClonedrops the prototype), and falling back to sharing would be the aliasing this PR removes.Third case, found while measuring: the replay silently dropped a rect it could not place — resize a placed 100x100 rect to 4000 in a 256x128 bin and the copy came back with fewer rects than the original, silently. That throws now.
All four new assertions fail on the previous commit with those exact symptoms. Tests 109 passed / 2 skipped; coverage still 100% (452/452, 344/344, 76/76, 407/407).
Verified
tag(expected Rectangle{…} not to be Rectangle{…},expected 999 to be 100,expected undefined to be 'one').clone.rects[0]is not the source object, the source's rects are untouched in both directions, adding to one bin does not grow the other, custom class instances keep prototype + extra fields,tag/dataare carried over, and the copied bin reproduces the source placements.npm test109 passed / 2 skipped (was 98);coverstill 100% on statements, branches, functions and lines (452/452, 344/344, 76/76, 407/407, no gap left in any file);lint0/0,format:check,typecheckandverify:package(bundler, node16, nodenext) green.Behaviour change to note
Any consumer that relied on mutating a rect through one bin to reach the other loses that path — that aliasing is the defect. Read-only use, such as cloning a bin to test whether something still fits, sees the same placements as before (measured). I typed this
fix:rather thanfix!because no signature changed and the read-only results are identical; retitle with!if you would rather have the release treat it as breaking.