diff --git a/.llm/context.md b/.llm/context.md index 606602e6..0f511925 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -141,8 +141,9 @@ frontmatter validity, index freshness, and pointer-file delegation; see and its bound; reserve `Clone()` for an acceptable `object` return. Counting loops hoist `List.Count`, interface `Count`, and UIToolkit `childCount` reads out of the condition (per-iteration property/interface dispatch, PR #73). **Never hoist a `Length`/`Count` read - when the body mutates that collection** - the inline re-read is what makes the loop - terminate. Traps: [hot-path-allocations](./skills/hot-path-allocations/SKILL.md). + when the body changes that collection's count** - the inline re-read is what makes the loop + terminate. One read per method (a count has one source of truth), and the three exceptions + to that: [hot-path-allocations](./skills/hot-path-allocations/SKILL.md). 19. Comparison operators read left-to-right in ascending order: only `<`, `<=` and `==`. Never `>` or `>=` -- write `0 <= index` and `b < a`, not `index >= 0` or `a > b` (issue #51). Enforced by `npm --prefix tooling~ run lint:comparison-direction` (pre-commit + CI; `:fix` @@ -228,7 +229,7 @@ frontmatter validity, index freshness, and pointer-file delegation; see - Completion providers always receive a context scoped to the command's own arguments: `ActiveArgumentIndex`/`PrecedingArguments` are relative to that command, and the router shifts them per routing level (`CommandCompletionContext.ForSubcommand`). Never hand a provider a parent-shifted context. -- Details: [register-terminal-command](./skills/register-terminal-command/SKILL.md). + Details: [register-terminal-command](./skills/register-terminal-command/SKILL.md). ### User-Facing Copy (STE) @@ -248,11 +249,10 @@ Details: [simple-writing](./skills/simple-writing/SKILL.md). `CHANGELOG.md` records only changes a package consumer can observe: public API and serialized-data changes, behavior changes, fixes, install-size or console-output changes. -Internal work (refactors, tooling, style enforcement, linters, measurement, CI lanes) -stays out, and so do internal numbers (timings, allocation counts, test tallies) - those -live in PR descriptions, issues, and `progress/` logs. If a user cannot observe the -difference, it does not belong in the changelog; new `Unreleased` entries keep the -existing `Added/Changed/Fixed/Removed` buckets per the policy in the file header. +Internal work (refactors, tooling, style enforcement, linters, measurement, CI lanes) stays +out, and so do internal numbers - those live in PR descriptions, issues, and `progress/` +logs. If a user cannot observe the difference it does not belong in the changelog; new +`Unreleased` entries keep the existing buckets per the policy in the file header. ### LLM Attribution (GitHub) diff --git a/.llm/skills/hot-path-allocations/SKILL.md b/.llm/skills/hot-path-allocations/SKILL.md index 48443601..744f621b 100644 --- a/.llm/skills/hot-path-allocations/SKILL.md +++ b/.llm/skills/hot-path-allocations/SKILL.md @@ -116,28 +116,50 @@ first-inserted-wins for case-variant duplicates. - Hoist `List.Count`, interface `Count`, and UIToolkit `childCount` out of a counting loop's condition: those are property or interface reads, re-dispatched per iteration. -- **Never hoist a `Length`/`Count` read when the loop body mutates that collection.** The - inline re-read is what makes the loop terminate; a hoisted value pins a stale count. Three - in-repo loops are traps that read like hoistable candidates, and all three are commented - as such: `CommandShell`'s provider and scan-assembly removals (`RemoveAt` in the body), - `CommandAutoComplete.CollapseCaseInsensitiveDuplicates`' outer rewrite loop - (`words[writeIndex] = ...` in the body - its *inner* read loop is safe and is hoisted), and - `TerminalUI`'s `while (content.childCount < logs.Count)`, which appends the labels it is - counting. Hoisting any of them is a bug, not an optimization. -- **One count per copy, and it is the one the allocation used.** - `new CommandArg[arguments.Count]` followed by - `for (int i = 0; i < materialized.Length; ++i)` states the same count twice, - in two expressions that can drift: change the allocation - a subrange, a - clamped size, a filter - and the copy silently under-runs instead of - failing. Hoist it and let it size both, the way - `BorrowedCommandArguments.ToArray` already does. This is a drift rule, not a - speed one, and it is a review rule rather than a lint rule: a token-level - linter cannot see that two expressions name the same count. +- **Never hoist a `Length`/`Count` read when the loop body changes that collection's + COUNT.** Overwriting an element (`words[writeIndex] = ...`) is not a change of count; only + an add, a remove, or a clear is. The inline re-read is what makes such a loop terminate, so + a hoisted value pins a stale count. Three in-repo loops are traps that read like hoistable + candidates: `CyclicBuffer.Resize`'s `RemoveRange`, the `Fix Invalid Fonts` `RemoveAt` loop + in `TerminalFontPackEditor`, and `TerminalUI`'s `while (content.childCount < logCount)`, + which appends the labels it is counting. Hoisting any of them is a bug, not an optimization. + `KebabCase`'s trailing `builder.Length -= 1` is a fourth, in a `while` that shrinks the + buffer it reads. `CollapseCaseInsensitiveDuplicates` used to be listed here on the same + wrong reasoning - its outer loop only overwrites elements - and was hoisted once that was + corrected, so treat an inherited justification as a claim to verify, not as a fact. +- **One read per method, so a count has one source of truth.** A `Length`/`Count` read + three or more times in one method states one number in three places, and they can + drift: change the allocation - a subrange, a clamped size, a filter - and the loop + bound silently under-runs instead of failing. Read it once into a local and let it + size the allocation and the bound, the way `BorrowedCommandArguments.ToArray` and + `LogTextSanitizer.SanitizeInto` do. This is a **drift rule, not a speed one**, and it + is a review rule rather than a lint rule: a token-level linter cannot see that three + expressions name the same count. +- **Two reasons a second read is a different number, and both leave the reads alone.** A + reviewer who hoists either introduces a bug: + 1. The loop body changes that collection's count (the rule above). + 2. The variable is reassigned between the reads, so they are genuinely different + values - `TextFieldPaste.TryApply`'s second read is the post-trim `flattened`, and + the `_history.Count` in `CommandHistoryTests` follows a `Push` that changed it. +- **A test is a third kind of "do not touch", for a different reason.** The count is what + the assertions compare: either the repetition IS the assertion + (`CountReflectsNumberOfEntries` reads `history.Count` once per push, with a different + expected value each time - hoisting passes vacuously) or the bound is the subject under + test. Either way a hoist changes what is being checked. Do not sweep tests mechanically. +- **Sweep for it, then classify every hit, and never inherit a classification.** Three or + more reads of one `X.Length`/`X.Count` in a method body, across `Runtime/`, `Editor/`, + `Tests/`, `Samples~`, `Generator~`, is a hand-reviewable list. The tally is not the + deliverable and should not be quoted: the first sweep that quoted one classified 9 of 22 + sites as hoistable and called the rest traps, and a second, independent sweep of the same + rule found 11 more hoistable sites in the same files - including one 65 lines below a site + the first sweep had already hoisted, and a site the first sweep had excused by believing + an inherited comment. Write the per-site decision, not the count. - Hoisting `string.Length`/`array.Length` is a **measured wash, not a win**: with tiered JIT disabled the inline and hoisted forms were identical at the median (0.00%, n=2000 x 7 interleaved reps, 13- and 200-char workloads). The JIT already hoists the load; the "bounds-check elision" rationale that used to sit in context.md rule 18 was never real. - Write whichever form reads clearly and keep one form per type. + Write whichever form reads clearly - except where the one-read rule above applies, which + wins. ## Probe methodology (allocation tests) @@ -182,5 +204,7 @@ first-inserted-wins for case-variant duplicates. - Shared rented buffer? Lease-guarded slots + evict oversized on return. - New mutation site on a snapshotted collection? Bump the version. - New copy or fill loop? One count, read once, sizing the allocation and the bound. +- New method that reads one count three or more times? One read, unless the loop mutates + that collection, the variable is reassigned, or it is a test (see Loop bounds). - New allocation test? Warm first; pin through `AllocationAssertions`. - New lambda argument on a memoization/registration path? Captureless means `static`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e099974..27470338 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- The package's own inspectors now escape the names they print. The custom inspectors for `TerminalUI`, `TerminalThemePack`, and `TerminalFontPack` built their tooltips and dropdown labels from theme names, font names, command names, and asset paths with no escaping, so a name carrying a bidirectional override, a zero-width joiner, an invisible tag character, or a control character read one way there and another way in the value it selected. Those tooltips and popup labels render the same visible escapes the console shows. The value an index selects is still the raw name, so `SetTheme`, `SetFont`, the theme and font dropdowns, and the disabled-command list are unchanged. One limit, the same one the log has: a name holding the literal text `Admin\u202Eexe` prints the same as a name holding a real override. - A command that throws now says so in the console. A handler exception used to escape the terminal's input path, so in a device or standalone build it produced no in-game output at all and only Unity's own logger saw it. It is now contained and reported like every other command failure - one line in the log and in the quick-launch bar's error bar, naming the command, the exception type, and its message - while `Debug.LogException` keeps the stack trace in the Editor console and the player log. Later commands still run, and `RunCommand` still answers `true`, because the command ran and the failure is on the error queue. - The quick-launch bar no longer closes over its own error. A command that ran and then reported a controlled error through the shell's error queue - a builder validation failure, a rejected value, a thrown handler - returned success to the bar, which closed and took the error text with it, so the developer saw nothing at all. The bar now keeps the error visible, keeps the rejected line editable, and reports the submission as failed. A command that printed output still keeps the bar open, unchanged. - History recall now parks the caret at the end of the recalled line. Pressing Up or Down in a line the developer had already typed left the caret where it was, so the next character landed in the middle of the command they had just recalled. A recalled line is a new value, so the caret moves with it; applying a suggestion from the bar does the same. As with every other caret write, this needs Unity 2022.1 or newer: on 2021.3 the engine places the caret after the value lands. diff --git a/Documentation~/theming.md b/Documentation~/theming.md index c97f5212..1c59f90c 100644 --- a/Documentation~/theming.md +++ b/Documentation~/theming.md @@ -12,6 +12,20 @@ packs live under `Packs/`; add your own assets to extend either list. `list-themes` and `list-fonts` print every entry in the active packs. +A font draws only the glyphs it carries, and a character it lacks shows a +missing-glyph box. All 42 shipped families cover basic Latin. Two carry +more, and `list-fonts` prints them by these names: + +- `MPLUS1Code-Regular` - CJK and kana. In `Large`, `Everything-Basic`, and + `Everything-All`. +- `NanumGothicCoding-Regular` - Hangul and kana. In `Everything-Basic` and + `Everything-All`. + +Six families carry between one and five emoji between them, so an emoji +usually shows a box. To see a script the font does not have, add a font +that carries it to a pack and set that font. The console keeps the text +whole either way. + ## Switching at runtime `TerminalUI` exposes both switches. `persist: true` stores the choice diff --git a/Editor/CustomEditors/TerminalFontPackEditor.cs b/Editor/CustomEditors/TerminalFontPackEditor.cs index 99a897e6..5e5fb3fc 100644 --- a/Editor/CustomEditors/TerminalFontPackEditor.cs +++ b/Editor/CustomEditors/TerminalFontPackEditor.cs @@ -164,7 +164,7 @@ public override void OnInspectorGUI() GUIContent loadFromCurrentDirectoryContent = new( "Load From Current Directory", - $"Loads all fonts from '{assetPath}'" + $"Loads all fonts from '{LogTextSanitizer.Sanitize(assetPath)}'" ); if (GUILayout.Button(loadFromCurrentDirectoryContent)) diff --git a/Editor/CustomEditors/TerminalThemePackEditor.cs b/Editor/CustomEditors/TerminalThemePackEditor.cs index c6b89bd1..e233395f 100644 --- a/Editor/CustomEditors/TerminalThemePackEditor.cs +++ b/Editor/CustomEditors/TerminalThemePackEditor.cs @@ -115,7 +115,7 @@ public override void OnInspectorGUI() GUIContent loadFromCurrentDirectoryContent = new( "Load From Current Directory", - $"Loads all themes from '{assetPath}'" + $"Loads all themes from '{LogTextSanitizer.Sanitize(assetPath)}'" ); if (GUILayout.Button(loadFromCurrentDirectoryContent)) diff --git a/Editor/CustomEditors/TerminalUIEditor.cs b/Editor/CustomEditors/TerminalUIEditor.cs index bfd31c7e..c65d610c 100644 --- a/Editor/CustomEditors/TerminalUIEditor.cs +++ b/Editor/CustomEditors/TerminalUIEditor.cs @@ -33,10 +33,13 @@ public sealed class TerminalUIEditor : Editor private static string[] _themeDisplayNames; private static bool _fontKeyCacheStale = true; private static string[] _fontKeyCache = Array.Empty(); + private static string[] _fontKeyLabels = Array.Empty(); private static bool _secondFontKeyCacheStale = true; private static string[] _secondFontKeyCache = Array.Empty(); + private static string[] _secondFontKeyLabels = Array.Empty(); private int _commandIndex; + private string[] _ignorableCommandLabels = Array.Empty(); private TerminalUI _lastSeen; private readonly HashSet _allCommands = new(StringComparer.OrdinalIgnoreCase); @@ -616,7 +619,7 @@ private static string[] PackNames(List themePacks) { return RefreshCache( themePacks, - static pack => pack.name, + static pack => LogTextSanitizer.Sanitize(pack.name), ref _themePackNamesSource, ref _themePackNames ); @@ -626,7 +629,7 @@ private static string[] PackNames(List fontPacks) { return RefreshCache( fontPacks, - static pack => pack.name, + static pack => LogTextSanitizer.Sanitize(pack.name), ref _fontPackNamesSource, ref _fontPackNames ); @@ -644,7 +647,8 @@ ref _themeDisplayNames /* Strips the "-theme"/"theme-" markers only when present, so clean - names never allocate a replacement string. + names never allocate a replacement string, and escapes the result + for the popup that prints it. */ private static string FriendlyThemeName(string themeName) { @@ -653,12 +657,14 @@ private static string FriendlyThemeName(string themeName) || themeName.Contains("theme-", StringComparison.OrdinalIgnoreCase) ) { - return themeName - .Replace("-theme", string.Empty, StringComparison.OrdinalIgnoreCase) - .Replace("theme-", string.Empty, StringComparison.OrdinalIgnoreCase); + return LogTextSanitizer.Sanitize( + themeName + .Replace("-theme", string.Empty, StringComparison.OrdinalIgnoreCase) + .Replace("theme-", string.Empty, StringComparison.OrdinalIgnoreCase) + ); } - return themeName; + return LogTextSanitizer.Sanitize(themeName); } /* @@ -666,12 +672,19 @@ private static string FriendlyThemeName(string themeName) view, so the caller decides when its contents may have changed (force) and otherwise the cache holds on reference + count. Rebuilds use ICollection.CopyTo, the bulk operation. + + Two arrays, not one: a key is both the text the popup prints and + the key the font dictionaries are read by, so the popup gets the + escaped copy and the lookup keeps the raw one. A same-length + change of contents is a pre-existing limit of the count stamp, + and it hits both arrays equally. */ private static string[] RefreshKeyCache( ICollection keys, bool force, ref bool stale, - ref string[] cached + ref string[] cached, + ref string[] labels ) { if (!force && !stale && cached.Length == keys.Count) @@ -681,6 +694,7 @@ ref string[] cached string[] rebuilt = new string[keys.Count]; keys.CopyTo(rebuilt, 0); + LogTextSanitizer.SanitizeInto(rebuilt, ref labels); stale = false; return cached = rebuilt; } @@ -762,7 +776,8 @@ private string[] FontKeys() _fontsByPrefix.Keys, force: false, ref _fontKeyCacheStale, - ref _fontKeyCache + ref _fontKeyCache, + ref _fontKeyLabels ); } @@ -772,7 +787,8 @@ private string[] SecondFontKeys(SortedDictionary availableFonts) availableFonts.Keys, force: _fontKeyCacheStale, ref _secondFontKeyCacheStale, - ref _secondFontKeyCache + ref _secondFontKeyCache, + ref _secondFontKeyLabels ); } @@ -1337,7 +1353,7 @@ _themeIndex is int themeIndex string selectedTheme = terminal._themePack._themeNames[themeIndex]; GUIContent setThemeContent = new( "Set Theme", - $"Will set the current theme to {selectedTheme}" + $"Will set the current theme to {LogTextSanitizer.Sanitize(selectedTheme)}" ); bool clicked = !string.Equals( selectedTheme, @@ -1418,22 +1434,23 @@ private bool CheckForIgnoredCommandUpdates(TerminalUI terminal) { string[] ignorableCommands = new string[_intermediateResults.Count]; _intermediateResults.CopyTo(ignorableCommands, 0); + LogTextSanitizer.SanitizeInto(ignorableCommands, ref _ignorableCommandLabels); EditorGUILayout.BeginHorizontal(); try { - _commandIndex = EditorGUILayout.Popup(_commandIndex, ignorableCommands); + _commandIndex = EditorGUILayout.Popup(_commandIndex, _ignorableCommandLabels); if (0 <= _commandIndex && _commandIndex < ignorableCommands.Length) { + string ignorableCommand = ignorableCommands[_commandIndex]; GUIContent ignoreContent = new( "Ignore Command", - $"Ignores the {ignorableCommands[_commandIndex]} command" + $"Ignores the {LogTextSanitizer.Sanitize(ignorableCommand)} command" ); if (GUILayout.Button(ignoreContent)) { - string command = ignorableCommands[_commandIndex]; - terminal._disabledCommands.Add(command); + terminal._disabledCommands.Add(ignorableCommand); anyChanged = true; } } @@ -1528,7 +1545,7 @@ private bool RenderSelectableFonts(TerminalUI terminal) string[] fontKeys = FontKeys(); int selectedFontKeyIndex = EditorGUILayout.Popup( _fontKey.GetValueOrDefault(-1), - fontKeys + _fontKeyLabels ); _fontKey = selectedFontKeyIndex < 0 ? null : selectedFontKeyIndex; @@ -1556,7 +1573,7 @@ _fontKey is int fontKeyIndex { int selectedSecondFontKeyIndex = EditorGUILayout.Popup( _secondFontKey.GetValueOrDefault(-1), - secondFontKeys + _secondFontKeyLabels ); _secondFontKey = selectedSecondFontKeyIndex < 0 @@ -1592,7 +1609,7 @@ _secondFontKey is int secondFontKeyIndex { GUIContent setFontContent = new( "Set Font", - $"Update the terminal's font to {selectedFont.name}" + $"Update the terminal's font to {LogTextSanitizer.Sanitize(selectedFont.name)}" ); bool clicked = selectedFont != terminal._persistedFont diff --git a/Editor/Helper/TerminalThemeStyleSheetHelper.cs b/Editor/Helper/TerminalThemeStyleSheetHelper.cs index bbc52936..16340598 100644 --- a/Editor/Helper/TerminalThemeStyleSheetHelper.cs +++ b/Editor/Helper/TerminalThemeStyleSheetHelper.cs @@ -1,4 +1,4 @@ -namespace WallstopStudios.DxCommandTerminal.Editor.Helper +namespace WallstopStudios.DxCommandTerminal.Editor.Helper { #if UNITY_EDITOR using System; @@ -73,8 +73,9 @@ public static string[] GetAvailableThemes(StyleSheet styleSheetAsset) SortedSet selectors = new(StringComparer.OrdinalIgnoreCase); + int contentLength = ussContent.Length; int lastIndex = 0; - while (lastIndex < ussContent.Length) + while (lastIndex < contentLength) { int braceIndex = ussContent.IndexOf('{', lastIndex); if (braceIndex < 0) @@ -114,7 +115,7 @@ public static string[] GetAvailableThemes(StyleSheet styleSheetAsset) int nextObjectBraceIndex = ussContent.IndexOf('}', braceIndex + 1); if (nextObjectBraceIndex < 0) { - nextObjectBraceIndex = ussContent.Length; + nextObjectBraceIndex = contentLength; } string objectContents = ussContent.Substring( selectorStartIndex, @@ -139,7 +140,7 @@ public static string[] GetAvailableThemes(StyleSheet styleSheetAsset) } int nextBraceIndex = ussContent.IndexOf('}', braceIndex + 1); - lastIndex = nextBraceIndex < 0 ? ussContent.Length : nextBraceIndex + 1; + lastIndex = nextBraceIndex < 0 ? contentLength : nextBraceIndex + 1; } string[] result = new string[selectors.Count]; diff --git a/Generator~/WallstopStudios.DxCommandTerminal.SourceGenerators/CatalogEmitter.cs b/Generator~/WallstopStudios.DxCommandTerminal.SourceGenerators/CatalogEmitter.cs index 65b2f280..82760539 100644 --- a/Generator~/WallstopStudios.DxCommandTerminal.SourceGenerators/CatalogEmitter.cs +++ b/Generator~/WallstopStudios.DxCommandTerminal.SourceGenerators/CatalogEmitter.cs @@ -80,7 +80,9 @@ kilobytes per compilation otherwise. internal static string Emit(List commands, bool preserveCatalog) { - int capacity = BaseCapacityEstimate + PerCommandCapacityEstimate * commands.Count; + /* One read: emitting never adds to or removes from the list. */ + int commandCount = commands.Count; + int capacity = BaseCapacityEstimate + PerCommandCapacityEstimate * commandCount; StringBuilder builder = RentBuilder(capacity); bool needsUnboundFinder = false; @@ -148,7 +150,7 @@ keep their per-site requirements. .Append(EntryListType) .AppendLine(" entries = new " + EntryListType + "();"); - for (int i = 0; i < commands.Count; i++) + for (int i = 0; i < commandCount; i++) { EmitCommand(builder, i, commands[i]); } @@ -162,7 +164,7 @@ keep their per-site requirements. EmitUnboundMethodFinder(builder); } - for (int i = 0; i < commands.Count; i++) + for (int i = 0; i < commandCount; i++) { CommandModel command = commands[i]; if (command.HasValidSignature && !command.DirectlyBindable) diff --git a/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll b/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll index b946554f..121006c2 100644 Binary files a/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll and b/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll differ diff --git a/Runtime/CommandTerminal/Backend/CommandAutoComplete.cs b/Runtime/CommandTerminal/Backend/CommandAutoComplete.cs index e2eae9cd..757fa901 100644 --- a/Runtime/CommandTerminal/Backend/CommandAutoComplete.cs +++ b/Runtime/CommandTerminal/Backend/CommandAutoComplete.cs @@ -75,17 +75,17 @@ private static void CollapseCaseInsensitiveDuplicates(List words) { int writeIndex = 0; int readIndex = 0; - while (readIndex < words.Count) + /* + One read, and the inner loop reuses it: the collapse overwrites + elements and reads, and only the RemoveRange below changes the + count - so the re-read that terminates a loop is not what bounds + this one. + */ + int wordCount = words.Count; + while (readIndex < wordCount) { string representative = words[readIndex]; int runEnd = readIndex + 1; - /* - The outer loop rewrites entries, so its Count read must - stay inline - hoisting it would pin a stale length. This - inner loop only reads, and it runs once per entry, so its - Count is hoisted. - */ - int wordCount = words.Count; while (runEnd < wordCount) { string candidate = words[runEnd]; diff --git a/Runtime/CommandTerminal/Backend/CommandBuilder.cs b/Runtime/CommandTerminal/Backend/CommandBuilder.cs index 893a35de..f0cf2d34 100644 --- a/Runtime/CommandTerminal/Backend/CommandBuilder.cs +++ b/Runtime/CommandTerminal/Backend/CommandBuilder.cs @@ -98,8 +98,9 @@ private static void RunTyped( int offset ) { - object[] parsed = specs.Length == 0 ? NoValues : new object[specs.Length]; - for (int i = 0; i < specs.Length; ++i) + int specCount = specs.Length; + object[] parsed = specCount == 0 ? NoValues : new object[specCount]; + for (int i = 0; i < specCount; ++i) { CommandArgument spec = specs[i]; if (spec.IsRemaining) @@ -165,14 +166,15 @@ The dispatching level guarantees required arguments private static string BuildUsageHint(string name, CommandArgument[] specs) { - if (specs.Length == 0) + int specCount = specs.Length; + if (specCount == 0) { return name; } - StringBuilder builder = new(name.Length + 16 * specs.Length); + StringBuilder builder = new(name.Length + 16 * specCount); builder.Append(name); - for (int i = 0; i < specs.Length; ++i) + for (int i = 0; i < specCount; ++i) { builder.Append(' '); specs[i].AppendUsage(builder); @@ -314,14 +316,18 @@ int offset return; } - if (!route.HasRemaining && route.Specs.Length < available) + if (!route.HasRemaining) { - owner.IssueErrorMessage( - $"'{route.FullPath}': expects at most {route.Specs.Length} argument" - + (route.Specs.Length == 1 ? string.Empty : "s") - + $"\n -> Usage: {route.PathUsage}" - ); - return; + int routeSpecCount = route.Specs.Length; + if (routeSpecCount < available) + { + owner.IssueErrorMessage( + $"'{route.FullPath}': expects at most {routeSpecCount} argument" + + (routeSpecCount == 1 ? string.Empty : "s") + + $"\n -> Usage: {route.PathUsage}" + ); + return; + } } RunTyped( @@ -486,12 +492,13 @@ public CommandBuilder Arg( ) { EnsureArgumentNameAvailable(name); - if (0 < _arguments.Count && _arguments[_arguments.Count - 1].IsRemaining) + int argumentCount = _arguments.Count; + if (0 < argumentCount && _arguments[argumentCount - 1].IsRemaining) { throw new CommandConfigurationException( CommandConfigurationFailure.InvalidRemainingArgument, $"Command '{Name}': argument '{name}' cannot follow the remaining " - + $"argument '{_arguments[_arguments.Count - 1].Name}'; the remaining " + + $"argument '{_arguments[argumentCount - 1].Name}'; the remaining " + "argument consumes every trailing token.", Name, argumentName: name diff --git a/Runtime/CommandTerminal/Backend/CommandCompatibilityBake.cs b/Runtime/CommandTerminal/Backend/CommandCompatibilityBake.cs index 71d3ac0c..1e342e63 100644 --- a/Runtime/CommandTerminal/Backend/CommandCompatibilityBake.cs +++ b/Runtime/CommandTerminal/Backend/CommandCompatibilityBake.cs @@ -220,7 +220,7 @@ out string manifest lines.Add( $" " ); - for (; i < ordered.Count; ) + for (; i < entryCount; ) { if (!IsSameAssembly(ordered[i], entry.AssemblyName)) { @@ -232,7 +232,7 @@ out string manifest $" " ); string lastMethodName = null; - for (; i < ordered.Count; ++i) + for (; i < entryCount; ++i) { if ( !IsSameAssembly(ordered[i], entry.AssemblyName) diff --git a/Runtime/CommandTerminal/Backend/CommandHistory.cs b/Runtime/CommandTerminal/Backend/CommandHistory.cs index 6d19c869..baf40bdd 100644 --- a/Runtime/CommandTerminal/Backend/CommandHistory.cs +++ b/Runtime/CommandTerminal/Backend/CommandHistory.cs @@ -78,24 +78,25 @@ public string Next(bool skipSameCommands) } _direction = 1; + int count = _history.Count; while ( skipSameCommands && 0 <= _position - && _position < _history.Count + && _position < count && _seenInDirection.Contains(_history[_position].text) ) { ++_position; } - if (0 <= _position && _position < _history.Count) + if (0 <= _position && _position < count) { string text = _history[_position].text; _seenInDirection.Add(text); return text; } - _position = _history.Count; + _position = count; return string.Empty; } @@ -108,17 +109,18 @@ public string Previous(bool skipSameCommands) } _direction = -1; + int count = _history.Count; while ( skipSameCommands && 0 <= _position - && _position < _history.Count + && _position < count && _seenInDirection.Contains(_history[_position].text) ) { --_position; } - if (0 <= _position && _position < _history.Count) + if (0 <= _position && _position < count) { string text = _history[_position].text; _seenInDirection.Add(text); diff --git a/Runtime/CommandTerminal/Backend/CommandShell.cs b/Runtime/CommandTerminal/Backend/CommandShell.cs index c9018119..b477a203 100644 --- a/Runtime/CommandTerminal/Backend/CommandShell.cs +++ b/Runtime/CommandTerminal/Backend/CommandShell.cs @@ -274,7 +274,9 @@ public static IDisposable IncludeDiscoveryAssembly(Assembly assembly) public static bool TryEatArgument(ref string stringValue, out CommandArg arg) { stringValue = stringValue.TrimStart(); - if (stringValue.Length == 0) + /* One read: stringValue is reassigned only after the last use. */ + int length = stringValue.Length; + if (length == 0) { arg = default; return false; @@ -286,7 +288,7 @@ public static bool TryEatArgument(ref string stringValue, out CommandArg arg) int closingQuoteIndex = -1; // Find the matching closing quote. - for (int i = 1; i < stringValue.Length; ++i) + for (int i = 1; i < length; ++i) { if (stringValue[i] == firstChar) { @@ -322,14 +324,13 @@ have to agree or a command runs on different arguments than the one completion edited. */ int end = 0; - while (end < stringValue.Length && !CommandTokenizer.IsSeparator(stringValue[end])) + while (end < length && !CommandTokenizer.IsSeparator(stringValue[end])) { ++end; } arg = new CommandArg(stringValue.Substring(0, end)); - stringValue = - end < stringValue.Length ? stringValue.Substring(end + 1) : string.Empty; + stringValue = end < length ? stringValue.Substring(end + 1) : string.Empty; } return true; @@ -1233,9 +1234,9 @@ occurrence wins and provider order is preserved. } } - if (writeIndex < results.Count) + if (writeIndex < resultCount) { - results.RemoveRange(writeIndex, results.Count - writeIndex); + results.RemoveRange(writeIndex, resultCount - writeIndex); } return true; diff --git a/Runtime/CommandTerminal/Themes/TerminalThemeAsset.cs b/Runtime/CommandTerminal/Themes/TerminalThemeAsset.cs index 787b7035..cfda08ee 100644 --- a/Runtime/CommandTerminal/Themes/TerminalThemeAsset.cs +++ b/Runtime/CommandTerminal/Themes/TerminalThemeAsset.cs @@ -164,12 +164,13 @@ internal static string KebabCase(string value) return "unnamed-theme"; } - using CachedStringBuilder.Scope scope = new(value.Length + 16); + int length = value.Length; + using CachedStringBuilder.Scope scope = new(length + 16); StringBuilder builder = scope.Builder; bool previousWasBoundary = true; /* Counting, not foreach: a string's enumerator is a class (rule 11). */ - for (int i = 0; i < value.Length; ++i) + for (int i = 0; i < length; ++i) { char c = value[i]; if (char.IsUpper(c)) diff --git a/Runtime/CommandTerminal/UI/CommandPaletteUI.cs b/Runtime/CommandTerminal/UI/CommandPaletteUI.cs index 6c6d0fc9..abe84513 100644 --- a/Runtime/CommandTerminal/UI/CommandPaletteUI.cs +++ b/Runtime/CommandTerminal/UI/CommandPaletteUI.cs @@ -961,9 +961,9 @@ CommandExecutionContext context private void RefreshRows() { - EnsureRowCapacity(_matchNames.Count); - CommandShell shell = Terminal.Shell; int matchCount = _matchNames.Count; + EnsureRowCapacity(matchCount); + CommandShell shell = Terminal.Shell; for (int index = 0; index < matchCount; ++index) { VisualElement row = _rows[index]; @@ -995,7 +995,7 @@ private void RefreshRows() : LogTextSanitizer.Sanitize(help); } - int excessRowStart = _matchNames.Count; + int excessRowStart = matchCount; int rowCount = _rows.Count; for (int index = excessRowStart; index < rowCount; ++index) { @@ -1123,13 +1123,14 @@ The value change rides SetValueWithoutNotify so exactly one the inserted token like the terminal's token completion. */ _input.SetValueWithoutNotify(newInput); - int caretIndex = replacementStart + insertion.Length; + int insertionLength = insertion.Length; + int caretIndex = replacementStart + insertionLength; QueueCaret(caretIndex); int completionCaret = caretIndex; if ( - 1 < insertion.Length + 1 < insertionLength && CommandArg.Quotes.Contains(insertion[0]) - && insertion[insertion.Length - 1] == insertion[0] + && insertion[insertionLength - 1] == insertion[0] ) { --completionCaret; diff --git a/Runtime/CommandTerminal/UI/TerminalUI.cs b/Runtime/CommandTerminal/UI/TerminalUI.cs index 773b47c6..51ee5569 100644 --- a/Runtime/CommandTerminal/UI/TerminalUI.cs +++ b/Runtime/CommandTerminal/UI/TerminalUI.cs @@ -2081,17 +2081,22 @@ change re-derives them. { _tokenCompletions.Clear(); _tokenCompletions.AddRange(_tokenCompletionsTemp); - _tokenCompletionIndex = searchForward ? 0 : _tokenCompletions.Count - 1; + } + + /* One read, after the refill: the cycling below cannot change it. */ + int completionCount = _tokenCompletions.Count; + if (!equivalent) + { + _tokenCompletionIndex = searchForward ? 0 : completionCount - 1; } else if (searchForward) { - _tokenCompletionIndex = (_tokenCompletionIndex.Value + 1) % _tokenCompletions.Count; + _tokenCompletionIndex = (_tokenCompletionIndex.Value + 1) % completionCount; } else { _tokenCompletionIndex = - (_tokenCompletionIndex.Value - 1 + _tokenCompletions.Count) - % _tokenCompletions.Count; + (_tokenCompletionIndex.Value - 1 + completionCount) % completionCount; } ApplyTokenCompletion(_tokenCompletions[_tokenCompletionIndex.Value]); @@ -2666,21 +2671,28 @@ guarded local instead of re-deriving nullability per access. VisualElement content = _logScrollView.contentContainer; bool dirty = _lastSeenBufferVersion != buffer.Version; - if (content.childCount != logs.Count) + + /* + One read of the log count: the loops below add to and remove + from `content`, so the `while` re-reads `content.childCount` in + its own condition and that re-read is what terminates it. The + log list is only read here, so its count is one number. + */ + int logCount = logs.Count; + if (content.childCount != logCount) { dirty = true; - if (content.childCount < logs.Count) + if (content.childCount < logCount) { - while (content.childCount < logs.Count) + while (content.childCount < logCount) { Label logText = new(); logText.AddToClassList("terminal-output-label"); content.Add(logText); } } - else if (logs.Count < content.childCount) + else if (logCount < content.childCount) { - int logCount = logs.Count; for (int i = content.childCount - 1; logCount <= i; --i) { content.RemoveAt(i); @@ -2692,7 +2704,6 @@ guarded local instead of re-deriving nullability per access. if (dirty) { - int logCount = logs.Count; int childCount = content.childCount; for (int i = 0; i < logCount && i < childCount; ++i) { @@ -2723,7 +2734,7 @@ guarded local instead of re-deriving nullability per access. } } - if (logs.Count == content.childCount) + if (logCount == content.childCount) { _lastSeenBufferVersion = buffer.Version; } diff --git a/Runtime/Extensions/SerializedPropertyExtensions.cs b/Runtime/Extensions/SerializedPropertyExtensions.cs index 93cfc97a..f4eeff33 100644 --- a/Runtime/Extensions/SerializedPropertyExtensions.cs +++ b/Runtime/Extensions/SerializedPropertyExtensions.cs @@ -176,7 +176,8 @@ out FieldInfo fieldInfo FieldInfo enclosingField = null; // Traverse the path but stop at the second-to-last field - for (int i = 0; i < pathParts.Length - 1; ++i) + int partCount = pathParts.Length; + for (int i = 0; i < partCount - 1; ++i) { string fieldName = pathParts[i]; @@ -184,7 +185,7 @@ out FieldInfo fieldInfo { // Move to "data[i]" ++i; - if (pathParts.Length <= i) + if (partCount <= i) { break; } @@ -206,7 +207,7 @@ out FieldInfo fieldInfo } // Move deeper but stop before the last property in the path - if (i < pathParts.Length - 2) + if (i < partCount - 2) { obj = enclosingField.GetValue(obj); type = enclosingField.FieldType; diff --git a/Runtime/Helper/CachedSlotStorage.cs b/Runtime/Helper/CachedSlotStorage.cs index c9a3a629..d8ac1639 100644 --- a/Runtime/Helper/CachedSlotStorage.cs +++ b/Runtime/Helper/CachedSlotStorage.cs @@ -37,19 +37,22 @@ public static void Ensure(int slot) lock (GrowthGate) { - if (slot < _slots.Length) + /* One read: the lock holds, and only the write below moves it. */ + T[] slots = _slots; + int currentLength = slots.Length; + if (slot < currentLength) { return; } - int capacity = _slots.Length * 2; + int capacity = currentLength * 2; while (capacity <= slot) { capacity *= 2; } T[] grown = new T[capacity]; - Array.Copy(_slots, grown, _slots.Length); + Array.Copy(slots, grown, currentLength); Volatile.Write(ref _slots, grown); } } diff --git a/Runtime/Helper/DirectoryHelper.cs b/Runtime/Helper/DirectoryHelper.cs index 19d787c2..bcfa79f0 100644 --- a/Runtime/Helper/DirectoryHelper.cs +++ b/Runtime/Helper/DirectoryHelper.cs @@ -108,23 +108,12 @@ internal static string AbsoluteToUnityRelativePath(string absolutePath) if (absolutePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase)) { // +1 to remove the leading slash only if projectRoot doesn't end with one + int projectRootLength = projectRoot.Length; int startIndex = projectRoot.EndsWith("/", StringComparison.OrdinalIgnoreCase) - ? projectRoot.Length - : projectRoot.Length + 1; + ? projectRootLength + : projectRootLength + 1; return startIndex < absolutePath.Length ? absolutePath[startIndex..] : string.Empty; } - if (absolutePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase)) - { - int startIndex = projectRoot.EndsWith("/", StringComparison.OrdinalIgnoreCase) - ? projectRoot.Length - : projectRoot.Length + 1; - if (startIndex < absolutePath.Length) - { - return "Assets/" + absolutePath[startIndex..]; - } - - return "Assets"; - } return string.Empty; } diff --git a/Runtime/Helper/LogTextSanitizer.cs b/Runtime/Helper/LogTextSanitizer.cs index a755c53c..5389ed69 100644 --- a/Runtime/Helper/LogTextSanitizer.cs +++ b/Runtime/Helper/LogTextSanitizer.cs @@ -1,5 +1,6 @@ namespace WallstopStudios.DxCommandTerminal.Helper { + using System; using System.Globalization; using System.Text; @@ -12,13 +13,19 @@ namespace WallstopStudios.DxCommandTerminal.Helper noise or - worse - reads one way and copies out as another. "Adminexe" is the case that matters. - Three rendering paths never pass this funnel: the palette's error - bar, the terminal's suggestion bar, and the palette's result rows, - which print a completion candidate - a history line, a GameObject - name, a description. They call this too, so the same text reads one - way wherever a developer reads it. A row shows the escaped text and - still applies the raw one: the escape is for the reader, and the - command the developer runs is the text they chose. + Four rendering paths do not come through the log funnel, and each + calls this itself so the same text reads one way wherever a developer + reads it: the palette's error bar, the terminal's suggestion bar, the + palette's result rows - which print a completion candidate, a history + line, a GameObject name, a description - and the package's own + inspectors, which print theme names, font names, command names, and + asset paths into IMGUI tooltips and popup labels. + + The rule those four share: a name printed for a reader is escaped and + the value the developer acts on is raw. A row shows the escaped text + and still applies the raw one; a popup shows an escaped label and its + index still selects the raw name. The escape is for the reader, and + the command that runs is the text they chose. Escaping rather than stripping is the deliberate choice: dropping the override would show "Adminexe", a name that looks legitimate and @@ -119,6 +126,30 @@ neither half is Format on its own - so the category has to return builder.ToString(); } + /* + Fills the display copy a popup shows, refilled in place when its + length already fits: that is what keeps an inspector redrawing + every frame from allocating one array per frame. One read of the + length sizes the allocation and the loop - the loop body writes + `copy`, never `names`, so the re-read that terminates a mutating + loop is not needed here. `names` is never null (every caller owns + a `new string[count]`), and passing one array as both arguments + is harmless: each element is read before it is written. + */ + public static void SanitizeInto(string[] names, ref string[] copy) + { + int length = names.Length; + if (copy == null || copy.Length != length) + { + copy = length == 0 ? Array.Empty() : new string[length]; + } + + for (int i = 0; i < length; ++i) + { + copy[i] = Sanitize(names[i]); + } + } + /* The hot path: a clean message returns the same reference, so a write that allocated nothing still allocates nothing (pinned by diff --git a/Tests/Editor/LogTextSanitizerTests.cs b/Tests/Editor/LogTextSanitizerTests.cs index d0ccabeb..a8505a78 100644 --- a/Tests/Editor/LogTextSanitizerTests.cs +++ b/Tests/Editor/LogTextSanitizerTests.cs @@ -1,5 +1,6 @@ namespace WallstopStudios.DxCommandTerminal.Tests.Runtime { + using System; using System.Globalization; using Backend; using Helper; @@ -191,6 +192,47 @@ Both the escape and the carriage-return fold have to hold. ); } + [Test] + public void ADisplayCopyIsItsOwnArrayAndRefillsInPlace() + { + /* + A popup shows this copy while the index it returns selects the + name the caller still holds, so the copy must be a separate + array, and an inspector that redraws every frame must be able + to refill it without a second allocation. + */ + string[] names = { "Admin\u202Eexe", "plain" }; + + /* Both production callers start here, not from null. */ + string[] copy = Array.Empty(); + LogTextSanitizer.SanitizeInto(names, ref copy); + Assert.AreEqual(new[] { "Admin\\u202Eexe", "plain" }, copy, "Both names escape"); + Assert.AreEqual( + "Admin\u202Eexe", + names[0], + "The caller's own name stays raw: the popup index selects it" + ); + Assert.AreSame("plain", copy[1], "A clean name passes through by reference"); + + LogTextSanitizer.SanitizeInto(new[] { "one", "two", "three" }, ref copy); + Assert.AreEqual(3, copy.Length, "A different length needs a new array"); + Assert.AreEqual(new[] { "one", "two", "three" }, copy, "The refill holds every name"); + + string[] refilled = copy; + LogTextSanitizer.SanitizeInto(new[] { "four", "five", "six" }, ref copy); + Assert.AreSame(refilled, copy, "A same-length refill must not reallocate"); + Assert.AreEqual("four", copy[0], "A refill replaces the contents"); + + string[] fromNull = null; + LogTextSanitizer.SanitizeInto(names, ref fromNull); + Assert.AreEqual( + new[] { "Admin\\u202Eexe", "plain" }, + fromNull, + "A null copy is allocated and filled from the caller's names" + ); + Assert.AreNotSame(names, fromNull, "The caller's array is never the copy"); + } + [Test] public void NothingIsTruncated() { diff --git a/tooling~/scripts/tests/display-text.test.mjs b/tooling~/scripts/tests/display-text.test.mjs new file mode 100644 index 00000000..2abe4a2f --- /dev/null +++ b/tooling~/scripts/tests/display-text.test.mjs @@ -0,0 +1,461 @@ +/* + Contract tests for the display-text rule: text on its way to a display + sink is escaped, and the raw text is what the code acts on. + + Why a source-level contract and not a Unity test: the sinks this covers + are the package's own IMGUI inspectors, and no CI lane compiles the + Editor assembly, so a test that needs an editor is a test that never + runs. The runtime sinks are pinned by LogTextSanitizerTests in the + editor's own test assembly; what was missing is the inspector side, and + a rule checkable without Unity is the only gate available. + + Two rules, deliberately different in strength: + + 1. Precise, and the one that carries weight. Every hole of an + interpolated string inside a GUIContent construction must be a + LogTextSanitizer.Sanitize call: a tooltip that names a theme, a + font, a command, or an asset path has to escape it, and a tooltip + made of the package's own words is not a finding. Read from the + source, not run, so it also reads the shapes Unity would never + execute - an editor-only branch, a platform-guarded line. What it + does not cover is stated below rather than assumed. + 2. A backstop for the popups, whose labels are escaped in the caching + builder that fills them and are therefore not visible at the call. + + Rule 1 sees exactly one shape: a `GUIContent = new(` or + `new GUIContent(` whose argument list holds an interpolated string. Not + seen, so a future reader is not misled: a tooltip assembled by + concatenation, a tooltip held in a local and passed as a variable, a + `GUIContent` bound by an object initializer or on a later line, an + expression-bodied factory, a label API other than a GUIContent + construction (a bare `GUILayout.Label($"... {name}")`), and a hole whose + own braces would need balancing. None of those exists in the package + today; widening the gate to them is a separate piece of work. + + Rule 2 watches `EditorGUILayout.Popup` only - the call the three + inspectors use. `EditorGUI.Popup` and `EditorGUILayout.IntPopup` are + the same hazard and are not watched. + + The scanner has its own tests below: a gate that cannot fail is worse + than no gate, and an earlier version of it walked only the top level of + each root and passed on the very sinks it was written for. +*/ +import test from "node:test"; +import assert from "node:assert"; +import path from "node:path"; +import fs from "node:fs"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../../.."); +/* The call, with its paren: a prefix test would accept a same-named sibling. */ +const SANITIZE_CALL = "LogTextSanitizer.Sanitize("; +/* The backstop only asks whether the file reaches the sanitizer at all. */ +const SANITIZER = "LogTextSanitizer."; + +/** Every shipped C# file that can print a name. Tests and tooling are exempt. */ +function shippedCSharpFiles() { + const files = []; + for (const root of ["Runtime", "Editor", "Packs", "Styles", "Samples~"]) { + collectCSharpFiles(path.join(repoRoot, root), files); + } + + return files; +} + +function collectCSharpFiles(directory, files) { + if (!fs.existsSync(directory)) { + return; + } + + for (const entry of fs.readdirSync(directory, { withFileTypes: true })) { + const full = path.join(directory, entry.name); + if (entry.isDirectory()) { + collectCSharpFiles(full, files); + } else if (entry.name.endsWith(".cs")) { + files.push(full); + } + } +} + +/* + One left-to-right pass that classifies the file into the spans a + GUIContent walk has to step over: string literals (whose text may hold + brackets, braces, and the token itself) and comments (whose text is not + code at all). + */ +function classify(text) { + const literals = []; + const comments = []; + let index = 0; + while (index < text.length) { + const character = text[index]; + const next = text[index + 1]; + + if (character === "/" && next === "/") { + const start = index; + index = text.indexOf("\n", index); + if (index < 0) { + index = text.length; + } + + comments.push({ start, end: index }); + continue; + } + + if (character === "/" && next === "*") { + const start = index; + index = text.indexOf("*/", index + 2); + index = index < 0 ? text.length : index + 2; + comments.push({ start, end: index }); + continue; + } + + if (character === "'") { + index += 1; + while (index < text.length) { + if (text[index] === "\\") { + index += 2; + continue; + } + + if (text[index] === "'") { + index += 1; + break; + } + + ++index; + } + + continue; + } + + if (character === '"' || (character === "@" && next === '"')) { + const verbatim = character === "@"; + const start = index; + index += verbatim ? 2 : 1; + while (index < text.length) { + if (text[index] === "\\" && !verbatim) { + index += 2; + continue; + } + + if (text[index] === '"') { + if (verbatim && text[index + 1] === '"') { + index += 2; + continue; + } + + index += 1; + break; + } + + ++index; + } + + /* + `$@` is a verbatim interpolated string and `@` is a verbatim one that + is not, and `start` indexes the `@` in both cases - so the character + that decides is the one immediately before it. Reading a second + character back instead matches nothing, and every `$@"...{x}"` hole + then goes unreported. + */ + literals.push({ start, end: index, interpolated: text[start - 1] === "$", verbatim }); + continue; + } + + ++index; + } + + return { literals, comments }; +} + +function startsInside(spans, index) { + for (const span of spans) { + if (span.start > index) { + return false; + } + + if (span.start <= index && index < span.end) { + return true; + } + } + + return false; +} + +/** The index of the paren that closes the call opened at openParen. */ +function callCloseParen(text, openParen, literals) { + let depth = 0; + let index = openParen; + let literal = 0; + while (index < text.length) { + while (literal < literals.length && literals[literal].start < index) { + ++literal; + } + + if (literal < literals.length && literals[literal].start === index) { + index = literals[literal].end; + continue; + } + + if (text[index] === "(") { + ++depth; + } else if (text[index] === ")") { + --depth; + if (depth === 0) { + return index; + } + } + + ++index; + } + + return -1; +} + +function lineOf(text, index) { + return text.slice(0, index).split("\n").length; +} + +/** Interpolated holes of every interpolated literal inside the given text. */ +function interpolationHoles(source, literals) { + const holes = []; + for (const literal of literals) { + if (!literal.interpolated) { + continue; + } + + const text = source.slice(literal.start, literal.end); + /* + A verbatim interpolated string doubles a literal brace, so "{{x}}" is + the text {x} and not a hole. Blank the doubled pairs before looking + for holes, or every one of them reads as an unescaped name. + */ + const body = literal.verbatim ? text.replace(/\{\{|\}\}/g, " ") : text; + for (const match of body.matchAll(/\{([^{}]*)\}/g)) { + holes.push({ hole: match[1].trim(), offset: literal.start + match.index }); + } + } + + return holes; +} + +/* + A GUIContent construction, not a GUIContent-typed parameter: the + constructor's argument list is where a name reaches a label. The regex + requires the `new`, so a method that takes a GUIContent is not read as + one and the walk cannot land on an unrelated call. + */ +const GUICONTENT_CONSTRUCTION = + /\bGUIContent\s+\w+\s*=\s*new\s*\(|\bnew\s+GUIContent\s*\(/g; + +/** Every unescaped name interpolated into a GUIContent, as "line: hole". */ +function unescapedTooltipNames(text) { + const { literals, comments } = classify(text); + const findings = []; + for (const match of text.matchAll(GUICONTENT_CONSTRUCTION)) { + if (startsInside(comments, match.index) || startsInside(literals, match.index)) { + continue; + } + + const openParen = match.index + match[0].length - 1; + const closeParen = callCloseParen(text, openParen, literals); + if (closeParen < 0) { + continue; + } + + const argumentText = text.slice(openParen + 1, closeParen); + const argumentLiterals = literals + .filter((literal) => literal.start > openParen && literal.end <= closeParen) + .map((literal) => ({ ...literal, start: literal.start - openParen, end: literal.end - openParen })); + for (const { hole, offset } of interpolationHoles(argumentText, argumentLiterals)) { + if (!hole.startsWith(SANITIZE_CALL)) { + findings.push(`line ${lineOf(text, openParen + offset)}: ${hole}`); + } + } + } + + return findings; +} + +/* + A popup returns an index, so its labels are display-only and the value + behind the index is the raw name. Escaping those labels means a call the + gate cannot see: the escape lives in the caching builder that fills the + array, and a label array stamped on a source and count is rebuilt only + when that source changes. Following the call would take a call graph, so + rule 2 is a backstop instead: a file that pops up project names has to + reach the sanitizer somewhere. It cannot tell an escaped label from a + raw one, and it does not claim to. + */ +function popupsWithoutASanitizer(text) { + const { literals, comments } = classify(text); + const code = codeText(text, literals, comments); + return code.includes("EditorGUILayout.Popup(") && !code.includes(SANITIZER); +} + +/* + The file's own code, with every comment and string literal removed: a + backstop satisfied by a TODO or a Debug.Log of the sanitizer's own name + is not a backstop, and a mention in either is the cheapest way to write + one by accident. + */ +function codeText(text, literals, comments) { + let result = ""; + let index = 0; + /* + Ordered by position, and the order matters: the two lists are collected in + separate passes, so concatenating them walks backwards the moment a + literal precedes a comment, and every region between is emitted twice - + comment text included, which is the one thing this function removes. + */ + for (const span of [...comments, ...literals].sort((a, b) => a.start - b.start)) { + result += text.slice(index, span.start); + index = span.end; + } + + return result + text.slice(index); +} + +test("display text: a tooltip escapes every name it interpolates", () => { + const findings = []; + for (const file of shippedCSharpFiles()) { + const text = fs.readFileSync(file, "utf8"); + for (const finding of unescapedTooltipNames(text)) { + findings.push(`${path.relative(repoRoot, file)} ${finding}`); + } + } + + assert.deepEqual( + findings, + [], + "a name that reads one way in a tooltip is not the name the project holds:\n" + + findings.join("\n") + ); +}); + +test("display text: an inspector that pops up names reaches the sanitizer", () => { + const findings = []; + for (const file of shippedCSharpFiles()) { + const text = fs.readFileSync(file, "utf8"); + if (popupsWithoutASanitizer(text)) { + findings.push( + `${path.relative(repoRoot, file)} pops up project names and never reaches ${SANITIZER}` + ); + } + } + + assert.deepEqual(findings, [], findings.join("\n")); +}); + +test("display text: the scanner names a raw tooltip hole", () => { + const shapes = [ + { lines: 4, line: 3 }, + { lines: 1, line: 1 } + ]; + const sources = [ + [ + "GUIContent tip = new(", + ' "Set Theme",', + ' $"Will set the current theme to {theme}"', + ");" + ], + ['GUIContent tip = new("Set Theme", $"Will set the current theme to {theme}");'] + ]; + for (const [index, { line }] of shapes.entries()) { + assert.deepEqual( + unescapedTooltipNames(sources[index].join("\n")), + [`line ${line}: theme`], + "a raw name in a tooltip must be reported, wrapped or on one line" + ); + } +}); + +test("display text: the scanner accepts an escaped tooltip hole", () => { + const shapes = [ + [`GUIContent tip = new("Set Theme", $"Will set {${SANITIZE_CALL}theme)}");`], + ["GUIContent tip = new(", ' "Set Theme",', ` $"Will set {${SANITIZE_CALL}theme)}"`, ");"] + ]; + for (const lines of shapes) { + assert.deepEqual(unescapedTooltipNames(lines.join("\n")), []); + } +}); + +test("display text: the scanner is not fooled by the shapes around it", () => { + const sources = [ + [ + "GUIContent tip = new(", + ' "Label",', + ' @"a""b (c) {d}",', + ' $"Will set {name}"', + ");" + ], + ['// GUIContent tip = new("x", $"raw {name} in a line comment");'], + ['/* GUIContent tip = new("x", $"raw {name} in a block comment"); */'], + ['var text = @"GUIContent = new(""x"", $""raw {name} in a string"")";'], + ['public override float GetPropertyHeight(SerializedProperty property, GUIContent label)'] + ]; + assert.deepEqual( + unescapedTooltipNames(sources[0].join("\n")), + ["line 4: name"], + "a bracket or brace inside an earlier argument must not move the hole" + ); + for (const source of sources.slice(1)) { + assert.deepEqual(unescapedTooltipNames(source.join("\n")), [], source.join("\n")); + } + +}); + +test("display text: a verbatim interpolated tooltip is read, and its doubled brace is not a hole", () => { + assert.deepEqual( + unescapedTooltipNames([`GUIContent tip = new("a", $@"Will set {name} now");`].join("\n")), + ["line 1: name"], + "a $@ hole is a hole: not seeing it is a false negative, not a clean file" + ); + assert.deepEqual( + unescapedTooltipNames([`GUIContent tip = new("a", $@"literal {{name}} text");`].join("\n")), + [], + "a doubled brace in a verbatim string is literal text, not a hole: {name}" + ); + assert.deepEqual( + unescapedTooltipNames([`GUIContent tip = new("a", $@"Will set {${SANITIZE_CALL}name)}");`].join("\n")), + [], + "an escaped hole in a verbatim string is still escaped" + ); +}); + +test("display text: the popup backstop is not satisfied by a mention", () => { + /* + The mentions sit between literals, which is the shape that breaks a + removal walk whose spans are not ordered: a comment before every literal + passes whether or not the walk is correct, so interleave them. + */ + assert.ok( + popupsWithoutASanitizer( + [ + 'string a = "first";', + "// TODO: the labels should call LogTextSanitizer.Sanitize", + 'string b = "second";', + "EditorGUILayout.Popup(0, names);", + "// end of file TODO" + ].join("\n") + ), + "a mention in a comment is not a call, even between literals" + ); + assert.ok( + popupsWithoutASanitizer(['Debug.Log("LogTextSanitizer.Sanitize");', "EditorGUILayout.Popup(0, names);"].join("\n")), + "a mention in a string literal is not a call" + ); + assert.ok( + !popupsWithoutASanitizer( + [ + "private static void Draw(string[] names, ref string[] labels)", + "{", + " LogTextSanitizer.SanitizeInto(names, ref labels);", + " EditorGUILayout.Popup(0, labels);", + "}" + ].join("\n") + ), + "a real call in code satisfies the backstop" + ); +});