From 0133661ad4a3e2f7a26af6ede40da6c1480b2b25 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Moritz=20H=C3=B6lting?=
 <87192362+moritz-hoelting@users.noreply.github.com>
Date: Fri, 20 Sep 2024 16:50:40 +0200
Subject: [PATCH] implement error on conflicting function names and
 deterministic function generation order

---
 src/transpile/error.rs      | 33 +++++++++++++++++++++++++++++++--
 src/transpile/transpiler.rs | 31 ++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/src/transpile/error.rs b/src/transpile/error.rs
index b383c3b..24a50a8 100644
--- a/src/transpile/error.rs
+++ b/src/transpile/error.rs
@@ -1,6 +1,6 @@
 //! Errors that can occur during transpilation.
 
-use std::{collections::HashMap, fmt::Display};
+use std::{collections::BTreeMap, fmt::Display};
 
 use getset::Getters;
 use itertools::Itertools;
@@ -27,6 +27,8 @@ pub enum TranspileError {
     LuaDisabled,
     #[error(transparent)]
     LuaRuntimeError(#[from] LuaRuntimeError),
+    #[error(transparent)]
+    ConflictingFunctionNames(#[from] ConflictingFunctionNames),
 }
 
 /// The result of a transpilation operation.
@@ -44,7 +46,7 @@ pub struct MissingFunctionDeclaration {
 impl MissingFunctionDeclaration {
     pub(super) fn from_context(
         identifier_span: Span,
-        functions: &HashMap<(String, String), FunctionData>,
+        functions: &BTreeMap<(String, String), FunctionData>,
     ) -> Self {
         let own_name = identifier_span.str();
         let own_program_identifier = identifier_span.source_file().identifier();
@@ -163,3 +165,30 @@ impl Display for UnexpectedExpression {
 }
 
 impl std::error::Error for UnexpectedExpression {}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ConflictingFunctionNames {
+    pub definition: Span,
+    pub name: String,
+}
+
+impl Display for ConflictingFunctionNames {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "{}",
+            Message::new(
+                Severity::Error,
+                format!("the following function declaration conflicts with an existing function with name `{}`", self.name)
+            )
+        )?;
+
+        write!(
+            f,
+            "\n{}",
+            SourceCodeDisplay::new(&self.definition, Option::<u8>::None)
+        )
+    }
+}
+
+impl std::error::Error for ConflictingFunctionNames {}
diff --git a/src/transpile/transpiler.rs b/src/transpile/transpiler.rs
index 6e053e6..8711503 100644
--- a/src/transpile/transpiler.rs
+++ b/src/transpile/transpiler.rs
@@ -1,7 +1,11 @@
 //! Transpiler for `ShulkerScript`
 
 use chksum_md5 as md5;
-use std::{collections::HashMap, iter, sync::RwLock};
+use std::{
+    collections::{BTreeMap, HashMap},
+    iter,
+    sync::RwLock,
+};
 
 use shulkerbox::datapack::{self, Command, Datapack, Execute};
 
@@ -20,7 +24,7 @@ use crate::{
             Statement,
         },
     },
-    transpile::error::MissingFunctionDeclaration,
+    transpile::error::{ConflictingFunctionNames, MissingFunctionDeclaration},
 };
 
 use super::error::{TranspileError, TranspileResult, UnexpectedExpression};
@@ -30,7 +34,7 @@ use super::error::{TranspileError, TranspileResult, UnexpectedExpression};
 pub struct Transpiler {
     datapack: shulkerbox::datapack::Datapack,
     /// Key: (program identifier, function name)
-    functions: RwLock<HashMap<(String, String), FunctionData>>,
+    functions: RwLock<BTreeMap<(String, String), FunctionData>>,
     function_locations: RwLock<HashMap<(String, String), (String, bool)>>,
     aliases: RwLock<HashMap<(String, String), (String, String)>>,
 }
@@ -50,7 +54,7 @@ impl Transpiler {
     pub fn new(pack_format: u8) -> Self {
         Self {
             datapack: shulkerbox::datapack::Datapack::new(pack_format),
-            functions: RwLock::new(HashMap::new()),
+            functions: RwLock::new(BTreeMap::new()),
             function_locations: RwLock::new(HashMap::new()),
             aliases: RwLock::new(HashMap::new()),
         }
@@ -80,7 +84,7 @@ impl Transpiler {
 
         let mut always_transpile_functions = Vec::new();
 
-        #[allow(clippy::significant_drop_in_scrutinee)]
+        // #[allow(clippy::significant_drop_in_scrutinee)]
         {
             let functions = self.functions.read().unwrap();
             for (_, data) in functions.iter() {
@@ -144,6 +148,7 @@ impl Transpiler {
                         )
                     })
                     .collect();
+                #[allow(clippy::significant_drop_tightening)]
                 self.functions.write().unwrap().insert(
                     (program_identifier, name),
                     FunctionData {
@@ -268,10 +273,18 @@ impl Transpiler {
                 )
                 .unwrap_or_else(|| identifier_span.str().to_string());
 
-            let function = self
-                .datapack
-                .namespace_mut(&function_data.namespace)
-                .function_mut(&modified_name);
+            let namespace = self.datapack.namespace_mut(&function_data.namespace);
+
+            if namespace.function(&modified_name).is_some() {
+                let err = TranspileError::ConflictingFunctionNames(ConflictingFunctionNames {
+                    name: modified_name,
+                    definition: identifier_span.clone(),
+                });
+                handler.receive(err.clone());
+                return Err(err);
+            }
+
+            let function = namespace.function_mut(&modified_name);
             function.get_commands_mut().extend(commands);
 
             let function_location = format!(