From 48bc2281c74732559c74c2d678895cdfeec08126 Mon Sep 17 00:00:00 2001
From: har7an <99636919+har7an@users.noreply.github.com>
Date: Wed, 9 Nov 2022 07:28:02 +0000
Subject: [PATCH] Fix: better error reporting when failing to load plugins
 (#1912) (#1914)

* utils/input/plugins: Return `Result` when loading

a plugin and failing to find it. Since we look up the plugin in multiple
locations, make sure we preserve each of these lookup failures and
report them to the user. This way the user can see what locations were
actually checked.

In addition, before performing the lookup, deduplicate the array of
locations to check. This prevents errors where we look up the same
plugin multiple times in the exact same locations, which can leave a bad
impression on anyone reading it who isn't familiar with the code.

* server/wasm_vm: Remove obsolete context

which was previouly required because the function it is attached to
returned only an `Option` instead of a `Result`.
---
 zellij-server/src/wasm_vm.rs      |  1 -
 zellij-utils/src/input/plugins.rs | 39 +++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/zellij-server/src/wasm_vm.rs b/zellij-server/src/wasm_vm.rs
index a7bb81cf..f9369b51 100644
--- a/zellij-server/src/wasm_vm.rs
+++ b/zellij-server/src/wasm_vm.rs
@@ -353,7 +353,6 @@ fn start_plugin(
     // The plugins blob as stored on the filesystem
     let wasm_bytes = plugin
         .resolve_wasm_bytes(&data_dir.join("plugins/"))
-        .context("cannot resolve wasm bytes")
         .with_context(err_context)
         .fatal();
 
diff --git a/zellij-utils/src/input/plugins.rs b/zellij-utils/src/input/plugins.rs
index 55ea4283..46e1a40f 100644
--- a/zellij-utils/src/input/plugins.rs
+++ b/zellij-utils/src/input/plugins.rs
@@ -10,6 +10,7 @@ use url::Url;
 
 use super::layout::{RunPlugin, RunPluginLocation};
 pub use crate::data::PluginTag;
+use crate::errors::prelude::*;
 
 use std::collections::BTreeMap;
 use std::fmt;
@@ -98,11 +99,39 @@ impl PluginConfig {
     ///   /home/bob/.zellij/plugins/tab-bar.wasm
     /// ```
     ///
-    pub fn resolve_wasm_bytes(&self, plugin_dir: &Path) -> Option<Vec<u8>> {
-        fs::read(&self.path)
-            .or_else(|_| fs::read(&self.path.with_extension("wasm")))
-            .or_else(|_| fs::read(plugin_dir.join(&self.path).with_extension("wasm")))
-            .ok()
+    pub fn resolve_wasm_bytes(&self, plugin_dir: &Path) -> Result<Vec<u8>> {
+        let err_context =
+            |err: std::io::Error, path: &PathBuf| format!("{}: '{}'", err, path.display());
+
+        // Locations we check for valid plugins
+        let paths_arr = [
+            &self.path,
+            &self.path.with_extension("wasm"),
+            &plugin_dir.join(&self.path).with_extension("wasm"),
+        ];
+        // Throw out dupes, because it's confusing to read that zellij checked the same plugin
+        // location multiple times
+        let mut paths = paths_arr.to_vec();
+        paths.sort_unstable();
+        paths.dedup();
+
+        // This looks weird and usually we would handle errors like this differently, but in this
+        // case it's helpful for users and developers alike. This way we preserve all the lookup
+        // errors and can report all of them back. We must initialize `last_err` with something,
+        // and since the user will only get to see it when loading a plugin failed, we may as well
+        // spell it out right here.
+        let mut last_err: Result<Vec<u8>> = Err(anyhow!("failed to load plugin from disk"));
+        for path in paths {
+            match fs::read(&path) {
+                Ok(val) => return Ok(val),
+                Err(err) => {
+                    last_err = last_err.with_context(|| err_context(err, &path));
+                },
+            }
+        }
+
+        // Not reached if a plugin is found!
+        return last_err;
     }
 
     /// Sets the tab index inside of the plugin type of the run field.