always use success uid for conditionals, never use fallback success_uid

Cargo.toml

@@ -21,15 +21,18 @@ zip = ["dep:zip"]
 
 [dependencies]
 chksum-md5 = "0.1.0"
-getset = "0.1.5"
+getset = "0.1.6"
 serde = { version = "1.0.219", optional = true, features = ["derive"] }
-serde_json = "1.0.140"
+serde_json = "1.0.143"
 tracing = "0.1.41"
-zip = { version = "4.0.0", default-features = false, features = ["deflate", "time"], optional = true }
+zip = { version = "5.1.0", default-features = false, features = ["deflate", "time"], optional = true }
 
 [dev-dependencies]
-tempfile = "3.20.0"
+tempfile = "3.22.0"
 
 [package.metadata.docs.rs]
 all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
+
+[package.metadata.cargo-feature-combinations]
+exclude_features = [ "default" ]

src/datapack/command/execute/conditional.rs

@@ -1,5 +1,6 @@
 use chksum_md5 as md5;
 use std::{
+    cell::LazyCell,
     collections::HashSet,
     ops::{BitAnd, BitOr, Not},
 };
@@ -69,7 +70,7 @@ fn compile_using_data_storage(
     let then_count = then.get_count(options);
 
     let str_cond = cond.clone().compile(options, global_state, function_state);
-    let require_grouping_uid = (el.is_some() || then_count > 1).then(|| {
+    let success_uid = LazyCell::new(|| {
         // calculate a unique condition id for the else check
         let uid = function_state.request_uid();
         let pre_hash = function_state.path().to_owned() + ":" + &uid.to_string();
@@ -77,7 +78,8 @@ fn compile_using_data_storage(
         md5::hash(pre_hash).to_hex_lowercase()
     });
     #[allow(clippy::option_if_let_else)]
-    let then = if let Some(success_uid) = require_grouping_uid.as_deref() {
+    let then = if el.is_some() || then_count > 1 {
+        let success_uid = &*success_uid;
         // prepare commands for grouping
         let mut group_cmds = match then.clone() {
             Execute::Run(cmd) => vec![*cmd],
@@ -118,10 +120,7 @@ fn compile_using_data_storage(
     };
     // if the conditions have multiple parts joined by a disjunction, commands need to be grouped
     let each_or_cmd = (str_cond.len() > 1).then(|| {
-        let success_uid = require_grouping_uid.as_deref().unwrap_or_else(|| {
-            tracing::error!("No success_uid found for each_or_cmd, using default");
-            "if_success"
-        });
+        let success_uid = &*success_uid;
         (
             format!("data modify storage shulkerbox:cond {success_uid} set value true"),
             combine_conditions_commands(
@@ -135,10 +134,7 @@ fn compile_using_data_storage(
     });
     // build the condition for each then command
     let successful_cond = if each_or_cmd.is_some() {
-        let success_uid = require_grouping_uid.as_deref().unwrap_or_else(|| {
-            tracing::error!("No success_uid found for each_or_cmd, using default");
-            "if_success"
-        });
+        let success_uid = &*success_uid;
         Condition::Atom(MacroString::from(format!(
             "data storage shulkerbox:cond {{{success_uid}:1b}}"
         )))
@@ -151,10 +147,7 @@ fn compile_using_data_storage(
     // build the else part
     let el_commands = el
         .map(|el| {
-            let success_uid = require_grouping_uid.as_deref().unwrap_or_else(|| {
-                tracing::error!("No success_uid found for each_or_cmd, using default");
-                "if_success"
-            });
+            let success_uid = &*success_uid;
             let else_cond = (!Condition::Atom(MacroString::from(format!(
                 "data storage shulkerbox:cond {{{success_uid}:1b}}"
             ))))
@@ -173,10 +166,7 @@ fn compile_using_data_storage(
 
     // reset the success storage if needed
     let reset_success_storage = if each_or_cmd.is_some() || el.is_some() {
-        let success_uid = require_grouping_uid.as_deref().unwrap_or_else(|| {
-            tracing::error!("No success_uid found for each_or_cmd, using default");
-            "if_success"
-        });
+        let success_uid = &*success_uid;
         Some(
             CompiledCommand::new(format!("data remove storage shulkerbox:cond {success_uid}"))
                 .with_forbid_prefix(true),

src/datapack/command/mod.rs

@@ -334,7 +334,6 @@ impl Group {
                 let (prepare_data_storage, function_invocation) = get_group_invocation_commands(
                     &function_path,
                     namespace,
-                    commands,
                     &contained_macros,
                     block_pass_macros,
                     macro_data_storage_name,
@@ -570,7 +569,6 @@ impl Hash for Group {
 fn get_group_invocation_commands(
     function_path: &str,
     namespace: &str,
-    commands: &[Command],
     contained_macros: &HashSet<&str>,
     block_pass_macros: Option<&HashSet<String>>,
     macro_data_storage_name: Option<&str>,
@@ -579,25 +577,27 @@ fn get_group_invocation_commands(
 
     let mut function_invocation = format!("function {namespace}:{function_path}");
 
-    if commands.contains_macros() {
+    if contained_macros.is_empty() {
+        (None, Command::Raw(function_invocation))
+    } else {
         // WARNING: this seems to be the only way to pass macros to the function called.
         // Because everything is passed as a string, it looses one "level" of escaping per pass.
         let mut macros_parts = contained_macros
             .iter()
             .filter(|&&m| block_pass_macros.is_none_or(|b| !b.contains(m)))
-            .flat_map(|&m| {
-                vec![
+            .map(|&m| {
+                [
                     MacroStringPart::String(format!(r#"{m}:""#)),
                     MacroStringPart::MacroUsage(m.to_string()),
                     MacroStringPart::String(r#"""#.to_string()),
                 ]
             })
-            .fold(Vec::new(), |mut acc, part| {
+            .fold(Vec::new(), |mut acc, parts| {
                 if !acc.is_empty() {
                     acc.push(MacroStringPart::String(",".to_string()));
                 }
 
-                acc.push(part);
+                acc.extend(parts);
 
                 acc
             });
@@ -632,8 +632,6 @@ fn get_group_invocation_commands(
 
             (None, Command::UsesMacro(macro_string))
         }
-    } else {
-        (None, Command::Raw(function_invocation))
     }
 }
 
@@ -777,16 +775,11 @@ impl While {
             for cmd in &self.commands {
                 macros.extend(cmd.get_macros());
             }
+            macros.extend(self.condition.get_macros());
             macros
         };
-        let (prepare_data_storage, function_invocation) = get_group_invocation_commands(
-            &function_path,
-            namespace,
-            &self.commands,
-            &contained_macros,
-            None,
-            None,
-        );
+        let (prepare_data_storage, function_invocation) =
+            get_group_invocation_commands(&function_path, namespace, &contained_macros, None, None);
 
         let execute_tail = if let Some(prepare_datastorage_cmd) = prepare_data_storage {
             Execute::Runs(vec![prepare_datastorage_cmd, function_invocation])
@@ -807,20 +800,105 @@ impl While {
         let contains_return = commands.contains_return();
 
         if contains_return {
-            todo!("While loops with return commands are not yet supported");
-        }
+            let full_path = format!(
+                "{namespace}:{function_path}",
+                namespace = function_state.namespace()
+            );
+            let return_data_path = md5::hash(&full_path).to_hex_lowercase();
 
+            let mut pre_cmds = Command::Raw(format!(
+                "data remove storage shulkerbox:return {return_data_path}"
+            ))
+            .compile(options, global_state, function_state)
+            .into_iter()
+            .map(|c| c.with_forbid_prefix(true))
+            .collect::<Vec<_>>();
+            let post_condition = Condition::Atom(
+                format!("data storage shulkerbox:return {return_data_path}").into(),
+            );
+
+            let post_cmd_store = global_state
+                .read()
+                .unwrap()
+                .functions_with_special_return
+                .get(&format!(
+                    "{}:{}",
+                    function_state.namespace(),
+                    function_state.path()
+                ))
+                .cloned().map(|parent_return_data_path| {
+                    Command::Execute(Execute::If(
+                    post_condition.clone(),
+                    Box::new(Execute::Run(Box::new(Command::Raw(format!(
+                        "data modify storage shulkerbox:return {parent_return_data_path} set from storage shulkerbox:return {return_data_path}"
+                    ))))),
+                    None,
+                ))
+                });
+
+            let post_cmd_return = Command::Execute(Execute::If(
+                post_condition,
+                Box::new(Execute::Run(Box::new(Command::Raw(format!(
+                    "return run data get storage shulkerbox:return {return_data_path}"
+                ))))),
+                None,
+            ));
+
+            let post_cmds = post_cmd_store
+                .into_iter()
+                .chain(std::iter::once(post_cmd_return))
+                .flat_map(|cmd| cmd.compile(options, global_state, function_state))
+                .map(|c| c.with_forbid_prefix(true))
+                .collect::<Vec<_>>();
+
+            global_state
+                .write()
+                .unwrap()
+                .functions_with_special_return
+                .insert(full_path, return_data_path);
+
+            pre_cmds.extend(conditional_run_cmd.compile(options, global_state, function_state));
+            pre_cmds.extend(post_cmds);
+            pre_cmds
+        } else {
             conditional_run_cmd.compile(options, global_state, function_state)
         }
+    }
 
     #[must_use]
     pub fn get_count(&self, options: &CompileOptions) -> usize {
-        Execute::If(
+        let if_count = Execute::If(
             self.condition.clone(),
             Box::new(Execute::Run(Box::new(Command::Raw(String::new())))),
             None,
         )
-        .get_count(options)
+        .get_count(options);
+
+        let additional_return_cmds = if self.contains_return() {
+            let post_cmd_store = Command::Execute(Execute::If(
+                Condition::Atom("".into()),
+                Box::new(Execute::Run(Box::new(Command::Raw(String::new())))),
+                None,
+            ))
+            .get_count(options);
+
+            let post_cmd_return = Command::Execute(Execute::If(
+                Condition::Atom("".into()),
+                Box::new(Execute::Run(Box::new(Command::Raw(String::new())))),
+                None,
+            ))
+            .get_count(options);
+
+            let post_cmds = post_cmd_store + post_cmd_return;
+
+            Some(post_cmds + 1)
+        } else {
+            None
+        };
+
+        additional_return_cmds.map_or(if_count, |additional_return_cmds| {
+            additional_return_cmds + if_count
+        })
     }
 
     #[must_use]

src/lib.rs

@@ -10,7 +10,7 @@
 )]
 #![warn(clippy::all, clippy::pedantic, clippy::perf)]
 #![allow(clippy::missing_panics_doc, clippy::missing_const_for_fn)]
-#![cfg_attr(docsrs, feature(doc_cfg))]
+#![cfg_attr(docsrs, feature(doc_auto_cfg))]
 
 pub mod datapack;
 pub mod util;

src/virtual_fs.rs

@@ -2,7 +2,7 @@
 
 use std::collections::HashMap;
 
-#[cfg(feature = "zip")]
+#[cfg(all(feature = "fs_access", feature = "zip"))]
 use zip::ZipWriter;
 
 /// Folder representation in virtual file system