From a49580f8d1f864bdf3e89748f0153505f8b899e4 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Mon, 10 Aug 2026 23:47:57 -0400 Subject: [PATCH] Fix function-level merge gap handling (defects 1+2 of the gap-handling bug) Fixes both compile-breaking defects in docs/bugs/ function-level-merge-gap-handling.md: - Plain member declarations (var/default/autobind) are extracted as scope-qualified units (ScriptUnit.ScopedName, via a top-level type-range prescan) so a mod adding one participates in per-unit resolution instead of being silently dropped with vanilla's gap text (defect 2). - Insertion slots emit the inserting side's own contiguous span, preserving position relative to class braces and the separators between consecutive inserted units (defect 1); ambiguous placements (both sides inserting at a brace-carrying slot, or deleted anchors next to one) decline instead of guessing. - A post-reassembly sanity gate declines any rescue whose output has a member-shaped declaration at brace depth 0 or unbalanced braces - validated against the real broken output (fails on exactly the orphaned accessor the game rejected) with zero false positives on real vanilla r4Player.ws/player.ws/actor.ws/baseEffect.ws, all of which also round-trip byte-exact through the extended extractor. 17 new tests (145 total). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- .gitignore | 4 + WitcherScriptMerger.Core/CLAUDE.md | 42 +- .../Tools/FunctionLevelMergeEngine.cs | 382 ++++++++++++++++-- .../Tools/ScriptUnitExtractor.cs | 197 ++++++++- WitcherScriptMerger.Core/Tools/UnitAligner.cs | 24 +- .../Tools/FunctionLevelMergeEngineTests.cs | 153 +++++++ .../Tools/ScriptUnitExtractorTests.cs | 134 +++++- .../Tools/UnitAlignerTests.cs | 9 +- docs/bugs/artifacts/.gitignore | 4 + .../bugs/function-level-merge-gap-handling.md | 154 +++++++ 10 files changed, 1021 insertions(+), 82 deletions(-) create mode 100644 docs/bugs/artifacts/.gitignore create mode 100644 docs/bugs/function-level-merge-gap-handling.md diff --git a/.gitignore b/.gitignore index 059652d..95ae484 100644 --- a/.gitignore +++ b/.gitignore @@ -233,3 +233,7 @@ HANDOFF*.md # state into this repo (session logs, caches, scratch indexes), add a scoped # entry here rather than committing it - see the AI-assisted development # section of CONTRIBUTING.md. + +# Local build/packaging scratch + editor config +dist-local/ +.vscode/ diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index 53a5d6c..be736a1 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -455,13 +455,41 @@ competing deletion, a mod's gap comment not making it into the output) is record GUI's `MergeReportForm` — a deliberate deferral, not an oversight, since that's UI work on the host side rather than engine work here. -**Scope note on non-function content ("gaps"):** a gap is only ever compared, and only -ever produces a `Decisions` note (never a decline), when it sits between two vanilla -units both sides kept and neither side inserted anything at that slot — comparison uses -the same whitespace-tolerant spirit as `IsWhitespaceOnlyDifference` above, deliberately -NOT comment-stripped (unlike the extraction masking pass) since the whole point is to -detect when comment *content* differs, not just formatting. Reassembly always keeps -vanilla's own gap text verbatim regardless. +**Scope note on non-unit content ("gaps"), revised by gap-handling v2** (see +`docs/bugs/function-level-merge-gap-handling.md` for the two real, compile-breaking +defects the original design produced on a live install): + +- **Plain member declarations are units now, not gap content.** `[specifiers] var a, b + : T;`, `default x = value;`, and `autobind` declarations extract as + `ScriptUnitKind.MemberDeclaration` units, so a mod adding/editing one participates in + per-unit resolution instead of being silently reverted to vanilla's gap text (that + revert dropped mod-added declarations while the code referencing them survived — + defect 2). Unit identity is **scope-qualified** (`ScriptUnit.ScopedName`, + `"CR4Player::mCSMCR"`, states as `"Combat@CR4Player::phase"`) via a top-level + type-range prescan, because member names — unlike function names — recur across the + several classes a real `.ws` file contains; `UnitAligner` matches on `ScopedName`. +- **Insertion slots emit the inserting side's own span.** For a slot where exactly one + side inserts units, reassembly emits that side's contiguous text (its gaps + inserted + units, verbatim) between its two anchor units, instead of vanilla's gap followed by + bare concatenated unit texts — vanilla's gap can contain a class-closing brace, and + the old emission placed mod-added class members *after* it, at global scope, with + their separators eaten (defect 1). When the span's anchors aren't well-defined (a + neighboring vanilla unit deleted on the inserting side) or both sides insert at the + same slot, emission falls back to vanilla-gap-plus-line-break-synthesized units — + unless the gap carries a structural brace, in which case the file **declines** + (placement would be a guess). +- **A post-reassembly sanity gate** (`PassesReassemblySanityGate`, public so external + validation tooling can reuse it) walks the output's structural mask and declines the + rescue on any member-shaped declaration at brace depth 0 (an access modifier, + `default`, or `var`/`autobind` line — invalid WitcherScript, the exact + "`'public' has no sense for global function`" class of compile error), on negative + or nonzero final brace depth, or on output that no longer scans cleanly. Validated + against the real broken output preserved in `docs/bugs/artifacts/` (gate fails it on + exactly the orphaned accessor the game rejected) with zero false positives across + real vanilla `r4Player.ws`/`player.ws`/`actor.ws`/`baseEffect.ws`. +- A gap that still exists between two intact units is compared as before — + whitespace-tolerant, deliberately NOT comment-stripped — producing a `Decisions` note + when content differs; vanilla's gap text still wins at non-insertion slots. ## Text-merge input encoding diff --git a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs index 4c2c059..b9df6af 100644 --- a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs @@ -1,6 +1,7 @@ using System.Collections.Generic; using System.Linq; using System.Text; +using System.Text.RegularExpressions; using DiffPlex; namespace WitcherScriptMerger.Tools @@ -118,40 +119,132 @@ public static FunctionLevelMergeResult TryMerge( var merged = new StringBuilder(); for (var slot = 0; slot <= vanillaCount; ++slot) { - switch (GetGapEligibility(oldAlignment, newAlignment, slot, vanillaCount)) + var oldHasInsertions = oldAlignment.InsertionsAtSlot[slot].Count > 0; + var newHasInsertions = newAlignment.InsertionsAtSlot[slot].Count > 0; + // Set when this slot's emission ended on a synthesized (line-break- + // prefixed) unit text rather than gap text - the vanilla unit that + // follows needs its own synthesized break too, since its usual leading + // separator lived in the vanilla gap consumed before the insertions. + var endedOnSynthesizedUnit = false; + + if (!oldHasInsertions && !newHasInsertions) { - case GapEligibility.Eligible: - NoteGapMismatchIfAny( - baseDoc.Gaps[slot], - oldDoc.Gaps[GetSideGapIndex(oldAlignment, slot, vanillaCount)], - newDoc.Gaps[GetSideGapIndex(newAlignment, slot, vanillaCount)], - oldDescription, newDescription, decisions); - break; - - case GapEligibility.IneligibleDeletion: - // Unlike an insertion (already visible in the reassembled output), - // a deletion nearby means non-function content either side may have - // changed here (a default value, an undecorated var, ...) has no - // well-defined single gap index to compare at all - see - // GetGapEligibility's own comment. Silently keeping vanilla's text - // with zero signal would contradict this class's own "never empty - // content silently" contract, so a conservative, location-described - // caveat is emitted instead of a precise diff. - decisions.Add( - $"content {DescribeSlot(baseDoc.Units, slot, vanillaCount)} wasn't automatically " + - "verified because a nearby function was removed by one side - if either mod changed " + - "non-function content here, review manually."); - break; + switch (GetGapEligibility(oldAlignment, newAlignment, slot, vanillaCount)) + { + case GapEligibility.Eligible: + NoteGapMismatchIfAny( + baseDoc.Gaps[slot], + oldDoc.Gaps[GetSideGapIndex(oldAlignment, slot, vanillaCount)], + newDoc.Gaps[GetSideGapIndex(newAlignment, slot, vanillaCount)], + oldDescription, newDescription, decisions); + break; + + case GapEligibility.IneligibleDeletion: + // Non-unit content either side may have changed here has no + // well-defined single gap index to compare at all - see + // GetGapEligibility's own comment. Silently keeping vanilla's + // text with zero signal would contradict this class's own + // "never empty content silently" contract, so a conservative, + // location-described caveat is emitted instead of a precise + // diff. + decisions.Add( + $"content {DescribeSlot(baseDoc.Units, slot, vanillaCount)} wasn't automatically " + + "verified because a nearby function was removed by one side - if either mod changed " + + "non-function content here, review manually."); + break; + } + + merged.Append(baseDoc.Gaps[slot]); + } + else if (oldHasInsertions != newHasInsertions) + { + // Exactly one side inserts at this slot - the overwhelmingly common + // case. Emit that side's own contiguous span (its gaps and inserted + // units, verbatim and in its own order) instead of vanilla's gap + // followed by bare concatenated unit texts. This is the fix for + // docs/bugs/function-level-merge-gap-handling.md defect 1: vanilla's + // gap at this slot can contain structural content (a class-closing + // brace), and appending insertions after it emitted mod-added class + // members at global scope; and the separators between consecutive + // inserted units live in the *side's* gaps, which the old emission + // discarded entirely, running declarations together onto one line. + var insertingOld = oldHasInsertions; + if (!TryAppendSideInsertionSpan( + merged, slot, vanillaCount, + insertingOld ? oldDoc : newDoc, + insertingOld ? oldAlignment : newAlignment, + insertingOld ? oldText : newText, + insertingOld ? oldDescription : newDescription, + insertingOld ? newDoc : oldDoc, + insertingOld ? newAlignment : oldAlignment, + insertingOld ? newDescription : oldDescription, + baseDoc, decisions)) + { + // The span's anchors aren't well-defined (a neighboring vanilla + // unit was deleted on the inserting side). Fall back to + // vanilla-gap-plus-units - but only when vanilla's gap carries no + // structural braces (placement relative to a brace would be a + // guess), and with a synthesized line break in front of each unit + // so declarations can never run together. + if (GapHasStructuralBrace(baseDoc.Gaps[slot])) + return DeclineWithWarning( + $"function-level merge declined: mods insert new declarations {DescribeSlot(baseDoc.Units, slot, vanillaCount)}, " + + "where a neighboring vanilla unit was also removed and the surrounding content contains structural braces - " + + "placement cannot be determined safely."); + + merged.Append(baseDoc.Gaps[slot]); + foreach (var insertion in insertionsPerSlot[slot]) + AppendWithLineBreak(merged, insertion); + endedOnSynthesizedUnit = insertionsPerSlot[slot].Count > 0; + } + } + else + { + // Both sides insert at the same slot (rare). The reconciled unit list + // is already collision-checked; emission keeps vanilla's gap, so if + // that gap contains structural braces there is no safe answer to + // "before or after the brace?" for a merged list drawn from two + // different documents - decline rather than guess. + if (GapHasStructuralBrace(baseDoc.Gaps[slot])) + return DeclineWithWarning( + $"function-level merge declined: both mods insert new declarations {DescribeSlot(baseDoc.Units, slot, vanillaCount)} " + + "and the surrounding content contains structural braces - relative placement cannot be determined safely."); + + merged.Append(baseDoc.Gaps[slot]); + foreach (var insertion in insertionsPerSlot[slot]) + AppendWithLineBreak(merged, insertion); + endedOnSynthesizedUnit = insertionsPerSlot[slot].Count > 0; } - merged.Append(baseDoc.Gaps[slot]); - foreach (var insertion in insertionsPerSlot[slot]) - merged.Append(insertion); if (slot < vanillaCount) - merged.Append(resolvedUnits[slot]); + { + if (endedOnSynthesizedUnit && resolvedUnits[slot].Length > 0) + AppendWithLineBreak(merged, resolvedUnits[slot]); + else + merged.Append(resolvedUnits[slot]); + } } - return new FunctionLevelMergeResult(true, merged.ToString(), decisions); + var mergedText = merged.ToString(); + + // Post-reassembly sanity gate (docs/bugs/function-level-merge-gap-handling.md, + // "Suggested regression checks" #1): member-shaped declarations at brace + // depth 0 are invalid WitcherScript ("'public' has no sense for global + // function ..."), and unbalanced braces never compile. Either means this + // engine assembled something structurally wrong - decline, which falls back + // to the whole-file conflict-marker sidecar, rather than report a + // successful merge the game will refuse to compile. Cheap, and catches + // whole classes of future interleaving bugs, not just the two known ones. + if (!PassesReassemblySanityGate(mergedText, out var gateFailure)) + return DeclineWithWarning("function-level merge declined by output sanity check: " + gateFailure); + + return new FunctionLevelMergeResult(true, mergedText, decisions); + } + + static FunctionLevelMergeResult DeclineWithWarning(string message) + { + AppState.Notifier.ShowMessage(message, "Function-Level Merge", NotifyButtons.OK, DialogIcon.Warning); + return FunctionLevelMergeResult.Declined; } #region Per-unit resolution @@ -179,8 +272,8 @@ static string ResolveUnit( if (newText == baseText) return string.Empty; // new side never touched it either - the deletion propagates decisions.Add( - $"function {baseUnit.Name}: kept {newDescription}'s edit; {oldDescription} had deleted this " + - "function (a deletion never silently overrides a surviving edit)."); + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {newDescription}'s edit; {oldDescription} had deleted it " + + "(a deletion never silently overrides a surviving edit)."); return newText; } @@ -189,8 +282,8 @@ static string ResolveUnit( if (oldText == baseText) return string.Empty; decisions.Add( - $"function {baseUnit.Name}: kept {oldDescription}'s edit; {newDescription} had deleted this " + - "function (a deletion never silently overrides a surviving edit)."); + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {oldDescription}'s edit; {newDescription} had deleted it " + + "(a deletion never silently overrides a surviving edit)."); return oldText; } @@ -229,9 +322,9 @@ static string ResolveUnit( if (newDistinctness > oldDistinctness) { decisions.Add( - $"function {baseUnit.Name}: kept {newDescription}'s version ({newDistinctness} changed diff " + + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {newDescription}'s version ({newDistinctness} changed diff " + $"blocks vs. vanilla, more distinct than {oldDescription}'s {oldDistinctness}), discarded " + - $"{oldDescription}'s conflicting change to this function."); + $"{oldDescription}'s conflicting change here."); return newText; } @@ -240,9 +333,9 @@ static string ResolveUnit( // convention of always picking one deterministic side rather than guessing // further - see its IsWhitespaceOnlyDifference comment. decisions.Add( - $"function {baseUnit.Name}: kept {oldDescription}'s version ({oldDistinctness} changed diff " + + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {oldDescription}'s version ({oldDistinctness} changed diff " + $"blocks vs. vanilla" + (newDistinctness == oldDistinctness ? ", tied with" : ", more distinct than") + - $" {newDescription}'s {newDistinctness}), discarded {newDescription}'s conflicting change to this function."); + $" {newDescription}'s {newDistinctness}), discarded {newDescription}'s conflicting change here."); return oldText; } @@ -275,7 +368,7 @@ static List ReconcileInsertions( if (oldInsertions.Count == 0 && newInsertions.Count == 0) return new List(); - // Two insertions with the same name on ONE side (e.g. a mod's own + // Two insertions with the same scoped name on ONE side (e.g. a mod's own // copy-paste mistake) is an ambiguity this method can't safely resolve - // which occurrence did the mod author actually intend? Decline rather than // guess, same policy as the same-name-different-body-across-sides case @@ -283,15 +376,15 @@ static List ReconcileInsertions( if (HasDuplicateNames(oldInsertions) || HasDuplicateNames(newInsertions)) return null; - var newByName = newInsertions.ToDictionary(u => u.Name); + var newByName = newInsertions.ToDictionary(u => u.ScopedName); var consumedNewNames = new HashSet(); var result = new List(); foreach (var oldUnit in oldInsertions) { - if (newByName.TryGetValue(oldUnit.Name, out var newUnit)) + if (newByName.TryGetValue(oldUnit.ScopedName, out var newUnit)) { - consumedNewNames.Add(oldUnit.Name); + consumedNewNames.Add(oldUnit.ScopedName); if (oldUnit.FullText != newUnit.FullText) return null; result.Add(oldUnit.FullText); @@ -302,13 +395,218 @@ static List ReconcileInsertions( } } foreach (var newUnit in newInsertions) - if (!consumedNewNames.Contains(newUnit.Name)) + if (!consumedNewNames.Contains(newUnit.ScopedName)) result.Add(newUnit.FullText); return result; } - static bool HasDuplicateNames(List units) => units.Select(u => u.Name).Distinct().Count() != units.Count; + static bool HasDuplicateNames(List units) => units.Select(u => u.ScopedName).Distinct().Count() != units.Count; + + #endregion + + #region Insertion emission + + // Emits the inserting side's own contiguous text span for a slot: from the end + // of its unit matched to vanilla unit slot-1 (or offset 0 for the leading slot) + // to the start of its unit matched to vanilla unit slot (or end-of-text for the + // trailing slot). The span is a verbatim substring of the side's own text, so + // inserted units keep their exact position relative to any structural content + // (class braces) and their own separators. Returns false - fall back to the + // caller's conservative path - when either anchor is undefined because the + // neighboring vanilla unit was deleted on the inserting side. + static bool TryAppendSideInsertionSpan( + StringBuilder merged, int slot, int vanillaCount, + ScriptDocument insertingDoc, UnitAlignment insertingAlignment, string insertingText, string insertingDescription, + ScriptDocument otherDoc, UnitAlignment otherAlignment, string otherDescription, + ScriptDocument baseDoc, List decisions) + { + int prevSideIndex; // index into insertingDoc.Units of the unit before the span, or -1 + if (slot == 0) + { + prevSideIndex = -1; + } + else + { + var matched = insertingAlignment.MatchedSideIndex[slot - 1]; + if (!matched.HasValue) + return false; + prevSideIndex = matched.Value; + } + + int nextSideIndex; // index into insertingDoc.Units of the unit after the span, or Units.Count + if (slot == vanillaCount) + { + nextSideIndex = insertingDoc.Units.Count; + } + else + { + var matched = insertingAlignment.MatchedSideIndex[slot]; + if (!matched.HasValue) + return false; + nextSideIndex = matched.Value; + } + + var spanStart = prevSideIndex < 0 ? 0 : insertingDoc.Units[prevSideIndex].EndOffset; + var spanEnd = nextSideIndex >= insertingDoc.Units.Count ? insertingText.Length : insertingDoc.Units[nextSideIndex].StartOffset; + merged.Append(insertingText, spanStart, spanEnd - spanStart); + + // Audit notes. Taking the inserting side's span means ITS surrounding gap + // text wins over vanilla's at this slot (necessarily - the inserted + // declarations live inside it); note when that surrounding text differs + // from vanilla's beyond whitespace, and separately note when the OTHER + // side's own gap content here (comparable only if its own neighbors are + // intact) differs from vanilla and is therefore not preserved. + var insertingGapConcat = new StringBuilder(); + for (var g = prevSideIndex + 1; g <= nextSideIndex; ++g) + insertingGapConcat.Append(insertingDoc.Gaps[g]); + if (NormalizeGap(insertingGapConcat.ToString()) != NormalizeGap(baseDoc.Gaps[slot])) + decisions.Add( + $"content {DescribeSlot(baseDoc.Units, slot, vanillaCount)}: kept {insertingDescription}'s " + + "surrounding text (it inserted new declarations here); vanilla's own text at this position was superseded."); + + if (GetGapEligibilityOneSide(otherAlignment, slot, vanillaCount)) + { + var otherGap = otherDoc.Gaps[GetSideGapIndex(otherAlignment, slot, vanillaCount)]; + if (NormalizeGap(otherGap) != NormalizeGap(baseDoc.Gaps[slot])) + decisions.Add($"content from {otherDescription} near this position was not preserved ({insertingDescription}'s text kept)."); + } + + return true; + } + + // Appends unit text preceded by a line break unless the builder already ends + // with one - the conservative-fallback separator synthesis, so two declarations + // can never run together onto one line even when the side's own separator gaps + // aren't safely identifiable. + static void AppendWithLineBreak(StringBuilder merged, string text) + { + if (merged.Length > 0 && merged[merged.Length - 1] != '\n') + merged.Append("\r\n"); + merged.Append(text); + } + + // True when a gap contains a brace outside strings/comments - i.e. structural + // content (a class opening/closing brace) that makes "where do inserted units + // go relative to it?" ambiguous for any emission that isn't a verbatim + // side-span. Unparseable gap content is treated as structural (conservative). + static bool GapHasStructuralBrace(string gap) + { + if (gap.IndexOf('{') < 0 && gap.IndexOf('}') < 0) + return false; + try + { + var mask = ScriptUnitExtractor.BuildStructuralMask(gap); + return mask.IndexOf('{') >= 0 || mask.IndexOf('}') >= 0; + } + catch (ScriptUnitExtractor.ExtractionException) + { + return true; + } + } + + // Like GetGapEligibility but for one side only (the non-inserting side of a + // single-side insertion slot): its gap index for this slot is well-defined iff + // both neighboring vanilla units are matched on it. + static bool GetGapEligibilityOneSide(UnitAlignment alignment, int slot, int vanillaCount) + { + if (alignment.InsertionsAtSlot[slot].Count > 0) + return false; + if (slot > 0 && !alignment.MatchedSideIndex[slot - 1].HasValue) + return false; + if (slot < vanillaCount && !alignment.MatchedSideIndex[slot].HasValue) + return false; + return true; + } + + #endregion + + #region Output sanity gate + + // Member-shaped line starts that are invalid at brace depth 0 in WitcherScript: + // access modifiers ("'public' has no sense for global function ...", the exact + // compile error the original bug produced), default-value statements, and var + // declarations (the language has no globals). Deliberately narrow - global + // `function`/`exec function`/`statemachine class` etc. are all legal at depth 0 + // and must never trip this. + static readonly Regex DepthZeroMemberShapeRegex = new Regex( + @"^(?:(?:public|private|protected)\b|default\s+\w+\s*=|(?:(?:editable|saved|import|const)\s+)*(?:var|autobind)\s)", + RegexOptions.Compiled); + + // Walks the reassembled output's structural mask line by line, tracking brace + // depth, and fails on: a member-shaped declaration appearing at depth 0 + // (including after a closing brace on the same line - the bug's + // "}public function ..." mangling), depth ever going negative, or a + // nonzero final depth. Public (not just for tests): external validation + // tooling - e.g. a live-install regression harness checking real merged + // output - can reuse exactly the gate the engine itself applies. + public static bool PassesReassemblySanityGate(string mergedText, out string failureReason) + { + string mask; + try + { + mask = ScriptUnitExtractor.BuildStructuralMask(mergedText); + } + catch (ScriptUnitExtractor.ExtractionException ex) + { + failureReason = "reassembled output does not scan cleanly (" + ex.Message + ")"; + return false; + } + + var depth = 0; + var lineStart = 0; + while (lineStart <= mask.Length) + { + var lineEnd = mask.IndexOf('\n', lineStart); + if (lineEnd < 0) + lineEnd = mask.Length; + + // Find the first position in this line at which depth is 0, then test + // the remainder of the line from there - so a declaration mangled onto + // the tail of a class-closing-brace line is still caught. + var depthZeroAt = depth == 0 ? lineStart : -1; + for (var i = lineStart; i < lineEnd; ++i) + { + if (mask[i] == '{') + { + ++depth; + } + else if (mask[i] == '}') + { + --depth; + if (depth < 0) + { + failureReason = "unbalanced braces (extra '}') in reassembled output"; + return false; + } + if (depth == 0 && depthZeroAt < 0) + depthZeroAt = i + 1; + } + } + + if (depthZeroAt >= 0) + { + var content = mask.Substring(depthZeroAt, lineEnd - depthZeroAt).TrimStart(' ', '\t'); + if (DepthZeroMemberShapeRegex.IsMatch(content)) + { + failureReason = "member-shaped declaration at global scope: \"" + + content.TrimEnd('\r', '\n', ' ', '\t') + "\""; + return false; + } + } + + lineStart = lineEnd + 1; + } + + if (depth != 0) + { + failureReason = "unbalanced braces (unclosed '{') in reassembled output"; + return false; + } + + failureReason = null; + return true; + } #endregion diff --git a/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs b/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs index 9a7bc9d..f14a654 100644 --- a/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs +++ b/WitcherScriptMerger.Core/Tools/ScriptUnitExtractor.cs @@ -9,31 +9,56 @@ public enum ScriptUnitKind { Function, Field, + // A plain (non-@addField) member declaration: `[specifiers] var a, b : T;`, + // `default x = value;`, or `[specifiers] autobind c : T = ...;`. Promoted to + // unit status (rather than living in gap territory) because real mods add + // these to vanilla classes routinely, and gap content always reverts to + // vanilla's own text on reassembly - which silently dropped such declarations + // while the code referencing them survived, producing merged output the game + // refuses to compile (docs/bugs/function-level-merge-gap-handling.md, + // defect 2). + MemberDeclaration, } // One function/event declaration (with its body, or just a signature for a - // body-less forward/interface declaration) or one @addField-decorated field - // declaration, as a verbatim slice of the original file text. FullText always - // includes any immediately-preceding annotation lines (@wrapMethod/@addMethod/ - // @replaceMethod/@addField) - see ScriptUnitExtractor.Extract. + // body-less forward/interface declaration), one @addField-decorated field + // declaration, or one plain member declaration (var/default/autobind - see + // ScriptUnitKind.MemberDeclaration), as a verbatim slice of the original file + // text. FullText always includes any immediately-preceding annotation lines + // (@wrapMethod/@addMethod/@replaceMethod/@addField) - see + // ScriptUnitExtractor.Extract. public readonly struct ScriptUnit { public string Name { get; } + // Name qualified by the enclosing top-level type ("CR4Player::mCSMCR"), or + // just Name for a global-scope unit. This - not Name - is what UnitAligner + // matches on: member names ("owner", "isActive", ...) recur across the + // multiple classes a single real .ws file contains, so name-only identity + // would routinely mis-align a member of one class against a same-named member + // of another. (Function names were measured not to collide within real files, + // but scoping them too costs nothing and closes the same latent risk.) + public string ScopedName { get; } public ScriptUnitKind Kind { get; } public bool HasBody { get; } public int StartOffset { get; } public int EndOffset { get; } public string FullText { get; } - public ScriptUnit(string name, ScriptUnitKind kind, bool hasBody, int startOffset, int endOffset, string fullText) + public ScriptUnit(string name, string scopedName, ScriptUnitKind kind, bool hasBody, int startOffset, int endOffset, string fullText) { Name = name; + ScopedName = scopedName; Kind = kind; HasBody = hasBody; StartOffset = startOffset; EndOffset = endOffset; FullText = fullText; } + + // The human-facing noun for audit/decision messages - "function OnSpawned" vs + // "declaration mCSMCR" - so FunctionLevelMergeEngine's notes don't call a + // variable a function. + public string DescribeKind() => Kind == ScriptUnitKind.Function ? "function" : "declaration"; } // A file split into alternating gap/unit segments: Gaps[0] + Units[0].FullText + @@ -101,6 +126,31 @@ public ExtractionException(string message) : base(message) { } static readonly Regex AddFieldAnnotationRegex = new Regex(@"^@addField\s*\([^()\r\n]*\)\s*$", RegexOptions.Compiled); static readonly Regex FieldNameRegex = new Regex(@"\bvar\s+(?\w+)\s*:", RegexOptions.Compiled); + // Plain member declarations (ScriptUnitKind.MemberDeclaration). Three shapes, + // all ';'-terminated: + // [specifiers] var a, b : Type; (multi-declarator lists are one unit, + // keyed by the comma-joined name list - + // a mod splitting/merging declarators + // aligns as delete+insert, which the + // engine already resolves conservatively) + // default x = value; + // [specifiers] autobind c : Type = ...; + // ^-anchored against the mask like DeclarationRegex, so a masked-out string/ + // comment can never fake one. Local variables inside function bodies are never + // reached: Extract consumes an entire function body as one unit and resumes + // scanning after it, so member scans only ever run over between-unit territory. + static readonly Regex MemberDeclRegex = new Regex( + @"^[ \t]*(?:(?:" + SpecifierAlternation + @")\s+)*(?:(?var|autobind)\s+(?\w+(?:\s*,\s*\w+)*)\s*:|(?default)\s+(?\w+)\s*=)", + RegexOptions.Compiled | RegexOptions.Multiline); + + // Top-level type headers, for scope tracking (see ScriptUnit.ScopedName). + // Matched against the mask; types are top-level-only in WitcherScript (see this + // class's own header comment), so scanning header -> matching close brace -> + // next header never has to consider nesting. + static readonly Regex TypeHeaderRegex = new Regex( + @"\b(?class|state|struct|enum)\s+(?\w+)(?:\s+in\s+(?\w+))?", + RegexOptions.Compiled); + enum SpanKind : byte { None, @@ -117,6 +167,7 @@ public static ScriptDocument Extract(string text) var mask = BuildMask(text, kinds, blankStringsToo: true); var lineStarts = ComputeLineStarts(text); var addFieldLineStarts = FindAllAddFieldAnnotationLineStarts(mask, lineStarts); + var typeRanges = FindTypeRanges(mask); var gaps = new List(); var units = new List(); @@ -134,6 +185,7 @@ public static ScriptDocument Extract(string text) while (pos <= text.Length) { var funcMatch = DeclarationRegex.Match(mask, pos); + var memberMatch = MemberDeclRegex.Match(mask, pos); while (addFieldIndex < addFieldLineStarts.Count && addFieldLineStarts[addFieldIndex] < pos) ++addFieldIndex; @@ -141,15 +193,22 @@ public static ScriptDocument Extract(string text) var funcPos = funcMatch.Success ? funcMatch.Index : int.MaxValue; var fieldPos = fieldLineStart ?? int.MaxValue; + var memberPos = memberMatch.Success ? memberMatch.Index : int.MaxValue; - if (funcPos == int.MaxValue && fieldPos == int.MaxValue) + if (funcPos == int.MaxValue && fieldPos == int.MaxValue && memberPos == int.MaxValue) break; + // Earliest match wins. An @addField unit's own `var ...` line also + // matches MemberDeclRegex, but its annotation line sits strictly + // earlier, so the field extraction always claims it first and consumes + // through the terminating ';' before the member scan can see it. ScriptUnit unit; - if (fieldPos < funcPos) - unit = ExtractField(text, mask, lineStarts, fieldPos, cursor); + if (fieldPos <= funcPos && fieldPos <= memberPos) + unit = ExtractField(text, mask, lineStarts, fieldPos, cursor, typeRanges); + else if (memberPos < funcPos) + unit = ExtractMemberDeclaration(text, mask, memberMatch, cursor, typeRanges); else - unit = ExtractFunction(text, mask, lineStarts, funcMatch, cursor); + unit = ExtractFunction(text, mask, lineStarts, funcMatch, cursor, typeRanges); gaps.Add(text.Substring(cursor, unit.StartOffset - cursor)); units.Add(unit); @@ -184,11 +243,23 @@ public static string StripComments(string text) return BuildMask(text, kinds, blankStringsToo: false); } + // Strings AND comments blanked - the same brace-safe scratch buffer Extract + // uses internally, exposed for FunctionLevelMergeEngine's post-rescue sanity + // gate (which needs to walk brace depth over reassembled output without a + // literal/comment brace corrupting the count). Internal, not public: a + // structural scratch buffer, not part of this class's stable contract. Throws + // ExtractionException on unterminated constructs, same as Extract. + internal static string BuildStructuralMask(string text) + { + var kinds = ClassifySpans(text); + return BuildMask(text, kinds, blankStringsToo: true); + } + #endregion #region Unit extraction - static ScriptUnit ExtractFunction(string text, string mask, List lineStarts, Match declMatch, int cursor) + static ScriptUnit ExtractFunction(string text, string mask, List lineStarts, Match declMatch, int cursor, List typeRanges) { var unitStart = Math.Max(cursor, ExtendStartBackwardOverAnnotations(mask, lineStarts, declMatch.Index)); @@ -228,12 +299,13 @@ static ScriptUnit ExtractFunction(string text, string mask, List lineStarts unitEnd = closeBrace + 1; } + var name = declMatch.Groups["name"].Value; return new ScriptUnit( - declMatch.Groups["name"].Value, ScriptUnitKind.Function, hasBody, + name, QualifyName(typeRanges, declMatch.Index, name), ScriptUnitKind.Function, hasBody, unitStart, unitEnd, text.Substring(unitStart, unitEnd - unitStart)); } - static ScriptUnit ExtractField(string text, string mask, List lineStarts, int annotationLineStart, int cursor) + static ScriptUnit ExtractField(string text, string mask, List lineStarts, int annotationLineStart, int cursor, List typeRanges) { var unitStart = Math.Max(cursor, ExtendStartBackwardOverAnnotations(mask, lineStarts, annotationLineStart)); @@ -249,7 +321,106 @@ static ScriptUnit ExtractField(string text, string mask, List lineStarts, i var nameMatch = FieldNameRegex.Match(mask, annotationLineEnd, terminator - annotationLineEnd); var name = nameMatch.Success ? nameMatch.Groups["name"].Value : "@addField#" + unitStart; - return new ScriptUnit(name, ScriptUnitKind.Field, hasBody: false, unitStart, unitEnd, fullText); + return new ScriptUnit(name, QualifyName(typeRanges, annotationLineStart, name), ScriptUnitKind.Field, hasBody: false, unitStart, unitEnd, fullText); + } + + // A plain member declaration - see MemberDeclRegex for the shapes covered. The + // unit is the whole statement through its terminating ';'. `default x = ...` is + // keyed "default:x", distinct from the member variable x it initializes - a mod + // changing a default value and a mod changing the declaration are different + // edits to different statements, and conflating their identities would make one + // mod's default-value change look like an edit to the other's declaration. + static ScriptUnit ExtractMemberDeclaration(string text, string mask, Match declMatch, int cursor, List typeRanges) + { + var unitStart = Math.Max(cursor, declMatch.Index); + + var terminator = FindNextChar(mask, declMatch.Index + declMatch.Length, ';'); + if (terminator < 0) + throw new ExtractionException( + "Reached end of file looking for the ';' terminating the member declaration " + + "starting at offset " + declMatch.Index + "."); + + var unitEnd = terminator + 1; + string name; + if (declMatch.Groups["defkw"].Success) + { + name = "default:" + declMatch.Groups["defname"].Value; + } + else + { + // Multi-declarator lists ("var a, b : int;") stay one unit, keyed by the + // whitespace-normalized comma-joined list. + name = Regex.Replace(declMatch.Groups["names"].Value, @"\s+", ""); + } + + return new ScriptUnit( + name, QualifyName(typeRanges, declMatch.Index, name), ScriptUnitKind.MemberDeclaration, + hasBody: false, unitStart, unitEnd, text.Substring(unitStart, unitEnd - unitStart)); + } + + // One top-level type's body span: units whose start offset falls inside + // (OpenBrace, CloseBrace) get ScopeName as their ScopedName qualifier. + readonly struct TypeRange + { + public string ScopeName { get; } + public int OpenBrace { get; } + public int CloseBrace { get; } + + public TypeRange(string scopeName, int openBrace, int closeBrace) + { + ScopeName = scopeName; + OpenBrace = openBrace; + CloseBrace = closeBrace; + } + } + + // Finds every top-level type's body span, for scope-qualifying unit names. + // Types are top-level only in WitcherScript (never nested - see this class's + // header comment), so each header's matching close brace can be found by plain + // depth counting and the scan can resume after the header rather than after the + // body (a body-less `import class CX;`-style declaration has no braces at all). + // A `state Combat in CR4Player` header qualifies as "Combat@CR4Player" - the + // same state name recurs across parent classes in real game scripts. + static List FindTypeRanges(string mask) + { + var result = new List(); + foreach (Match header in TypeHeaderRegex.Matches(mask)) + { + // Skip a header that sits inside a previously recorded type's body - + // grammar says that can't happen, but a stray masked keyword in an + // unusual construct shouldn't corrupt scope attribution for the rest of + // the file. + if (result.Count > 0 && header.Index < result[result.Count - 1].CloseBrace) + continue; + + var terminator = FindNextSemicolonOrBrace(mask, header.Index + header.Length); + if (terminator < 0 || mask[terminator] == ';') + continue; // body-less declaration - nothing to scope + + var closeBrace = FindMatchingDelimiter(mask, terminator, '{', '}'); + if (closeBrace < 0) + throw new ExtractionException( + "Unbalanced braces in the body of type '" + header.Groups["name"].Value + + "' starting at offset " + header.Index + "."); + + var scopeName = header.Groups["parent"].Success + ? header.Groups["name"].Value + "@" + header.Groups["parent"].Value + : header.Groups["name"].Value; + result.Add(new TypeRange(scopeName, terminator, closeBrace)); + } + return result; + } + + static string QualifyName(List typeRanges, int offset, string name) + { + foreach (var range in typeRanges) + { + if (offset > range.OpenBrace && offset < range.CloseBrace) + return range.ScopeName + "::" + name; + if (range.OpenBrace > offset) + break; // ranges are in file order; nothing later can contain offset + } + return name; } // Walks backward over any immediately preceding @-annotation lines (tolerating diff --git a/WitcherScriptMerger.Core/Tools/UnitAligner.cs b/WitcherScriptMerger.Core/Tools/UnitAligner.cs index e844775..048615a 100644 --- a/WitcherScriptMerger.Core/Tools/UnitAligner.cs +++ b/WitcherScriptMerger.Core/Tools/UnitAligner.cs @@ -27,16 +27,18 @@ public UnitAlignment(IReadOnlyList matchedSideIndex, IReadOnlyList vanillaUnits, IReadOnlyList sideUnits) @@ -52,7 +54,7 @@ public static UnitAlignment Align(IReadOnlyList vanillaUnits, IReadO { for (var j = m - 1; j >= 0; --j) { - dp[i, j] = vanillaUnits[i].Name == sideUnits[j].Name + dp[i, j] = vanillaUnits[i].ScopedName == sideUnits[j].ScopedName ? dp[i + 1, j + 1] + 1 : Math.Max(dp[i + 1, j], dp[i, j + 1]); } @@ -71,7 +73,7 @@ public static UnitAlignment Align(IReadOnlyList vanillaUnits, IReadO var si = 0; while (vi < n && si < m) { - if (vanillaUnits[vi].Name == sideUnits[si].Name) + if (vanillaUnits[vi].ScopedName == sideUnits[si].ScopedName) { matched[vi] = si; ++vi; diff --git a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs index 5aad082..0d478f3 100644 --- a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs +++ b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs @@ -313,5 +313,158 @@ public void TryMerge_FunctionLevelDiffAlgorithmException_FallsBackToTiebreakRath Assert.True(result.Applied); Assert.Single(result.Decisions); } + + #region Gap-handling v2 (docs/bugs/function-level-merge-gap-handling.md) + + // Defect 1's exact shape: a mod appends new members at the END of a class, so + // its insertions align to the slot between vanilla's last class member and the + // next global-scope unit - and vanilla's gap at that slot contains the + // class-closing brace. The old emission appended the inserted units AFTER that + // brace (global scope, "'public' has no sense for global function ...") with + // their separators eaten. The fix emits the inserting side's own span, keeping + // both position and separators. + [Fact] + public void TryMerge_ModAppendsMembersAtClassEnd_InsertedInsideClassNotAfterClosingBrace() + { + var baseText = + "class C\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}\r\n\r\n" + + "exec function E()\r\n{\r\n\treturn;\r\n}\r\n"; + var oldText = + "class C\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n\r\n" + + "\tfunction B()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n\r\n" + + "\tprivate var voiceLast : float;\r\n}\r\n\r\n" + + "exec function E()\r\n{\r\n\treturn;\r\n}\r\n"; + + var result = Merge(baseText, oldText, baseText); + + Assert.True(result.Applied); + // The inserting side's span is emitted verbatim, so with the other side + // untouched the whole file should equal the inserting side's own text. + Assert.Equal(oldText, result.MergedText); + // Belt and braces: the inserted declaration sits BEFORE the class-closing + // brace, and nothing got mangled onto a brace line. + var closingBrace = result.MergedText.IndexOf("\r\n}", System.StringComparison.Ordinal); + Assert.True(result.MergedText.IndexOf("voiceLast", System.StringComparison.Ordinal) < result.MergedText.LastIndexOf("}\r\n\r\nexec", System.StringComparison.Ordinal)); + Assert.True(FunctionLevelMergeEngine.PassesReassemblySanityGate(result.MergedText, out _)); + } + + // Defect 2's exact shape: one mod adds a plain member declaration (previously + // gap content, silently reverted to vanilla), the other edits a different + // function. Both changes must survive. + [Fact] + public void TryMerge_OneModAddsMemberDeclarationOtherEditsFunction_BothSurvive() + { + var baseText = + "class C\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n\r\n" + + "\tfunction B()\r\n\t{\r\n\t\tx = 1;\r\n\t}\r\n}\r\n"; + var oldText = + "class C\r\n{\r\n\tprivate var mCS : int;\r\n\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n\r\n" + + "\tfunction B()\r\n\t{\r\n\t\tx = 1;\r\n\t}\r\n}\r\n"; + var newText = + "class C\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n\r\n" + + "\tfunction B()\r\n\t{\r\n\t\tx = 2;\r\n\t}\r\n}\r\n"; + + var result = Merge(baseText, oldText, newText); + + Assert.True(result.Applied); + Assert.Contains("private var mCS : int;", result.MergedText); + Assert.Contains("x = 2;", result.MergedText); + Assert.True(FunctionLevelMergeEngine.PassesReassemblySanityGate(result.MergedText, out _)); + } + + // A mod CHANGING a default value (not just adding one) now resolves through + // per-unit resolution: only one side touched it, so that side wins - no + // vanilla-gap revert, no note needed. + [Fact] + public void TryMerge_OneModChangesDefaultValue_ChangeSurvives() + { + var baseText = "class C\r\n{\r\n\tvar d : float;\r\n\tdefault d = 4.5f;\r\n\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}\r\n"; + var oldText = baseText.Replace("default d = 4.5f;", "default d = 9.0f;"); + + var result = Merge(baseText, oldText, baseText); + + Assert.True(result.Applied); + Assert.Contains("default d = 9.0f;", result.MergedText); + Assert.DoesNotContain("4.5f", result.MergedText); + } + + [Fact] + public void TryMerge_BothSidesInsertAtSameGlobalSlot_BothEmittedOnSeparateLines() + { + var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var oldText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("X", "\tx();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var newText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("Y", "\ty();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + + var result = Merge(baseText, oldText, newText); + + Assert.True(result.Applied); + Assert.Contains("function X()", result.MergedText); + Assert.Contains("function Y()", result.MergedText); + // Never two declarations run together on one line. + Assert.DoesNotContain("}function", result.MergedText); + Assert.True(FunctionLevelMergeEngine.PassesReassemblySanityGate(result.MergedText, out _)); + } + + // Both sides inserting into a slot whose vanilla gap carries a structural brace + // (a class boundary) has no safe placement answer - decline rather than guess. + [Fact] + public void TryMerge_BothSidesInsertAtClassBoundarySlot_Declines() + { + var baseText = + "class C\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}\r\n\r\n" + + "exec function E()\r\n{\r\n\treturn;\r\n}\r\n"; + var oldText = baseText.Replace( + "\t}\r\n}", + "\t}\r\n\r\n\tfunction FromOld()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}"); + var newText = baseText.Replace( + "\t}\r\n}", + "\t}\r\n\r\n\tfunction FromNew()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}"); + + var result = Merge(baseText, oldText, newText); + + Assert.False(result.Applied); + } + + [Fact] + public void PassesReassemblySanityGate_MemberShapedDeclarationAtGlobalScope_Fails() + { + var text = + "class C\r\n{\r\n\tvar x : int;\r\n}\r\n\r\n" + + "\tpublic function Orphaned() : float\r\n\t{\r\n\t\treturn 1;\r\n\t}\r\n"; + + Assert.False(FunctionLevelMergeEngine.PassesReassemblySanityGate(text, out var reason)); + Assert.Contains("global scope", reason); + } + + // The bug's exact mangled shape: a declaration glued onto the tail of the + // class-closing-brace line ("}private var q : int;"). + [Fact] + public void PassesReassemblySanityGate_DeclarationMangledOntoClosingBraceLine_Fails() + { + var text = "class C\r\n{\r\n\tvar x : int;\r\n}\tprivate var q : int;\r\n"; + + Assert.False(FunctionLevelMergeEngine.PassesReassemblySanityGate(text, out var reason)); + Assert.Contains("global scope", reason); + } + + [Fact] + public void PassesReassemblySanityGate_ValidGlobalScopeShapes_Pass() + { + var text = + "statemachine class CR4Player extends CPlayer\r\n{\r\n\tprivate var x : int;\r\n\tdefault x = 1;\r\n}\r\n\r\n" + + "exec function foo()\r\n{\r\n\tvar local : int;\r\n\tlocal = 1;\r\n}\r\n\r\n" + + "function globalHelper() : bool\r\n{\r\n\treturn true;\r\n}\r\n"; + + Assert.True(FunctionLevelMergeEngine.PassesReassemblySanityGate(text, out _)); + } + + [Fact] + public void PassesReassemblySanityGate_UnbalancedBraces_Fails() + { + Assert.False(FunctionLevelMergeEngine.PassesReassemblySanityGate("class C\r\n{\r\n\tvar x : int;\r\n", out var reason)); + Assert.Contains("unbalanced", reason); + } + + #endregion } } diff --git a/WitcherScriptMerger.Tests/Tools/ScriptUnitExtractorTests.cs b/WitcherScriptMerger.Tests/Tools/ScriptUnitExtractorTests.cs index 86f6a14..fcc6171 100644 --- a/WitcherScriptMerger.Tests/Tools/ScriptUnitExtractorTests.cs +++ b/WitcherScriptMerger.Tests/Tools/ScriptUnitExtractorTests.cs @@ -170,20 +170,28 @@ public void Extract_MultipleFunctionsWithGapsBetween_FindsAllInOrderAndRoundTrip var doc = ScriptUnitExtractor.Extract(text); Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); - Assert.Equal(new[] { "A", "B" }, doc.Units.Select(u => u.Name)); - Assert.Equal(3, doc.Gaps.Count); + // "var x : int;" is a MemberDeclaration unit too (gap-handling v2 - see + // docs/bugs/function-level-merge-gap-handling.md defect 2), so three units + // come back, in file order, all scope-qualified by the enclosing class. + Assert.Equal(new[] { "x", "A", "B" }, doc.Units.Select(u => u.Name)); + Assert.Equal(new[] { "Foo::x", "Foo::A", "Foo::B" }, doc.Units.Select(u => u.ScopedName)); + Assert.Equal(4, doc.Gaps.Count); } [Fact] - public void Extract_NoFunctionsAtAll_ReturnsSingleGapAndRoundTrips() + public void Extract_OnlyMemberDeclarations_ExtractedAsScopedUnitsAndRoundTrips() { var text = "class Foo\r\n{\r\n\tvar x : int;\r\n\tvar y : int;\r\n}\r\n"; var doc = ScriptUnitExtractor.Extract(text); + // Plain member declarations are units now (gap-handling v2): a mod adding + // one to a vanilla class must participate in per-unit resolution rather + // than being silently dropped with vanilla's gap text - see + // docs/bugs/function-level-merge-gap-handling.md defect 2. Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); - Assert.Empty(doc.Units); - Assert.Single(doc.Gaps); + Assert.Equal(new[] { "Foo::x", "Foo::y" }, doc.Units.Select(u => u.ScopedName)); + Assert.All(doc.Units, u => Assert.Equal(ScriptUnitKind.MemberDeclaration, u.Kind)); } [Fact] @@ -237,5 +245,121 @@ public void StripComments_BlanksCommentsButPreservesStringContentAndLineStructur Assert.Contains("keep { this }", stripped); Assert.Equal(text.Split('\n').Length, stripped.Split('\n').Length); } + + #region Member declarations & scoping (gap-handling v2) + + [Fact] + public void Extract_DefaultStatement_IsItsOwnUnitDistinctFromTheVariable() + { + var text = "class Foo\r\n{\r\n\tvar d : float;\r\n\tdefault d = 4.5f;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + // The declaration and its default are separate statements a mod can edit + // independently - separate units with distinct identities. + Assert.Equal(new[] { "Foo::d", "Foo::default:d" }, doc.Units.Select(u => u.ScopedName)); + } + + [Fact] + public void Extract_MultiDeclaratorVarLine_OneUnitKeyedByJoinedNames() + { + var text = "class Foo\r\n{\r\n\tprivate var a, b : int;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + var unit = Assert.Single(doc.Units); + Assert.Equal("Foo::a,b", unit.ScopedName); + } + + [Fact] + public void Extract_SameMemberNameInTwoClasses_ScopedNamesDiffer() + { + var text = + "class Foo\r\n{\r\n\tvar owner : int;\r\n}\r\n\r\n" + + "class Bar\r\n{\r\n\tvar owner : int;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + // Bare names collide; scoped names must not - this is what stops the + // aligner from matching Foo's member against Bar's. + Assert.Equal(new[] { "Foo::owner", "Bar::owner" }, doc.Units.Select(u => u.ScopedName)); + } + + [Fact] + public void Extract_LocalVariablesInsideFunctionBodies_AreNotUnits() + { + var text = + "class Foo\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\tvar local : int;\r\n\t\tlocal = 1;\r\n\t}\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + // The function body is consumed wholesale; the local var inside it must + // never surface as its own unit. + var unit = Assert.Single(doc.Units); + Assert.Equal("Foo::A", unit.ScopedName); + } + + [Fact] + public void Extract_StateHeader_ScopeIncludesParentClass() + { + var text = + "state Combat in CR4Player\r\n{\r\n\tvar phase : int;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + var unit = Assert.Single(doc.Units); + // Same state name recurs across parent classes in real game scripts, so + // the parent is part of the scope identity. + Assert.Equal("Combat@CR4Player::phase", unit.ScopedName); + } + + [Fact] + public void Extract_StructMember_ScopedToStruct() + { + var text = "struct SPoint\r\n{\r\n\tvar x : float;\r\n\tvar y : float;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + Assert.Equal(new[] { "SPoint::x", "SPoint::y" }, doc.Units.Select(u => u.ScopedName)); + } + + [Fact] + public void Extract_GlobalFunctionAfterClass_NotScopedToTheClass() + { + var text = + "class Foo\r\n{\r\n\tfunction A()\r\n\t{\r\n\t\treturn;\r\n\t}\r\n}\r\n\r\n" + + "exec function E()\r\n{\r\n\treturn;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + Assert.Equal(new[] { "Foo::A", "E" }, doc.Units.Select(u => u.ScopedName)); + } + + [Fact] + public void Extract_AddFieldUnit_StillWinsOverPlainMemberScanForItsOwnVarLine() + { + var text = + "@addField(CR4Player)\r\nprivate var injected : bool;\r\n\r\n" + + "@wrapMethod(CR4Player)\r\nfunction Wrapped()\r\n{\r\n\treturn;\r\n}\r\n"; + + var doc = ScriptUnitExtractor.Extract(text); + + Assert.Equal(text, ScriptUnitExtractor.Reassemble(doc)); + Assert.Equal(2, doc.Units.Count); + // The @addField's own var line must not double-extract as a plain member + // declaration - the annotation-led Field unit claims it first. + Assert.Equal(ScriptUnitKind.Field, doc.Units[0].Kind); + Assert.Equal("injected", doc.Units[0].Name); + Assert.Equal(ScriptUnitKind.Function, doc.Units[1].Kind); + } + + #endregion } } diff --git a/WitcherScriptMerger.Tests/Tools/UnitAlignerTests.cs b/WitcherScriptMerger.Tests/Tools/UnitAlignerTests.cs index 72fce21..21149f7 100644 --- a/WitcherScriptMerger.Tests/Tools/UnitAlignerTests.cs +++ b/WitcherScriptMerger.Tests/Tools/UnitAlignerTests.cs @@ -7,13 +7,14 @@ namespace WitcherScriptMerger.Tests.Tools // Regression coverage for UnitAligner - the function-level merge engine's // vanilla-vs-one-side alignment (see WitcherScriptMerger.Core/CLAUDE.md once this // lands there). Builds ScriptUnit fixtures directly via its public constructor - // (offsets/FullText are irrelevant to alignment, which only reads Name) rather than - // running the real extractor - keeps these fixtures focused purely on the - // alignment algorithm. + // (offsets/FullText are irrelevant to alignment, which only reads ScopedName) + // rather than running the real extractor - keeps these fixtures focused purely on + // the alignment algorithm. The fixtures pass the bare name as the scoped name too, + // which is exactly what the extractor produces for global-scope units. public class UnitAlignerTests { static ScriptUnit Unit(string name) => - new ScriptUnit(name, ScriptUnitKind.Function, hasBody: true, startOffset: 0, endOffset: 0, fullText: name); + new ScriptUnit(name, name, ScriptUnitKind.Function, hasBody: true, startOffset: 0, endOffset: 0, fullText: name); static List Units(params string[] names) { diff --git a/docs/bugs/artifacts/.gitignore b/docs/bugs/artifacts/.gitignore new file mode 100644 index 0000000..dccb6e6 --- /dev/null +++ b/docs/bugs/artifacts/.gitignore @@ -0,0 +1,4 @@ +# Raw third-party merge output kept as a reproduction fixture. +# CDPR-derived script content - deliberately never committed. +* +!.gitignore diff --git a/docs/bugs/function-level-merge-gap-handling.md b/docs/bugs/function-level-merge-gap-handling.md new file mode 100644 index 0000000..c806a72 --- /dev/null +++ b/docs/bugs/function-level-merge-gap-handling.md @@ -0,0 +1,154 @@ +# Function-level merge fallback corrupts non-unit content + +**Status:** open +**Found:** 2026-08-10, WSM 0.6.2 (`WitcherScriptMerger.exe mcp` → `merge_conflicts`) +**Severity:** high — produces merged output that does not compile, silently +**Component:** `WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs`, +`ScriptUnitExtractor.cs`; entered via `DiffPlexMergeEngine.TryFunctionLevelRescue` +(`DiffPlexMergeEngine.cs:236`, `:274`) + +## Summary + +When a whole-file merge can't auto-solve and `TryFunctionLevelRescue` takes over, +the *units* (functions/events/`@addField` fields) are merged correctly, but the +**gaps between them are not**. Two distinct failures come out of that, both of +which produce a `.ws` file the game refuses to compile: + +1. **Plain member declarations are dropped.** `ScriptUnitExtractor` only promotes + `@addField`-decorated fields to units, so an ordinary WitcherScript + `private var x : bool;` / `default y = 4.5f;` lives in a *gap*. On the rescue + path the accumulated side's gaps are discarded in favour of vanilla's, so + those declarations vanish while the code that references them survives. +2. **A unit is emitted outside its class, and its separators are eaten.** Two + functions were reassembled *after* the class's closing brace, and the newlines + around them were lost, running three declarations together. + +Both are reported by the engine's own audit text, but only as neutral-sounding +notes — this is the line that actually means "declarations were lost": + +``` +content from accumulated merge (…) near this position was not preserved +(vanilla formatting/content kept). +``` + +## How it was hit + +Real load order, 198 mods, game build 4.04. Three files needed the rescue: +`game\player\r4Player.ws`, `game\player\player.ws`, +`game\gameplay\damage\damageManagerProcessor.ws`. All three came out broken; +the other 38 merges in the same run were fine. + +``` +merge_conflicts(relativePaths: ) +→ merged: 41, skipped: 0 +``` + +Reordering to dodge the fallback does **not** help. The lossy pair is inherent +(accumulated ⊕ `modSmoothMovement`, accumulated ⊕ `modFatality`); moving the +last mod to the front only changes which mods sit in the accumulated prefix, and +the same content is lost. + +## Defect 1 — unit emitted outside the class + +`modImmersiveSound` declares two accessors inside `CR4Player`. In the merged +output they landed after the class-closing brace, at global scope: + +``` + } + +} ← CR4Player closes + ← blank + public function GetVoiceSetLastPlayed() : float + { + return voicesetLastPlayed; + } public function SetVoiceSetLastPlayed( time : float ) ← two decls, one line + { + voicesetLastPlayed = time; + }exec function setcam(a:int, b:bool) ← runs into the next unit +``` + +Game output: + +``` +Error [mod0000_mergedfiles]game\player\r4player.ws(15564): 'public' has no sense for global function 'GetVoiceSetLastPlayed'. +Error [mod0000_mergedfiles]game\player\r4player.ws(15567): 'public' has no sense for global function 'SetVoiceSetLastPlayed'. +``` + +Note the two symptoms are separable: wrong *position* relative to the class +brace, and lost *separators* between adjacent units. `ScriptUnitExtractor`'s +contract says `Gaps[0] + Units[0].FullText + … ` reassembles byte-for-byte, so +the defect is in how the two documents' gap/unit sequences are interleaved on +the merge path, not in `Reassemble` itself. + +## Defect 2 — dropped declarations + +Ten declaration lines present in a source mod and in neither vanilla nor the +merged output, across the three rescued files: + +| File | Mod | Lost | +|---|---|---| +| `game/player/player.ws` | modCriSlowMoCR | `public var mCSMCR : CCSMCR;` | +| `game/gameplay/damage/damageManagerProcessor.ws` | modCriSlowMoCR | `private var mCSMCR : CCSMCR;` | +| `game/player/r4Player.ws` | modCriSlowMoCR | `private var slowActive : bool;`, `private var isSlowDeathTimer : bool;`, `IsSlowActive()`, `DeactivateSlowMoCam()`, `aardSlowTimer()`, `igniSlowTimer()` | +| `game/player/r4Player.ws` | modBloodAndSteel | `public var basHeavySpeedID : int; default basHeavySpeedID = -1;` | +| `game/player/r4Player.ws` | modImmersiveSound | `private var voicesetLastPlayed : float;`, `default interactDist = 4.5f;` | + +Game output (abridged — ~30 lines of the same shape): + +``` +Error [mod0000_mergedfiles]game\player\r4player.ws(213): I dont know any 'mCSMCR' +Error [mod0000_mergedfiles]game\player\r4player.ws(235): 'mCSMCR' is not a member of 'handle:CR4Player' +Error [mod0000_mergedfiles]game\player\r4player.ws(735): I dont know any 'IsSlowActive' +Error [mod0000_mergedfiles]game\player\r4player.ws(754): I dont know any 'slowActive' +``` + +The four *functions* in that table are a subtler variant: they were not dropped +outright, they were emitted mangled onto a preceding `}` line — +`} timer function DeactivateSlowMoCam(dt : float, id : int) {` — i.e. the same +lost-separator failure as Defect 1. They compile, but exact-line comparison +against the source treats them as missing, which is a trap for any repair +tooling (it cost a round of duplicate-definition errors here). + +## Suggested regression checks + +Both are cheap to assert over merged output and would have caught this run: + +1. **No member-shaped declaration at brace depth 0.** Walk the merged file + tracking depth; flag any line matching + `^\s+(public|private|protected|editable|saved|timer|event|final)\b|^\s+function\s` + while depth is 0. Clean output scores 0; the raw fixture scores 1. +2. **No declaration present in a source and absent from the merge.** For each + source mod of the merged path, every declaration-shaped line that isn't in + vanilla must appear in the output. Compare on normalised text, not raw lines — + see the mangling note above, or this check reports false positives. +3. `ScriptUnitExtractor.Reassemble` round-trip on the *merge* path, not just the + extract path. + +## Reproduction fixture + +`docs/bugs/artifacts/r4Player.merged-raw.ws` — the untouched engine output from +the failing run (`mod0000_MergedFiles` ⊕ 11 mods, `modSmoothMovement` last). +It contains both defects: the orphaned accessors at the tail, and the ten +missing declarations. That directory is gitignored: the file is CDPR-derived +third-party script content and should not be committed. + +## Related, lesser finding + +`FileMerger.ConfirmOutputOverwrite` (`FileMerger.cs:827`) calls +`ShowMessage(..., NotifyButtons.YesNo, ...)` with **no `defaultResult`**, so +`HeadlessMergeNotifier` falls through to its generic table (`YesNo → No`). The +call sites at `FileMerger.cs:606` and `:658` run "regardless of dryRun" so a +preview predicts reality — which is right — but the consequence is that headless +and MCP `merge_conflicts` can only ever *create* merged output, never refresh it: +any conflict whose merged file already exists is reported as `skipped`, with the +prompt text on stderr as the only clue. + +``` +[Overwrite?] The output file below already exists! Overwrite? +G:\…\Mods\mod0000_MergedFiles\content\scripts\game\actor.ws +``` + +That also makes `dryRun` unable to answer "would this auto-solve?" for any +already-merged file — the skip happens before a merge is attempted. Supplying an +explicit `defaultResult` at that call site (or an opt-in overwrite/force +parameter on `merge_conflicts`) would resolve both.