From ae78ccda8dc6924c956ee32412e092dbc123ab63 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 00:02:06 +0000 Subject: [PATCH 1/8] Escape the names the package's inspectors print Why: a theme, font, or command name carrying a bidi override reached the custom inspectors unescaped, so a name read one way in a tooltip and another way in the value the index selected. What changed: - Escape theme, font, command, and asset-path names in all three editors. - Popups show the escaped copy; the index still selects the raw name. - Pin the tooltips with a source gate; no CI lane compiles the editors. How we know: the gate names all five unescaped tooltips on the old sources and passes on the new. --- CHANGELOG.md | 1 + .../CustomEditors/TerminalFontPackEditor.cs | 2 +- .../CustomEditors/TerminalThemePackEditor.cs | 2 +- Editor/CustomEditors/TerminalUIEditor.cs | 55 ++- Runtime/Helper/LogTextSanitizer.cs | 34 +- Tests/Editor/LogTextSanitizerTests.cs | 30 ++ tooling~/scripts/tests/display-text.test.mjs | 355 ++++++++++++++++++ 7 files changed, 453 insertions(+), 26 deletions(-) create mode 100644 tooling~/scripts/tests/display-text.test.mjs 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/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..60a4ce8d 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); @@ -612,11 +615,15 @@ ref string[] cached return cached; } + /* + Display-only: the popup returns an index and the value is read + back out of the source list, so these carry the escaped name. + */ 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 +633,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 +651,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 +661,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 +676,17 @@ 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. */ 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 +696,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 +778,8 @@ private string[] FontKeys() _fontsByPrefix.Keys, force: false, ref _fontKeyCacheStale, - ref _fontKeyCache + ref _fontKeyCache, + ref _fontKeyLabels ); } @@ -772,7 +789,8 @@ private string[] SecondFontKeys(SortedDictionary availableFonts) availableFonts.Keys, force: _fontKeyCacheStale, ref _secondFontKeyCacheStale, - ref _secondFontKeyCache + ref _secondFontKeyCache, + ref _secondFontKeyLabels ); } @@ -1337,7 +1355,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 +1436,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 +1547,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 +1575,7 @@ _fontKey is int fontKeyIndex { int selectedSecondFontKeyIndex = EditorGUILayout.Popup( _secondFontKey.GetValueOrDefault(-1), - secondFontKeys + _secondFontKeyLabels ); _secondFontKey = selectedSecondFontKeyIndex < 0 @@ -1592,7 +1611,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/Runtime/Helper/LogTextSanitizer.cs b/Runtime/Helper/LogTextSanitizer.cs index a755c53c..b852292e 100644 --- a/Runtime/Helper/LogTextSanitizer.cs +++ b/Runtime/Helper/LogTextSanitizer.cs @@ -12,12 +12,14 @@ 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 + Four rendering paths never pass this funnel: 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. 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. Escaping rather than stripping is the deliberate choice: dropping @@ -119,6 +121,26 @@ neither half is Format on its own - so the category has to return builder.ToString(); } + /* + The display copy a popup shows. A popup returns an index and the + caller reads the name back out of the raw list, so the copy is a + separate array: refilled in place when its length already fits, + which is what keeps an editor that redraws every frame from + allocating one. The caller's own array is never written to. + */ + public static void SanitizeInto(string[] names, ref string[] copy) + { + if (copy == null || copy.Length != names.Length) + { + copy = new string[names.Length]; + } + + for (int i = 0; i < names.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..0acbf244 100644 --- a/Tests/Editor/LogTextSanitizerTests.cs +++ b/Tests/Editor/LogTextSanitizerTests.cs @@ -191,6 +191,36 @@ 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" }; + string[] copy = null; + 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"); + } + [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..ff3f6f47 --- /dev/null +++ b/tooling~/scripts/tests/display-text.test.mjs @@ -0,0 +1,355 @@ +/* + 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. 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; a tooltip made of the package's own words is not a + finding. + 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. + + 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)), "../../.."); +const SANITIZER = "LogTextSanitizer.Sanitize"; + +/** 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; + } + + literals.push({ + start, + end: index, + interpolated: !verbatim && text[start - 1] === "$" + }); + 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 text a constructor call is given: from its open paren to its match. */ +function callArgumentText(text, openParen, literals) { + const close = callCloseParen(text, openParen, literals); + return close < 0 ? "" : text.slice(openParen + 1, close); +} + +/** 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; + } + + for (const match of source.slice(literal.start, literal.end).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(SANITIZER)) { + 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) { + return text.includes("EditorGUILayout.Popup(") && !text.includes(SANITIZER); +} + +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 calls ${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 {${SANITIZER}(theme)}");`], + ["GUIContent tip = new(", ' "Set Theme",', ` $"Will set {${SANITIZER}(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")); + } +}); From e92907fe1a7f48108f3d8173587e3a7e1ba277b8 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 00:02:09 +0000 Subject: [PATCH 2/8] State what a font can draw, in the theme guide Why: most shipped fonts are Latin-only, and the guide did not say a character the selected font lacks shows a missing-glyph box. What changed: - One paragraph in the theme guide, with the two exceptions measured. --- Documentation~/theming.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation~/theming.md b/Documentation~/theming.md index c97f5212..8e99f640 100644 --- a/Documentation~/theming.md +++ b/Documentation~/theming.md @@ -12,6 +12,12 @@ 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. Most shipped fonts are Latin-only, +`M PLUS 1 Code` carries CJK, `Nanum Gothic Coding` carries Hangul, and no +shipped font carries emoji, so a character the selected font lacks shows a +missing-glyph box. Add a font that has the glyph to a pack, then set it. +The console keeps the text whole either way. + ## Switching at runtime `TerminalUI` exposes both switches. `persist: true` stores the choice From dc524d242791aaafa91be7d02d47578297181c60 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 00:25:19 +0000 Subject: [PATCH 3/8] Answer the second review: facts, and a gate that cannot go green for free Why: the docs claimed no shipped font carries emoji and named two fonts by a name the product never shows; the popup backstop was satisfied by a mention in a comment; the same rule was stated six times. What changed: - Name the asset names and the packs, and bound the emoji claim. - Read the popup backstop from code, so a TODO does not satisfy it. - State the rule once, and say what the gate does not cover. - Cover the null and empty starting copies the callers use. --- Documentation~/theming.md | 12 ++- Editor/CustomEditors/TerminalUIEditor.cs | 10 +- Runtime/Helper/LogTextSanitizer.cs | 22 ++-- Tests/Editor/LogTextSanitizerTests.cs | 9 +- tooling~/scripts/tests/display-text.test.mjs | 108 +++++++++++++++---- 5 files changed, 123 insertions(+), 38 deletions(-) diff --git a/Documentation~/theming.md b/Documentation~/theming.md index 8e99f640..f25f135a 100644 --- a/Documentation~/theming.md +++ b/Documentation~/theming.md @@ -12,11 +12,13 @@ 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. Most shipped fonts are Latin-only, -`M PLUS 1 Code` carries CJK, `Nanum Gothic Coding` carries Hangul, and no -shipped font carries emoji, so a character the selected font lacks shows a -missing-glyph box. Add a font that has the glyph to a pack, then set it. -The console keeps the text whole either way. +A font draws only the glyphs it carries, and a character it lacks shows a +missing-glyph box. Most shipped fonts are Latin-only. Two carry more: +`MPLUS1Code` has CJK and kana, in `Large` and both `Everything` packs, and +`NanumGothicCoding` has Hangul and kana, in both `Everything` packs. Emoji +coverage is a handful of characters at most, so an emoji 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 diff --git a/Editor/CustomEditors/TerminalUIEditor.cs b/Editor/CustomEditors/TerminalUIEditor.cs index 60a4ce8d..3d4dfeea 100644 --- a/Editor/CustomEditors/TerminalUIEditor.cs +++ b/Editor/CustomEditors/TerminalUIEditor.cs @@ -615,10 +615,7 @@ ref string[] cached return cached; } - /* - Display-only: the popup returns an index and the value is read - back out of the source list, so these carry the escaped name. - */ + /* Display-only: the index selects the pack, so the label escapes. */ private static string[] PackNames(List themePacks) { return RefreshCache( @@ -679,7 +676,9 @@ private static string FriendlyThemeName(string themeName) 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. + 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, @@ -1544,6 +1543,7 @@ private bool RenderSelectableFonts(TerminalUI terminal) GUILayout.Label("Select Font:"); } + /* The labels are the copy; fontKeys is what the lookup reads. */ string[] fontKeys = FontKeys(); int selectedFontKeyIndex = EditorGUILayout.Popup( _fontKey.GetValueOrDefault(-1), diff --git a/Runtime/Helper/LogTextSanitizer.cs b/Runtime/Helper/LogTextSanitizer.cs index b852292e..f77f7932 100644 --- a/Runtime/Helper/LogTextSanitizer.cs +++ b/Runtime/Helper/LogTextSanitizer.cs @@ -12,15 +12,19 @@ namespace WallstopStudios.DxCommandTerminal.Helper noise or - worse - reads one way and copies out as another. "Adminexe" is the case that matters. - Four rendering paths never pass this funnel: the palette's error + Four rendering paths do not pass the log funnel: 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. 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. + one way wherever a developer reads it. + + The rule those paths share: a name printed for a reader is escaped, + and the value the developer acts on is the raw one. A row shows the + escaped text and still applies the raw text; 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 @@ -122,11 +126,11 @@ neither half is Format on its own - so the category has to } /* - The display copy a popup shows. A popup returns an index and the - caller reads the name back out of the raw list, so the copy is a - separate array: refilled in place when its length already fits, - which is what keeps an editor that redraws every frame from - allocating one. The caller's own array is never written to. + 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. The caller's + array is never written to, and `names` is never null - every + caller owns a `new string[count]`. */ public static void SanitizeInto(string[] names, ref string[] copy) { diff --git a/Tests/Editor/LogTextSanitizerTests.cs b/Tests/Editor/LogTextSanitizerTests.cs index 0acbf244..e6215361 100644 --- a/Tests/Editor/LogTextSanitizerTests.cs +++ b/Tests/Editor/LogTextSanitizerTests.cs @@ -201,7 +201,9 @@ public void ADisplayCopyIsItsOwnArrayAndRefillsInPlace() to refill it without a second allocation. */ string[] names = { "Admin\u202Eexe", "plain" }; - string[] copy = null; + + /* 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( @@ -219,6 +221,11 @@ to refill it without a second allocation. 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(copy, fromNull, "A null copy is allocated, never the input"); + Assert.AreNotSame(names, fromNull, "The caller's array is never the copy"); } [Test] diff --git a/tooling~/scripts/tests/display-text.test.mjs b/tooling~/scripts/tests/display-text.test.mjs index ff3f6f47..51c34dfa 100644 --- a/tooling~/scripts/tests/display-text.test.mjs +++ b/tooling~/scripts/tests/display-text.test.mjs @@ -11,14 +11,23 @@ Two rules, deliberately different in strength: - 1. Precise. 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; a tooltip made of the package's own words is not a - finding. + 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. + Not covered, so a future reader is not misled: a label API other than + a GUIContent construction (a bare `GUILayout.Label($"... {name}")`), a + `tooltip` set through an object initializer, 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. + 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. @@ -30,7 +39,10 @@ import fs from "node:fs"; import { fileURLToPath } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../../.."); -const SANITIZER = "LogTextSanitizer.Sanitize"; +/* 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() { @@ -132,11 +144,11 @@ function classify(text) { ++index; } - literals.push({ - start, - end: index, - interpolated: !verbatim && text[start - 1] === "$" - }); + const interpolated = + !verbatim && text[start - 1] === "$" + ? true + : verbatim && text[start - 1] === "@" && text[start - 2] === "$"; + literals.push({ start, end: index, interpolated, verbatim }); continue; } @@ -208,7 +220,14 @@ function interpolationHoles(source, literals) { continue; } - for (const match of source.slice(literal.start, literal.end).matchAll(/\{([^{}]*)\}/g)) { + 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 }); } } @@ -245,7 +264,7 @@ function unescapedTooltipNames(text) { .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(SANITIZER)) { + if (!hole.startsWith(SANITIZE_CALL)) { findings.push(`line ${lineOf(text, openParen + offset)}: ${hole}`); } } @@ -265,7 +284,26 @@ function unescapedTooltipNames(text) { raw one, and it does not claim to. */ function popupsWithoutASanitizer(text) { - return text.includes("EditorGUILayout.Popup(") && !text.includes(SANITIZER); + 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; + for (const span of [...comments, ...literals]) { + result += text.slice(index, span.start); + index = span.end; + } + + return result + text.slice(index); } test("display text: a tooltip escapes every name it interpolates", () => { @@ -291,7 +329,7 @@ test("display text: an inspector that pops up names reaches the sanitizer", () = const text = fs.readFileSync(file, "utf8"); if (popupsWithoutASanitizer(text)) { findings.push( - `${path.relative(repoRoot, file)} pops up project names and never calls ${SANITIZER}` + `${path.relative(repoRoot, file)} pops up project names and never reaches ${SANITIZER}` ); } } @@ -320,10 +358,12 @@ test("display text: the scanner names a raw tooltip hole", () => { "a raw name in a tooltip must be reported, wrapped or on one line" ); } -});test("display text: the scanner accepts an escaped tooltip hole", () => { +}); + +test("display text: the scanner accepts an escaped tooltip hole", () => { const shapes = [ - [`GUIContent tip = new("Set Theme", $"Will set {${SANITIZER}(theme)}");`], - ["GUIContent tip = new(", ' "Set Theme",', ` $"Will set {${SANITIZER}(theme)}"`, ");"] + [`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")), []); @@ -352,4 +392,36 @@ test("display text: the scanner is not fooled by the shapes around it", () => { for (const source of sources.slice(1)) { assert.deepEqual(unescapedTooltipNames(source.join("\n")), [], source.join("\n")); } + + assert.deepEqual( + unescapedTooltipNames([`GUIContent tip = new("a", $@"literal {{theme}} text");`].join("\n")), + [], + 'a doubled brace in a verbatim string is literal text, not a hole: {theme}' + ); +}); + +test("display text: the popup backstop is not satisfied by a mention", () => { + assert.ok( + popupsWithoutASanitizer( + ["// TODO: the labels should call LogTextSanitizer.Sanitize", "EditorGUILayout.Popup(0, names);"] + .join("\n") + ), + "a mention in a comment is not a call" + ); + 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" + ); }); From 212225bf52dd0d986eb99a91634f58224daccf83 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 00:53:37 +0000 Subject: [PATCH 4/8] Fix the test the third review found broken, and the gate it found unsound Why: two of the sanitizer test's assertions could not have run - one would not compile without a using, one compared the wrong array - and the gate's comment removal walked its spans out of order, so a TODO still satisfied the popup backstop. What changed: - Add the using, and assert the literals the null copy must hold. - Order the removed spans, and interleave the control that tests it. - State the rule once, and list what the gate does not see. - Name the shipped fonts as list-fonts prints them, with their packs. --- Documentation~/theming.md | 18 +++++--- Editor/CustomEditors/TerminalUIEditor.cs | 2 - Runtime/Helper/LogTextSanitizer.cs | 34 +++++++------- Tests/Editor/LogTextSanitizerTests.cs | 7 ++- tooling~/scripts/tests/display-text.test.mjs | 48 ++++++++++++++------ 5 files changed, 69 insertions(+), 40 deletions(-) diff --git a/Documentation~/theming.md b/Documentation~/theming.md index f25f135a..1c59f90c 100644 --- a/Documentation~/theming.md +++ b/Documentation~/theming.md @@ -13,12 +13,18 @@ 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. Most shipped fonts are Latin-only. Two carry more: -`MPLUS1Code` has CJK and kana, in `Large` and both `Everything` packs, and -`NanumGothicCoding` has Hangul and kana, in both `Everything` packs. Emoji -coverage is a handful of characters at most, so an emoji 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. +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 diff --git a/Editor/CustomEditors/TerminalUIEditor.cs b/Editor/CustomEditors/TerminalUIEditor.cs index 3d4dfeea..c65d610c 100644 --- a/Editor/CustomEditors/TerminalUIEditor.cs +++ b/Editor/CustomEditors/TerminalUIEditor.cs @@ -615,7 +615,6 @@ ref string[] cached return cached; } - /* Display-only: the index selects the pack, so the label escapes. */ private static string[] PackNames(List themePacks) { return RefreshCache( @@ -1543,7 +1542,6 @@ private bool RenderSelectableFonts(TerminalUI terminal) GUILayout.Label("Select Font:"); } - /* The labels are the copy; fontKeys is what the lookup reads. */ string[] fontKeys = FontKeys(); int selectedFontKeyIndex = EditorGUILayout.Popup( _fontKey.GetValueOrDefault(-1), diff --git a/Runtime/Helper/LogTextSanitizer.cs b/Runtime/Helper/LogTextSanitizer.cs index f77f7932..d11954f9 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,19 +13,19 @@ namespace WallstopStudios.DxCommandTerminal.Helper noise or - worse - reads one way and copies out as another. "Adminexe" is the case that matters. - Four rendering paths do not pass the log funnel: 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. They call this too, so the same text reads - one way wherever a developer reads it. + 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 paths share: a name printed for a reader is escaped, - and the value the developer acts on is the raw one. A row shows the - escaped text and still applies the raw text; 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. + 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 @@ -128,15 +129,16 @@ neither half is Format on its own - so the category has to /* 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. The caller's - array is never written to, and `names` is never null - every - caller owns a `new string[count]`. + every frame from allocating one array per frame. `names` is never + null - every caller owns a `new string[count]` - and passing the + same array as both arguments is harmless, because each element is + read before it is written. */ public static void SanitizeInto(string[] names, ref string[] copy) { if (copy == null || copy.Length != names.Length) { - copy = new string[names.Length]; + copy = names.Length == 0 ? Array.Empty() : new string[names.Length]; } for (int i = 0; i < names.Length; ++i) diff --git a/Tests/Editor/LogTextSanitizerTests.cs b/Tests/Editor/LogTextSanitizerTests.cs index e6215361..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; @@ -224,7 +225,11 @@ to refill it without a second allocation. string[] fromNull = null; LogTextSanitizer.SanitizeInto(names, ref fromNull); - Assert.AreEqual(copy, fromNull, "A null copy is allocated, never the input"); + 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"); } diff --git a/tooling~/scripts/tests/display-text.test.mjs b/tooling~/scripts/tests/display-text.test.mjs index 51c34dfa..226f096e 100644 --- a/tooling~/scripts/tests/display-text.test.mjs +++ b/tooling~/scripts/tests/display-text.test.mjs @@ -22,11 +22,19 @@ 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. - Not covered, so a future reader is not misled: a label API other than - a GUIContent construction (a bare `GUILayout.Label($"... {name}")`), a - `tooltip` set through an object initializer, 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 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 @@ -172,12 +180,6 @@ function startsInside(spans, index) { return false; } -/** The text a constructor call is given: from its open paren to its match. */ -function callArgumentText(text, openParen, literals) { - const close = callCloseParen(text, openParen, literals); - return close < 0 ? "" : text.slice(openParen + 1, close); -} - /** The index of the paren that closes the call opened at openParen. */ function callCloseParen(text, openParen, literals) { let depth = 0; @@ -298,7 +300,13 @@ function popupsWithoutASanitizer(text) { function codeText(text, literals, comments) { let result = ""; let index = 0; - for (const span of [...comments, ...literals]) { + /* + 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; } @@ -401,12 +409,22 @@ test("display text: the scanner is not fooled by the shapes around it", () => { }); 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( - ["// TODO: the labels should call LogTextSanitizer.Sanitize", "EditorGUILayout.Popup(0, names);"] - .join("\n") + [ + '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" + "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")), From cbc58b941506de7522eafa986d8ea4b0b49eeff0 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 01:14:09 +0000 Subject: [PATCH 5/8] One source of truth for a count, and the verbatim-hole false negative Why: the sanitizer read names.Length three times, and a $@ tooltip hole was never reported because the check read two characters back from the @. What changed: - Read each count once, in the eight production methods where one value answers every read; leave the six where the loop mutates the collection or the variable is reassigned, and say so at each. - Drop the unreachable second StartsWith block in AbsoluteToUnityRelativePath. - Fix the $@ detection and add a control that fails without the fix. --- .../CatalogEmitter.cs | 8 +++-- .../CommandTerminal/Backend/CommandBuilder.cs | 7 ++-- .../CommandTerminal/Backend/CommandHistory.cs | 13 +++++--- .../CommandTerminal/Backend/CommandShell.cs | 11 ++++--- .../Themes/TerminalThemeAsset.cs | 5 +-- Runtime/CommandTerminal/UI/TerminalUI.cs | 33 ++++++++++++------- Runtime/Helper/CachedSlotStorage.cs | 9 +++-- Runtime/Helper/DirectoryHelper.cs | 17 ++-------- Runtime/Helper/LogTextSanitizer.cs | 17 ++++++---- tooling~/scripts/tests/display-text.test.mjs | 30 +++++++++++++---- 10 files changed, 90 insertions(+), 60 deletions(-) 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/CommandTerminal/Backend/CommandBuilder.cs b/Runtime/CommandTerminal/Backend/CommandBuilder.cs index 893a35de..bb56c2bb 100644 --- a/Runtime/CommandTerminal/Backend/CommandBuilder.cs +++ b/Runtime/CommandTerminal/Backend/CommandBuilder.cs @@ -165,14 +165,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); diff --git a/Runtime/CommandTerminal/Backend/CommandHistory.cs b/Runtime/CommandTerminal/Backend/CommandHistory.cs index 6d19c869..22d196d6 100644 --- a/Runtime/CommandTerminal/Backend/CommandHistory.cs +++ b/Runtime/CommandTerminal/Backend/CommandHistory.cs @@ -78,24 +78,26 @@ public string Next(bool skipSameCommands) } _direction = 1; + /* One read: the loops below move _position, never the history. */ + 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 +110,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..c384c65b 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; 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/TerminalUI.cs b/Runtime/CommandTerminal/UI/TerminalUI.cs index 773b47c6..38d381e1 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`, which is why its own childCount stays inline - + that re-read is what terminates them. The log list is only + read here, so its count is the same number throughout. + */ + 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/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 d11954f9..5389ed69 100644 --- a/Runtime/Helper/LogTextSanitizer.cs +++ b/Runtime/Helper/LogTextSanitizer.cs @@ -129,19 +129,22 @@ neither half is Format on its own - so the category has to /* 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. `names` is never - null - every caller owns a `new string[count]` - and passing the - same array as both arguments is harmless, because each element is - read before it is written. + 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) { - if (copy == null || copy.Length != names.Length) + int length = names.Length; + if (copy == null || copy.Length != length) { - copy = names.Length == 0 ? Array.Empty() : new string[names.Length]; + copy = length == 0 ? Array.Empty() : new string[length]; } - for (int i = 0; i < names.Length; ++i) + for (int i = 0; i < length; ++i) { copy[i] = Sanitize(names[i]); } diff --git a/tooling~/scripts/tests/display-text.test.mjs b/tooling~/scripts/tests/display-text.test.mjs index 226f096e..2abe4a2f 100644 --- a/tooling~/scripts/tests/display-text.test.mjs +++ b/tooling~/scripts/tests/display-text.test.mjs @@ -152,11 +152,14 @@ function classify(text) { ++index; } - const interpolated = - !verbatim && text[start - 1] === "$" - ? true - : verbatim && text[start - 1] === "@" && text[start - 2] === "$"; - literals.push({ start, end: index, interpolated, verbatim }); + /* + `$@` 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; } @@ -401,10 +404,23 @@ test("display text: the scanner is not fooled by the shapes around it", () => { 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", $@"literal {{theme}} text");`].join("\n")), + unescapedTooltipNames([`GUIContent tip = new("a", $@"Will set {${SANITIZE_CALL}name)}");`].join("\n")), [], - 'a doubled brace in a verbatim string is literal text, not a hole: {theme}' + "an escaped hole in a verbatim string is still escaped" ); }); From aba2f86bac80eb056c44c9d9fa7cf42ef87f9eec Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 01:19:56 +0000 Subject: [PATCH 6/8] Write the one-read rule and its four exceptions into the skill Why: the count rule lived only as "never hoist a mutated collection's length", so the safe direction had no guidance and the drift half of it was invisible. What changed: - State one read per method as a drift rule, not a speed one, in hot-path-allocations and in context.md rule 18. - Name the four reasons a second read is a different number, each with the in-repo site that proves it. - Record that the test sites are excluded, and which one breaks if hoisted. --- .llm/context.md | 14 ++++----- .llm/skills/hot-path-allocations/SKILL.md | 35 +++++++++++++++++------ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/.llm/context.md b/.llm/context.md index 606602e6..f22bbb38 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -142,7 +142,8 @@ frontmatter validity, index freshness, and pointer-file delegation; see `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). + terminate. One read per method (a count has one source of truth), and the four 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..98380059 100644 --- a/.llm/skills/hot-path-allocations/SKILL.md +++ b/.llm/skills/hot-path-allocations/SKILL.md @@ -124,15 +124,30 @@ first-inserted-wins for case-variant duplicates. (`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. +- **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. +- **Four reasons a second read is a different number, and each needs the reads left + alone.** A reviewer who hoists these introduces a bug, so each carries a comment: + 1. The loop body mutates that collection (`CyclicBuffer.Resize`'s `RemoveRange`, the + `Fix Invalid Fonts` `RemoveAt` loop, `KebabCase`'s trailing `builder.Length -= 1`). + 2. The variable is reassigned between the reads, so they are different values + (`TerminalThemeStyleSheetHelper.GetAvailableThemes`' `Regex.Replace`, and + `TextFieldPaste.TryApply`'s second read of the post-trim `flattened`). + 3. The collection is a live view handed out per access, so a reference stamp would + never match (`TerminalUIEditor.RefreshKeyCache` takes `Keys`). + 4. It is a test, and the repetition is the assertion - `CountReflectsNumberOfEntries` + reads `history.Count` after every `Push`; hoisting there breaks the test it is. +- **Sweep for it, then classify every hit.** A count of three or more reads of one + `X.Length`/`X.Count` per method body, across `Runtime/`, `Editor/`, `Tests/`, + `Samples~`, `Generator~`, is a short enough list to review by hand - the session that + raised it found 22 sites and classified 9 as hoistable, 6 as traps, 7 as tests. + The classification, not the count, is the work. - 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 @@ -182,5 +197,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`. From fe082ebb8227363eff2b4362cae45fcd9a3ca450 Mon Sep 17 00:00:00 2001 From: wallstop Date: Tue, 29 Sep 2026 01:23:11 +0000 Subject: [PATCH 7/8] Rebuild the shipped generator payload for the emitter change Why: CatalogEmitter gained the hoisted count, so the committed DLL no longer matched its sources and the two-checkout gate failed. How we know: verify-analyzer-payload.ps1 passes for both payloads. --- ...ios.DxCommandTerminal.SourceGenerators.dll | Bin 32256 -> 32256 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll b/Runtime/Analyzers/WallstopStudios.DxCommandTerminal.SourceGenerators.dll index b946554f2e67b7a24003d41ec1280a202397f106..121006c2bf7f9378e98173e7a1ea7d0b9541020a 100644 GIT binary patch delta 3302 zcma)i?!I^Thx`gecZGqKUs^>61VQ|Pq|pW{F=$dFgULvuq7+kG zO&2On)DqjxV{BqcYeieNM5m&yc4%T6#;T=FO)~0?N@}AFBp5ZZX*8zoxp$w6|8-~h zaK7i9 zjQCo~Cq5PQ$G`#CEFZHXR~Y~T_*jDvz~u^p7a&M+&=%E%b^7N{@UR9y#R6*f7{3}0 z2-D$ntjh;ba}6?zgrO#S`jaU6EELM8!@lf)scncBYKYJ40|XoA>ho;VLqoU6wrOT-1eLR8I#4f zyPQ6!PY)ZSgOfUt)F!?&a_dU&&AD-e&B7+-I2#_I2^!9D9ErFrK)U)2(iO<5?B3^K z>5(&L2F%dUnN`gz9f(!UDUx2vO_D#8ERb9$nJI~qKXi$#MiA_f+$q^5c~G)PGSx4} zlxEuw{9!83jl?UDSmWG7YDfXmzzNs$}*|B_?PVkP!EMCoygV6o&PT9XEsP=C^ko?KgUhEns`}Yhn1KMa38Pnk*DK;6`De$pR%#nFSxh6TZ(6~h&F;s&K z2ICqy{4PEPdLh6Q=($d`R%B;Bn932>2F0`gLJ}AuQZL?Bsn&0^QzDt<>q-+NqVPNS zM6SSDTkUoia1{n^^*AX9EK84#{4thL$AYYwI!CcMSf3M9C*3971xLrn)D>e6H$bH$ zLv;9_F`sj&u+s zPl0C&gc`u{`er^F+HLh;XOxeD@Kg~KTcFVqH(`WQf-ui|Gbb@J%!05?2DZY}_zE8j z9dx<@Y=u&FJ7fS93l)Xau3z&oEVY$S*~iDhVOuS59pO1}bXsiW0oKd&;F_(P*hxMf zDobLq9>+U;0u+?SRJnOB!t z0B*3l*X0>3QUo42;VFetkn4SfxZPU~7x5nsjW)j%YlvIb7#Gt>%~c02(5Tmw9}Fa3 z@;4G6buT7%;`175Ic=-e1ZzSjZ0;rA#n2da0m?JES`b` z<^{5!c-ICONUrIR>LV6&8i{9K2T|` zc*(fUhHxQ6We97TlbGPDrv_uaqm^2=(@aZTGdxBDnvzZwGMuZZ&CeI0Y%gj>WmxWHiLSC$%x9z=ZHJq14 zFUg{pWXllafd%RiPSu|xhUwPZ!}j8f>Or~4A-d4J$lt|(6H#kfD}P@drV@kd4O!wX zY&X7CZ_D*{$aUY4MLXn9(BVh-0iDc5`U;?D$D{`{%VE+BWhZeI1T_WL!4YB;l$s{2g)L?t9XMmWc@TgwF$Lxj)1cCu z7lCYeg~TNIJFy5Z5NE|NB>Jw_MxfGjyLJ;66goRy-&!|qzI(| z{X(!mN!{O?63O-Mb`&P=S=lvj+u*>Bjpe~Z)^J@=2{_}r&83Att747Uzt;X8BJy4q hb}$-Bz*ehe%}dbSp*Tal7OA4SVnEIn-G;IrH}@Pq$xClh*zD|KrILb1yZ1>^vvwVm`atp zl0TjQoSAdZ%$+;y4d21=9env`sw=(w?Y7nw>}uPy0IMU2qqULhk2GBk6Ue*{OZDfxOD4`%( zLDjFWF91+;4Kj*_p(c3GB~dt*!nvQHGXu)M<0(^eV;nio=phefQgY(;W~#KZF`k^w zlxbyS0*nAp=5Y7&s9Sg=>MgqRNSwwWb>--K z$VVCCgL-xj^XoEa(B*f@hn5p0W4w519^J3|T_Hmh)2SHIDfSUPk%(Ul=IPDD*3UvF z4b>Y*P>Vi26hp#2h7J#%8wP2RGppxKjg7KTn3*uueaf5@UZDZb2`5Whk}Z-0Q$w#~*T4)KL42b9f$qywvD6tmM?aLz5!R-P z+v0^pFj7fv9#YAXJ=w{2Ci!|Z#E3R{+xu;77WMa@A3bNNKQgRCMIev6L#7 z)3FirPE#xfHswUsN8VEIfg@w1>MNs?8{ngRMvVNC@g(Ok+flVp$d4@Mk*yBB|F3^JHkAB-8~Q>u%u>L%XByx?biZA-_n( z#1`muH%=O+>{OUHOs2x(A$5tR!b+*O!w+x=PlryrBmuU=Wc4-31b2y0ZE(W#CJ#Y_ zqx{NVo&~>m)O^o=o&!fpqa*jRLp%>|I%+lR<6~{8E{g^aXn*4QP(C@TD$P^806ufn z&cK)SXhY@E*dg^Yp9DB1s@`ya%?rVDRFTrei=fz1?|E+VVwmA5kJ`seV4b7p1;8o= zyWNo$o+dsS4&Mv+rjLELFRFYV1_|IS?m z*YK#e6>ed(`xWwZ^VcAS=IRi`j=3+|K<|Qag94>Nz)=wIR*327!xUmH=15PD^b|-> ziDU|tSw)mcv_4(7ULft6(k_wqTxrjhOB%$N+!Z)e+S46RH=m7m61(L7KBloYrJkBN zVJs(my?X_*!`O%4qeOqeI+Ll=f?=G*W3bOWL-sTLJT}N(S?y4)cn~W29oh>q zcaxmk3cZ0RpcQ=XJhn-euZuQeqY^AY)k689D~@Dccf!(W+K9u;ciYLSI?5fHwBEcdW7m zE0`Gno=p?FWk=occRF<4&>d)0(%2@pUfF={)Jm_M)+@&hV%XEB93W3GQJjDSa@()L zAMt(V8thU(Aip>hhvlvumaA+4@lZq_ga-LgK9I)`WG9So zlr(%7Gt@S$AWt70G_%ytWTHN(;l=71$L>j=6JuedZbBj)C8oeGmkFtm>o(~nY5Qld!w{BI`%TSYjsFZhxBy7GRXA*4OWS{7$7Q40txNfC_ z-2l(R7I*_5#|5|=$FT+M8v7^nD&v)6WsR~yX;+RbR~1kfskLf@x<%cm>f#pyjKL&} zfes3_{wVarS1|1OPdyBox>Xnz2y~NQo1+!&vcWom^?-24jAj|}INpuJE3ivs;Jx&KLxOqmeGgr0d5>|Coh;nR@!PIh_t z^9iTx3Tk&p29~9=fUCz Date: Tue, 29 Sep 2026 01:50:15 +0000 Subject: [PATCH 8/8] Complete the count sweep, and correct two wrong reasons Why: the first sweep hoisted 9 of 22 sites and called the rest traps. A second pass found 11 more hoistable, one 65 lines below a site already hoisted, and one excused by an inherited comment that was itself false. What changed: - Hoist the 11: RunTyped, RouteInvocation, AddArgument, the completion dedupe, the bake loops, RefreshRows, the palette caret, the property path walk, GetAvailableThemes, and the collapse's outer bound. - An element overwrite is not a change of count, so the collapse's "must stay inline" comment was wrong; the loop is now hoisted. - Rule 18 and the skill say "changes that collection's count", and the skill no longer quotes a tally it cannot defend. --- .llm/context.md | 4 +- .llm/skills/hot-path-allocations/SKILL.md | 57 +++++++++++-------- .../Helper/TerminalThemeStyleSheetHelper.cs | 9 +-- .../Backend/CommandAutoComplete.cs | 16 +++--- .../CommandTerminal/Backend/CommandBuilder.cs | 28 +++++---- .../Backend/CommandCompatibilityBake.cs | 4 +- .../CommandTerminal/Backend/CommandHistory.cs | 1 - .../CommandTerminal/Backend/CommandShell.cs | 4 +- .../CommandTerminal/UI/CommandPaletteUI.cs | 13 +++-- Runtime/CommandTerminal/UI/TerminalUI.cs | 6 +- .../SerializedPropertyExtensions.cs | 7 ++- 11 files changed, 82 insertions(+), 67 deletions(-) diff --git a/.llm/context.md b/.llm/context.md index f22bbb38..0f511925 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -141,8 +141,8 @@ 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. One read per method (a count has one source of truth), and the four exceptions + 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). diff --git a/.llm/skills/hot-path-allocations/SKILL.md b/.llm/skills/hot-path-allocations/SKILL.md index 98380059..744f621b 100644 --- a/.llm/skills/hot-path-allocations/SKILL.md +++ b/.llm/skills/hot-path-allocations/SKILL.md @@ -116,14 +116,17 @@ 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. +- **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 @@ -132,27 +135,31 @@ first-inserted-wins for case-variant duplicates. `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. -- **Four reasons a second read is a different number, and each needs the reads left - alone.** A reviewer who hoists these introduces a bug, so each carries a comment: - 1. The loop body mutates that collection (`CyclicBuffer.Resize`'s `RemoveRange`, the - `Fix Invalid Fonts` `RemoveAt` loop, `KebabCase`'s trailing `builder.Length -= 1`). - 2. The variable is reassigned between the reads, so they are different values - (`TerminalThemeStyleSheetHelper.GetAvailableThemes`' `Regex.Replace`, and - `TextFieldPaste.TryApply`'s second read of the post-trim `flattened`). - 3. The collection is a live view handed out per access, so a reference stamp would - never match (`TerminalUIEditor.RefreshKeyCache` takes `Keys`). - 4. It is a test, and the repetition is the assertion - `CountReflectsNumberOfEntries` - reads `history.Count` after every `Push`; hoisting there breaks the test it is. -- **Sweep for it, then classify every hit.** A count of three or more reads of one - `X.Length`/`X.Count` per method body, across `Runtime/`, `Editor/`, `Tests/`, - `Samples~`, `Generator~`, is a short enough list to review by hand - the session that - raised it found 22 sites and classified 9 as hoistable, 6 as traps, 7 as tests. - The classification, not the count, is the work. +- **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) 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/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 bb56c2bb..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) @@ -315,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( @@ -487,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 22d196d6..baf40bdd 100644 --- a/Runtime/CommandTerminal/Backend/CommandHistory.cs +++ b/Runtime/CommandTerminal/Backend/CommandHistory.cs @@ -78,7 +78,6 @@ public string Next(bool skipSameCommands) } _direction = 1; - /* One read: the loops below move _position, never the history. */ int count = _history.Count; while ( skipSameCommands diff --git a/Runtime/CommandTerminal/Backend/CommandShell.cs b/Runtime/CommandTerminal/Backend/CommandShell.cs index c384c65b..b477a203 100644 --- a/Runtime/CommandTerminal/Backend/CommandShell.cs +++ b/Runtime/CommandTerminal/Backend/CommandShell.cs @@ -1234,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/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 38d381e1..51ee5569 100644 --- a/Runtime/CommandTerminal/UI/TerminalUI.cs +++ b/Runtime/CommandTerminal/UI/TerminalUI.cs @@ -2674,9 +2674,9 @@ guarded local instead of re-deriving nullability per access. /* One read of the log count: the loops below add to and remove - from `content`, which is why its own childCount stays inline - - that re-read is what terminates them. The log list is only - read here, so its count is the same number throughout. + 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) 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;