Skip to content

Conversation

@christophpurrer
Copy link
Contributor

Summary:
Changelog: [Internal]

Change:

#35265 added a struct generator - but it is not type safe :-(

E.g. you can write a TM Spec type as:

export type CustomType = {
   key: string;
   enabled: boolean;
   time?: number;
 }

And a C++ type as:

using CustomType = NativeSampleModuleBaseCustomType<float, bool, std::optional<int32_t>>;
template <>
struct Bridging<CustomType>
    : NativeSampleModuleBaseCustomTypeBridging<float, bool, std::optional<int32_t>> {};

and it will still compile :-( - which should not.

The reason is that the generated structs don't validate the members type :-(

template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomType {
  P0 key;
  P1 enabled;
  P2 time;
  bool operator==(const NativeSampleModuleBaseCustomType &other) const {
    return key == other.key && enabled == other.enabled && time == other.time;
  }
};

template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomTypeBridging {
  static NativeSampleModuleBaseCustomType<P0, P1, P2> fromJs(
      jsi::Runtime &rt,
      const jsi::Object &value,
      const std::shared_ptr<CallInvoker> &jsInvoker) {
    NativeSampleModuleBaseCustomType<P0, P1, P2> result{
      bridging::fromJs<P0>(rt, value.getProperty(rt, "key"), jsInvoker),
      bridging::fromJs<P1>(rt, value.getProperty(rt, "enabled"), jsInvoker),
      bridging::fromJs<P2>(rt, value.getProperty(rt, "time"), jsInvoker)};
    return result;
  }

  static jsi::Object toJs(
      jsi::Runtime &rt,
      const NativeSampleModuleBaseCustomType<P0, P1, P2> &value) {
    auto result = facebook::jsi::Object(rt);
    result.setProperty(rt, "key", bridging::toJs(rt, value.key));
    result.setProperty(rt, "enabled", bridging::toJs(rt, value.enabled));
    if (value.time) {
      result.setProperty(rt, "time", bridging::toJs(rt, value.time.value()));
    }
    keyToJs(rt, value.key);
    return result;
  }
};

This fixes that, by simply emitting conversion functions for each member such as

#ifdef DEBUG
  static bool keyToJs(jsi::Runtime &rt, P0 value) {
    return bridging::toJs(rt, value);
  }
  static double enabledToJs(jsi::Runtime &rt, P1 value) {
    return bridging::toJs(rt, value);
  }
  static jsi::String timeToJs(jsi::Runtime &rt, P2 value) {
    return bridging::toJs(rt, value);
  }
#endif

Differential Revision: D42082423

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Dec 15, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42082423

@analysis-bot
Copy link

analysis-bot commented Dec 15, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,468,282 -5
android hermes armeabi-v7a 7,787,993 +5
android hermes x86 8,943,383 -16
android hermes x86_64 8,800,516 -15
android jsc arm64-v8a 9,659,508 +6
android jsc armeabi-v7a 8,392,618 +2
android jsc x86 9,723,516 +1
android jsc x86_64 10,201,024 -6

Base commit: 03b17d9
Branch: main

@analysis-bot
Copy link

analysis-bot commented Dec 15, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 03b17d9
Branch: main

@pull-bot
Copy link

PR build artifact for 95917ee is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 95917ee is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Summary:
Pull Request resolved: facebook#35656

Changelog: [Internal]

## Change:

facebook#35265 added a struct generator - but it is not type safe :-(

E.g. you can write a TM Spec type as:

```
export type CustomType = {
   key: string;
   enabled: boolean;
   time?: number;
 }
```
And a C++ type as:
```
using CustomType = NativeSampleModuleBaseCustomType<float, bool, std::optional<int32_t>>;
template <>
struct Bridging<CustomType>
    : NativeSampleModuleBaseCustomTypeBridging<float, bool, std::optional<int32_t>> {};
```
and it will still compile :-( - which should not.

The reason is that the generated structs don't validate the members type :-(
```
template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomType {
  P0 key;
  P1 enabled;
  P2 time;
  bool operator==(const NativeSampleModuleBaseCustomType &other) const {
    return key == other.key && enabled == other.enabled && time == other.time;
  }
};

template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomTypeBridging {
  static NativeSampleModuleBaseCustomType<P0, P1, P2> fromJs(
      jsi::Runtime &rt,
      const jsi::Object &value,
      const std::shared_ptr<CallInvoker> &jsInvoker) {
    NativeSampleModuleBaseCustomType<P0, P1, P2> result{
      bridging::fromJs<P0>(rt, value.getProperty(rt, "key"), jsInvoker),
      bridging::fromJs<P1>(rt, value.getProperty(rt, "enabled"), jsInvoker),
      bridging::fromJs<P2>(rt, value.getProperty(rt, "time"), jsInvoker)};
    return result;
  }

  static jsi::Object toJs(
      jsi::Runtime &rt,
      const NativeSampleModuleBaseCustomType<P0, P1, P2> &value) {
    auto result = facebook::jsi::Object(rt);
    result.setProperty(rt, "key", bridging::toJs(rt, value.key));
    result.setProperty(rt, "enabled", bridging::toJs(rt, value.enabled));
    if (value.time) {
      result.setProperty(rt, "time", bridging::toJs(rt, value.time.value()));
    }
    keyToJs(rt, value.key);
    return result;
  }
};
```

This fixes that, by simply emitting conversion functions for each member such as
```
#ifdef DEBUG
  static bool keyToJs(jsi::Runtime &rt, P0 value) {
    return bridging::toJs(rt, value);
  }
  static double enabledToJs(jsi::Runtime &rt, P1 value) {
    return bridging::toJs(rt, value);
  }
  static jsi::String timeToJs(jsi::Runtime &rt, P2 value) {
    return bridging::toJs(rt, value);
  }
#endif
```

Differential Revision: D42082423

fbshipit-source-id: c7857944ba5d5a57623fc627df4d885867b025cf
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42082423

@pull-bot
Copy link

PR build artifact for 8ce46eb is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 8ce46eb is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in c7e1e00.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Dec 16, 2022
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#35656

Changelog: [Internal]

## Change:

facebook#35265 added a struct generator - but it is not type safe :-(

E.g. you can write a TM Spec type as:

```
export type CustomType = {
   key: string;
   enabled: boolean;
   time?: number;
 }
```
And a C++ type as:
```
using CustomType = NativeSampleModuleBaseCustomType<float, bool, std::optional<int32_t>>;
template <>
struct Bridging<CustomType>
    : NativeSampleModuleBaseCustomTypeBridging<float, bool, std::optional<int32_t>> {};
```
and it will still compile :-( - which should not.

The reason is that the generated structs don't validate the members type :-(
```
template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomType {
  P0 key;
  P1 enabled;
  P2 time;
  bool operator==(const NativeSampleModuleBaseCustomType &other) const {
    return key == other.key && enabled == other.enabled && time == other.time;
  }
};

template <typename P0, typename P1, typename P2>
struct NativeSampleModuleBaseCustomTypeBridging {
  static NativeSampleModuleBaseCustomType<P0, P1, P2> fromJs(
      jsi::Runtime &rt,
      const jsi::Object &value,
      const std::shared_ptr<CallInvoker> &jsInvoker) {
    NativeSampleModuleBaseCustomType<P0, P1, P2> result{
      bridging::fromJs<P0>(rt, value.getProperty(rt, "key"), jsInvoker),
      bridging::fromJs<P1>(rt, value.getProperty(rt, "enabled"), jsInvoker),
      bridging::fromJs<P2>(rt, value.getProperty(rt, "time"), jsInvoker)};
    return result;
  }

  static jsi::Object toJs(
      jsi::Runtime &rt,
      const NativeSampleModuleBaseCustomType<P0, P1, P2> &value) {
    auto result = facebook::jsi::Object(rt);
    result.setProperty(rt, "key", bridging::toJs(rt, value.key));
    result.setProperty(rt, "enabled", bridging::toJs(rt, value.enabled));
    if (value.time) {
      result.setProperty(rt, "time", bridging::toJs(rt, value.time.value()));
    }
    keyToJs(rt, value.key);
    return result;
  }
};
```

This fixes that, by simply emitting conversion functions for each member such as
```
#ifdef DEBUG
  static bool keyToJs(jsi::Runtime &rt, P0 value) {
    return bridging::toJs(rt, value);
  }
  static double enabledToJs(jsi::Runtime &rt, P1 value) {
    return bridging::toJs(rt, value);
  }
  static jsi::String timeToJs(jsi::Runtime &rt, P2 value) {
    return bridging::toJs(rt, value);
  }
#endif
```

Reviewed By: cipolleschi

Differential Revision: D42082423

fbshipit-source-id: 5133f14e2aa8351e9bbbf614117a3d5894b17fa6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants