remove useMapNativeAccessor stuff

Summary:
This was quite a rabit hole of remove deps -> delete dead code -> repeat.

Waaay simpler now with less duplicate lookups, redundant type verification, and extra function calls.

Reviewed By: mdvacca

Differential Revision: D14486283

fbshipit-source-id: 035db30181755d046a1ae99760468b954b2449df

ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/WritableNativeMapTest.java

@@ -3,6 +3,7 @@ package com.facebook.react.tests.core;
 import static org.fest.assertions.api.Assertions.assertThat;
 
 import androidx.test.runner.AndroidJUnit4;
+import com.facebook.react.bridge.NoSuchKeyException;
 import com.facebook.react.bridge.UnexpectedNativeTypeException;
 import com.facebook.react.bridge.WritableNativeArray;
 import com.facebook.react.bridge.WritableNativeMap;
@@ -27,7 +28,6 @@ public class WritableNativeMapTest {
     mMap.putMap("map", new WritableNativeMap());
     mMap.putArray("array", new WritableNativeArray());
     mMap.putBoolean("dvacca", true);
-    mMap.setUseNativeAccessor(true);
   }
 
   @Test
@@ -90,14 +90,13 @@ public class WritableNativeMapTest {
     mMap.getArray("string");
   }
 
-  @Ignore("Needs to be implemented")
   @Test
   public void testErrorMessageContainsKey() {
     String key = "fkg";
     try {
       mMap.getString(key);
-      Assert.fail("Expected an UnexpectedNativeTypeException to be thrown");
-    } catch (UnexpectedNativeTypeException e) {
+      Assert.fail("Expected an NoSuchKeyException to be thrown");
+    } catch (NoSuchKeyException e) {
       assertThat(e.getMessage()).contains(key);
     }
   }

ReactAndroid/src/main/java/com/facebook/react/bridge/ReadableNativeMap.java

@@ -36,20 +36,14 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
   private @Nullable HashMap<String, ReadableType> mLocalTypeMap;
   private static int mJniCallCounter;
 
