From 81d162bba76fbe6c3923a219da66977a04b164e2 Mon Sep 17 00:00:00 2001 From: Bryan Phelps Date: Tue, 19 Jan 2021 09:37:45 -0800 Subject: [PATCH] fix(extension/#2676): Part 1 - Fix error message with missing extension dependency (#3007) __Issue:__ When an extension has a required dependency that is not installed, Onivim would fail to activate the extension silently. __Defect:__ Onivim was not properly handling the extension activation error - usually, it's a `string`, but in the case of a missing dependency, it is a JSON object - the parser was only handling the string case. __Fix:__ Implement a decoder to handle either case. Bubble up the activation error to the user. Related #2676 Related #1058 --- CHANGES_CURRENT.md | 1 + src/Exthost/ExtensionActivationError.re | 32 ++++++ src/Exthost/ExtensionId.re | 2 + src/Exthost/Exthost.re | 1 + src/Exthost/Exthost.rei | 30 +++--- src/Exthost/Msg.re | 138 +++++++++++++----------- src/Feature/Extensions/Model.re | 25 ++++- 7 files changed, 147 insertions(+), 82 deletions(-) create mode 100644 src/Exthost/ExtensionActivationError.re diff --git a/CHANGES_CURRENT.md b/CHANGES_CURRENT.md index d5b97f0e94..7a7d689bcd 100644 --- a/CHANGES_CURRENT.md +++ b/CHANGES_CURRENT.md @@ -3,6 +3,7 @@ ### Bug Fixes - #3008 - SCM: Fix index-out-of-bound exception when rendering diff markers +- #3007 - Extensions: Show 'missing dependency' activation error to user ### Performance diff --git a/src/Exthost/ExtensionActivationError.re b/src/Exthost/ExtensionActivationError.re new file mode 100644 index 0000000000..8121b1d676 --- /dev/null +++ b/src/Exthost/ExtensionActivationError.re @@ -0,0 +1,32 @@ +open Oni_Core; + +[@deriving show] +type t = + | Message(string) + | MissingDependency(ExtensionId.t); + +let toString = + fun + | Message(str) => str + | MissingDependency(extensionId) => + Printf.sprintf( + "Missing required extension: %s", + ExtensionId.toString(extensionId), + ); + +module Decode = { + open Json.Decode; + let message = string |> map(str => Message(str)); + + let missingDependency = + obj(({field, _}) => {field.required("dependency", ExtensionId.decode)}) + |> map(extensionId => MissingDependency(extensionId)); + + let decode = + one_of([ + ("message", message), + ("missingDependency", missingDependency), + ]); +}; + +let decode = Decode.decode; diff --git a/src/Exthost/ExtensionId.re b/src/Exthost/ExtensionId.re index 0d9ad593c0..02e30f3e37 100644 --- a/src/Exthost/ExtensionId.re +++ b/src/Exthost/ExtensionId.re @@ -15,3 +15,5 @@ let decode = ); let encode = Json.Encode.string; + +let toString = Fun.id; diff --git a/src/Exthost/Exthost.re b/src/Exthost/Exthost.re index dbe51dae68..c6b864e8ea 100644 --- a/src/Exthost/Exthost.re +++ b/src/Exthost/Exthost.re @@ -19,6 +19,7 @@ module DocumentSelector = DocumentSelector; module DocumentSymbol = DocumentSymbol; module Edit = Edit; module Eol = Eol; +module ExtensionActivationError = ExtensionActivationError; module ExtensionActivationReason = ExtensionActivationReason; module ExtensionId = ExtensionId; module Files = Files; diff --git a/src/Exthost/Exthost.rei b/src/Exthost/Exthost.rei index b161625850..8159fedc7a 100644 --- a/src/Exthost/Exthost.rei +++ b/src/Exthost/Exthost.rei @@ -146,6 +146,15 @@ module Edit: { }; }; +module ExtensionActivationError: { + [@deriving show] + type t = + | Message(string) + | MissingDependency(ExtensionId.t); + + let toString: t => string; +}; + module ExtensionActivationReason: { type t; @@ -159,6 +168,8 @@ module ExtensionId: { [@deriving show] type t = string; + let toString: t => string; + let decode: Json.decoder(t); }; @@ -174,17 +185,6 @@ module DefinitionLink: { let decode: Json.decoder(t); }; -// module DocumentFilter: { -// [@deriving show] -// type t; - -// let matches: (~filetype: string, t) => bool; - -// let decode: Json.decoder(t); - -// let toString: t => string; -// }; - module DocumentSelector: { [@deriving show] type t = list(DocumentFilter.t); @@ -1220,12 +1220,14 @@ module Msg: { activateCallTime: int, activateResolvedTime: int, }) - //activationEvent: option(string), | ExtensionActivationError({ extensionId: ExtensionId.t, - errorMessage: string, + error: ExtensionActivationError.t, }) - | ExtensionRuntimeError({extensionId: ExtensionId.t}); + | ExtensionRuntimeError({ + extensionId: ExtensionId.t, + errorsJson: list(Yojson.Safe.t), + }); }; module FileSystem: { diff --git a/src/Exthost/Msg.re b/src/Exthost/Msg.re index 83d6e4cf6c..b87c7dbfbe 100644 --- a/src/Exthost/Msg.re +++ b/src/Exthost/Msg.re @@ -538,76 +538,86 @@ module ExtensionService = { activateCallTime: int, activateResolvedTime: int, }) - //activationEvent: option(string), | ExtensionActivationError({ extensionId: ExtensionId.t, - errorMessage: string, + error: ExtensionActivationError.t, }) - | ExtensionRuntimeError({extensionId: ExtensionId.t}); - let withExtensionId = (f, extensionIdJson) => { - extensionIdJson - |> Json.Decode.decode_value(ExtensionId.decode) - |> Result.map(f) - |> Result.map_error(Json.Decode.string_of_error); - }; + | ExtensionRuntimeError({ + extensionId: ExtensionId.t, + errorsJson: list(Yojson.Safe.t), + }); let handle = (method, args: Yojson.Safe.t) => { - switch (method, args) { - | ("$activateExtension", `List([extensionIdJson])) => - extensionIdJson - |> withExtensionId(extensionId => { - ActivateExtension({extensionId, activationEvent: None}) - }) - | ("$activateExtension", `List([extensionIdJson, activationEventJson])) => - let activationEvent = - switch (activationEventJson) { - | `String(v) => Some(v) - | _ => None + Base.Result.Let_syntax.( + { + switch (method, args) { + | ("$activateExtension", `List([extensionIdJson])) => + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + Ok(ActivateExtension({extensionId, activationEvent: None})); + + | ( + "$activateExtension", + `List([extensionIdJson, activationEventJson]), + ) => + let activationEvent = + switch (activationEventJson) { + | `String(v) => Some(v) + | _ => None + }; + + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + Ok(ActivateExtension({extensionId, activationEvent})); + | ( + "$onExtensionActivationError", + `List([extensionIdJson, errorJson]), + ) => + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + + let%bind error = + errorJson + |> Internal.decode_value(ExtensionActivationError.decode); + + Ok(ExtensionActivationError({extensionId, error})); + + | ("$onWillActivateExtension", `List([extensionIdJson])) => + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + Ok(WillActivateExtension({extensionId: extensionId})); + | ( + "$onDidActivateExtension", + `List([ + extensionIdJson, + `Int(codeLoadingTime), + `Int(activateCallTime), + `Int(activateResolvedTime), + ..._args, + ]), + ) => + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + Ok( + DidActivateExtension({ + extensionId, + codeLoadingTime, + activateCallTime, + activateResolvedTime, + }), + ); + | ( + "$onExtensionRuntimeError", + `List([extensionIdJson, ...errorsJson]), + ) => + let%bind extensionId = + extensionIdJson |> Internal.decode_value(ExtensionId.decode); + Ok(ExtensionRuntimeError({extensionId, errorsJson})); + + | _ => Error("Unhandled method: " ++ method) }; - - extensionIdJson - |> withExtensionId(extensionId => { - ActivateExtension({extensionId, activationEvent}) - }); - | ( - "$onExtensionActivationError", - `List([extensionIdJson, `String(errorMessage)]), - ) => - extensionIdJson - |> withExtensionId(extensionId => { - ExtensionActivationError({extensionId, errorMessage}) - }) - | ("$onWillActivateExtension", `List([extensionIdJson])) => - extensionIdJson - |> withExtensionId(extensionId => { - WillActivateExtension({extensionId: extensionId}) - }) - | ( - "$onDidActivateExtension", - `List([ - extensionIdJson, - `Int(codeLoadingTime), - `Int(activateCallTime), - `Int(activateResolvedTime), - ..._args, - ]), - ) => - extensionIdJson - |> withExtensionId(extensionId => { - DidActivateExtension({ - extensionId, - codeLoadingTime, - activateCallTime, - activateResolvedTime, - }) - }) - | ("$onExtensionRuntimeError", `List([extensionIdJson, ..._args])) => - extensionIdJson - |> withExtensionId(extensionId => { - ExtensionRuntimeError({extensionId: extensionId}) - }) - | _ => Error("Unhandled method: " ++ method) - }; + } + ); }; }; diff --git a/src/Feature/Extensions/Model.re b/src/Feature/Extensions/Model.re index 3c759d9c4e..600a48939e 100644 --- a/src/Feature/Extensions/Model.re +++ b/src/Feature/Extensions/Model.re @@ -608,15 +608,32 @@ let getExtensions = Internal.getExtensions; let update = (~extHostClient, msg, model) => { switch (msg) { - | Exthost(WillActivateExtension(_)) - | Exthost(ExtensionRuntimeError(_)) => (model, Nothing) + | Exthost(WillActivateExtension(_)) => (model, Nothing) + + | Exthost(ExtensionRuntimeError({extensionId, errorsJson})) => ( + model, + NotifyFailure( + Printf.sprintf( + "Extension runtime error %s:%s", + Exthost.ExtensionId.toString(extensionId), + Yojson.Safe.to_string(`List(errorsJson)), + ), + ), + ) + | Exthost(ActivateExtension({extensionId, _})) => ( Internal.markActivated(extensionId, model), Nothing, ) - | Exthost(ExtensionActivationError({errorMessage, _})) => ( + | Exthost(ExtensionActivationError({error, extensionId})) => ( model, - NotifyFailure(Printf.sprintf("Error: %s", errorMessage)), + NotifyFailure( + Printf.sprintf( + "Error activating extension %s: %s", + Exthost.ExtensionId.toString(extensionId), + Exthost.ExtensionActivationError.toString(error), + ), + ), ) | Exthost(DidActivateExtension({extensionId, _})) => ( Internal.markActivated(extensionId, model),