Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
|
@lavenzg has exported this pull request. If you are a Meta employee, you can view the originating Diff in D123943189. |
…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
changed the title
lavenzg
force-pushed
the
export-D123943189
branch
from
October 7, 2026 21:45
ff67900 to
f610edd
Compare
meta-codesync Bot
pushed a commit
to facebook/hermes
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 facebook/hermes
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 facebook/hermes
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
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
2 participants
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" (590396bd4 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 (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 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.X-link: facebook/hermes#2205
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
Reviewed By: avp
Differential Revision: D123943189
Pulled By: lavenzg