Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
IRuntime::length(const String&) was added in "Add `String::length` API" (D96030262) without decorator overrides, so calls on a decorated runtime fall back to the default Runtime::length, which copies the string through utf16() instead of reaching the wrapped runtime's own length(). Forward it like the other string methods, and wrap it in Around in WithRuntimeDecorator.
mrousavy
force-pushed
the
decorator-string-length
branch
from
September 26, 2026 13:16
93f8987 to
f0dab2b
Compare
|
@lavenzg has imported this pull request. If you are a Meta employee, you can view this in D123943189. |
lavenzg
pushed a commit
to lavenzg/react-native
that referenced
this pull request
Oct 7, 2026
…eact#58936) Summary: `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396bd4 on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (facebook/hermes#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. X-link: facebook/hermes#2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
…2205) Summary: X-link: react/react-native#58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396b on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. Pull Request resolved: #2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
meta-codesync Bot
pushed a commit
that referenced
this pull request
Oct 8, 2026
…2205) Summary: X-link: react/react-native#58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396b on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. Pull Request resolved: #2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
meta-codesync Bot
pushed a commit
to react/react-native
that referenced
this pull request
Oct 8, 2026
…58936) Summary: Pull Request resolved: #58936 `IRuntime::length(const String&)` came in with "Add `String::length` API" (590396bd4 on static_h, D96030262), and that commit didn't touch `decorator.h`. Neither `RuntimeDecorator` nor `WithRuntimeDecorator` overrides it, so on a decorated runtime `length()` falls back to the default `utf16(str).size()` and never reaches the wrapped runtime. It looks like an oversight. The same stack added `push`, `tryGetMutableBuffer`, `detached` and the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overload `length(const TypedArray&)`. The `createError*` commit (facebook/hermes#2203) and this one didn't. For Hermes-backed decorators the value is already right, because Hermes' `utf16()` is exact, but every call copies the whole string instead of reading the length. Runtimes with a native `length()` and the default `utf16()` get wrong answers. React Native's `JSCRuntime` is one: built against the system JavaScriptCore and wrapped in a `RuntimeDecorator`, `'a\uD800b'` has length 1 instead of 3 and `'x\uDC00yz'` has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here. `WithRuntimeDecorator` needs the override too. Otherwise it would inherit the plain forward and `length()` would skip `Around`, which it currently goes through via `utf16()`. `TracingRuntime` no longer records an incidental `Utf16Record` when `length()` is called. That record only came from the `utf16()` fallback. I didn't add a record for `length()`: strings are immutable, and `TracingRuntime` already leaves pure queries like `size(Array)` untraced. X-link: facebook/hermes#2205 Test Plan: Added `JSITest.DecoratorForwardsStringLength`. An inner decorator returns a sentinel from `length()` and counts `length()` and `utf16()` calls. The test checks that a `RuntimeDecorator` and a `WithRuntimeDecorator` over it both reach that `length()` without calling `utf16()`, and that `Around` runs once. It fails without the change (it gets 5 from the `utf16()` fallback) and passes with it. The full `APITests` binary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: avp Differential Revision: D123943189 Pulled By: lavenzg fbshipit-source-id: 519fdde9f2ad46ce906567bfd1d94d787015915c
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IRuntime::length(const String&)came in with "AddString::lengthAPI" (590396b on static_h, D96030262), and that commit didn't touchdecorator.h. NeitherRuntimeDecoratornorWithRuntimeDecoratoroverrides it, so on a decorated runtimelength()falls back to the defaultutf16(str).size()and never reaches the wrapped runtime.It looks like an oversight. The same stack added
push,tryGetMutableBuffer,detachedand the TypedArray methods, and each of those commits added decorator overrides, including one for this method's overloadlength(const TypedArray&). ThecreateError*commit (#2203) and this one didn't.For Hermes-backed decorators the value is already right, because Hermes'
utf16()is exact, but every call copies the whole string instead of reading the length. Runtimes with a nativelength()and the defaultutf16()get wrong answers. React Native'sJSCRuntimeis one: built against the system JavaScriptCore and wrapped in aRuntimeDecorator,'a\uD800b'has length 1 instead of 3 and'x\uDC00yz'has 1 instead of 4. With this change both match the undecorated runtime. That setup lives outside this repo, so it isn't a test here.WithRuntimeDecoratorneeds the override too. Otherwise it would inherit the plain forward andlength()would skipAround, which it currently goes through viautf16().TracingRuntimeno longer records an incidentalUtf16Recordwhenlength()is called. That record only came from theutf16()fallback. I didn't add a record forlength(): strings are immutable, andTracingRuntimealready leaves pure queries likesize(Array)untraced.Test Plan
Added
JSITest.DecoratorForwardsStringLength. An inner decorator returns a sentinel fromlength()and countslength()andutf16()calls. The test checks that aRuntimeDecoratorand aWithRuntimeDecoratorover it both reach thatlength()without callingutf16(), and thatAroundruns once. It fails without the change (it gets 5 from theutf16()fallback) and passes with it. The fullAPITestsbinary passes (357 tests) in a Debug + ASan build, including the SynthTrace and TraceInterpreter tests.🤖 Generated with Claude Code