Improve error messages from bad snapshots

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Fixed plugin assets flashing in on first load ([#121](https://github.com/LPGhatguy/rojo/issues/121))
 * Changed Rojo's HTTP server from Rouille to Hyper, which reduced the release size by around a megabyte.
 * Added property type inference to projects, which makes specifying services a lot easier ([#130](https://github.com/LPGhatguy/rojo/pull/130))
+* Made error messages from invalid and missing files more user-friendly
 
 ## [0.5.0 Alpha 4](https://github.com/LPGhatguy/rojo/releases/tag/v0.5.0-alpha.4) (February 8, 2019)
 * Added support for nested partitions ([#102](https://github.com/LPGhatguy/rojo/issues/102))

server/src/commands/build.rs

@@ -8,9 +8,10 @@ use log::info;
 use failure::Fail;
 
 use crate::{
-    rbx_session::construct_oneoff_tree,
-    project::{Project, ProjectLoadFuzzyError},
     imfs::{Imfs, FsError},
+    project::{Project, ProjectLoadFuzzyError},
+    rbx_session::construct_oneoff_tree,
+    rbx_snapshot::SnapshotError,
 };
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -59,6 +60,9 @@ pub enum BuildError {
 
     #[fail(display = "{}", _0)]
     FsError(#[fail(cause)] FsError),
+
+    #[fail(display = "{}", _0)]
+    SnapshotError(#[fail(cause)] SnapshotError),
 }
 
 impl_from!(BuildError {
@@ -67,6 +71,7 @@ impl_from!(BuildError {
     rbx_xml::EncodeError => XmlModelEncodeError,
     rbx_binary::EncodeError => BinaryModelEncodeError,
     FsError => FsError,
+    SnapshotError => SnapshotError,
 });
 
 pub fn build(options: &BuildOptions) -> Result<(), BuildError> {
@@ -86,7 +91,7 @@ pub fn build(options: &BuildOptions) -> Result<(), BuildError> {
 
     let mut imfs = Imfs::new();
     imfs.add_roots_from_project(&project)?;
-    let tree = construct_oneoff_tree(&project, &imfs);
+    let tree = construct_oneoff_tree(&project, &imfs)?;
     let mut file = File::create(&options.output_file)?;
 
     match output_kind {

server/src/commands/serve.rs

@@ -10,7 +10,7 @@ use crate::{
     project::{Project, ProjectLoadFuzzyError},
     web::LiveServer,
     imfs::FsError,
-    live_session::LiveSession,
+    live_session::{LiveSession, LiveSessionError},
 };
 
 const DEFAULT_PORT: u16 = 34872;
@@ -28,11 +28,15 @@ pub enum ServeError {
 
    #[fail(display = "{}", _0)]
    FsError(#[fail(cause)] FsError),
+
+   #[fail(display = "{}", _0)]
+   LiveSessionError(#[fail(cause)] LiveSessionError),
 }
 
 impl_from!(ServeError {
     ProjectLoadFuzzyError => ProjectLoadError,
     FsError => FsError,
+    LiveSessionError => LiveSessionError,
 });
 
 pub fn serve(options: &ServeOptions) -> Result<(), ServeError> {

server/src/commands/upload.rs

@@ -9,9 +9,10 @@ use failure::Fail;
 use reqwest::header::{ACCEPT, USER_AGENT, CONTENT_TYPE, COOKIE};
 
 use crate::{
-    rbx_session::construct_oneoff_tree,
-    project::{Project, ProjectLoadFuzzyError},
     imfs::{Imfs, FsError},
+    project::{Project, ProjectLoadFuzzyError},
+    rbx_session::construct_oneoff_tree,
+    rbx_snapshot::SnapshotError,
 };
 
 #[derive(Debug, Fail)]
@@ -36,6 +37,9 @@ pub enum UploadError {
 
     #[fail(display = "{}", _0)]
     FsError(#[fail(cause)] FsError),
+
+    #[fail(display = "{}", _0)]
+    SnapshotError(#[fail(cause)] SnapshotError),
 }
 
 impl_from!(UploadError {
@@ -44,6 +48,7 @@ impl_from!(UploadError {
     reqwest::Error => HttpError,
     rbx_xml::EncodeError => XmlModelEncodeError,
     FsError => FsError,
+    SnapshotError => SnapshotError,
 });
 
 #[derive(Debug)]
@@ -67,7 +72,7 @@ pub fn upload(options: &UploadOptions) -> Result<(), UploadError> {
 
     let mut imfs = Imfs::new();
     imfs.add_roots_from_project(&project)?;
-    let tree = construct_oneoff_tree(&project, &imfs);
+    let tree = construct_oneoff_tree(&project, &imfs)?;
 
     let root_id = tree.get_root_id();
     let mut contents = Vec::new();

server/src/live_session.rs

@@ -2,16 +2,33 @@ use std::{
     sync::{Arc, Mutex},
 };
 
+use failure::Fail;
+
 use crate::{
     fs_watcher::FsWatcher,
     imfs::{Imfs, FsError},
     message_queue::MessageQueue,
     project::Project,
     rbx_session::RbxSession,
+    rbx_snapshot::SnapshotError,
     session_id::SessionId,
     snapshot_reconciler::InstanceChanges,
 };
 
+#[derive(Debug, Fail)]
+pub enum LiveSessionError {
+    #[fail(display = "{}", _0)]
+    Fs(#[fail(cause)] FsError),
+
+    #[fail(display = "{}", _0)]
+    Snapshot(#[fail(cause)] SnapshotError),
+}
+
+impl_from!(LiveSessionError {
+    FsError => Fs,
+    SnapshotError => Snapshot,
+});
+
 /// Contains all of the state for a Rojo live-sync session.
 pub struct LiveSession {
     pub project: Arc<Project>,
@@ -23,7 +40,7 @@ pub struct LiveSession {
 }
 
 impl LiveSession {
-    pub fn new(project: Arc<Project>) -> Result<LiveSession, FsError> {
+    pub fn new(project: Arc<Project>) -> Result<LiveSession, LiveSessionError> {
         let imfs = {
             let mut imfs = Imfs::new();
             imfs.add_roots_from_project(&project)?;
@@ -36,7 +53,7 @@ impl LiveSession {
             Arc::clone(&project),
             Arc::clone(&imfs),
             Arc::clone(&message_queue),
-        )));
+        )?));
 
         let fs_watcher = FsWatcher::start(
             Arc::clone(&imfs),

server/src/rbx_session.rs

@@ -8,7 +8,7 @@ use std::{
 
 use rlua::Lua;
 use serde_derive::{Serialize, Deserialize};
-use log::{info, trace};
+use log::{info, trace, error};
 use rbx_dom_weak::{RbxTree, RbxId};
 
 use crate::{
@@ -17,6 +17,7 @@ use crate::{
     imfs::{Imfs, ImfsItem},
     path_map::PathMap,
     rbx_snapshot::{
+        SnapshotError,
         SnapshotContext,
         SnapshotPluginContext,
         SnapshotPluginEntry,
@@ -31,6 +32,12 @@ const INIT_SCRIPT: &str = "init.lua";
 const INIT_SERVER_SCRIPT: &str = "init.server.lua";
 const INIT_CLIENT_SCRIPT: &str = "init.client.lua";
 
+fn show_snapshot_error(path: &Path, error: SnapshotError) {
+    error!("Rojo couldn't turn one of the project's files into Roblox instances.");
+    error!("Any changes to the file have been ignored.");
+    error!("{}", error);
+}
+
 /// `source_path` or `project_definition` or both must both be Some.
 #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
 pub struct MetadataPerInstance {
@@ -66,7 +73,7 @@ impl RbxSession {
         project: Arc<Project>,
         imfs: Arc<Mutex<Imfs>>,
         message_queue: Arc<MessageQueue<InstanceChanges>>,
-    ) -> RbxSession {
+    ) -> Result<RbxSession, SnapshotError> {
         let mut instances_per_path = PathMap::new();
         let mut metadata_per_instance = HashMap::new();
 
@@ -104,16 +111,22 @@ impl RbxSession {
 
         let tree = {
             let temp_imfs = imfs.lock().unwrap();
-            reify_initial_tree(&project, &context, &temp_imfs, &mut instances_per_path, &mut metadata_per_instance)
+            reify_initial_tree(
+                &project,
+                &context,
+                &temp_imfs,
+                &mut instances_per_path,
+                &mut metadata_per_instance,
+            )?
         };
 
-        RbxSession {
+        Ok(RbxSession {
             tree,
             instances_per_path,
             metadata_per_instance,
             message_queue,
             imfs,
-        }
+        })
     }
 
     fn path_created_or_updated(&mut self, path: &Path) {
@@ -155,20 +168,24 @@ impl RbxSession {
                 let maybe_snapshot = match &instance_metadata.project_definition {
                     Some((instance_name, project_node)) => {
                         snapshot_project_node(&context, &imfs, &project_node, Cow::Owned(instance_name.clone()))
-                            .unwrap_or_else(|_| panic!("Could not generate instance snapshot for path {}", path_to_snapshot.display()))
+                            // .unwrap_or_else(|_| panic!("Could not generate instance snapshot for path {}", path_to_snapshot.display()))
                     },
                     None => {
                         snapshot_imfs_path(&context, &imfs, &path_to_snapshot, None)
-                            .unwrap_or_else(|_| panic!("Could not generate instance snapshot for path {}", path_to_snapshot.display()))
+                            // .unwrap_or_else(|_| panic!("Could not generate instance snapshot for path {}", path_to_snapshot.display()))
                     },
                 };
 
                 let snapshot = match maybe_snapshot {
-                    Some(snapshot) => snapshot,
+                    Ok(Some(snapshot)) => snapshot,
-                    None => {
+                    Ok(None) => {
                         trace!("Path resulted in no snapshot being generated.");
                         return;
                     },
+                    Err(err) => {
+                        show_snapshot_error(&path_to_snapshot, err);
+                        return;
+                    },
                 };
 
                 trace!("Snapshot: {:#?}", snapshot);
@@ -243,12 +260,13 @@ impl RbxSession {
     }
 }
 
-pub fn construct_oneoff_tree(project: &Project, imfs: &Imfs) -> RbxTree {
+pub fn construct_oneoff_tree(project: &Project, imfs: &Imfs) -> Result<RbxTree, SnapshotError> {
     let mut instances_per_path = PathMap::new();
     let mut metadata_per_instance = HashMap::new();
     let context = SnapshotContext {
         plugin_context: None,
     };
+
     reify_initial_tree(project, &context, imfs, &mut instances_per_path, &mut metadata_per_instance)
 }
 
@@ -258,13 +276,14 @@ fn reify_initial_tree(
     imfs: &Imfs,
     instances_per_path: &mut PathMap<HashSet<RbxId>>,
     metadata_per_instance: &mut HashMap<RbxId, MetadataPerInstance>,
-) -> RbxTree {
+) -> Result<RbxTree, SnapshotError> {
-    let snapshot = snapshot_project_tree(&context, imfs, project)
+    let snapshot = match snapshot_project_tree(&context, imfs, project)? {
-        .expect("Could not snapshot project tree")
+        Some(snapshot) => snapshot,
-        .expect("Project did not produce any instances");
+        None => panic!("Project did not produce any instances"),
+    };
 
     let mut changes = InstanceChanges::default();
     let tree = reify_root(&snapshot, instances_per_path, metadata_per_instance, &mut changes);
 
-    tree
+    Ok(tree)
 }

test-projects/malformed-stuff/default.project.json

@@ -0,0 +1,6 @@
+{
+  "name": "malformed-stuff",
+  "tree": {
+    "$path": "src"
+  }
+}

test-projects/malformed-stuff/src/bad-model.model.json

@@ -0,0 +1,2 @@
+ahhh this isn't a JSON model
+bamboozled again