Merge pull request #906 from Scriptwonder/fix/duplicate-detection-false-positives

Scriptwonder · web-flow · commit 546e4ec89281 · 2026-03-09T21:21:49.000-04:00
[bug-fix]Duplicate signature fix
diff --git a/MCPForUnity/Editor/Tools/ManageScript.cs b/MCPForUnity/Editor/Tools/ManageScript.cs
@@ -2276,6 +2276,9 @@ private static bool ValidateScriptSyntax(string contents, ValidationLevel level,
                 return false;
             }
 
+            // Basic structural validation: check for duplicate method signatures
+            CheckDuplicateMethodSignatures(contents, errorList);
+
 #if USE_ROSLYN
             // Advanced Roslyn-based validation: only run for Standard+; fail on Roslyn errors
             if (level >= ValidationLevel.Standard)
@@ -2518,6 +2521,57 @@ private static bool ValidateScriptSyntaxRoslyn(string contents, ValidationLevel
         }
 #endif
 
+        private static int CountTopLevelParams(string paramStr)
+        {
+            if (string.IsNullOrWhiteSpace(paramStr)) return 0;
+            int depth = 0, count = 1;
+            foreach (char c in paramStr)
+            {
+                if (c == '<' || c == '(' || c == '[') depth++;
+                else if (c == '>' || c == ')' || c == ']') depth--;
+                else if (c == ',' && depth == 0) count++;
+            }
+            return count;
+        }
+
+        /// <summary>
+        /// Extracts only the type portions from a parameter list, dropping parameter names.
+        /// e.g. "Dictionary&lt;string, int&gt; data, List&lt;Vector3&gt; points" → "Dictionary&lt;string, int&gt;, List&lt;Vector3&gt;"
+        /// </summary>
+        private static string ExtractParamTypes(string paramStr)
+        {
+            if (string.IsNullOrWhiteSpace(paramStr)) return "";
+            var types = new System.Text.StringBuilder();
+            // Split at top-level commas (respecting <> depth)
+            int depth = 0, start = 0;
+            for (int i = 0; i <= paramStr.Length; i++)
+            {
+                char c = i < paramStr.Length ? paramStr[i] : ','; // sentinel
+                if (c == '<' || c == '(' || c == '[') depth++;
+                else if (c == '>' || c == ')' || c == ']') depth--;
+                else if (c == ',' && depth == 0)
+                {
+                    string param = paramStr.Substring(start, i - start).Trim();
+                    if (types.Length > 0) types.Append(", ");
+                    // The type is everything except the last token (the name).
+                    // But if the last token ends with '>' or ']', it's all type (e.g. "List<int>").
+                    // Find the last whitespace that is NOT inside <> brackets.
+                    int lastSplit = -1;
+                    int d2 = 0;
+                    for (int j = 0; j < param.Length; j++)
+                    {
+                        char pc = param[j];
+                        if (pc == '<' || pc == '(' || pc == '[') d2++;
+                        else if (pc == '>' || pc == ')' || pc == ']') d2--;
+                        else if (d2 == 0 && char.IsWhiteSpace(pc)) lastSplit = j;
+                    }
+                    types.Append(lastSplit > 0 ? param.Substring(0, lastSplit).Trim() : param);
+                    start = i + 1;
+                }
+            }
+            return Regex.Replace(types.ToString(), @"\s+", " ");
+        }
+
         /// <summary>
         /// Validates Unity-specific coding rules and best practices
         /// //TODO: Naive Unity Checks and not really yield any results, need to be improved