-  public static void setUseNativeAccessor(boolean useNativeAccessor) {
-    ReactFeatureFlags.useMapNativeAccessor = useNativeAccessor;
-  }
-
   public static int getJNIPassCounter() {
     return mJniCallCounter;
   }
 
   private HashMap<String, Object> getLocalMap() {
-    // Fast return for the common case
     if (mLocalMap != null) {
       return mLocalMap;
     }
-    // Check and when necessary get keys atomically
     synchronized (this) {
       if (mKeys == null) {
         mKeys = Assertions.assertNotNull(importKeys());
@@ -73,11 +67,9 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
   private native Object[] importValues();
 
   private @Nonnull HashMap<String, ReadableType> getLocalTypeMap() {
-    // Fast and non-blocking return for common case
     if (mLocalTypeMap != null) {
       return mLocalTypeMap;
     }
-    // Check and when necessary get keys
     synchronized (this) {
       if (mKeys == null) {
         mKeys = Assertions.assertNotNull(importKeys());
@@ -101,29 +93,17 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
 
   @Override
   public boolean hasKey(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return hasKeyNative(name);
-    }
     return getLocalMap().containsKey(name);
   }
 
-  private native boolean hasKeyNative(String name);
-
   @Override
   public boolean isNull(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return isNullNative(name);
-    }
     if (getLocalMap().containsKey(name)) {
       return getLocalMap().get(name) == null;
     }
     throw new NoSuchKeyException(name);
   }
 
-  private native boolean isNullNative(@Nonnull String name);
-
   private @Nonnull Object getValue(@Nonnull String name) {
     if (hasKey(name) && !(isNull(name))) {
       return Assertions.assertNotNull(getLocalMap().get(name));
@@ -152,7 +132,7 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
 
   private void checkInstance(String name, Object value, Class type) {
     if (value != null && !type.isInstance(value)) {
-      throw new ClassCastException(
+      throw new UnexpectedNativeTypeException(
           "Value for "
               + name
               + " cannot be cast from "
@@ -164,86 +144,43 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
 
   @Override
   public boolean getBoolean(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getBooleanNative(name);
-    }
     return getValue(name, Boolean.class).booleanValue();
   }
 
-  private native boolean getBooleanNative(String name);
-
   @Override
   public double getDouble(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getDoubleNative(name);
-    }
     return getValue(name, Double.class).doubleValue();
   }
 
-  private native double getDoubleNative(String name);
-
   @Override
   public int getInt(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getIntNative(name);
-    }
-
     // All numbers coming out of native are doubles, so cast here then truncate
     return getValue(name, Double.class).intValue();
   }
 
-  private native int getIntNative(String name);
-
   @Override
   public @Nullable String getString(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getStringNative(name);
-    }
     return getNullableValue(name, String.class);
   }
 
-  private native String getStringNative(String name);
-
   @Override
   public @Nullable ReadableArray getArray(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getArrayNative(name);
-    }
     return getNullableValue(name, ReadableArray.class);
   }
 
-  private native ReadableNativeArray getArrayNative(String name);
-
   @Override
   public @Nullable ReadableNativeMap getMap(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getMapNative(name);
-    }
     return getNullableValue(name, ReadableNativeMap.class);
   }
 
-  private native ReadableNativeMap getMapNative(String name);
-
   @Override
   public @Nonnull ReadableType getType(@Nonnull String name) {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      mJniCallCounter++;
-      return getTypeNative(name);
-    }
     if (getLocalTypeMap().containsKey(name)) {
       return Assertions.assertNotNull(getLocalTypeMap().get(name));
     }
     throw new NoSuchKeyException(name);
   }
 
-  private native ReadableType getTypeNative(String name);
-
   @Override
   public @Nonnull Dynamic getDynamic(@Nonnull String name) {
     return DynamicFromMap.create(this, name);
@@ -275,42 +212,6 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
 
   @Override
   public @Nonnull HashMap<String, Object> toHashMap() {
-    if (ReactFeatureFlags.useMapNativeAccessor) {
-      ReadableMapKeySetIterator iterator = keySetIterator();
-      HashMap<String, Object> hashMap = new HashMap<>();
-
-      while (iterator.hasNextKey()) {
-        // increment for hasNextKey call
-        mJniCallCounter++;
-        String key = iterator.nextKey();
-        // increment for nextKey call
-        mJniCallCounter++;
-        switch (getType(key)) {
-          case Null:
-            hashMap.put(key, null);
-            break;
-          case Boolean:
-            hashMap.put(key, getBoolean(key));
-            break;
-          case Number:
-            hashMap.put(key, getDouble(key));
-            break;
-          case String:
-            hashMap.put(key, getString(key));
-            break;
-          case Map:
-            hashMap.put(key, Assertions.assertNotNull(getMap(key)).toHashMap());
-            break;
-          case Array:
-            hashMap.put(key, Assertions.assertNotNull(getArray(key)).toArrayList());
-            break;
-          default:
-            throw new IllegalArgumentException("Could not convert object with key: " + key + ".");
-        }
-      }
-      return hashMap;
-    }
-
     // we can almost just return getLocalMap(), but we need to convert nested arrays and maps to the
     // correct types first
     HashMap<String, Object> hashMap = new HashMap<>(getLocalMap());
@@ -337,25 +238,21 @@ public class ReadableNativeMap extends NativeMap implements ReadableMap {
     return hashMap;
   }
 
-  /** Implementation of a {@link ReadableNativeMap} iterator in native memory. */
-  @DoNotStrip
   private static class ReadableNativeMapKeySetIterator implements ReadableMapKeySetIterator {
-    @DoNotStrip private final HybridData mHybridData;
-
-    // Need to hold a strong ref to the map so that our native references remain valid.
-    @DoNotStrip private final ReadableNativeMap mMap;
+    private final Iterator<String> mIterator;
 
     public ReadableNativeMapKeySetIterator(ReadableNativeMap readableNativeMap) {
-      mMap = readableNativeMap;
-      mHybridData = initHybrid(readableNativeMap);
+      mIterator = readableNativeMap.getLocalMap().keySet().iterator();
     }
 
     @Override
-    public native boolean hasNextKey();
+    public boolean hasNextKey() {
+      return mIterator.hasNext();
+    }
 
     @Override
-    public native String nextKey();
-
-    private static native HybridData initHybrid(ReadableNativeMap readableNativeMap);
+    public String nextKey() {
+      return mIterator.next();
+    }
   }
 }

ReactAndroid/src/main/jni/react/jni/OnLoad.cpp

@@ -76,7 +76,6 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
     NativeMap::registerNatives();
     ReadableNativeMap::registerNatives();
     WritableNativeMap::registerNatives();
-    ReadableNativeMapKeySetIterator::registerNatives();
 
     #ifdef WITH_INSPECTOR
     JInspector::registerNatives();

ReactAndroid/src/main/jni/react/jni/ReadableNativeArray.cpp

@@ -12,6 +12,31 @@ using namespace facebook::jni;
 namespace facebook {
 namespace react {
 
+jint makeJIntOrThrow(int64_t integer) {
+  jint javaint = static_cast<jint>(integer);
+  if (integer != javaint) {
+    throwNewJavaException(
+      exceptions::gUnexpectedNativeTypeExceptionClass,
+      "Value '%lld' doesn't fit into a 32 bit signed int", integer);
+  }
+  return javaint;
+}
+
+int64_t convertDynamicIfIntegral(const folly::dynamic& val) {
+  if (val.isInt()) {
+    return val.getInt();
+  }
+  double dbl = val.getDouble();
+  int64_t result = static_cast<int64_t>(dbl);
+  if (dbl != result) {
+    throwNewJavaException(
+      exceptions::gUnexpectedNativeTypeExceptionClass,
+      "Tried to read an int, but got a non-integral double: %f", dbl);
+  }
+  return result;
+}
+
+
 
 // This attribute exports the ctor symbol, so ReadableNativeArray to be
 // constructed from other DSOs.

ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.cpp

@@ -10,10 +10,6 @@ using namespace facebook::jni;
 namespace facebook {
 namespace react {
 
-namespace {
-const char *gNoSuchKeyExceptionClass = "com/facebook/react/bridge/NoSuchKeyException";
-} // namespace
-
 void ReadableNativeMap::mapException(const std::exception& ex) {
   if (dynamic_cast<const folly::TypeError*>(&ex) != nullptr) {
     throwNewJavaException(exceptions::gUnexpectedNativeTypeExceptionClass, ex.what());
@@ -28,8 +24,8 @@ local_ref<JArrayClass<jstring>> ReadableNativeMap::importKeys() {
   }
   jint size = keys_.value().size();
   auto jarray = JArrayClass<jstring>::newArray(size);
-  for (jint i = 0; i < size; i++) {
-    (*jarray)[i] = make_jstring(keys_.value()[i].getString());
+  for (jint ii = 0; ii < size; ii++) {
+    (*jarray)[ii] = make_jstring(keys_.value()[ii].getString());
   }
   return jarray;
 }
@@ -37,39 +33,40 @@ local_ref<JArrayClass<jstring>> ReadableNativeMap::importKeys() {
 local_ref<JArrayClass<jobject>> ReadableNativeMap::importValues() {
   jint size = keys_.value().size();
   auto jarray = JArrayClass<jobject>::newArray(size);
-  for (jint i = 0; i < size; i++) {
-    std::string key = keys_.value()[i].getString().c_str();
-    const auto element = map_.at(key);
+  for (jint ii = 0; ii < size; ii++) {
+    const std::string &key = keys_.value()[ii].getString();
+    const auto &element = map_.at(key);
     switch(element.type()) {
       case folly::dynamic::Type::NULLT: {
-        jarray->setElement(i, nullptr);
+        jarray->setElement(ii, nullptr);
         break;
       }
       case folly::dynamic::Type::BOOL: {
-        (*jarray)[i] =
-              JBoolean::valueOf(ReadableNativeMap::getBooleanKey(key));
+        (*jarray)[ii] = JBoolean::valueOf(element.getBool());
+        break;
+      }
+      case folly::dynamic::Type::INT64: {
+        (*jarray)[ii] = JDouble::valueOf(element.getInt());
         break;
       }
-      case folly::dynamic::Type::INT64:
       case folly::dynamic::Type::DOUBLE: {
-        (*jarray)[i] =
-          JDouble::valueOf(ReadableNativeMap::getDoubleKey(key));
+        (*jarray)[ii] = JDouble::valueOf(element.getDouble());
         break;
       }
       case folly::dynamic::Type::STRING: {
-        (*jarray)[i] = ReadableNativeMap::getStringKey(key);
+        (*jarray)[ii] = make_jstring(element.getString());
         break;
       }
       case folly::dynamic::Type::OBJECT: {
-        (*jarray)[i] = ReadableNativeMap::getMapKey(key);
+        (*jarray)[ii] = ReadableNativeMap::newObjectCxxArgs(element);
         break;
       }
       case folly::dynamic::Type::ARRAY: {
-        (*jarray)[i] = ReadableNativeMap::getArrayKey(key);
+        (*jarray)[ii] = ReadableNativeArray::newObjectCxxArgs(element);
         break;
       }
       default: {
-        jarray->setElement(i,nullptr);
+        jarray->setElement(ii, nullptr);
         break;
       }
     }
@@ -80,80 +77,13 @@ local_ref<JArrayClass<jobject>> ReadableNativeMap::importValues() {
 local_ref<JArrayClass<jobject>> ReadableNativeMap::importTypes() {
   jint size = keys_.value().size();
   auto jarray = JArrayClass<jobject>::newArray(size);
-  for (jint i = 0; i < size; i++) {
-    std::string key = keys_.value()[i].getString().c_str();
-    (*jarray)[i] = ReadableNativeMap::getValueType(key);
+  for (jint ii = 0; ii < size; ii++) {
+    const std::string &key = keys_.value()[ii].getString();
+    (*jarray)[ii] = ReadableType::getType(map_.at(key).type());
   }
   return jarray;
 }
 
-bool ReadableNativeMap::hasKey(const std::string& key) {
-  return map_.find(key) != map_.items().end();
-}
-
-const folly::dynamic& ReadableNativeMap::getMapValue(const std::string& key) {
-  try {
-    return map_.at(key);
-  } catch (const std::out_of_range& ex) {
-    throwNewJavaException(gNoSuchKeyExceptionClass, ex.what());
-  }
-}
-
-bool ReadableNativeMap::isNull(const std::string& key) {
-  return getMapValue(key).isNull();
-}
-
-bool ReadableNativeMap::getBooleanKey(const std::string& key) {
-  return getMapValue(key).getBool();
-}
-
-double ReadableNativeMap::getDoubleKey(const std::string& key) {
-  const folly::dynamic& val = getMapValue(key);
-  if (val.isInt()) {
-    return val.getInt();
-  }
-  return val.getDouble();
-}
-
-jint ReadableNativeMap::getIntKey(const std::string& key) {
-  const folly::dynamic& val = getMapValue(key);
-  int64_t integer = convertDynamicIfIntegral(val);
-  return makeJIntOrThrow(integer);
-}
-
-local_ref<jstring> ReadableNativeMap::getStringKey(const std::string& key) {
-  const folly::dynamic& val = getMapValue(key);
-  if (val.isNull()) {
-    return local_ref<jstring>(nullptr);
-  }
-  return make_jstring(val.getString().c_str());
-}
-
-local_ref<ReadableNativeArray::jhybridobject> ReadableNativeMap::getArrayKey(const std::string& key) {
-  auto& value = getMapValue(key);
-  if (value.isNull()) {
-    return local_ref<ReadableNativeArray::jhybridobject>(nullptr);
-  } else {
-    return ReadableNativeArray::newObjectCxxArgs(value);
-  }
-}
-
-local_ref<ReadableNativeMap::jhybridobject> ReadableNativeMap::getMapKey(const std::string& key) {
-  auto& value = getMapValue(key);
-  if (value.isNull()) {
-    return local_ref<ReadableNativeMap::jhybridobject>(nullptr);
-  } else if (!value.isObject()) {
-    throwNewJavaException(exceptions::gUnexpectedNativeTypeExceptionClass,
-                          "expected Map, got a %s", value.typeName());
-  } else {
-    return ReadableNativeMap::newObjectCxxArgs(value);
-  }
-}
-
-local_ref<ReadableType> ReadableNativeMap::getValueType(const std::string& key) {
-  return ReadableType::getType(getMapValue(key).type());
-}
-
 local_ref<ReadableNativeMap::jhybridobject> ReadableNativeMap::createWithContents(folly::dynamic&& map) {
   if (map.isNull()) {
     return local_ref<jhybridobject>(nullptr);
@@ -172,71 +102,8 @@ void ReadableNativeMap::registerNatives() {
       makeNativeMethod("importKeys", ReadableNativeMap::importKeys),
       makeNativeMethod("importValues", ReadableNativeMap::importValues),
       makeNativeMethod("importTypes", ReadableNativeMap::importTypes),
-      makeNativeMethod("hasKeyNative", ReadableNativeMap::hasKey),
-      makeNativeMethod("isNullNative", ReadableNativeMap::isNull),
-      makeNativeMethod("getBooleanNative", ReadableNativeMap::getBooleanKey),
-      makeNativeMethod("getDoubleNative", ReadableNativeMap::getDoubleKey),
-      makeNativeMethod("getIntNative", ReadableNativeMap::getIntKey),
-      makeNativeMethod("getStringNative", ReadableNativeMap::getStringKey),
-      makeNativeMethod("getArrayNative", ReadableNativeMap::getArrayKey),
-      makeNativeMethod("getMapNative", ReadableNativeMap::getMapKey),
-      makeNativeMethod("getTypeNative", ReadableNativeMap::getValueType),
   });
 }
 
-ReadableNativeMapKeySetIterator::ReadableNativeMapKeySetIterator(const folly::dynamic& map)
-  : iter_(map.items().begin())
-  , map_(map) {}
-
-local_ref<ReadableNativeMapKeySetIterator::jhybriddata> ReadableNativeMapKeySetIterator::initHybrid(alias_ref<jclass>, ReadableNativeMap* nativeMap) {
-  return makeCxxInstance(nativeMap->map_);
-}
-
-bool ReadableNativeMapKeySetIterator::hasNextKey() {
-  return iter_ != map_.items().end();
-}
-
-local_ref<jstring> ReadableNativeMapKeySetIterator::nextKey() {
-  if (!hasNextKey()) {
-    throwNewJavaException("com/facebook/react/bridge/InvalidIteratorException",
-                          "No such element exists");
-  }
-  auto ret = make_jstring(iter_->first.c_str());
-  ++iter_;
-  return ret;
-}
-
-void ReadableNativeMapKeySetIterator::registerNatives() {
-  registerHybrid({
-      makeNativeMethod("hasNextKey", ReadableNativeMapKeySetIterator::hasNextKey),
-      makeNativeMethod("nextKey", ReadableNativeMapKeySetIterator::nextKey),
-      makeNativeMethod("initHybrid", ReadableNativeMapKeySetIterator::initHybrid),
-    });
-}
-
-jint makeJIntOrThrow(int64_t integer) {
-  jint javaint = static_cast<jint>(integer);
-  if (integer != javaint) {
-    throwNewJavaException(
-      exceptions::gUnexpectedNativeTypeExceptionClass,
-      "Value '%lld' doesn't fit into a 32 bit signed int", integer);
-  }
-  return javaint;
-}
-
-int64_t convertDynamicIfIntegral(const folly::dynamic& val) {
-  if (val.isInt()) {
-    return val.getInt();
-  }
-  double dbl = val.getDouble();
-  int64_t result = static_cast<int64_t>(dbl);
-  if (dbl != result) {
-    throwNewJavaException(
-      exceptions::gUnexpectedNativeTypeExceptionClass,
-      "Tried to read an int, but got a non-integral double: %f", dbl);
-  }
-  return result;
-}
-
 }  // namespace react
 }  // namespace facebook

ReactAndroid/src/main/jni/react/jni/ReadableNativeMap.h

@@ -29,16 +29,6 @@ struct ReadableNativeMap : jni::HybridClass<ReadableNativeMap, NativeMap> {
   jni::local_ref<jni::JArrayClass<jstring>> importKeys();
   jni::local_ref<jni::JArrayClass<jobject>> importValues();
   jni::local_ref<jni::JArrayClass<jobject>> importTypes();
-  bool hasKey(const std::string& key);
-  const folly::dynamic& getMapValue(const std::string& key);
-  bool isNull(const std::string& key);
-  bool getBooleanKey(const std::string& key);
-  double getDoubleKey(const std::string& key);
-  jint getIntKey(const std::string& key);
-  jni::local_ref<jstring> getStringKey(const std::string& key);
-  jni::local_ref<ReadableNativeArray::jhybridobject> getArrayKey(const std::string& key);
-  jni::local_ref<jhybridobject> getMapKey(const std::string& key);
-  jni::local_ref<ReadableType> getValueType(const std::string& key);
   folly::Optional<folly::dynamic> keys_;
   static jni::local_ref<jhybridobject> createWithContents(folly::dynamic&& map);
 
@@ -51,24 +41,5 @@ struct ReadableNativeMap : jni::HybridClass<ReadableNativeMap, NativeMap> {
   friend struct WritableNativeMap;
 };
 
-struct ReadableNativeMapKeySetIterator : jni::HybridClass<ReadableNativeMapKeySetIterator> {
-  static auto constexpr kJavaDescriptor = "Lcom/facebook/react/bridge/ReadableNativeMap$ReadableNativeMapKeySetIterator;";
-
-  ReadableNativeMapKeySetIterator(const folly::dynamic& map);
-
-  bool hasNextKey();
-  jni::local_ref<jstring> nextKey();
-
-  static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jclass>, ReadableNativeMap* nativeMap);
-  static void registerNatives();
-
-  folly::dynamic::const_item_iterator iter_;
-  // The Java side holds a strong ref to the Java ReadableNativeMap.
-  const folly::dynamic& map_;
-};
-
-jint makeJIntOrThrow(int64_t integer);
-int64_t convertDynamicIfIntegral(const folly::dynamic&);
-
 } // namespace react
 } // namespace facebook