[all] Replace Result<T> with optional<T> plus out Error parameter

12 files changed, 101 insertions, 70 deletions

diff --git a/platform/android/src/style/conversion/filter.hpp b/platform/android/src/style/conversion/filter.hpp
index fc36d3a044..1f0abcf3a4 100644
--- a/platform/android/src/style/conversion/filter.hpp
+++ b/platform/android/src/style/conversion/filter.hpp
@@ -17,9 +17,10 @@ inline optional<mbgl::style::Filter> toFilter(jni::JNIEnv& env, jni::Array<jni::
     mbgl::optional<mbgl::style::Filter> filter;
     if (jfilter) {
       Value filterValue(env, jfilter);
-      auto converted = mbgl::style::conversion::convert<mbgl::style::Filter>(filterValue);
+      mbgl::style::conversion::Error error;
+      auto converted = mbgl::style::conversion::convert<mbgl::style::Filter>(filterValue, error);
       if (!converted) {
-          mbgl::Log::Error(mbgl::Event::JNI, "Error converting filter: " + converted.error().message);
+          mbgl::Log::Error(mbgl::Event::JNI, "Error converting filter: " + error.message);
       }
       filter = std::move(*converted);
     }
diff --git a/platform/android/src/style/conversion/geojson.hpp b/platform/android/src/style/conversion/geojson.hpp
index 415d96f467..6ac6abcd7c 100644
--- a/platform/android/src/style/conversion/geojson.hpp
+++ b/platform/android/src/style/conversion/geojson.hpp
@@ -17,12 +17,13 @@ namespace style {
 namespace conversion {
 
 template <>
-Result<GeoJSON> convertGeoJSON(const mbgl::android::Value& value) {
+optional<GeoJSON> convertGeoJSON(const mbgl::android::Value& value, Error& error) {
 
     // Value should be a string wrapped in an object
     mbgl::android::Value jsonValue = value.get("data");
     if(value.isNull()) {
-        return Error { "no json data found" };
+        error = { "no json data found" };
+        return {};
     }
     std::string jsonString = value.get("data").toString();
 
@@ -32,12 +33,14 @@ Result<GeoJSON> convertGeoJSON(const mbgl::android::Value& value) {
     if (d.HasParseError()) {
         std::stringstream message;
         message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError());
-        return Error { message.str() };
+        error = { message.str() };
+        return {};
     }
 
-    conversion::Result<GeoJSON> geoJSON = conversion::convertGeoJSON<JSValue>(d);
+    optional<GeoJSON> geoJSON = conversion::convertGeoJSON<JSValue>(d, error);
     if (!geoJSON) {
-        return Error { geoJSON.error().message };
+        error = { error.message };
+        return {};
     }
 
     return geoJSON;
@@ -46,8 +49,8 @@ Result<GeoJSON> convertGeoJSON(const mbgl::android::Value& value) {
 template <>
 struct Converter<GeoJSON> {
 
-    Result<GeoJSON> operator()(const mbgl::android::Value& value) const {
-        return convertGeoJSON(value);
+    optional<GeoJSON> operator()(const mbgl::android::Value& value, Error& error) const {
+        return convertGeoJSON(value, error);
     }
 
 };
diff --git a/platform/android/src/style/conversion/url_or_tileset.hpp b/platform/android/src/style/conversion/url_or_tileset.hpp
index 4e502324d0..00ef913d41 100644
--- a/platform/android/src/style/conversion/url_or_tileset.hpp
+++ b/platform/android/src/style/conversion/url_or_tileset.hpp
@@ -12,27 +12,25 @@
 #include <string>
 
 namespace mbgl {
-namespace style {
-namespace conversion {
-
-template <>
-struct Converter<variant<std::string, Tileset>> {
-
-    template <class V>
-    Result<variant<std::string, Tileset>> operator()(const V& value) const {
-        if (isObject(value)) {
-            Result<Tileset> tileset = convert<Tileset>(value);
-            if (!tileset) {
-                return tileset.error();
-            }
-            return *tileset;
-        } else {
-            return *toString(value);
+namespace android {
+
+// This conversion is expected not to fail because it's used only in contexts where
+// the value was originally a String or TileSet object on the Java side. If it fails
+// to convert, it's a bug in our serialization or Java-side static typing.
+inline variant<std::string, Tileset> convertURLOrTileset(const Value& value) {
+    using namespace mbgl::style::conversion;
+
+    if (isObject(value)) {
+        Error error;
+        optional<Tileset> tileset = convert<Tileset>(value, error);
+        if (!tileset) {
+            throw std::logic_error(error.message);
         }
+        return { *tileset };
+    } else {
+        return { *toString(value) };
     }
-
-};
-
 }
+
 }
 }
diff --git a/platform/android/src/style/layers/layer.cpp b/platform/android/src/style/layers/layer.cpp
index dbf71fd2af..6e93cf4daf 100644
--- a/platform/android/src/style/layers/layer.cpp
+++ b/platform/android/src/style/layers/layer.cpp
@@ -104,9 +104,10 @@ namespace android {
         Value wrapped(env, jfilter);
         Filter filter;
 
-        Result<Filter> converted = convert<Filter>(wrapped);
+        Error error;
+        optional<Filter> converted = convert<Filter>(wrapped, error);
         if (!converted) {
-            mbgl::Log::Error(mbgl::Event::JNI, "Error setting filter: " + converted.error().message);
+            mbgl::Log::Error(mbgl::Event::JNI, "Error setting filter: " + error.message);
             return;
         }
         filter = std::move(*converted);
diff --git a/platform/android/src/style/sources/geojson_source.cpp b/platform/android/src/style/sources/geojson_source.cpp
index 7ab98e47c0..0c2d25f9fc 100644
--- a/platform/android/src/style/sources/geojson_source.cpp
+++ b/platform/android/src/style/sources/geojson_source.cpp
@@ -9,7 +9,7 @@
 #include "../../conversion/conversion.hpp"
 #include "../../conversion/collection.hpp"
 #include "../../geometry/conversion/feature.hpp"
-#include "../conversion/url_or_tileset.hpp"
+
 #include <mbgl/style/conversion.hpp>
 #include <mbgl/style/conversion/geojson_options.hpp>
 
@@ -18,11 +18,26 @@
 namespace mbgl {
 namespace android {
 
+    // This conversion is expected not to fail because it's used only in contexts where
+    // the value was originally a GeoJsonOptions object on the Java side. If it fails
+    // to convert, it's a bug in our serialization or Java-side static typing.
+    static style::GeoJSONOptions convertGeoJSONOptions(jni::JNIEnv& env, jni::Object<> options) {
+        using namespace mbgl::style::conversion;
+        if (!options) {
+            return style::GeoJSONOptions();
+        }
+        Error error;
+        optional<style::GeoJSONOptions> result = convert<style::GeoJSONOptions>(Value(env, options), error);
+        if (!result) {
+            throw std::logic_error(error.message);
+        }
+        return *result;
+    }
+
     GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, jni::String sourceId, jni::Object<> options)
         : Source(env, std::make_unique<mbgl::style::GeoJSONSource>(
                 jni::Make<std::string>(env, sourceId),
-                options ? *style::conversion::convert<style::GeoJSONOptions>(Value(env, options)) : style::GeoJSONOptions()
-                )
+                convertGeoJSONOptions(env, options))
             ) {
     }
 
@@ -36,9 +51,10 @@ namespace android {
         using namespace mbgl::style::conversion;
 
         // Convert the jni object
-        Result<GeoJSON> converted = convert<GeoJSON>(Value(env, json));
+        Error error;
+        optional<GeoJSON> converted = convert<GeoJSON>(Value(env, json), error);
         if(!converted) {
-            mbgl::Log::Error(mbgl::Event::JNI, "Error setting geo json: " + converted.error().message);
+            mbgl::Log::Error(mbgl::Event::JNI, "Error setting geo json: " + error.message);
             return;
         }
 
diff --git a/platform/android/src/style/sources/raster_source.cpp b/platform/android/src/style/sources/raster_source.cpp
index 42ac4cda99..0234901a77 100644
--- a/platform/android/src/style/sources/raster_source.cpp
+++ b/platform/android/src/style/sources/raster_source.cpp
@@ -16,7 +16,7 @@ namespace android {
             env,
             std::make_unique<mbgl::style::RasterSource>(
                 jni::Make<std::string>(env, sourceId),
-                *style::conversion::convert<variant<std::string, Tileset>>(Value(env, urlOrTileSet)),
+                convertURLOrTileset(Value(env, urlOrTileSet)),
                 tileSize
             )
         ) {
diff --git a/platform/android/src/style/sources/vector_source.cpp b/platform/android/src/style/sources/vector_source.cpp
index 67777f50d4..4852a9b84f 100644
--- a/platform/android/src/style/sources/vector_source.cpp
+++ b/platform/android/src/style/sources/vector_source.cpp
@@ -23,7 +23,7 @@ namespace android {
             env,
             std::make_unique<mbgl::style::VectorSource>(
                 jni::Make<std::string>(env, sourceId),
-                *style::conversion::convert<variant<std::string, Tileset>>(Value(env, urlOrTileSet))
+                convertURLOrTileset(Value(env, urlOrTileSet))
             )
         ) {
     }
diff --git a/platform/darwin/src/MGLStyleValue_Private.h b/platform/darwin/src/MGLStyleValue_Private.h
index 90d6622c93..263b54d7e5 100644
--- a/platform/darwin/src/MGLStyleValue_Private.h
+++ b/platform/darwin/src/MGLStyleValue_Private.h
@@ -123,8 +123,9 @@ public:
             return toMBGLConstantValue((MGLConstantStyleValue<ObjCType> *)value);
         } else if ([value isKindOfClass:[MGLStyleFunction class]]) {
             auto rawValue = toRawStyleSpecValue((MGLStyleFunction<ObjCType> *) value);
-            auto result = mbgl::style::conversion::convert<mbgl::style::DataDrivenPropertyValue<MBGLType>>(rawValue);
-            NSCAssert(result, @(result.error().message.c_str()));
+            mbgl::style::conversion::Error error;
+            auto result = mbgl::style::conversion::convert<mbgl::style::DataDrivenPropertyValue<MBGLType>>(rawValue, error);
+            NSCAssert(result, @(error.message.c_str()));
             return *result;
         } else {
             return {};
diff --git a/platform/node/src/node_geojson.hpp b/platform/node/src/node_geojson.hpp
index ace4c91426..e4b2ca47e7 100644
--- a/platform/node/src/node_geojson.hpp
+++ b/platform/node/src/node_geojson.hpp
@@ -5,8 +5,9 @@ namespace style {
 namespace conversion {
 
 template <>
-Result<GeoJSON> convertGeoJSON(const v8::Local<v8::Value>&) {
-    return Error { "not implemented" };
+optional<GeoJSON> convertGeoJSON(const v8::Local<v8::Value>&, Error& error) {
+    error = { "not implemented" };
+    return {};
 }
 
 } // namespace conversion
diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp
index a3a11a1907..881c1b2fb7 100644
--- a/platform/node/src/node_map.cpp
+++ b/platform/node/src/node_map.cpp
@@ -538,9 +538,10 @@ void NodeMap::AddSource(const Nan::FunctionCallbackInfo<v8::Value>& info) {
         return Nan::ThrowTypeError("First argument must be a string");
     }
 
-    Result<std::unique_ptr<Source>> source = convert<std::unique_ptr<Source>>(info[1], *Nan::Utf8String(info[0]));
+    Error error;
+    mbgl::optional<std::unique_ptr<Source>> source = convert<std::unique_ptr<Source>>(info[1], error, *Nan::Utf8String(info[0]));
     if (!source) {
-        Nan::ThrowTypeError(source.error().message.c_str());
+        Nan::ThrowTypeError(error.message.c_str());
         return;
     }
 
@@ -558,9 +559,10 @@ void NodeMap::AddLayer(const Nan::FunctionCallbackInfo<v8::Value>& info) {
         return Nan::ThrowTypeError("One argument required");
     }
 
-    Result<std::unique_ptr<Layer>> layer = convert<std::unique_ptr<Layer>>(info[0]);
+    Error error;
+    mbgl::optional<std::unique_ptr<Layer>> layer = convert<std::unique_ptr<Layer>>(info[0], error);
     if (!layer) {
-        Nan::ThrowTypeError(layer.error().message.c_str());
+        Nan::ThrowTypeError(error.message.c_str());
         return;
     }
 
@@ -774,9 +776,10 @@ void NodeMap::SetFilter(const Nan::FunctionCallbackInfo<v8::Value>& info) {
     Filter filter;
 
     if (!info[1]->IsNull() && !info[1]->IsUndefined()) {
-        Result<Filter> converted = convert<Filter>(info[1]);
+        Error error;
+        mbgl::optional<Filter> converted = convert<Filter>(info[1], error);
         if (!converted) {
-            Nan::ThrowTypeError(converted.error().message.c_str());
+            Nan::ThrowTypeError(error.message.c_str());
             return;
         }
         filter = std::move(*converted);
@@ -908,23 +911,24 @@ void NodeMap::QueryRenderedFeatures(const Nan::FunctionCallbackInfo<v8::Value>&
         //Check if filter is provided. If set it must be a valid Filter object
         if (Nan::Has(options, Nan::New("filter").ToLocalChecked()).FromJust()) {
             auto filterOption = Nan::Get(options, Nan::New("filter").ToLocalChecked()).ToLocalChecked();
-            Result<Filter> converted = convert<Filter>(filterOption);
+            Error error;
+            mbgl::optional<Filter> converted = convert<Filter>(filterOption, error);
             if (!converted) {
-                return Nan::ThrowTypeError(converted.error().message.c_str());
+                return Nan::ThrowTypeError(error.message.c_str());
             }
             queryOptions.filter = std::move(*converted);
         }
     }
 
     try {
-        std::vector<mbgl::Feature> result;
+        std::vector<mbgl::Feature> optional;
 
         if (Nan::Get(posOrBox, 0).ToLocalChecked()->IsArray()) {
 
             auto pos0 = Nan::Get(posOrBox, 0).ToLocalChecked().As<v8::Array>();
             auto pos1 = Nan::Get(posOrBox, 1).ToLocalChecked().As<v8::Array>();
 
-            result = nodeMap->map->queryRenderedFeatures(mbgl::ScreenBox {
+            optional = nodeMap->map->queryRenderedFeatures(mbgl::ScreenBox {
                 {
                     Nan::Get(pos0, 0).ToLocalChecked()->NumberValue(),
                     Nan::Get(pos0, 1).ToLocalChecked()->NumberValue()
@@ -935,15 +939,15 @@ void NodeMap::QueryRenderedFeatures(const Nan::FunctionCallbackInfo<v8::Value>&
             },  queryOptions);
 
         } else {
-            result = nodeMap->map->queryRenderedFeatures(mbgl::ScreenCoordinate {
+            optional = nodeMap->map->queryRenderedFeatures(mbgl::ScreenCoordinate {
                 Nan::Get(posOrBox, 0).ToLocalChecked()->NumberValue(),
                 Nan::Get(posOrBox, 1).ToLocalChecked()->NumberValue()
             }, queryOptions);
         }
 
         auto array = Nan::New<v8::Array>();
-        for (unsigned int i = 0; i < result.size(); i++) {
-            array->Set(i, toJS(result[i]));
+        for (unsigned int i = 0; i < optional.size(); i++) {
+            array->Set(i, toJS(optional[i]));
         }
         info.GetReturnValue().Set(array);
     } catch (const std::exception &ex) {
diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp
index b1c0a11ee1..3d5da10c36 100644
--- a/platform/qt/src/qmapboxgl.cpp
+++ b/platform/qt/src/qmapboxgl.cpp
@@ -1250,9 +1250,10 @@ void QMapboxGL::addSource(const QString &id, const QVariantMap &params)
     using namespace mbgl::style;
     using namespace mbgl::style::conversion;
 
-    Result<std::unique_ptr<Source>> source = convert<std::unique_ptr<Source>>(QVariant(params), id.toStdString());
+    Error error;
+    mbgl::optional<std::unique_ptr<Source>> source = convert<std::unique_ptr<Source>>(QVariant(params), error, id.toStdString());
     if (!source) {
-        qWarning() << "Unable to add source:" << source.error().message.c_str();
+        qWarning() << "Unable to add source:" << error.message.c_str();
         return;
     }
 
@@ -1291,7 +1292,8 @@ void QMapboxGL::updateSource(const QString &id, const QVariantMap &params)
     }
 
     if (params.contains("data")) {
-        auto result = convertGeoJSON(params["data"]);
+        Error error;
+        auto result = convertGeoJSON(params["data"], error);
         if (result) {
             sourceGeoJSON->setGeoJSON(*result);
         }
@@ -1362,9 +1364,10 @@ void QMapboxGL::addLayer(const QVariantMap &params)
     using namespace mbgl::style;
     using namespace mbgl::style::conversion;
 
-    Result<std::unique_ptr<Layer>> layer = convert<std::unique_ptr<Layer>>(QVariant(params));
+    Error error;
+    mbgl::optional<std::unique_ptr<Layer>> layer = convert<std::unique_ptr<Layer>>(QVariant(params), error);
     if (!layer) {
-        qWarning() << "Unable to add layer:" << layer.error().message.c_str();
+        qWarning() << "Unable to add layer:" << error.message.c_str();
         return;
     }
 
@@ -1445,9 +1448,10 @@ void QMapboxGL::setFilter(const QString& layer, const QVariant& filter)
 
     Filter filter_;
 
-    Result<Filter> converted = convert<Filter>(filter);
+    Error error;
+    mbgl::optional<Filter> converted = convert<Filter>(filter, error);
     if (!converted) {
-        qWarning() << "Error parsing filter:" << converted.error().message.c_str();
+        qWarning() << "Error parsing filter:" << error.message.c_str();
         return;
     }
     filter_ = std::move(*converted);
diff --git a/platform/qt/src/qt_geojson.hpp b/platform/qt/src/qt_geojson.hpp
index 038b051941..7c50c663dd 100644
--- a/platform/qt/src/qt_geojson.hpp
+++ b/platform/qt/src/qt_geojson.hpp
@@ -180,20 +180,21 @@ namespace style {
 namespace conversion {
 
 template <>
-Result<GeoJSON> convertGeoJSON(const QMapbox::Feature& feature) {
-    return Result<GeoJSON> { GeoJSON { asMapboxGLFeature(feature) } };
+optional<GeoJSON> convertGeoJSON(const QMapbox::Feature& feature, Error&) {
+    return GeoJSON { asMapboxGLFeature(feature) };
 }
 
 template <>
-Result<GeoJSON> convertGeoJSON(const QVariant& value) {
+optional<GeoJSON> convertGeoJSON(const QVariant& value, Error& error) {
 #if QT_VERSION >= 0x050000
     if (value.typeName() == QStringLiteral("QMapbox::Feature")) {
 #else
     if (value.typeName() == QString("QMapbox::Feature")) {
 #endif
-        return convertGeoJSON(value.value<QMapbox::Feature>());
+        return convertGeoJSON(value.value<QMapbox::Feature>(), error);
     } else if (value.type() != QVariant::ByteArray) {
-        return Error { "JSON data must be in QByteArray" };
+        error = { "JSON data must be in QByteArray" };
+        return {};
     }
 
     auto data = value.toByteArray();
@@ -208,13 +209,14 @@ Result<GeoJSON> convertGeoJSON(const QVariant& value) {
     if (d.HasParseError()) {
         std::stringstream message;
         message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError());
-
-        return Error { message.str() };
+        error = { message.str() };
+        return {};
     }
 
-    Result<GeoJSON> geoJSON = convertGeoJSON<JSValue>(d);
+    optional<GeoJSON> geoJSON = convertGeoJSON<JSValue>(d, error);
     if (!geoJSON) {
-        return Error { geoJSON.error().message };
+        error = { error.message };
+        return {};
     }
 
     return geoJSON;