@@ -2581,6 +2635,121 @@ private static void ValidateScriptSyntaxUnity(string contents, System.Collection
             {
                 errors.Add("WARNING: String concatenation in Update() can cause garbage collection issues");
             }
+
+        }
+
+        /// <summary>
+        /// Checks for duplicate method signatures (same name + param types + scope).
+        /// Catches corruption from anchor_replace overshoot and similar edit mistakes.
+        /// Runs at Basic level since duplicates are structural errors, not style warnings.
+        /// </summary>
+        private static void CheckDuplicateMethodSignatures(string contents, System.Collections.Generic.List<string> errors)
+        {
+            // Step 1: Build a code-only view (comments/strings replaced with spaces)
+            var codeChars = contents.ToCharArray();
+            {
+                var lexer = new CSharpLexer(contents);
+                while (true)
+                {
+                    int startPos = lexer.Position;
+                    if (!lexer.Advance(out _)) break;
+                    if (lexer.InNonCode)
+                    {
+                        for (int i = startPos; i < lexer.Position && i < codeChars.Length; i++)
+                            if (codeChars[i] != '\n') codeChars[i] = ' ';
+                    }
+                }
+            }
+            var codeOnly = new string(codeChars);
+
+            // Step 2: Build containing type name at each position via single pass
+            var typePattern = new Regex(
+                @"\b(?:class|struct|interface|record)\s+(\w+)",
+                RegexOptions.CultureInvariant, TimeSpan.FromSeconds(2));
+            // Map opening-brace position -> fully qualified type name
+            var typeBraceMap = new System.Collections.Generic.Dictionary<int, string>();
+            {
+                var typeMatches = typePattern.Matches(codeOnly);
+                // Pre-scan: for each type declaration, find its opening brace and record full name
+                // We need to process in order and track nesting, so do a single forward pass
+                var preStack = new System.Collections.Generic.List<(string name, int openDepth)>();
+                int preBd = 0;
+                int nextTm = 0;
+                for (int i = 0; i < codeOnly.Length; i++)
+                {
+                    while (nextTm < typeMatches.Count && typeMatches[nextTm].Index == i)
+                    {
+                        int bp = codeOnly.IndexOf('{', typeMatches[nextTm].Index + typeMatches[nextTm].Length);
+                        if (bp >= 0)
+                        {
+                            string tn = typeMatches[nextTm].Groups[1].Value;
+                            string fn = preStack.Count > 0 ? preStack[preStack.Count - 1].name + "." + tn : tn;
+                            typeBraceMap[bp] = fn;
+                        }
+                        nextTm++;
+                    }
+                    if (codeOnly[i] == '{')
+                    {
+                        if (typeBraceMap.TryGetValue(i, out string mappedType))
+                            preStack.Add((mappedType, preBd));
+                        preBd++;
+                    }
+                    else if (codeOnly[i] == '}')
+                    {
+                        preBd = Math.Max(0, preBd - 1);
+                        if (preStack.Count > 0 && preStack[preStack.Count - 1].openDepth == preBd)
+                            preStack.RemoveAt(preStack.Count - 1);
+                    }
+                }
+            }
+            // Second pass: build per-position containing type array
+            var containingTypeArr = new string[codeOnly.Length];
+            {
+                var stack = new System.Collections.Generic.List<(string name, int openDepth)>();
+                int bd2 = 0;
+                string current = "";
+                for (int i = 0; i < codeOnly.Length; i++)
+                {
+                    if (codeOnly[i] == '{')
+                    {
+                        if (typeBraceMap.TryGetValue(i, out string tn))
+                        {
+                            stack.Add((tn, bd2));
+                            current = tn;
+                        }
+                        bd2++;
+                    }
+                    else if (codeOnly[i] == '}')
+                    {
+                        bd2 = Math.Max(0, bd2 - 1);
+                        if (stack.Count > 0 && stack[stack.Count - 1].openDepth == bd2)
+                        {
+                            stack.RemoveAt(stack.Count - 1);
+                            current = stack.Count > 0 ? stack[stack.Count - 1].name : "";
+                        }
+                    }
+                    containingTypeArr[i] = current;
+                }
+            }
+
+            // Step 3: Match method signatures on code-only text (includes => for expression-bodied)
+            var methodSigPattern = new Regex(
+                @"(?:(?:public|private|protected|internal)\s+)?(?:(?:static|virtual|override|abstract|sealed|async|new)\s+)*\S+\s+(\w+)\s*\(([^)]*)\)\s*(?:where\s+\S+\s*:\s*\S+\s*)?(?:[{;]|=>)",
+                RegexOptions.Multiline | RegexOptions.CultureInvariant, TimeSpan.FromSeconds(2));
+            var sigMatches = methodSigPattern.Matches(codeOnly);
+            var seen = new System.Collections.Generic.Dictionary<string, int>(System.StringComparer.Ordinal);
+            foreach (Match sm in sigMatches)
+            {
+                string methodName = sm.Groups[1].Value;
+                int paramCount = CountTopLevelParams(sm.Groups[2].Value);
+                string paramTypes = ExtractParamTypes(sm.Groups[2].Value);
+                string containingType = containingTypeArr[sm.Index];
+                string key = $"{containingType}/{methodName}/{paramCount}/{paramTypes}";
+                if (seen.TryGetValue(key, out _))
+                    errors.Add($"ERROR: Duplicate method signature detected: '{methodName}' with {paramCount} parameter(s). This may indicate a corrupted edit.");
+                else
+                    seen[key] = 1;
+            }
         }
 
         /// <summary>
diff --git a/Server/src/services/tools/script_apply_edits.py b/Server/src/services/tools/script_apply_edits.py
@@ -678,7 +678,7 @@ def _err(code: str, message: str, *, expected: dict[str, Any] | None = None, rew
     - Use replace_method/delete_method for whole-method changes (keeps signatures balanced)
     - Avoid whole-file regex deletes; validators will guard unbalanced braces
     - For tail insertions, prefer anchor/regex_replace on final brace (class closing)
-    - Pass options.validate='standard' for structural checks; 'relaxed' for interior-only edits
+    - Pass options.validate='standard' for structural checks; 'basic' for interior-only edits
     Canonical fields (use these exact keys):
     - op: replace_method | insert_method | delete_method | anchor_insert | anchor_delete | anchor_replace
     - className: string (defaults to 'name' if omitted on method/class ops)
diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs
@@ -1,4 +1,6 @@
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using NUnit.Framework;
 using UnityEngine;
 using Newtonsoft.Json.Linq;
@@ -135,5 +137,154 @@ private bool BasicBalanceCheck(string contents)
 
             return braceCount == 0;
         }
