Skip to content

fix(runtime): clear data attributes after property delete - #10984

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10840-descriptor-readd
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10840-descriptor-readd

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

A successful ordinary-object delete checked a data descriptor's configurable bit, then removed the key without clearing its attribute side-table entry. A later assignment to the same name still saw writable: false and failed to recreate the property.

Clear the attribute entry after accepting a configurable data-property delete. This happens before the tombstone or compaction path, so both paths leave the name free for a later assignment. Refused deletes still return before clearing anything.

Verification

  • Added a regression test that failed on unmodified main because the read-only descriptor survived a successful delete. It now passes in both tombstone and compacting modes, with both enumerable and non-enumerable starting descriptors; it checks the recreated value and Object.keys order.
  • Added a refused-delete case confirming a non-configurable descriptor remains intact.
  • cargo test -p perry-runtime --lib delete -- --test-threads=1: 52 passed.
  • cargo test -p perry-runtime --lib object::descriptor_state -- --test-threads=1: 11 passed.
  • rustfmt --check and git diff --check: passed.

Closes #10840.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed deletion of configurable, non-writable properties so their previous attribute restrictions no longer affect the property when it is recreated.
    • Re-added properties now correctly support writable and enumerable behavior and preserve the expected key order.
    • Deletion of non-configurable properties continues to be refused without changing their descriptors.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ac7cad7a-7c31-4ec1-8df7-dc545979ff7d

📥 Commits

Reviewing files that changed from the base of the PR and between 62af46f and c0c3c01.

📒 Files selected for processing (2)
  • changelog.d/10984-delete-data-attributes.md
  • crates/perry-runtime/src/object/delete_rest.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d47d0437-774c-4173-8a62-808dce7626ba

📥 Commits

Reviewing files that changed from the base of the PR and between c1569e2 and 62af46f.

📒 Files selected for processing (2)
  • changelog.d/10984-delete-data-attributes.md
  • crates/perry-runtime/src/object/delete_rest.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The delete path now clears stored attributes for configurable properties before removal. Tests verify that deleted read-only properties can be recreated and that refused deletion preserves non-configurable descriptors. A changelog entry documents the fix.

Changes

Property deletion and recreation

Layer / File(s) Summary
Delete attribute cleanup and validation
crates/perry-runtime/src/object/delete_rest.rs, changelog.d/10984-delete-data-attributes.md
js_object_delete_field clears stored property attributes after a configurable delete. Tests cover property recreation across tombstone modes and enumerable values, plus preservation after a refused non-configurable delete. The changelog documents the behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 62af4

Configurable deleted properties can now be recreated normally, while refused non-configurable deletes remain intact. The covered behavior is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main runtime fix: clearing data attributes after property deletion.
Description check ✅ Passed The description explains the defect, implementation, regression coverage, verification commands, and linked issue. It does not use all template headings and omits the checklist, but it provides the ma…
Linked Issues check ✅ Passed The change addresses #10840. js_object_delete_field clears stored attributes after the configurable check and before removal on both tombstone and compacting paths. This removes the stale non-writab…
Out of Scope Changes check ✅ Passed The changes stay within #10840. They modify the delete runtime path, add focused regression tests, and add a changelog entry for the fix. These changes support the linked issue and do not show unrelat…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 …
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Not in merge train 257 (#11039): the cherry-pick conflicts with current main in crates/perry-runtime/src/object/delete_rest.rs.

Worth knowing before you resolve it: that file is under a raw-handle ceiling of 2 (scripts/raw_handle_debt.py), and a sibling PR (#11023) was also held from this train for taking it to 26. So resolve toward the current main version and keep new raw-handle sites out, or the rebase will land you in the same gate.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 265 (#11108), released as v0.5.1648 at 9d26936298.

Cherry-picked from this PR's head c0c3c01902 and validated as one tree with 15 other PRs — CI 22/22 green, all 6 gap-suite shards. A train rebase gives the commits new SHAs, so GitHub cannot auto-close the source PR; closing by hand.

Nothing needed from you. Thanks.

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.

delete of a NON-WRITABLE data property leaves the key un-re-addable: the next o.a = 9 is silently dropped (pre-existing on v0.5.1618)

1 participant