fix(extension/#3685): Fix nim activation error (#3709)

__Issue:__ The nim extension #3685 was failing to activate.

__Defect:__ The extension is expecting the storage folder to be created, but this is failing. The folder should be created by the extension host here: https://github.com/microsoft/vscode/blob/fc0ee4e4bc28d8efe5d576827d3bacdd919f3e7f/src/vs/workbench/api/common/extHostStoragePaths.ts#L60 - however, the `$stat` result is not throwing an exception, as expected by that code.

__Fix:__ When a file/folder is not available via `$stat`, send the response as an error, so it properly bubbles up to extensions as an extension.

Fixes #3685

Author: bryphe

CHANGES_CURRENT.md

@@ -28,6 +28,7 @@
 - #3692 - CodeLens: Fix delegated commands not executing (related #2380)
 - #3702 - OSX: Fix crash on open-with (fixes #3698)
 - #3703 - Sidebar: Fix window navigation to sidebar when closed or zen mode (related #3681)
+- #3709 - Extension: Fix activation error with nim extension (fixes #3685)
 
 ### Performance
 

development_extensions/oni-test-extension/extension.js

@@ -41,6 +41,24 @@ function activate(context) {
             })();
         }),
     );
+
+    cleanup(
+        vscode.commands.registerCommand("oni-test.exthost.runFileSystemTests", () => {
+            
+            (async () => {
+                // From docs: https://code.visualstudio.com/api/references/vscode-api#ExtensionContext
+                // Given path might not exist - but parent directory will exist.
+                const nonExistentPath = path.join(context.storagePath, "some-non-existent-path");
+                const nonExistentUri = vscode.Uri.file(nonExistentPath);
+                try {
+                    await vscode.workspace.fs.stat(nonExistentUri);
+                    fail("#3685: Non-existent path should not have successful stat")
+                } catch (exn) {
+                    pass()
+                }
+        })();
+    })
+    );
 }
 
 // this method is called when your extension is deactivated

development_extensions/oni-test-extension/package.json

@@ -8,7 +8,8 @@
 	},
 	"activationEvents": [
 		"onCommand:oni-test.showMessage",
-		"onCommand:oni-test.exthost.validateLogPathIsDirectory"
+		"onCommand:oni-test.exthost.validateLogPathIsDirectory",
+		"onCommand:oni-test.exthost.runFileSystemTests"
 	],
 	"main": "./extension.js",
 	"contributes": {},

integration_test/ExtHostFileSystemTest.re

@@ -0,0 +1,46 @@
+open Oni_Model;
+open Oni_IntegrationTestLib;
+
+// This test validates:
+// - The 'oni-dev' extension gets activated
+// - When typing in an 'oni-dev' buffer, we get some completion results
+runTest(~name="ExtHostContextTest", ({dispatch, wait, _}) => {
+  wait(~timeout=30.0, ~name="Exthost is initialized", (state: State.t) =>
+    Feature_Exthost.isInitialized(state.exthost)
+  );
+
+  // Wait until the extension is activated
+  // Give some time for the exthost to start
+  wait(
+    ~timeout=30.0,
+    ~name="Validate the 'oni-dev' extension gets activated",
+    (state: State.t) =>
+    List.exists(
+      id => id == "oni-dev-extension",
+      state.extensions |> Feature_Extensions.activatedIds,
+    )
+  );
+
+  // Kick off an oni-test command that should activate the oni-test-extension:
+  dispatch(
+    Actions.Extensions(
+      Feature_Extensions.Msg.command(
+        ~command="oni-test.exthost.runFileSystemTests",
+        ~arguments=[],
+      ),
+    ),
+  );
+
+  // Validate test passes
+  wait(
+    ~timeout=30.0,
+    ~name="Validate test passes",
+    (state: State.t) => {
+      let notifications = Feature_Notification.all(state.notifications);
+      notifications
+      |> List.exists((notification: Feature_Notification.notification) => {
+           notification.message == "PASS"
+         });
+    },
+  );
+});

integration_test/dune

@@ -9,6 +9,7 @@
    ExCommandKeybindingNormTest ExtConfigurationChangedTest ExtHostContextTest
    ExtHostBufferOpenTest ExtHostBufferUpdatesTest
    ExtHostCommandActivationTest ExtHostCompletionTest ExtHostDefinitionTest
+   ExtHostFileSystemTest
    ExtHostFormatSingleEditTest ExtHostFormatFullEditTest
    ExtHostWorkspaceSearchTest FileWatcherTest FormatOnSaveTest
    KeybindingsInvalidJsonTest KeySequenceJJTest InputIgnoreTest
@@ -47,6 +48,7 @@
    ExtConfigurationChangedTest.exe ExtHostBufferOpenTest.exe
    ExtHostBufferUpdatesTest.exe ExtHostCommandActivationTest.exe
    ExtHostContextTest.exe ExtHostCompletionTest.exe ExtHostDefinitionTest.exe
+   ExtHostFileSystemTest.exe
    ExtHostWorkspaceSearchTest.exe FileWatcherTest.exe FormatOnSaveTest.exe
    KeybindingsInvalidJsonTest.exe KeySequenceJJTest.exe InputIgnoreTest.exe
    InputContextKeysTest.exe InputRemapMotionTest.exe LanguageCssTest.exe

src/Exthost/Client.re

@@ -152,6 +152,16 @@ let start =
                 )
               );
               send(Outgoing.ReplyError({requestId, error: message}));
+
+            | ErrorJson({error}) =>
+              Log.tracef(m =>
+                m(
+                  "Responding to request %d with error: %s",
+                  requestId,
+                  error |> Yojson.Safe.to_string,
+                )
+              );
+              send(Outgoing.ReplyErrorJSON({requestId, error}));
             };
           };
 

