Backport #870 (optional project names) to 7.4.x (#871)

Unlike most of the other backports, this code couldn't be directly
translated so it had to be re-implemented. Luckily, it is very simple.
This implementation is a bit messy and heavy handed with potential
panics, but I think it's probably fine since file names that aren't
UTF-8 aren't really supported anyway. The original implementation is a
lot cleaner though.

The test snapshots are (almost) all identical between the 7.5
implementation and this one. The sole exception is with the path in the
`snapshot_middleware::project` test, since I didn't feel like adding a
`name` parameter to `snapshot_project` in this implementation.

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Rojo Changelog
 
 ## Unreleased Changes
+* Made the `name` field optional on project files ([#870])
+ 	Files named `default.project.json` inherit the name of the folder they're in and all other projects
+    are named as expect (e.g. `foo.project.json` becomes an Instance named `foo`)
+
+    There is no change in behavior if `name` is set.
 * Fixed incorrect results when building model pivots ([#865])
 * Fixed incorrect results when serving model pivots ([#868])
 * Rojo now converts any line endings to LF, preventing spurious diffs when syncing Lua files on Windows ([#854])
@@ -15,6 +20,7 @@
 [#854]: https://github.com/rojo-rbx/rojo/pull/854
 [#865]: https://github.com/rojo-rbx/rojo/pull/865
 [#868]: https://github.com/rojo-rbx/rojo/pull/868
+[#870]: https://github.com/rojo-rbx/rojo/pull/870
 
 ## [7.4.0] - January 16, 2024
 * Improved the visualization for array properties like Tags ([#829])

rojo-test/build-test-snapshots/end_to_end__tests__build__no_name_default_project.snap

@@ -0,0 +1,22 @@
+---
+source: tests/tests/build.rs
+expression: contents
+---
+<roblox version="4">
+  <Item class="Folder" referent="0">
+    <Properties>
+      <string name="Name">top-level</string>
+    </Properties>
+    <Item class="Folder" referent="1">
+      <Properties>
+        <string name="Name">second-level</string>
+      </Properties>
+      <Item class="IntValue" referent="2">
+        <Properties>
+          <string name="Name">third-level</string>
+          <int64 name="Value">1337</int64>
+        </Properties>
+      </Item>
+    </Item>
+  </Item>
+</roblox>

rojo-test/build-test-snapshots/end_to_end__tests__build__no_name_project.snap

@@ -0,0 +1,22 @@
+---
+source: tests/tests/build.rs
+expression: contents
+---
+<roblox version="4">
+  <Item class="Folder" referent="0">
+    <Properties>
+      <string name="Name">no_name_project</string>
+    </Properties>
+    <Item class="Folder" referent="1">
+      <Properties>
+        <string name="Name">second-level</string>
+      </Properties>
+      <Item class="BoolValue" referent="2">
+        <Properties>
+          <string name="Name">bool_value</string>
+          <bool name="Value">true</bool>
+        </Properties>
+      </Item>
+    </Item>
+  </Item>
+</roblox>

rojo-test/build-test-snapshots/end_to_end__tests__build__no_name_top_level_project.snap

@@ -0,0 +1,13 @@
+---
+source: tests/tests/build.rs
+assertion_line: 104
+expression: contents
+---
+<roblox version="4">
+  <Item class="StringValue" referent="0">
+    <Properties>
+      <string name="Name">no_name_top_level_project</string>
+      <string name="Value">If this isn't named `no_name_top_level_project`, something went wrong!</string>
+    </Properties>
+  </Item>
+</roblox>

rojo-test/build-tests/no_name_default_project/default.project.json

@@ -0,0 +1,9 @@
+{
+  "name": "top-level",
+  "tree": {
+    "$className": "Folder",
+    "second-level": {
+      "$path": "src"
+    }
+  }
+}

rojo-test/build-tests/no_name_default_project/src/third-level/default.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "IntValue",
+    "$properties": {
+      "Value": 1337
+    }
+  }
+}

rojo-test/build-tests/no_name_project/default.project.json

@@ -0,0 +1,9 @@
+{
+  "name": "no_name_project",
+  "tree": {
+    "$className": "Folder",
+    "second-level": {
+      "$path": "src"
+    }
+  }
+}

rojo-test/build-tests/no_name_project/src/bool_value.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "BoolValue",
+    "$properties": {
+      "Value": true
+    }
+  }
+}

rojo-test/build-tests/no_name_top_level_project/default.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "StringValue",
+    "$properties": {
+      "Value": "If this isn't named `no_name_top_level_project`, something went wrong!"
+    }
+  }
+}

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_all.snap

@@ -0,0 +1,39 @@
+---
+source: tests/tests/serve.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: Folder
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: top-level
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  id-3:
+    Children:
+      - id-4
+    ClassName: Folder
+    Id: id-3
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: second-level
+    Parent: id-2
+    Properties: {}
+  id-4:
+    Children: []
+    ClassName: IntValue
+    Id: id-4
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: third-level
+    Parent: id-3
+    Properties:
+      Value:
+        Int64: 1337
+messageCursor: 0
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_default_project_info.snap

@@ -0,0 +1,14 @@
+---
+source: tests/tests/serve.rs
+assertion_line: 316
+expression: redactions.redacted_yaml(info)
+---
+expectedPlaceIds: ~
+gameId: ~
+placeId: ~
+projectName: top-level
+protocolVersion: 4
+rootInstanceId: id-2
+serverVersion: "[server-version]"
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_all.snap

@@ -0,0 +1,40 @@
+---
+source: tests/tests/serve.rs
+assertion_line: 338
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: Folder
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: no_name_project
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  id-3:
+    Children:
+      - id-4
+    ClassName: Folder
+    Id: id-3
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: second-level
+    Parent: id-2
+    Properties: {}
+  id-4:
+    Children: []
+    ClassName: BoolValue
+    Id: id-4
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: bool_value
+    Parent: id-3
+    Properties:
+      Value:
+        Bool: true
+messageCursor: 0
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_project_info.snap

@@ -0,0 +1,14 @@
+---
+source: tests/tests/serve.rs
+assertion_line: 335
+expression: redactions.redacted_yaml(info)
+---
+expectedPlaceIds: ~
+gameId: ~
+placeId: ~
+projectName: no_name_project
+protocolVersion: 4
+rootInstanceId: id-2
+serverVersion: "[server-version]"
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_all.snap

@@ -0,0 +1,20 @@
+---
+source: tests/tests/serve.rs
+assertion_line: 306
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Children: []
+    ClassName: StringValue
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: no_name_top_level_project
+    Parent: "00000000000000000000000000000000"
+    Properties:
+      Value:
+        String: "If this isn't named `no_name_top_level_project`, something went wrong!"
+messageCursor: 0
+sessionId: id-1
+

rojo-test/serve-test-snapshots/end_to_end__tests__serve__no_name_top_level_project_info.snap

@@ -0,0 +1,14 @@
+---
+source: tests/tests/serve.rs
+assertion_line: 300
+expression: redactions.redacted_yaml(info)
+---
+expectedPlaceIds: ~
+gameId: ~
+placeId: ~
+projectName: no_name_top_level_project
+protocolVersion: 4
+rootInstanceId: id-2
+serverVersion: "[server-version]"
+sessionId: id-1
+

rojo-test/serve-tests/no_name_default_project/default.project.json

@@ -0,0 +1,9 @@
+{
+  "name": "top-level",
+  "tree": {
+    "$className": "Folder",
+    "second-level": {
+      "$path": "src"
+    }
+  }
+}

rojo-test/serve-tests/no_name_default_project/src/third-level/default.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "IntValue",
+    "$properties": {
+      "Value": 1337
+    }
+  }
+}

rojo-test/serve-tests/no_name_project/default.project.json

@@ -0,0 +1,9 @@
+{
+  "name": "no_name_project",
+  "tree": {
+    "$className": "Folder",
+    "second-level": {
+      "$path": "src"
+    }
+  }
+}

rojo-test/serve-tests/no_name_project/src/bool_value.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "BoolValue",
+    "$properties": {
+      "Value": true
+    }
+  }
+}

rojo-test/serve-tests/no_name_top_level_project/default.project.json

@@ -0,0 +1,8 @@
+{
+  "tree": {
+    "$className": "StringValue",
+    "$properties": {
+      "Value": "If this isn't named `no_name_top_level_project`, something went wrong!"
+    }
+  }
+}

src/project.rs

@@ -39,7 +39,7 @@ enum Error {
 #[serde(deny_unknown_fields, rename_all = "camelCase")]
 pub struct Project {
     /// The name of the top-level instance described by the project.
-    pub name: String,
+    pub name: Option<String>,
 
     /// The tree of instances described by this project. Projects always
     /// describe at least one instance.

src/serve_session.rs

@@ -110,7 +110,7 @@ impl ServeSession {
 
         log::debug!("Loading project file from {}", project_path.display());
 
-        let root_project = match vfs.read(&project_path).with_not_found()? {
+        let mut root_project = match vfs.read(&project_path).with_not_found()? {
             Some(contents) => Project::load_from_slice(&contents, &project_path)?,
             None => {
                 return Err(ServeSessionError::NoProjectFound {
@@ -118,6 +118,35 @@ impl ServeSession {
                 });
             }
         };
+        if root_project.name.is_none() {
+            if let Some(file_name) = project_path.file_name().and_then(|s| s.to_str()) {
+                if file_name == "default.project.json" {
+                    let folder_name = project_path
+                        .parent()
+                        .and_then(Path::file_name)
+                        .and_then(|s| s.to_str());
+                    if let Some(folder_name) = folder_name {
+                        root_project.name = Some(folder_name.to_string());
+                    } else {
+                        return Err(ServeSessionError::FolderNameInvalid {
+                            path: project_path.to_path_buf(),
+                        });
+                    }
+                } else if let Some(trimmed) = file_name.strip_suffix(".project.json") {
+                    root_project.name = Some(trimmed.to_string());
+                } else {
+                    return Err(ServeSessionError::ProjectNameInvalid {
+                        path: project_path.to_path_buf(),
+                    });
+                }
+            } else {
+                return Err(ServeSessionError::ProjectNameInvalid {
+                    path: project_path.to_path_buf(),
+                });
+            }
+        }
+        // Rebind it to make it no longer mutable
+        let root_project = root_project;
 
         let mut tree = RojoTree::new(InstanceSnapshot::new());
 
@@ -190,7 +219,10 @@ impl ServeSession {
     }
 
     pub fn project_name(&self) -> &str {
-        &self.root_project.name
+        self.root_project
+            .name
+            .as_ref()
+            .expect("all top-level projects must have their name set")
     }
 
     pub fn project_port(&self) -> Option<u16> {
@@ -231,6 +263,14 @@ pub enum ServeSessionError {
     )]
     NoProjectFound { path: PathBuf },
 
+    #[error("The folder for the provided project cannot be used as a project name: {}\n\
+            Consider setting the `name` field on this project.", .path.display())]
+    FolderNameInvalid { path: PathBuf },
+
+    #[error("The file name of the provided project cannot be used as a project name: {}.\n\
+            Consider setting the `name` field on this project.", .path.display())]
+    ProjectNameInvalid { path: PathBuf },
+
     #[error(transparent)]
     Io {
         #[from]

src/snapshot_middleware/project.rs

@@ -22,6 +22,25 @@ pub fn snapshot_project(
     let project = Project::load_from_slice(&vfs.read(path)?, path)
         .with_context(|| format!("File was not a valid Rojo project: {}", path.display()))?;
 
+    // This is not how I would normally do this, but this is a temporary
+    // implementation. The one in 7.5+ is better.
+    let project_name = project.name.as_deref().unwrap_or_else(|| {
+        let file_name = path
+            .file_name()
+            .and_then(|s| s.to_str())
+            .expect("project file names should be valid UTF-8");
+        if file_name == "default.project.json" {
+            path.parent()
+                .and_then(Path::file_name)
+                .and_then(|s| s.to_str())
+                .expect("default.project.json should be inside a folder with a valid UTF-8 name")
+        } else {
+            file_name
+                .strip_suffix(".project.json")
+                .expect("project file names should end with .project.json")
+        }
+    });
+
     let mut context = context.clone();
 
     let rules = project.glob_ignore_paths.iter().map(|glob| PathIgnoreRule {
@@ -37,7 +56,7 @@ pub fn snapshot_project(
             .unwrap(),
     );
 
-    match snapshot_project_node(&context, path, &project.name, &project.tree, vfs, None)? {
+    match snapshot_project_node(&context, path, project_name, &project.tree, vfs, None)? {
         Some(found_snapshot) => {
             let mut snapshot = found_snapshot;
             // Setting the instigating source to the project file path is a little
@@ -669,4 +688,36 @@ mod test {
 
         insta::assert_yaml_snapshot!(instance_snapshot);
     }
+
+    #[test]
+    fn no_name_project() {
+        let _ = env_logger::try_init();
+
+        let mut imfs = InMemoryFs::new();
+        imfs.load_snapshot(
+            "/no_name_project",
+            VfsSnapshot::dir(hashmap! {
+                "default.project.json" => VfsSnapshot::file(r#"
+                    {
+                        "tree": {
+                            "$className": "Model"
+                        }
+                    }
+                "#),
+            }),
+        )
+        .unwrap();
+
+        let vfs = Vfs::new(imfs);
+
+        let instance_snapshot = snapshot_project(
+            &InstanceContext::default(),
+            &vfs,
+            Path::new("/no_name_project/default.project.json"),
+        )
+        .expect("snapshot error")
+        .expect("snapshot returned no instances");
+
+        insta::assert_yaml_snapshot!(instance_snapshot);
+    }
 }

src/snapshot_middleware/snapshots/librojo__snapshot_middleware__project__test__no_name_project.snap

@@ -0,0 +1,18 @@
+---
+source: src/snapshot_middleware/project.rs
+expression: instance_snapshot
+---
+snapshot_id: "00000000000000000000000000000000"
+metadata:
+  ignore_unknown_instances: true
+  instigating_source:
+    Path: /no_name_project/default.project.json
+  relevant_paths:
+    - /no_name_project/default.project.json
+  context:
+    emit_legacy_scripts: true
+name: no_name_project
+class_name: Model
+properties: {}
+children: []
+

tests/tests/build.rs

@@ -59,6 +59,9 @@ gen_build_tests! {
     txt_in_folder,
     unresolved_values,
     weldconstraint,
+    no_name_default_project,
+    no_name_project,
+    no_name_top_level_project,
 }
 
 fn run_build_test(test_name: &str) {
@@ -70,7 +73,7 @@ fn run_build_test(test_name: &str) {
     let output_path = output_dir.path().join(format!("{}.rbxmx", test_name));
 
     let output = Command::new(ROJO_PATH)
-        .args(&[
+        .args([
             "build",
             input_path.to_str().unwrap(),
             "-o",

tests/tests/serve.rs

@@ -255,3 +255,57 @@ fn add_optional_folder() {
         );
     });
 }
+
+#[test]
+fn no_name_default_project() {
+    run_serve_test("no_name_default_project", |session, mut redactions| {
+        let info = session.get_api_rojo().unwrap();
+        let root_id = info.root_instance_id;
+
+        assert_yaml_snapshot!(
+            "no_name_default_project_info",
+            redactions.redacted_yaml(info)
+        );
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "no_name_default_project_all",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+    });
+}
+
+#[test]
+fn no_name_project() {
+    run_serve_test("no_name_project", |session, mut redactions| {
+        let info = session.get_api_rojo().unwrap();
+        let root_id = info.root_instance_id;
+
+        assert_yaml_snapshot!("no_name_project_info", redactions.redacted_yaml(info));
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "no_name_project_all",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+    });
+}
+
+#[test]
+fn no_name_top_level_project() {
+    run_serve_test("no_name_top_level_project", |session, mut redactions| {
+        let info = session.get_api_rojo().unwrap();
+        let root_id = info.root_instance_id;
+
+        assert_yaml_snapshot!(
+            "no_name_top_level_project_info",
+            redactions.redacted_yaml(info)
+        );
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "no_name_top_level_project_all",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+    });
+}