Skip to content

Fix 2 UBs, and cleanup GcRefCell#4422

Merged
hansl merged 8 commits intoboa-dev:mainfrom
hansl:fix-ub-erased-object
Sep 18, 2025
Merged

Fix 2 UBs, and cleanup GcRefCell#4422
hansl merged 8 commits intoboa-dev:mainfrom
hansl:fix-ub-erased-object

Conversation

@hansl
Copy link
Contributor

@hansl hansl commented Sep 18, 2025

  • Empty enums are uninhabited types and referencing them is UB.
  • Removed the ref in GcRefMut<> and GcRef<> in favor of NonNull<> for good hygiene.
  • Simplified the type of GcRefMut by having a ref to the flags instead of keeping a ref to the GcRefCell itself.
  • Unconstrained the types for Deref and DerefMut for Ref/RefMut.
  • Little things, like using assert_ne instead of assert!(... != ...), or typos in comments, etc.

- Empty enums are uninhabited types and referencing them is UB.
- Removed the ref in `GcRefMut<>` and `GcRef<>` in favor of `NonNull<>`
  for good hygiene.
- Simplified the type of `GcRefMut` by having a ref to the flags
  instead of keeping a ref to the `GcRefCell` itself.
- Unconstrained the types for `Deref` and `DerefMut` for `Ref`/`RefMut`.
@hansl hansl force-pushed the fix-ub-erased-object branch from 970281a to 1fe3793 Compare September 18, 2025 00:36
@codecov
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 61.11111% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.27%. Comparing base (6ddc2b4) to head (89e7fc7).
⚠️ Report is 530 commits behind head on main.

Files with missing lines Patch % Lines
core/gc/src/cell.rs 52.72% 26 Missing ⚠️
core/engine/src/interop/into_js_arguments.rs 0.00% 1 Missing ⚠️
core/engine/src/object/builtins/jsarraybuffer.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4422      +/-   ##
==========================================
+ Coverage   47.24%   51.27%   +4.02%     
==========================================
  Files         476      502      +26     
  Lines       46892    51291    +4399     
==========================================
+ Hits        22154    26297    +4143     
- Misses      24738    24994     +256     

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hansl hansl marked this pull request as ready for review September 18, 2025 00:52
@hansl hansl requested a review from a team September 18, 2025 00:52
@hansl hansl changed the title Cleanup on GcRefCell and fix an UB in Gc Fix 2 UBs, and cleanup GcRefCell Sep 18, 2025
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Had a couple nits and a question to start

@nekevss nekevss requested a review from a team September 18, 2025 15:09
@hansl hansl enabled auto-merge September 18, 2025 18:22
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hansl hansl added this pull request to the merge queue Sep 18, 2025
Merged via the queue into boa-dev:main with commit fdf98a6 Sep 18, 2025
16 checks passed
@hansl hansl deleted the fix-ub-erased-object branch September 18, 2025 20:04
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.

3 participants