+
+        /// <summary>
+        /// Calls ValidateScriptSyntax via reflection and returns the error list.
+        /// This exercises CheckDuplicateMethodSignatures (called from ValidateScriptSyntax).
+        /// </summary>
+        private List<string> CallValidateScriptSyntaxUnity(string contents)
+        {
+            var validationLevelType = typeof(ManageScript).GetNestedType("ValidationLevel",
+                BindingFlags.NonPublic);
+            Assert.IsNotNull(validationLevelType, "ValidationLevel enum must exist");
+            var basicLevel = Enum.ToObject(validationLevelType, 0); // ValidationLevel.Basic
+
+            var method = typeof(ManageScript).GetMethod("ValidateScriptSyntax",
+                BindingFlags.NonPublic | BindingFlags.Static, null,
+                new[] { typeof(string), validationLevelType, typeof(string[]).MakeByRefType() }, null);
+            Assert.IsNotNull(method, "ValidateScriptSyntax method must exist");
+
+            var args = new object[] { contents, basicLevel, null };
+            method.Invoke(null, args);
+            var errArray = (string[])args[2];
+            return errArray != null ? errArray.ToList() : new List<string>();
+        }
+
+        private bool HasDuplicateMethodError(List<string> errors)
+        {
+            return errors.Any(e => e.Contains("Duplicate method signature detected"));
+        }
+
+        // --- Duplicate method detection: false positive tests ---
+
+        [Test]
+        public void DuplicateDetection_LineCommentedMethod_NotFlagged()
+        {
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public void DoStuff(int x) { }
+    // public void DoStuff(int x) { }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsFalse(HasDuplicateMethodError(errors),
+                "A method in a line comment should not be flagged as duplicate");
+        }
+
+        [Test]
+        public void DuplicateDetection_BlockCommentedMethod_NotFlagged()
+        {
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public void DoStuff(int x) { }
+    /* public void DoStuff(int x) { } */
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsFalse(HasDuplicateMethodError(errors),
+                "A method in a block comment should not be flagged as duplicate");
+        }
+
+        [Test]
+        public void DuplicateDetection_InnerClassSameMethod_NotFlagged()
+        {
+            string code = @"using UnityEngine;
+public class Outer : MonoBehaviour
+{
+    public void Init(int x) { }
+
+    private class Inner
+    {
+        public void Init(int x) { }
+    }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsFalse(HasDuplicateMethodError(errors),
+                "Same method name in outer and inner class should not be flagged");
+        }
+
+        [Test]
+        public void DuplicateDetection_DifferentTypeOverloads_NotFlagged()
+        {
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public void Process(int x) { }
+    public void Process(string x) { }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsFalse(HasDuplicateMethodError(errors),
+                "Overloads with different param types but same count should not be flagged");
+        }
+
+        // --- Duplicate method detection: true positive tests ---
+
+        [Test]
+        public void DuplicateDetection_ExpressionBodiedDuplicate_Flagged()
+        {
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public int GetValue(int x) => x * 2;
+    public int GetValue(int x) => x * 3;
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsTrue(HasDuplicateMethodError(errors),
+                "Expression-bodied duplicate methods should be flagged");
+        }
+
+        [Test]
+        public void DuplicateDetection_ExactDuplicate_Flagged()
+        {
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public void DoStuff(int x) { }
+    public void DoStuff(int x) { }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsTrue(HasDuplicateMethodError(errors),
+                "Exact duplicate methods should be flagged");
+        }
+
+        [Test]
+        public void DuplicateDetection_SameTypeDifferentParamName_Flagged()
+        {
+            // This is the real anchor_replace corruption pattern
+            string code = @"using UnityEngine;
+public class Foo : MonoBehaviour
+{
+    public void Initialize(string name) { }
+    public void Initialize(string label) { }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsTrue(HasDuplicateMethodError(errors),
+                "Same-type different-name duplicates (corruption pattern) should be flagged");
+        }
+
+        [Test]
+        public void DuplicateDetection_GenericParamDuplicate_Flagged()
+        {
+            string code = @"using UnityEngine;
+using System.Collections.Generic;
+public class Foo : MonoBehaviour
+{
+    public void Process(Dictionary<string, int> data) { }
+    public void Process(Dictionary<string, int> other) { }
+}";
+            var errors = CallValidateScriptSyntaxUnity(code);
+            Assert.IsTrue(HasDuplicateMethodError(errors),
+                "Generic param duplicates with different names should be flagged");
+        }
     }
 }
