Skip to content

[RNE Rewrite] feat: add input validation and JSI conversion utilities#1296

Open
barhanc wants to merge 21 commits into
rne-rewritefrom
@bh/issue-1270
Open

[RNE Rewrite] feat: add input validation and JSI conversion utilities#1296
barhanc wants to merge 21 commits into
rne-rewritefrom
@bh/issue-1270

Conversation

@barhanc

@barhanc barhanc commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Description

Refactors the C++ layer by pulling two families of scattered, copy-pasted utilities out of individual translation units and into purpose-built, reusable modules:

  • cpp/core/conversions.{h,cpp} JSI -> C++ type coercions
  • cpp/core/tensor_helpers.{h,cpp}: tensor acquisition, locking, and shape/dtype validation

Motivation

The old code had two recurring problems that became impossible to ignore as the extension surface grew:

JSI coercion was inlined everywhere

Every file that needed to read a number, string, or array out of a jsi::Value had its own copy of the same defensive checks, cast chains, and error-message construction. A bug or policy change (e.g. how null is treated, or which format an error message takes) had to be hunted down and patched in every translation unit independently.

Tensor validation was duplicated across all extension operations

Every operation in box_ops, image_ops, operations, and tokenizer that accepted a Tensor argument re-implemented the same sequence: unwrap the jsi::Value, cast to TensorHostObject, acquire the appropriate
shared/exclusive lock, and assert dtype + shape constraints. This led to subtle inconsistencies in error messages and locking strategies between operations.

Introduces a breaking change?

  • Yes
  • No

Type of change

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Documentation update (improves or adds clarity to existing documentation)
  • Other (chores, tests, code style improvements etc.)

Tested on

  • iOS
  • Android

Testing instructions

  • Build all example apps on both Android and iOS and test that nothing is broken.

Screenshots

Related issues

Closes #1270
Closes #1268

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Additional notes

Since this PR changes quite a lot of stuff in core/ it can also be used for #1268

@barhanc barhanc self-assigned this Jul 1, 2026
@barhanc

barhanc commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@msluszniak This is still a draft and there are still quite some rough edges to polish, but could you take a look if this approach is something you would agree on and if there are any other aspects I missed. I was thinking that we could also use this PR for #1268 since adding the validation/conversion helpers modifies quite a lot of core code so it would be good to review and streamline also the other aspects.

@barhanc barhanc requested a review from msluszniak July 1, 2026 15:09

@msluszniak msluszniak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like this concept. I combines elegance of short macros with flexibility of manual code. Just very small nits, will dive into it later today / tomorrow. I like usage of std::format, we previously tried use it in old codebase, but because it required some version of iOS we eventually dropped it, but I don't think it will be a case now.

Comment thread packages/react-native-executorch/cpp/core/conversions.cpp
Comment thread packages/react-native-executorch/cpp/core/tensor_helpers.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor_helpers.cpp Outdated

@msluszniak msluszniak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another batch of comments.

Comment thread packages/react-native-executorch/cpp/core/model.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor_helpers.cpp Outdated
Comment thread packages/react-native-executorch/cpp/core/tensor_helpers.cpp Outdated
@barhanc barhanc requested a review from msluszniak July 2, 2026 16:57
@barhanc

barhanc commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

The core/ changes are ready for the next pass. Later I will also polish the code in extensions/.

@barhanc barhanc marked this pull request as ready for review July 3, 2026 00:24
@barhanc barhanc linked an issue Jul 3, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RNE Rewrite] Add macros for input validation in native C++ implementation [RNE Rewrite] Recheck and streamline the core native implementation

2 participants