Skip to content

Commit 69c51fa

Browse files
SimonCroppSimonCropp
committed
Treat preprocessor directives as trivia and bound the call search to the member
A directive's text is prose, and lexing it as code gave it two ways to derail a patch. `#region Test` declared an identifier that the member search took for the declaration of Test, which put the floor inside the body - and from there the member's own locals read as sibling declarations, so the recorded line was called stale and the one real call went unreachable. A title holding a stray quote or paren was worse: it opened a literal or unbalanced an argument scan that then ran on and hid whatever followed. Both languages skip a directive line whole now, from a `#` that starts its line to the end of it. Start of line only, since that is the one place C# allows a directive and F# writes a flexible type (`#seq<int>`) mid expression. The member bound only ever floored the search. The recorded line was checked against the next declaration, but the outward walk that runs when the hint misses had no ceiling and fanned out to the end of the file, so a member whose own call had changed or gone walked into the neighbour's test and rewrote it - reporting Applied. `NextMemberLine` replaces `MemberDeclaredBetween` and bounds the whole search to the member's span. A call past the next declaration is not a candidate, and a patch with nothing to match inside its own member says stale rather than taking someone else's. That also settles the skip the walk did on the hint: it was excluded as already tried, which was only true when the guard above had actually tried it. With the bound stated as a span, a hint inside the member is always tried and the walk has nothing to work around.
1 parent 2bf15e6 commit 69c51fa

6 files changed

Lines changed: 359 additions & 28 deletions

File tree

‎src/DiffEngine.Tests/InlinePatcherFsTests.cs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,4 +782,35 @@ public async Task TabIndentedFileUsesTabUnit()
782782
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
783783
await Assert.That(newSource).Contains("\tVerifier.Verify(15)\n\t\t.Snapshot(\"new\").ToTask()");
784784
}
785+
786+
/// <summary>
787+
/// F#'s directives - <c>#nowarn</c> above a binding, an <c>#if</c> block inside one - are
788+
/// trivia to the scan, exactly as C#'s are. A hash anywhere else stays code: it is how a
789+
/// flexible type (<c>#seq</c>) is written.
790+
/// </summary>
791+
[Test]
792+
public async Task DirectiveLinesAreTrivia()
793+
{
794+
var source = Source(
795+
"""
796+
module Tests
797+
798+
#nowarn "0044"
799+
800+
[<Fact>]
801+
let MyTest () =
802+
#if INTERACTIVE
803+
printfn "session"
804+
#endif
805+
Verifier.Verify(15).Snapshot("old").ToTask()
806+
""");
807+
808+
var status = TryApply(source, 10, InlinePatchMode.Set, null, "new", out var newSource, out _, originalValue: "old", memberName: "MyTest");
809+
810+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
811+
await Assert.That(newSource).Contains("Verify(15).Snapshot(\"new\").ToTask()");
812+
// The directives themselves are untouched
813+
await Assert.That(newSource).Contains("#nowarn \"0044\"");
814+
await Assert.That(newSource).Contains("#if INTERACTIVE");
815+
}
785816
}