src/Exthost/Exthost.rei

@@ -1890,6 +1890,8 @@ module Reply: {
 
   let error: string => t;
 
+  let errorJson: Yojson.Safe.t => t;
+
   let okEmpty: t;
 
   let okJson: Yojson.Safe.t => t;

src/Exthost/Protocol/Exthost_Protocol.re

@@ -157,6 +157,10 @@ module Message = {
           requestId: int,
           error: string,
         })
+      | ReplyErrorJSON({
+          requestId: int,
+          error: Yojson.Safe.t,
+        })
       | Terminate;
   };
 
@@ -262,6 +266,7 @@ module Message = {
       | ReplyOKJSON({requestId, _}) => requestId
       | ReplyOKBuffer({requestId, _}) => requestId
       | ReplyError({requestId, _}) => requestId
+      | ReplyErrorJSON({requestId, _}) => requestId
       | Terminate => Int.max_int;
     let requestId = getRequestId(msg) |> Int32.of_int;
 
@@ -325,6 +330,11 @@ module Message = {
       writePreamble(~buffer, ~msgType=replyErrError, ~requestId);
       writeLongString(buffer, "\"" ++ error ++ "\"");
       bufferToPacket(~buffer);
+    | ReplyErrorJSON({error, _}) =>
+      writePreamble(~buffer, ~msgType=replyErrError, ~requestId);
+      let err = error |> Yojson.Safe.to_string;
+      writeLongString(buffer, err);
+      bufferToPacket(~buffer);
     };
   };
 };

src/Exthost/Protocol/Exthost_Protocol.rei

@@ -72,6 +72,10 @@ module Message: {
           requestId: int,
           error: string,
         })
+      | ReplyErrorJSON({
+          requestId: int,
+          error: Yojson.Safe.t,
+        })
       | Terminate;
   };
 

src/Exthost/Reply.re

@@ -3,6 +3,7 @@ type t =
   | OkEmpty
   | OkBuffer({bytes: Bytes.t})
   | OkJson({json: Yojson.Safe.t})
+  | ErrorJson({error: Yojson.Safe.t})
   | ErrorMessage({message: string});
 
 let none = Nothing;
@@ -11,6 +12,8 @@ let okEmpty = OkEmpty;
 
 let error = message => ErrorMessage({message: message});
 
+let errorJson = json => ErrorJson({error: json});
+
 let okBuffer = bytes => OkBuffer({bytes: bytes});
 let okJson = json => OkJson({json: json});
 
@@ -22,4 +25,6 @@ let toDebugString =
     Printf.sprintf("OkBuffer(%s)", Bytes.to_string(bytes))
   | OkJson({json}) =>
     Printf.sprintf("OkJson(%s)", Yojson.Safe.to_string(json))
+  | ErrorJson({error}) =>
+    Printf.sprintf("ErrorMessage(%s)", Yojson.Safe.to_string(error))
   | ErrorMessage({message}) => Printf.sprintf("ErrorMessage(%s)", message);

src/Feature/FileSystem/Feature_FileSystem.re

@@ -8,6 +8,10 @@ module Internal = {
     v |> Json.Encode.encode_value(encoder) |> Reply.okJson;
   };
 
+  let mapEncoderError = (encoder, v) => {
+    v |> Json.Encode.encode_value(encoder) |> Reply.errorJson;
+  };
+
   let fileTypeFromStat: Luv.File.Stat.t => Files.FileType.t =
     (statResult: Luv.File.Stat.t) => {
       Luv.File.(
@@ -103,7 +107,7 @@ let update = (msg, model) => {
           Exthost.Files.FileSystemError.(
             make(Code.fileNotFound)
             |> Lwt.return
-            |> Lwt.map(Internal.mapEncoder(encode))
+            |> Lwt.map(Internal.mapEncoderError(encode))
           )
         },
       );
@@ -128,7 +132,7 @@ let update = (msg, model) => {
           Exthost.Files.FileSystemError.(
             make(Code.fileNotFound)
             |> Lwt.return
-            |> Lwt.map(Internal.mapEncoder(encode))
+            |> Lwt.map(Internal.mapEncoderError(encode))
           )
         },
       );
@@ -148,7 +152,7 @@ let update = (msg, model) => {
           Exthost.Files.FileSystemError.(
             make(Code.fileNotFound)
             |> Lwt.return
-            |> Lwt.map(Internal.mapEncoder(encode))
+            |> Lwt.map(Internal.mapEncoderError(encode))
           )
         },
       );
@@ -196,7 +200,7 @@ let update = (msg, model) => {
           Exthost.Files.FileSystemError.(
             make(Code.fileNotFound)
             |> Lwt.return
-            |> Lwt.map(Internal.mapEncoder(encode))
+            |> Lwt.map(Internal.mapEncoderError(encode))
           )
         },
       );

test/Exthost/FileSystemTest.re

@@ -70,7 +70,16 @@ describe("FileSystem", ({describe, _}) => {
 
       let failHandler =
         fun
-        | Msg.FileSystem(Stat(_)) => Lwt.fail_with("Error!")
+        | Msg.FileSystem(Stat(_)) => {
+            let err = Exthost.Files.FileSystemError.(make(Code.fileNotFound));
+            Lwt.return(
+              err
+              |> Oni_Core.Json.Encode.encode_value(
+                   Exthost.Files.FileSystemError.encode,
+                 )
+              |> Reply.errorJson,
+            );
+          }
         | _ => Lwt.return(Reply.okEmpty);
 
       test("success case", _ => {