Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jstz_engine): implement JsString and JsStr wrappers #740

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnyob
Copy link
Collaborator

@johnyob johnyob commented Jan 6, 2025

Context

Related Tasks: jstz-214

Description

Dependencies: #739

Adds type definitions for JsString and JsStr.
Additionally wraps most (if not all) functions that operate on strings.

Manually testing the PR

@johnyob johnyob force-pushed the ajob410@jstz-265-jsvalue-type-def branch from f92c260 to fe9eb18 Compare January 6, 2025 17:15
@johnyob johnyob force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch 2 times, most recently from e053775 to 072cbc2 Compare January 7, 2025 12:59
@johnyob johnyob requested a review from zcabter January 7, 2025 13:05
@johnyob johnyob self-assigned this Jan 7, 2025
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 80.25210% with 94 lines in your changes missing coverage. Please review.

Project coverage is 46.83%. Comparing base (b776228) to head (ce2b057).

Files with missing lines Patch % Lines
crates/jstz_engine/src/string/mod.rs 88.63% 44 Missing ⚠️
crates/jstz_engine/src/string/str.rs 48.75% 39 Missing and 2 partials ⚠️
crates/jstz_engine/src/gc/ptr.rs 0.00% 9 Missing ⚠️
Files with missing lines Coverage Δ
crates/jstz_engine/src/lib.rs 65.51% <ø> (ø)
crates/jstz_engine/src/gc/ptr.rs 72.29% <0.00%> (+1.79%) ⬆️
crates/jstz_engine/src/string/str.rs 48.75% <48.75%> (ø)
crates/jstz_engine/src/string/mod.rs 23.93% <88.63%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b776228...ce2b057. Read the comment docs.

@zcabter zcabter force-pushed the ajob410@jstz-265-jsvalue-type-def branch from fe9eb18 to 0a6202d Compare February 5, 2025 15:29
Base automatically changed from ajob410@jstz-265-jsvalue-type-def to main February 5, 2025 15:36
@zcabter zcabter force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch from 072cbc2 to a33ec7b Compare February 5, 2025 15:59
@zcabter zcabter assigned zcabter and unassigned johnyob Feb 5, 2025
@zcabter zcabter force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch from a33ec7b to d929480 Compare February 11, 2025 17:45
@zcabter zcabter marked this pull request as ready for review February 11, 2025 18:01
@zcabter zcabter force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch from d929480 to e4b8240 Compare February 11, 2025 18:21
@zcabter zcabter assigned johnyob and unassigned zcabter Feb 11, 2025
@zcabter
Copy link
Collaborator

zcabter commented Feb 11, 2025

Need review from @johnyob

}

/// Obtains a slice of a [`JsString`] as a [`JsStr`].
pub fn as_str<'cx, S>(&self, cx: &'cx Context<S>) -> JsStr<'cx>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want &'cx Context<S> or &'cx mut Context<S>? The idea behind &mut Context<S> in jstz_engine is that it is a capability for doing anything with SpiderMonkey. But we could be more permissive and have &Context<S> => no gc and &mut Context<S> => gc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually having looked at the code for the JS_* functions here, we do in fact allocate if the string are not linear 😔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated to mut Context

crates/jstz_engine/src/string/mod.rs Show resolved Hide resolved

impl<'a, C: Compartment> JsString<'a, C> {
/// Creates a new empty [`JsString`]
pub fn empty<S>(cx: &'a Context<S>) -> Self
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pub fn empty<S>(cx: &'a Context<S>) -> Self
pub fn empty<S>(cx: &'a mut Context<S>) -> Self

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

Comment on lines 251 to 231
pub fn code_point_at<'cx, S>(&self, index: usize, cx: &'cx Context<S>) -> u16
where
S: InCompartment<C> + CanAccess,
'cx: 'a,
{
unsafe {
let mut char = 0;
JS_GetStringCharAt(cx.as_raw_ptr(), self.as_raw_ptr(), index, &mut char);
char
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately code_point_at can allocate.

If passed a non-linear string, SpiderMonkey will use ensureLinear, which will allocate an equivalent linear string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated to mut Context

S: InCompartment<C> + CanAlloc,
{
unsafe {
/* https://searchfox.org/mozilla-central/source/js/public/String.h#54
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should refer to the mozjs SpiderMonkey code -- the version hosted here differs from the one we use

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated

Comment on lines 286 to 260
pub fn equals<S>(
s1: &JsString<'_, C>,
s2: &JsString<'_, C>,
cx: &'_ Context<S>,
) -> Option<bool>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similarly, equality can allocate since it will implicitly allocate linear strings if given a non-linear string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated to mut Context

@zcabter zcabter assigned zcabter and unassigned johnyob Feb 12, 2025
@zcabter zcabter force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch 2 times, most recently from 1af8aee to 91cce29 Compare February 12, 2025 21:58
@zcabter
Copy link
Collaborator

zcabter commented Feb 12, 2025

As per our offline discussion, we have taken the decision to always use mut Context whenever we call SM as a preventative measure to not accidentally enter UB land. By using mut Context and enforcing that 'cx:'a callers will be forced to root context created values before re-using the context.

@zcabter zcabter assigned johnyob and unassigned zcabter Feb 12, 2025
@zcabter zcabter force-pushed the ajob410@jstz-214-impl-jsstring-and-jsstr branch from 91cce29 to ce2b057 Compare February 12, 2025 22:38
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