‎src/DiffEngine.Tests/InlinePatcherTests.cs‎

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1769,4 +1769,250 @@ await Verify(value)
17691769
;
17701770
"""));
17711771
}
1772+
1773+
/// <summary>
1774+
/// A region named exactly after the test, which is what MarkdownSnippets leaves in every test
1775+
/// that doubles as a readme sample. The directive's text read as code, so the member search
1776+
/// took <c>#region Test</c> for the declaration of Test - and from that floor the body's own
1777+
/// locals looked like sibling members, which declared the hint stale and the one Verify call
1778+
/// unreachable.
1779+
/// </summary>
1780+
[Test]
1781+
public async Task AppendWithARegionNamedAfterTheMember()
1782+
{
1783+
var source = Source(
1784+
"""
1785+
class Tests
1786+
{
1787+
async Task Test()
1788+
{
1789+
#region Test
1790+
1791+
var value = Build();
1792+
1793+
#endregion
1794+
1795+
await Verify(value);
1796+
}
1797+
}
1798+
""");
1799+
1800+
var status = TryApply(source, 11, InlinePatchMode.Append, null, "new", out var newSource, out var reason, memberName: "Test");
1801+
1802+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1803+
await Assert.That(reason).IsEmpty();
1804+
await Assert.That(newSource).Contains(
1805+
" await Verify(value)\n" +
1806+
" .Snapshot(\"new\");");
1807+
}
1808+
1809+
/// <summary>
1810+
/// The same regions with a neighbour: displaced off its own member, the search walked into
1811+
/// the next test, found its Verify already carrying a Snapshot, and refused the append with a
1812+
/// message naming a line that plainly had none.
1813+
/// </summary>
1814+
[Test]
1815+
public async Task ARegionNamedAfterTheMemberDoesNotDisplaceAnAppend()
1816+
{
1817+
var source = Source(
1818+
"""
1819+
class Tests
1820+
{
1821+
async Task First()
1822+
{
1823+
#region First
1824+
1825+
var value = Build();
1826+
1827+
#endregion
1828+
1829+
await Verify(value);
1830+
}
1831+
1832+
async Task Second()
1833+
{
1834+
#region Second
1835+
1836+
var value = Build();
1837+
1838+
#endregion
1839+
1840+
await Verify(value)
1841+
.Snapshot("done");
1842+
}
1843+
}
1844+
""");
1845+
1846+
var status = TryApply(source, 11, InlinePatchMode.Append, null, "new", out var newSource, out _, memberName: "First");
1847+
1848+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1849+
await Assert.That(newSource).Contains(
1850+
" await Verify(value)\n" +
1851+
" .Snapshot(\"new\");");
1852+
// The neighbour keeps its own snapshot
1853+
await Assert.That(newSource).Contains(".Snapshot(\"done\");");
1854+
}
1855+
1856+
/// <summary>
1857+
/// And the Set shape of the same displacement: with the hint declared stale, the anchor was
1858+
/// matched against every call below the false floor and landed on the identical literal in
1859+
/// the next test, so accepting each sample rewrote the one after it.
1860+
/// </summary>
1861+
[Test]
1862+
public async Task ARegionNamedAfterTheMemberDoesNotDisplaceAnAnchoredSet()
1863+
{
1864+
var source = Source(
1865+
"""
1866+
class Tests
1867+
{
1868+
async Task First()
1869+
{
1870+
#region First
1871+
await Verify(value)
1872+
.Snapshot("one");
1873+
#endregion
1874+
}
1875+
1876+
async Task Second()
1877+
{
1878+
#region Second
1879+
await Verify(value)
1880+
.Snapshot("one");
1881+
#endregion
1882+
}
1883+
}
1884+
""");
1885+
1886+
var status = TryApply(source, 7, InlinePatchMode.Set, "\"one\"", "new", out var newSource, out _, memberName: "First");
1887+
1888+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1889+
var second = newSource.IndexOf("Second()", StringComparison.Ordinal);
1890+
await Assert.That(newSource.Substring(0, second)).Contains(".Snapshot(\"new\");");
1891+
await Assert.That(newSource.Substring(second)).Contains(".Snapshot(\"one\");");
1892+
}
1893+
1894+
/// <summary>
1895+
/// A directive whose title opens a verbatim string. Its text is prose, not code: lexed as
1896+
/// code, the <c>@"</c> opened a literal that never closes, which swallowed the rest of the
1897+
/// file and hid the one real call in it.
1898+
/// </summary>
1899+
[Test]
1900+
public async Task ADirectiveTitleWithAStrayQuoteDoesNotSwallowTheFile()
1901+
{
1902+
var source = Method(
1903+
"""
1904+
#region see @"C:\temp for context
1905+
await Snapshot("old");
1906+
""");
1907+
1908+
var status = TryApply(source, 6, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _);
1909+
1910+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1911+
await Assert.That(newSource).Contains("await Snapshot(\"new\");");
1912+
}
1913+
1914+
/// <summary>
1915+
/// A directive inside the argument list. It is trivia there like a comment is, so the
1916+
/// argument is the literal between the two rather than a span that starts at <c>#if</c> and
1917+
/// matches no anchor.
1918+
/// </summary>
1919+
[Test]
1920+
public async Task ADirectiveInsideTheArgumentListIsNotTheArgument()
1921+
{
1922+
var source = Method(
1923+
"""
1924+
await Snapshot(
1925+
#if DEBUG
1926+
"old"
1927+
#endif
1928+
);
1929+
""");
1930+
1931+
var status = TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _);
1932+
1933+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1934+
await Assert.That(newSource).Contains("\"new\"");
1935+
await Assert.That(newSource).DoesNotContain("\"old\"");
1936+
// The directives are around the argument, not part of it
1937+
await Assert.That(newSource).Contains("#if DEBUG");
1938+
await Assert.That(newSource).Contains("#endif");
1939+
}
1940+
1941+
/// <summary>
1942+
/// A call written twice under <c>#if</c>/<c>#else</c>. Both branches are source, only one was
1943+
/// compiled, and nothing at patch time can evaluate the condition - so the hint, which the
1944+
/// compiled branch stamped, is what says which twin the patch belongs to.
1945+
/// </summary>
1946+
[Test]
1947+
public async Task ConditionalTwinsPickTheCallAtTheHint()
1948+
{
1949+
var source = Source(string.Join(
1950+
"\n",
1951+
"class Tests",
1952+
"{",
1953+
" async Task Test()",
1954+
" {",
1955+
"#if NET48",
1956+
" await Verify(a);",
1957+
"#else",
1958+
" await Verify(b);",
1959+
"#endif",
1960+
" }",
1961+
"}"));
1962+
1963+
var status = TryApply(source, 8, InlinePatchMode.Append, null, "new", out var newSource, out _, memberName: "Test");
1964+
1965+
await Assert.That(status).IsEqualTo(PatchStatus.Applied);
1966+
await Assert.That(newSource).Contains(
1967+
" await Verify(b)\n" +
1968+
" .Snapshot(\"new\");");
1969+
await Assert.That(newSource).Contains("await Verify(a);\n");
1970+
}
1971+
1972+
/// <summary>
1973+
/// The member is a region with two ends. A member whose own call has gone used to send the
1974+
/// outward walk past the next declaration and into the neighbour's Verify, which appended a
1975+
/// snapshot to a test that never produced it.
1976+
/// </summary>
1977+
[Test]
1978+
public async Task AppendConfinedToTheMemberReportsRatherThanReachingTheNextTest()
1979+
{
1980+
var source = Source(
1981+
"""
1982+
class Tests
1983+
{
1984+
async Task First()
1985+
{
1986+
Prepare();
1987+
}
1988+
1989+
async Task Second()
1990+
{
1991+
await Verify(value);
1992+
}
1993+
}
1994+
""");
1995+
1996+
var status = TryApply(source, 5, InlinePatchMode.Append, null, "new", out _, out var reason, memberName: "First");
1997+
1998+
await Assert.That(status).IsEqualTo(PatchStatus.NotFound);
1999+
await Assert.That(reason).Contains("No Verify or Throws call");
2000+
}
2001+
2002+
/// <summary>
2003+
/// The anchored shape of the same overreach: First's literal changed since the run, the stale
2004+
/// anchor matches only the identical snapshot in Second, and near-miss hints landed the walk
2005+
/// on it. Stale is the honest answer; the neighbour's content is not evidence about First.
2006+
/// </summary>
2007+
[Test]
2008+
public async Task AnAnchorMatchingOnlyTheNextMemberIsStale()
2009+
{
2010+
var source = TwoCallSites("\"changed\"", "\"dup\"");
2011+
2012+
// Hint on B's declaration line, one above its snapshot; the patch came from A
2013+
var status = TryApply(source, 6, InlinePatchMode.Set, "\"dup\"", "new", out _, out var reason, memberName: "A");
2014+
2015+
await Assert.That(status).IsEqualTo(PatchStatus.NotFound);
2016+
await Assert.That(reason).Contains("Re-run the test.");
2017+
}
17722018
}

‎src/DiffEngine/Inline/CsLanguage.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ internal override SourceScan Scan(string source)
3838
continue;
3939
}
4040

41+
break;
42+
case '#':
43+
if (TrySkipDirective(source, ref index))
44+
{
45+
scan.AddSkip(start, index, comment: true);
46+
continue;
47+
}
48+
4149
break;
4250
case '\'':
4351
if (TrySkipCharLiteral(source, ref index))

‎src/DiffEngine/Inline/FsLanguage.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,14 @@ internal override SourceScan Scan(string source)
6969
continue;
7070
}
7171

72+
break;
73+
case '#':
74+
if (TrySkipDirective(source, ref index))
75+
{
76+
scan.AddSkip(start, index, comment: true);
77+
continue;
78+
}
79+
7280
break;
7381
case '\'':
7482
// Only where the tick cannot be part of the name in front of it, and only

0 commit comments

Comments
 (0)