From 84a6b566ae8d87ed290fa07e273a8f406e83758c Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Mon, 10 Aug 2026 22:36:17 +1000 Subject: [PATCH] Locate inline call sites lexically rather than by text search The patcher found its call site by searching the file for the previous expected expression, and its Snapshot/Verify identifiers with a plain IndexOf. Neither search knew what a comment or a string was, so several ordinary files were patched in the wrong place, silently: * Verify("Hello").Snapshot("Hello") replaced the verify argument * @"old" and "old"u8 were spliced through their quote, leaving the prefix * a Verify prefixed method declaration took .Snapshot() into its signature * a comment mid chain put .Snapshot() ahead of the rest of the chain * a stale hint reached a commented out example or another method * Remove pulled the trailing semicolon into the comment above the call CsScan lexes the file once and answers all of it: which offsets are code, where each comment and literal span runs, and whether an identifier is being declared or called. The lexer moves out of InlinePatcher rather than being written a second time, so the structural scan and the map cannot disagree. The content search now walks Snapshot call sites outward from the hint and compares their expected argument spans, so it can no longer land on anything that is not an expected argument. Generic calls (Snapshot, Verify) and comments inside the argument list were being refused, and now work. InlineApplier decodes with throwing encodings. It rewrites the whole file, so a byte that will not decode is not a local defect: every one of them would come back as a replacement character. A file that is not UTF-8, or not what its BOM claims, now fails with a message and is left as it was. --- src/DiffEngine.Tests/InlineApplierTests.cs | 46 ++ src/DiffEngine.Tests/InlinePatcherTests.cs | 242 ++++++++ src/DiffEngine/Inline/CsScan.cs | 510 +++++++++++++++++ src/DiffEngine/Inline/InlineApplier.cs | 23 +- src/DiffEngine/Inline/InlinePatcher.cs | 634 +++++++-------------- 5 files changed, 1030 insertions(+), 425 deletions(-) create mode 100644 src/DiffEngine/Inline/CsScan.cs diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 64b5f9e0..2f07a21a 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -82,6 +82,52 @@ public async Task Utf16Preserved() } } + // The whole file is rewritten, not just the patched span, so a byte that does not decode + // would come back as a replacement character everywhere it appears + [Test] + public async Task NonUtf8FileIsRefused() + { + byte[] bytes = + [ + .. Utf8("class C\n{\n // caf", bom: false), + 0xE9, // é in Latin-1, not valid UTF-8 + .. Utf8("\n void M() => Verify(value).Snapshot(\"old\");\n}", bom: false) + ]; + var path = WriteTemp(bytes); + try + { + var result = InlineApplier.Apply(new(path, 4, "\"old\"", "new")); + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Failed); + await Assert.That(result.Message!).Contains("Convert it to UTF-8"); + await Assert.That((await File.ReadAllBytesAsync(path)).SequenceEqual(bytes)).IsTrue(); + } + finally + { + File.Delete(path); + } + } + + [Test] + public async Task NonAsciiUtf8IsPreserved() + { + var text = "class C\n{\n // café ☕\n void M() => Verify(value).Snapshot(\"old\");\n}"; + var path = WriteTemp(Utf8(text, bom: false)); + try + { + var result = InlineApplier.Apply(new(path, 4, "\"old\"", "naïve ☕")); + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + var after = await File.ReadAllTextAsync(path); + await Assert.That(after).Contains("// café ☕"); + await Assert.That(after).Contains("naïve ☕"); + } + finally + { + File.Delete(path); + } + } + [Test] public async Task CrlfPreserved() { diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 71fc6bdf..c57741e5 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -652,4 +652,246 @@ public async Task SingleLineFileWithNoNewlines() await Assert.That(newSource).Contains("new"); await Assert.That(newSource).DoesNotContain("\"old\""); } + + // The verify argument is the same text as the snapshot, and comes first on the line + [Test] + public async Task SameLiteralInTheVerifyArgumentIsNotPatched() + { + var source = Method(" await Verify(\"same\").Snapshot(\"same\");"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, "\"same\"", "changed", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("await Verify(\"same\").Snapshot(\"\"\""); + await Assert.That(newSource).Contains("changed"); + } + + // The expression search must match a whole argument, not the quoted part of a longer literal + [Test] + public async Task PrefixedLiteralIsNotPatchedThroughItsQuote() + { + var source = Method(" await Verify(x).Snapshot(@\"old\");"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out _, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.NotFound); + await Assert.That(reason).Contains("Re-run the test"); + } + + [Test] + public async Task SuffixedLiteralIsNotPatchedThroughItsQuote() + { + var source = Method(" await Verify(x).Snapshot(\"old\"u8);"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out _, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.NotFound); + await Assert.That(reason).Contains("not a string literal"); + } + + [Test] + public async Task CommentedOutCallIsSkipped() + { + var source = string.Join( + "\n", + "class Tests", + "{", + " // await Verify(x).Snapshot(\"doc example\");", + " async Task Test() =>", + " await Verify(x).Snapshot();", + "}"); + + var status = InlinePatcher.TryApply(source, 3, InlinePatchMode.Set, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains(" // await Verify(x).Snapshot(\"doc example\");\n"); + await Assert.That(newSource).Contains(" await Verify(x).Snapshot(\"\"\""); + } + + [Test] + public async Task CallInsideAStringIsSkipped() + { + var source = Method( + " var text = \"await Snapshot(\\\"x\\\")\";\n" + + " await Verify(x).Snapshot();"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("var text = \"await Snapshot(\\\"x\\\")\";\n"); + await Assert.That(newSource).Contains("await Verify(x).Snapshot(\"\"\""); + } + + // A declaration is a name followed by parens too, so it has to be told apart by what precedes it + [Test] + public async Task SnapshotDeclarationIsNotMistakenForACall() + { + var source = string.Join( + "\n", + "static class Extensions", + "{", + " public static Task Snapshot(this Task task, string? expected = null) =>", + " task;", + "}"); + + var status = InlinePatcher.TryApply(source, 3, InlinePatchMode.Set, null, "new", out _, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.NotFound); + await Assert.That(reason).Contains("Could not find a Snapshot call"); + } + + [Test] + public async Task AppendSkipsAVerifyPrefixedDeclaration() + { + var source = string.Join( + "\n", + "class Tests", + "{", + " Task VerifyThing(string value) => Verify(value);", + "}"); + + var status = InlinePatcher.TryApply(source, 3, InlinePatchMode.Append, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains( + " Task VerifyThing(string value) => Verify(value)\n" + + " .Snapshot(\"\"\""); + } + + // Snapshot terminates the chain, so a comment in the middle of one must not end the walk + [Test] + public async Task AppendGoesAfterACommentInTheChain() + { + var source = Method( + " await Verify(value) // note\n" + + " .UseDirectory(\"snapshots\");"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Append, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains( + " .UseDirectory(\"snapshots\")\n" + + " .Snapshot(\"\"\""); + } + + [Test] + public async Task LiteralInACommentIsNotPatched() + { + var source = Method( + " // was \"old\"\n" + + " await Verify(x).Snapshot(\"old\");"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("// was \"old\"\n"); + await Assert.That(newSource).Contains("Snapshot(\"\"\""); + } + + [Test] + public async Task LiteralInAnotherMethodIsNotPatched() + { + var source = string.Join( + "\n", + "class Tests", + "{", + " void Helper() => Log(\"old\");", + "", + " async Task Test() =>", + " await Verify(x).Snapshot(\"old\");", + "}"); + + var status = InlinePatcher.TryApply(source, 3, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("void Helper() => Log(\"old\");"); + await Assert.That(newSource).Contains("Snapshot(\"\"\""); + } + + // The empty literal is two characters, and the opening of every raw literal holds a pair + [Test] + public async Task EmptyOriginalIsNotMatchedInsideARawDelimiter() + { + var source = string.Join( + "\n", + "class Tests", + "{", + " Task A() =>", + " Verify(a).Snapshot(\"\"\"", + " content", + " \"\"\");", + "", + " Task B() =>", + " Verify(b).Snapshot(\"\");", + "}"); + + var status = InlinePatcher.TryApply(source, 7, InlinePatchMode.Set, "\"\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains(" content\n \"\"\");"); + await Assert.That(newSource).Contains("Verify(b).Snapshot(\"\"\"\n new\n \"\"\");"); + } + + [Test] + public async Task GenericSnapshotCall() + { + var source = Method(" await Verify(x).Snapshot();"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains(".Snapshot(\"\"\""); + } + + [Test] + public async Task AppendToAGenericVerify() + { + var source = Method(" await Verify(value);"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Append, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("await Verify(value)\n .Snapshot(\"\"\""); + } + + [Test] + public async Task CommentInTheArgumentListIsNotTheArgument() + { + var source = Method(" await Verify(x).Snapshot(/* keep */);"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, null, "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("Snapshot(/* keep */\"\"\""); + } + + [Test] + public async Task CommentAfterTheArgumentIsKept() + { + var source = Method(" await Verify(x).Snapshot(\"old\" /* why */);"); + + var status = InlinePatcher.TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("\"\"\" /* why */);"); + } + + // Pulling the call up onto the line above would take the semicolon into the comment + [Test] + public async Task RemoveLeavesALineCommentAboveIntact() + { + var source = Method( + " await Verify(value)\n" + + " // note\n" + + " .Snapshot(\"old\");"); + + var status = InlinePatcher.TryApply(source, 7, InlinePatchMode.Remove, null, "", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).IsEqualTo( + Method( + " await Verify(value)\n" + + " // note\n" + + " ;")); + } } diff --git a/src/DiffEngine/Inline/CsScan.cs b/src/DiffEngine/Inline/CsScan.cs new file mode 100644 index 00000000..dfeeec9a --- /dev/null +++ b/src/DiffEngine/Inline/CsScan.cs @@ -0,0 +1,510 @@ +/// +/// A one pass lexical map of a C# file: where the comments, strings and char literals are, and +/// therefore which offsets are code. +/// +/// The patcher finds its call sites by scanning text, and a text scan that cannot see a comment +/// or a string patches a commented out example, or the middle of another test's snapshot content, +/// as readily as the real call. Lexing once and asking the map is cheaper than lexing per search, +/// and it is one implementation: every search agrees on what a string is because there is only +/// one answer to ask. +/// +/// +sealed class CsScan +{ + readonly string source; + readonly bool[] code; + + /// + /// Start of a comment or literal to the offset just past it. + /// + readonly Dictionary skips = new(); + + /// + /// The same spans keyed the other way round, for a scan working backwards. Ends are unique + /// because the spans cannot overlap. + /// + readonly Dictionary skipEnds = new(); + + public CsScan(string source) + { + this.source = source; + code = new bool[source.Length]; + var index = 0; + while (index < source.Length) + { + var start = index; + switch (source[index]) + { + case '/': + if (TrySkipComment(source, ref index)) + { + AddSkip(start, index); + continue; + } + + break; + case '\'': + if (TrySkipCharLiteral(source, ref index)) + { + AddSkip(start, index); + continue; + } + + // Unterminated: take the quote as code rather than swallowing the rest of + // the file over one stray character + index = start; + break; + case '"': + case '@': + case '$': + if (TrySkipStringLike(source, ref index)) + { + // A suffix (u8) is part of the literal token, so a search for "x" cannot + // match "x"u8 and splice over only the quoted part + while (index < source.Length && + IsIdentifierChar(source[index])) + { + index++; + } + + AddSkip(start, index); + continue; + } + + break; + } + + code[index] = true; + index++; + } + } + + void AddSkip(int start, int end) + { + skips.Add(start, end); + skipEnds[end] = start; + } + + /// + /// True when the offset is outside every comment, string and char literal. + /// + public bool IsCode(int index) => + index >= 0 && + index < code.Length && + code[index]; + + /// + /// When a comment or literal starts at , is the + /// offset just past it. Lets a structural scan step over trivia without lexing it again. + /// + public bool TryGetSkip(int index, out int end) => + skips.TryGetValue(index, out end); + + /// + /// True when a comment ends at , with set to + /// where it began. Only comments: a literal is content, and trimming one off a span would be + /// trimming off the value. + /// + public bool TryGetCommentEndingAt(int end, out int start) => + skipEnds.TryGetValue(end, out start) && + source[start] == '/'; + + /// + /// Advances past whitespace and comments. + /// + public void SkipTrivia(ref int index) + { + while (index < source.Length) + { + if (char.IsWhiteSpace(source[index])) + { + index++; + continue; + } + + if (source[index] == '/' && + skips.TryGetValue(index, out var end)) + { + index = end; + continue; + } + + return; + } + } + + /// + /// True when the identifier at is being declared rather than + /// called. The two are otherwise identical - name, parens, body - so the tell is the token in + /// front: a declaration is preceded by its return type, a call by a dot, an operator, or one + /// of the keywords that can introduce an expression. + /// + public bool IsDeclaration(int nameStart) + { + var index = PreviousSignificant(nameStart); + if (index < 0) + { + return false; + } + + var ch = source[index]; + if (ch == '>') + { + // Close of a generic return type (Task Name), unless it is a lambda arrow + return index == 0 || + source[index - 1] != '='; + } + + if (!IsIdentifierChar(ch)) + { + return false; + } + + var start = index; + while (start > 0 && + IsIdentifierChar(source[start - 1])) + { + start--; + } + + return !callablePredecessors.Contains(source.Substring(start, index - start + 1)); + } + + /// + /// The keywords that can sit immediately before a call. Anything else that reads as an + /// identifier there is a return type or a modifier, which makes what follows a declaration. + /// + static readonly HashSet callablePredecessors = new(StringComparer.Ordinal) + { + "and", "as", "await", "by", "case", "catch", "checked", "default", "do", "else", + "equals", "fixed", "foreach", "from", "goto", "group", "if", "in", "into", "is", + "join", "let", "lock", "new", "not", "on", "or", "orderby", "out", "params", "ref", + "return", "select", "stackalloc", "switch", "throw", "unchecked", "using", "when", + "where", "while", "with", "yield" + }; + + /// + /// The offset of the last character before that is neither + /// whitespace nor inside a comment, or -1 when there is none. + /// + int PreviousSignificant(int index) + { + index--; + while (index >= 0 && + (char.IsWhiteSpace(source[index]) || !code[index])) + { + index--; + } + + return index; + } + + /// + /// Advances past a type argument list, so Foo<Bar>(...) is located as readily as + /// Foo(...). Only the characters a type argument list can hold are accepted, and the caller + /// still has to find a '(' after it, so a comparison cannot be mistaken for one. + /// + public static bool TrySkipTypeArguments(string source, ref int index) + { + var cursor = index; + if (cursor >= source.Length || + source[cursor] != '<') + { + return false; + } + + cursor++; + var depth = 1; + while (cursor < source.Length) + { + var ch = source[cursor]; + if (ch == '<') + { + depth++; + cursor++; + continue; + } + + if (ch == '>') + { + depth--; + cursor++; + if (depth == 0) + { + index = cursor; + return true; + } + + continue; + } + + if (IsIdentifierChar(ch) || + ch is ',' or '.' or '?' or '[' or ']' or ':' or ' ' or '\t') + { + cursor++; + continue; + } + + return false; + } + + return false; + } + + public static bool IsIdentifierChar(char ch) => + char.IsLetterOrDigit(ch) || ch == '_'; + + static bool TrySkipComment(string source, ref int index) + { + if (index + 1 >= source.Length) + { + return false; + } + + var next = source[index + 1]; + if (next == '/') + { + var end = source.IndexOf('\n', index); + index = end < 0 ? source.Length : end + 1; + return true; + } + + if (next == '*') + { + var end = source.IndexOf("*/", index + 2, StringComparison.Ordinal); + index = end < 0 ? source.Length : end + 2; + return true; + } + + return false; + } + + static bool TrySkipCharLiteral(string source, ref int index) + { + // index at the opening quote + index++; + while (index < source.Length) + { + var ch = source[index]; + if (ch == '\\') + { + index += 2; + continue; + } + + if (ch == '\'') + { + index++; + return true; + } + + if (ch == '\n') + { + return false; + } + + index++; + } + + return false; + } + + // index at '$', '@' or '"'. Returns false when the characters do not start a string literal + // (eg '@identifier'); the caller then advances by one. + static bool TrySkipStringLike(string source, ref int index) + { + var cursor = index; + var dollars = 0; + var verbatim = false; + while (cursor < source.Length) + { + var ch = source[cursor]; + if (ch == '$') + { + dollars++; + cursor++; + continue; + } + + if (ch == '@') + { + verbatim = true; + cursor++; + continue; + } + + break; + } + + if (cursor >= source.Length || source[cursor] != '"') + { + return false; + } + + var quotes = QuoteRun(source, cursor); + if (quotes >= 3) + { + // Raw string (interpolated or not): skip blindly to a closing run of >= quotes. + // Interpolation holes are skipped as part of the content. + var search = cursor + quotes; + while (true) + { + if (search >= source.Length) + { + index = source.Length; + return true; + } + + if (source[search] != '"') + { + search++; + continue; + } + + var run = QuoteRun(source, search); + if (run >= quotes) + { + index = search + run; + return true; + } + + search += run; + } + } + + cursor += quotes == 2 ? 2 : 1; + if (quotes == 2) + { + // Empty string "" or interpolated empty string $"" + index = cursor; + return true; + } + + while (cursor < source.Length) + { + var ch = source[cursor]; + if (ch == '"') + { + if (verbatim && + cursor + 1 < source.Length && + source[cursor + 1] == '"') + { + cursor += 2; + continue; + } + + index = cursor + 1; + return true; + } + + if (!verbatim && ch == '\\') + { + cursor += 2; + continue; + } + + if (!verbatim && ch == '\n') + { + // Malformed: unterminated regular string. Stop at the line end. + index = cursor; + return true; + } + + if (dollars > 0 && ch == '{') + { + if (cursor + 1 < source.Length && source[cursor + 1] == '{') + { + cursor += 2; + continue; + } + + if (!TrySkipHole(source, ref cursor)) + { + index = source.Length; + return true; + } + + continue; + } + + if (dollars > 0 && ch == '}' && + cursor + 1 < source.Length && source[cursor + 1] == '}') + { + cursor += 2; + continue; + } + + cursor++; + } + + index = source.Length; + return true; + } + + // cursor at '{' of an interpolation hole; skips past the matching '}' + static bool TrySkipHole(string source, ref int cursor) + { + var depth = 1; + cursor++; + while (cursor < source.Length) + { + var ch = source[cursor]; + switch (ch) + { + case '/': + if (!TrySkipComment(source, ref cursor)) + { + cursor++; + } + + continue; + case '\'': + if (!TrySkipCharLiteral(source, ref cursor)) + { + return false; + } + + continue; + case '"': + case '@': + case '$': + if (!TrySkipStringLike(source, ref cursor)) + { + cursor++; + } + + continue; + case '{': + depth++; + cursor++; + continue; + case '}': + depth--; + cursor++; + if (depth == 0) + { + return true; + } + + continue; + default: + cursor++; + continue; + } + } + + return false; + } + + static int QuoteRun(string source, int index) + { + var count = 0; + while (index + count < source.Length && + source[index + count] == '"') + { + count++; + } + + return count; + } +} diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 2a195984..57920193 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -88,6 +88,12 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string { source = encoding.GetString(bytes, bomLength, bytes.Length - bomLength); } + catch (DecoderFallbackException exception) + { + return InlineApplyResult.Failed( + $"Could not decode as {encoding.WebName}: {fullPath}. Every byte that failed to decode would be replaced on write, so the file is left alone. Convert it to UTF-8 and re-run the test.", + exception); + } catch (Exception exception) { return InlineApplyResult.Failed($"Failed to decode: {fullPath}", exception); @@ -136,29 +142,36 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string return InlineApplyResult.Applied; } + /// + /// The encoding to read and write the file with. Every one of them throws rather than + /// substituting: the applier rewrites the whole file, not just the patched span, so a + /// replacement character for an undecodable byte is not a local defect but a file wide one. + /// A source file that is not what its BOM says, or is not UTF-8 when it has no BOM, has to + /// fail loudly and stay as it was. + /// static (Encoding encoding, int bomLength) DetectEncoding(byte[] bytes) { if (bytes is [0xFF, 0xFE, 0x00, 0x00, ..]) { - return (new UTF32Encoding(false, true), 4); + return (new UTF32Encoding(false, true, true), 4); } if (bytes is [0xEF, 0xBB, 0xBF, ..]) { - return (new UTF8Encoding(true), 3); + return (new UTF8Encoding(true, true), 3); } if (bytes is [0xFF, 0xFE, ..]) { - return (new UnicodeEncoding(false, true), 2); + return (new UnicodeEncoding(false, true, true), 2); } if (bytes is [0xFE, 0xFF, ..]) { - return (new UnicodeEncoding(true, true), 2); + return (new UnicodeEncoding(true, true, true), 2); } - return (new UTF8Encoding(false), 0); + return (new UTF8Encoding(false, true), 0); } static string MutexName(string normalizedPath) diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index 984c0af3..cdc97ad5 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -16,6 +16,12 @@ static class InlinePatcher /// const string methodName = "Snapshot"; + /// + /// The parameter the snapshot literal binds to. Positional in a normal call, but it can be + /// written by name. + /// + const string parameterName = "expected"; + /// /// Append mode has no Snapshot call to find, so it locates the verify invocation instead. /// Matched by prefix because every entry point is Verify, VerifyXml, VerifyJson and so on. @@ -35,47 +41,58 @@ public static PatchStatus TryApply( failReason = ""; var eol = DetectEol(source); var lineStarts = BuildLineStarts(source); + var scan = new CsScan(source); if (mode == InlinePatchMode.Remove) { - return TryRemove(source, lineStarts, lineHint, ref newSource, ref failReason); + return TryRemove(source, scan, lineStarts, lineHint, ref newSource, ref failReason); } if (mode == InlinePatchMode.Append) { - return TryAppend(source, lineStarts, lineHint, newContent, eol, ref newSource, ref failReason); + return TryAppend(source, scan, lineStarts, lineHint, newContent, eol, ref newSource, ref failReason); } if (!string.IsNullOrEmpty(originalExpression)) { - // Search for the previous expression verbatim, with newlines matched to the file's EOL + // Located by content: the Snapshot call whose expected argument is still the text the + // test run saw, nearest the hint first. Matched against expected arguments rather than + // searched for as plain text, because the same literal is just as likely to sit in a + // comment, in another test's snapshot content, or in the verify call on the same line, + // and splicing into one of those leaves a file that no longer compiles and a snapshot + // still unaccepted. // ReSharper disable once RedundantSuppressNullableWarningExpression var needle = NormalizeTo(originalExpression!, eol); - var occurrences = FindAll(source, needle); - if (occurrences.Count > 0) + foreach (var (_, openParen) in FindCalls(source, scan, lineStarts, lineHint, methodName, false)) { - var start = Nearest(occurrences, lineStarts, lineHint); + if (!TryReadArguments(source, scan, openParen, out var expected) || + !expected.Matches(source, needle)) + { + continue; + } + if (CsStringLiteral.TryParse(needle, out var oldValue) && oldValue == newContent) { return PatchStatus.AlreadyApplied; } - var indent = IndentForSpan(source, lineStarts, start); + var indent = IndentForSpan(source, lineStarts, expected.Start); var rendered = CsStringLiteral.RenderRaw(newContent, indent, eol); - newSource = Splice(source, start, start + needle.Length, rendered); + newSource = Splice(source, expected.Start, expected.End, rendered); return PatchStatus.Applied; } // Expression gone: another process may have applied the same patch already - return InsertOrCheck(source, lineStarts, lineHint, newContent, eol, alreadyOnly: true, ref newSource, ref failReason); + return InsertOrCheck(source, scan, lineStarts, lineHint, newContent, eol, alreadyOnly: true, ref newSource, ref failReason); } - return InsertOrCheck(source, lineStarts, lineHint, newContent, eol, alreadyOnly: false, ref newSource, ref failReason); + return InsertOrCheck(source, scan, lineStarts, lineHint, newContent, eol, alreadyOnly: false, ref newSource, ref failReason); } static PatchStatus InsertOrCheck( string source, + CsScan scan, List lineStarts, int lineHint, string newContent, @@ -84,24 +101,19 @@ static PatchStatus InsertOrCheck( ref string newSource, ref string failReason) { - if (!TryFindCall(source, lineStarts, lineHint, out var openParen)) + if (!TryFindCall(source, scan, lineStarts, lineHint, out var openParen)) { failReason = $"Could not find a {methodName} call near line {lineHint}. The source may have changed since the test run. Re-run the test."; return PatchStatus.NotFound; } - if (!TryScanArguments(source, openParen, out var closeParen, out var topCommas)) + if (!TryReadArguments(source, scan, openParen, out var expected)) { failReason = $"Could not parse the argument list of the {methodName} call near line {lineHint}."; return PatchStatus.NotFound; } - // expected is the first parameter of Snapshot, so the argument to set is the first one - var argStart = openParen + 1; - var argEnd = topCommas.Count > 0 ? topCommas[0] : closeParen; - TrimSpan(source, ref argStart, ref argEnd); - - if (argStart == argEnd) + if (expected.IsAbsent) { // The argument was left to its default if (alreadyOnly) @@ -110,17 +122,13 @@ static PatchStatus InsertOrCheck( return PatchStatus.NotFound; } - var emptyIndent = IndentForSpan(source, lineStarts, argStart); + var emptyIndent = IndentForSpan(source, lineStarts, expected.Start); var emptyRendered = CsStringLiteral.RenderRaw(newContent, emptyIndent, eol); - newSource = Splice(source, argStart, argStart, emptyRendered); + newSource = Splice(source, expected.Start, expected.Start, emptyRendered); return PatchStatus.Applied; } - var argOriginalStart = argStart; - var named = TryStripArgumentName(source, ref argStart, out var argumentName); - var argText = source.Substring(argStart, argEnd - argStart); - - if (named && argumentName != "expected") + if (expected.BlockedByName) { // Some other named argument came first (eg file:). // Insert a named expected argument before it. @@ -130,12 +138,13 @@ static PatchStatus InsertOrCheck( return PatchStatus.NotFound; } - var namedIndent = IndentForSpan(source, lineStarts, argOriginalStart); + var namedIndent = IndentForSpan(source, lineStarts, expected.ListStart); var namedRendered = CsStringLiteral.RenderRaw(newContent, namedIndent, eol); - newSource = Splice(source, argOriginalStart, argOriginalStart, "expected: " + namedRendered + ", "); + newSource = Splice(source, expected.ListStart, expected.ListStart, $"{parameterName}: {namedRendered}, "); return PatchStatus.Applied; } + var argText = source.Substring(expected.Start, expected.End - expected.Start); if (argText == "null") { if (alreadyOnly) @@ -144,9 +153,9 @@ static PatchStatus InsertOrCheck( return PatchStatus.NotFound; } - var indent = IndentForSpan(source, lineStarts, argStart); + var indent = IndentForSpan(source, lineStarts, expected.Start); var rendered = CsStringLiteral.RenderRaw(newContent, indent, eol); - newSource = Splice(source, argStart, argEnd, rendered); + newSource = Splice(source, expected.Start, expected.End, rendered); return PatchStatus.Applied; } @@ -170,6 +179,66 @@ static PatchStatus InsertOrCheck( static string StaleReason(int lineHint) => $"The previous expected expression was not found near line {lineHint}. The source may have changed since the test run. Re-run the test."; + /// + /// Where the expected argument of a call is, and in what shape. Worked out once because the + /// content search and the insert path need the same answer: one compares the span, the other + /// decides from the shape what to splice. + /// + readonly struct ExpectedArgument(int start, int end, int listStart, bool blockedByName) + { + /// + /// Start of the argument, past any expected: name. + /// + public int Start { get; } = start; + + public int End { get; } = end; + + /// + /// Start of the first argument, before any argument name. + /// + public int ListStart { get; } = listStart; + + /// + /// The first argument is named, and is not the expected one, so an expected argument has + /// to be inserted in front of it. + /// + public bool BlockedByName { get; } = blockedByName; + + /// + /// The argument was left to its default. + /// + public bool IsAbsent => Start == End; + + /// + /// True when the argument is character for character the given expression. + /// + public bool Matches(string source, string expression) => + !IsAbsent && + !BlockedByName && + End - Start == expression.Length && + string.CompareOrdinal(source, Start, expression, 0, expression.Length) == 0; + } + + static bool TryReadArguments(string source, CsScan scan, int openParen, out ExpectedArgument expected) + { + expected = default; + if (!TryScanArguments(source, scan, openParen, out var closeParen, out var topCommas)) + { + return false; + } + + // expected is the first parameter of Snapshot, so the argument to read is the first one + var start = openParen + 1; + var end = topCommas.Count > 0 ? topCommas[0] : closeParen; + TrimSpan(source, scan, ref start, ref end); + var listStart = start; + var blockedByName = start != end && + TryStripArgumentName(source, ref start, out var argumentName) && + argumentName != parameterName; + expected = new(start, end, listStart, blockedByName); + return true; + } + /// /// Appends a Snapshot call to the verify invocation, for a snapshot that has never been /// accepted. Snapshot terminates the chain, so the insertion point is the end of any calls @@ -177,6 +246,7 @@ static string StaleReason(int lineHint) => /// static PatchStatus TryAppend( string source, + CsScan scan, List lineStarts, int lineHint, string newContent, @@ -184,19 +254,19 @@ static PatchStatus TryAppend( ref string newSource, ref string failReason) { - if (!TryFindCall(source, lineStarts, lineHint, verifyPrefix, true, out var nameStart, out var openParen)) + if (!TryFindCall(source, scan, lineStarts, lineHint, verifyPrefix, true, out var nameStart, out var openParen)) { failReason = $"Could not find a {verifyPrefix} call near line {lineHint}. The source may have changed since the test run. Re-run the test."; return PatchStatus.NotFound; } - if (!TryScanArguments(source, openParen, out var closeParen, out _)) + if (!TryScanArguments(source, scan, openParen, out var closeParen, out _)) { failReason = $"Could not parse the argument list of the {verifyPrefix} call near line {lineHint}."; return PatchStatus.NotFound; } - var insertAt = WalkChain(source, closeParen + 1, methodName, out var alreadyChained); + var insertAt = WalkChain(source, scan, closeParen + 1, methodName, out var alreadyChained); // Another process may have appended one between the run and the accept if (alreadyChained) { @@ -221,18 +291,19 @@ static PatchStatus TryAppend( /// static PatchStatus TryRemove( string source, + CsScan scan, List lineStarts, int lineHint, ref string newSource, ref string failReason) { - if (!TryFindCall(source, lineStarts, lineHint, methodName, false, out var nameStart, out var openParen)) + if (!TryFindCall(source, scan, lineStarts, lineHint, methodName, false, out var nameStart, out var openParen)) { failReason = $"Could not find a {methodName} call near line {lineHint}. The source may have changed since the test run. Re-run the test."; return PatchStatus.NotFound; } - if (!TryScanArguments(source, openParen, out var closeParen, out _)) + if (!TryScanArguments(source, scan, openParen, out var closeParen, out _)) { failReason = $"Could not parse the argument list of the {methodName} call near line {lineHint}."; return PatchStatus.NotFound; @@ -254,6 +325,7 @@ static PatchStatus TryRemove( } start--; + var dotStart = start; // Then back over the indentation and line break it sat on while (start > 0 && (source[start - 1] == ' ' || source[start - 1] == '\t')) @@ -264,12 +336,16 @@ static PatchStatus TryRemove( if (start > 0 && source[start - 1] == '\n') { - start--; - if (start > 0 && - source[start - 1] == '\r') + var lineBreak = start - 1; + if (lineBreak > 0 && + source[lineBreak - 1] == '\r') { - start--; + lineBreak--; } + + // Not when the line above ends in a line comment: pulling the call up would take the + // semicolon that follows it into the comment + start = scan.IsCode(lineBreak) ? lineBreak : dotStart; } newSource = Splice(source, start, closeParen + 1, ""); @@ -280,34 +356,32 @@ static PatchStatus TryRemove( /// Walks the calls chained onto an invocation and returns the end of the chain. /// is set when one of them is a call to . /// - static int WalkChain(string source, int index, string name, out bool found) + static int WalkChain(string source, CsScan scan, int index, string name, out bool found) { found = false; while (true) { var cursor = index; - if (!TrySkipTo(source, ref cursor, '.')) + scan.SkipTrivia(ref cursor); + if (cursor >= source.Length || + source[cursor] != '.') { return index; } cursor++; - while (cursor < source.Length && - char.IsWhiteSpace(source[cursor])) - { - cursor++; - } + scan.SkipTrivia(ref cursor); var nameStart = cursor; while (cursor < source.Length && - IsIdentifierChar(source[cursor])) + CsScan.IsIdentifierChar(source[cursor])) { cursor++; } if (cursor == nameStart || - !TrySkipToParen(source, cursor, out var paren) || - !TryScanArguments(source, paren, out var closeParen, out _)) + !TrySkipToParen(source, scan, cursor, out var paren) || + !TryScanArguments(source, scan, paren, out var closeParen, out _)) { return index; } @@ -322,18 +396,6 @@ static int WalkChain(string source, int index, string name, out bool found) } } - static bool TrySkipTo(string source, ref int index, char ch) - { - while (index < source.Length && - char.IsWhiteSpace(source[index])) - { - index++; - } - - return index < source.Length && - source[index] == ch; - } - static string LeadingWhitespace(string source, List lineStarts, int offset) { var lineStart = lineStarts[LineOf(lineStarts, offset) - 1]; @@ -347,16 +409,12 @@ static string LeadingWhitespace(string source, List lineStarts, int offset) return source.Substring(lineStart, index - lineStart); } - static bool TryFindCall(string source, List lineStarts, int lineHint, out int openParen) => - TryFindCall(source, lineStarts, lineHint, methodName, false, out _, out openParen); + static bool TryFindCall(string source, CsScan scan, List lineStarts, int lineHint, out int openParen) => + TryFindCall(source, scan, lineStarts, lineHint, methodName, false, out _, out openParen); - /// - /// Locates a call by name, searching outward from the hint. matches - /// any identifier starting with , which is how the several Verify - /// overloads are found with one search. - /// static bool TryFindCall( string source, + CsScan scan, List lineStarts, int lineHint, string name, @@ -364,16 +422,39 @@ static bool TryFindCall( out int nameStart, out int openParen) { + foreach (var call in FindCalls(source, scan, lineStarts, lineHint, name, byPrefix)) + { + (nameStart, openParen) = call; + return true; + } + nameStart = -1; openParen = -1; + return false; + } + + /// + /// Locates calls by name, searching outward from the hint: hint, hint+1, hint-1, hint+2 and so + /// on. A tie goes to the line at or after the hint, because a file that moved under a pending + /// patch usually grew above the call rather than below it. + /// matches any identifier starting with , + /// which is how the several Verify overloads are found with one search. + /// + static IEnumerable<(int nameStart, int openParen)> FindCalls( + string source, + CsScan scan, + List lineStarts, + int lineHint, + string name, + bool byPrefix) + { var lineCount = lineStarts.Count; lineHint = Math.Min(Math.Max(lineHint, 1), lineCount); - // Outward from the hint: hint, hint-1, hint+1, hint-2, ... for (var distance = 0; distance < lineCount; distance++) { var candidates = distance == 0 ? new[] { lineHint } - : new[] { lineHint - distance, lineHint + distance }; + : new[] { lineHint + distance, lineHint - distance }; foreach (var line in candidates) { if (line < 1 || line > lineCount) @@ -396,50 +477,51 @@ static bool TryFindCall( if (byPrefix) { while (identifierEnd < source.Length && - IsIdentifierChar(source[identifierEnd])) + CsScan.IsIdentifierChar(source[identifierEnd])) { identifierEnd++; } } else if (identifierEnd < source.Length && - IsIdentifierChar(source[identifierEnd])) + CsScan.IsIdentifierChar(source[identifierEnd])) { index += name.Length; continue; } - if (StartsToken(source, index) && - TrySkipToParen(source, identifierEnd, out var paren)) + // In code, a whole token, an invocation rather than a declaration, and + // followed by an argument list. A commented out example passes none of these + if (scan.IsCode(index) && + StartsToken(source, index) && + !scan.IsDeclaration(index) && + TrySkipToParen(source, scan, identifierEnd, out var paren)) { - nameStart = index; - openParen = paren; - return true; + yield return (index, paren); } index += name.Length; } } } - - return false; } static bool StartsToken(string source, int index) => index == 0 || - !IsIdentifierChar(source[index - 1]); + !CsScan.IsIdentifierChar(source[index - 1]); - static bool IsIdentifierChar(char ch) => - char.IsLetterOrDigit(ch) || ch == '_'; - - static bool TrySkipToParen(string source, int index, out int paren) + static bool TrySkipToParen(string source, CsScan scan, int index, out int paren) { paren = -1; - while (index < source.Length && char.IsWhiteSpace(source[index])) + scan.SkipTrivia(ref index); + if (index < source.Length && + source[index] == '<' && + CsScan.TrySkipTypeArguments(source, ref index)) { - index++; + scan.SkipTrivia(ref index); } - if (index < source.Length && source[index] == '(') + if (index < source.Length && + source[index] == '(') { paren = index; return true; @@ -449,8 +531,8 @@ static bool TrySkipToParen(string source, int index, out int paren) } // Scans a balanced argument list starting at the open paren. - // Records top level comma positions. Skips strings, chars and comments. - static bool TryScanArguments(string source, int openParen, out int closeParen, out List topCommas) + // Records top level comma positions. Comments and literals are stepped over whole. + static bool TryScanArguments(string source, CsScan scan, int openParen, out int closeParen, out List topCommas) { closeParen = -1; topCommas = []; @@ -458,32 +540,14 @@ static bool TryScanArguments(string source, int openParen, out int closeParen, o var index = openParen + 1; while (index < source.Length) { - var ch = source[index]; - switch (ch) + if (scan.TryGetSkip(index, out var skipTo)) { - case '/': - if (!TrySkipComment(source, ref index)) - { - index++; - } - - continue; - case '\'': - if (!TrySkipCharLiteral(source, ref index)) - { - return false; - } - - continue; - case '"': - case '@': - case '$': - if (!TrySkipStringLike(source, ref index)) - { - index++; - } + index = skipTo; + continue; + } - continue; + switch (source[index]) + { case '(': case '[': case '{': @@ -527,267 +591,6 @@ static bool TryScanArguments(string source, int openParen, out int closeParen, o return false; } - static bool TrySkipComment(string source, ref int index) - { - if (index + 1 >= source.Length) - { - return false; - } - - var next = source[index + 1]; - if (next == '/') - { - var end = source.IndexOf('\n', index); - index = end < 0 ? source.Length : end + 1; - return true; - } - - if (next == '*') - { - var end = source.IndexOf("*/", index + 2, StringComparison.Ordinal); - index = end < 0 ? source.Length : end + 2; - return true; - } - - return false; - } - - static bool TrySkipCharLiteral(string source, ref int index) - { - // index at the opening quote - index++; - while (index < source.Length) - { - var ch = source[index]; - if (ch == '\\') - { - index += 2; - continue; - } - - if (ch == '\'') - { - index++; - return true; - } - - if (ch == '\n') - { - return false; - } - - index++; - } - - return false; - } - - // index at '$', '@' or '"'. Returns false when the characters do not start a string literal - // (eg '@identifier'); the caller then advances by one. - static bool TrySkipStringLike(string source, ref int index) - { - var cursor = index; - var dollars = 0; - var verbatim = false; - while (cursor < source.Length) - { - var ch = source[cursor]; - if (ch == '$') - { - dollars++; - cursor++; - continue; - } - - if (ch == '@') - { - verbatim = true; - cursor++; - continue; - } - - break; - } - - if (cursor >= source.Length || source[cursor] != '"') - { - return false; - } - - var quotes = QuoteRun(source, cursor); - if (quotes >= 3) - { - // Raw string (interpolated or not): skip blindly to a closing run of >= quotes. - // Interpolation holes are skipped as part of the content. - var search = cursor + quotes; - while (true) - { - if (search >= source.Length) - { - index = source.Length; - return true; - } - - if (source[search] != '"') - { - search++; - continue; - } - - var run = QuoteRun(source, search); - if (run >= quotes) - { - index = search + run; - return true; - } - - search += run; - } - } - - cursor += quotes == 2 ? 2 : 1; - if (quotes == 2 && dollars == 0) - { - // Empty string "" - index = cursor; - return true; - } - - if (quotes == 2) - { - // Interpolated empty string $"" - index = cursor; - return true; - } - - while (cursor < source.Length) - { - var ch = source[cursor]; - if (ch == '"') - { - if (verbatim && - cursor + 1 < source.Length && - source[cursor + 1] == '"') - { - cursor += 2; - continue; - } - - index = cursor + 1; - return true; - } - - if (!verbatim && ch == '\\') - { - cursor += 2; - continue; - } - - if (!verbatim && ch == '\n') - { - // Malformed: unterminated regular string. Stop at the line end. - index = cursor; - return true; - } - - if (dollars > 0 && ch == '{') - { - if (cursor + 1 < source.Length && source[cursor + 1] == '{') - { - cursor += 2; - continue; - } - - if (!TrySkipHole(source, ref cursor)) - { - index = source.Length; - return true; - } - - continue; - } - - if (dollars > 0 && ch == '}' && - cursor + 1 < source.Length && source[cursor + 1] == '}') - { - cursor += 2; - continue; - } - - cursor++; - } - - index = source.Length; - return true; - } - - // cursor at '{' of an interpolation hole; skips past the matching '}' - static bool TrySkipHole(string source, ref int cursor) - { - var depth = 1; - cursor++; - while (cursor < source.Length) - { - var ch = source[cursor]; - switch (ch) - { - case '/': - if (!TrySkipComment(source, ref cursor)) - { - cursor++; - } - - continue; - case '\'': - if (!TrySkipCharLiteral(source, ref cursor)) - { - return false; - } - - continue; - case '"': - case '@': - case '$': - if (!TrySkipStringLike(source, ref cursor)) - { - cursor++; - } - - continue; - case '{': - depth++; - cursor++; - continue; - case '}': - depth--; - cursor++; - if (depth == 0) - { - return true; - } - - continue; - default: - cursor++; - continue; - } - } - - return false; - } - - static int QuoteRun(string source, int index) - { - var count = 0; - while (index + count < source.Length && - source[index + count] == '"') - { - count++; - } - - return count; - } - static bool TryStripArgumentName(string source, ref int start, out string name) { name = ""; @@ -797,7 +600,7 @@ static bool TryStripArgumentName(string source, ref int start, out string name) return false; } - while (index < source.Length && IsIdentifierChar(source[index])) + while (index < source.Length && CsScan.IsIdentifierChar(source[index])) { index++; } @@ -826,16 +629,48 @@ static bool TryStripArgumentName(string source, ref int start, out string name) return true; } - static void TrimSpan(string source, ref int start, ref int end) + /// + /// Narrows a span to the expression in it: whitespace and comments are not part of the + /// argument, and leaving a comment in makes the argument read as something other than the + /// literal it is. + /// + static void TrimSpan(string source, CsScan scan, ref int start, ref int end) { - while (start < end && char.IsWhiteSpace(source[start])) + while (start < end) { - start++; + if (char.IsWhiteSpace(source[start])) + { + start++; + continue; + } + + if (source[start] == '/' && + scan.TryGetSkip(start, out var afterComment) && + afterComment <= end) + { + start = afterComment; + continue; + } + + break; } - while (end > start && char.IsWhiteSpace(source[end - 1])) + while (end > start) { - end--; + if (char.IsWhiteSpace(source[end - 1])) + { + end--; + continue; + } + + if (scan.TryGetCommentEndingAt(end, out var commentStart) && + commentStart >= start) + { + end = commentStart; + continue; + } + + break; } } @@ -920,47 +755,6 @@ static int LineOf(List lineStarts, int offset) return low + 1; } - static List FindAll(string source, string needle) - { - List result = []; - var index = 0; - while (true) - { - index = source.IndexOf(needle, index, StringComparison.Ordinal); - if (index < 0) - { - break; - } - - result.Add(index); - index++; - } - - return result; - } - - static int Nearest(List occurrences, List lineStarts, int lineHint) - { - var best = occurrences[0]; - var bestDistance = int.MaxValue; - var bestAfter = false; - foreach (var occurrence in occurrences) - { - var line = LineOf(lineStarts, occurrence); - var distance = Math.Abs(line - lineHint); - var after = line >= lineHint; - if (distance < bestDistance || - distance == bestDistance && after && !bestAfter) - { - best = occurrence; - bestDistance = distance; - bestAfter = after; - } - } - - return best; - } - static string IndentForSpan(string source, List lineStarts, int spanStart) { var line = LineOf(lineStarts, spanStart);