em 02: data-flow classification (params/returns/locals buckets)

- Tooling/DataFlowClassifier: buckets a resolved selection via Roslyn DataFlowAnalysis. Selection flow + tail flow (statements AFTER the selection in the same block) both use the two-argument AnalyzeDataFlow overload on the contiguous run; the em 01 spike verdict is pinned in tests — a synthetic BlockSyntax (parent decision #3) throws ArgumentException "statements not within tree" and would re-bind symbols, breaking the written-inside ∩ read-after identity match. - Buckets per parent spec as plain-string records: in-params = local + parameter reads (filtering the implicit `this`; fields/properties stay in the em 03 extract-first bucket), ByRef = reassigned inside via a non-declaration write (declaration initializers are not write-backs), returns = written ∧ read-after (tail, branch-insensitive over-approximation) + trailing `return X;` simple-name candidate, locals = written ∧ never read-after. - CLI now prints the raw bucket dump after the statement count line; classification failures exit 1 with a clean message (exit 0/1/2 contract preserved). - Tests: 6 new (in-param incl. `this` non-leak, multi-statement two-argument path, trailing-return, ref-vs-in differential, extract-first non-leak, synthetic-block dead path, single-statement one-argument path); shared DemoFixture loader; fixed pre-existing CS8602 in DemoFixtureTests. 36/36 green.
2026-09-12 15:52:16 +01:00
parent b012d0aaa0
commit 7f0981e012
5 changed files with 542 additions and 11 deletions
@@ -0,0 +1,185 @@
+using ExtractMethod.Tooling;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace BeforeAfter.Tests.ExtractMethod;
+
+/// <summary>
+/// Tests for yak em 02: the data-flow classifier (buckets: params / returns /
+/// locals) over the checked-in Demo.cs fixture. Line anchors are the exact
+/// fixture lines pinned by em 01 (see Demo.cs header comment):
+///   ScoreReads body 56..58,  Summarize body 68..76 (for-loop 69..72),
+///   Heaviest body 87..93.
+/// </summary>
+public class DataFlowClassificationTests
+{
+    // ---------------------------------------------------------------------
+    // in-param bucket (ScoreReads, full body 56..58).
+    // ReadInside = {_seed→field, seed, score, bonus}; the classifier must
+    // report the reads that are locals/params — seed/score/bonus — as in-
+    // params, and must NOT leak `this` (the implicit this-read behind the
+    // _seed field access; filtered via IParameterSymbol.IsThis) into params,
+    // nor the _seed field itself (decision #6 keeps fields for em 03).
+    // WrittenInside = {seed, score}; no tail, trailing return is an expression
+    // (score + bonus) which v1 cannot name -> no returns, both written vars
+    // are scratch locals.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void ScoreReads_full_body_reads_report_three_in_params_and_no_returns()
+    {
+        var suggestion = ResolveAndClassify(56, 58);
+
+        Assert.Equal(new[] { "bonus", "score", "seed" }, suggestion.Params.Select(p => p.Name));
+        // None is reassigned inside (declaration initializers are not write-backs) -> no ref.
+        Assert.All(suggestion.Params, p => Assert.False(p.ByRef));
+        Assert.Empty(suggestion.Returns);
+        // v1 reports the read locals ALSO as scratch locals (decision #6 over-
+        // reports local reads); em 03 dedupes them into the signature.
+        Assert.Equal(new[] { "score", "seed" }, suggestion.Locals);
+    }
+
+    // ---------------------------------------------------------------------
+    // multi-statement range case (Summarize 68..72: LocalDeclarationStatement
+    // + ForStatement): the resolution, the selection data flow AND the tail
+    // data flow all go through the two-argument overload with Succeeded=true
+    // (the synth-block trick from parent decision #3 is the DEAD path — see
+    // Test for it below). Buckets: total written inside + read at line 74 in
+    // the tail -> return; for-var i never read after -> local.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void Summarize_multi_statement_range_flows_via_two_argument_overload_and_buckets_total_return_i_local()
+    {
+        var (tree, compilation) = DemoFixture.Load();
+        var model = compilation.GetSemanticModel(tree);
+        var report = SelectionResolver.Resolve(tree, 68, 72);
+        Assert.True(report.Succeeded, report.Error);
+
+        var selectionFlow = DataFlowClassifier.AnalyzeSelectionFlow(model, report);
+        Assert.True(selectionFlow.Succeeded, "two-argument AnalyzeDataFlow must succeed on a multi-statement selection");
+
+        var tailFlow = DataFlowClassifier.AnalyzeTailFlow(model, report);
+        Assert.NotNull(tailFlow);
+        Assert.True(tailFlow.Succeeded, "two-argument AnalyzeDataFlow must succeed on the (multi-statement) tail region");
+
+        var suggestion = DataFlowClassifier.Classify(model, report);
+        Assert.Equal(new[] { "total" }, suggestion.Returns);
+        Assert.Equal(new[] { "i" }, suggestion.Locals);
+
+        // limit is read in the for-header (68..72) — a plain by-value in-param.
+        var limit = Assert.Single(suggestion.Params.Where(p => p.Name == "limit"));
+        Assert.False(limit.ByRef);
+    }
+
+    // ---------------------------------------------------------------------
+    // return + ref-param buckets (Summarize, full body 68..76).
+    // Returns: the tail is empty, so the tail-flow rule yields nothing; the
+    // selection ends with `return message;` -> message is the trailing-return
+    // candidate (parent spec). Locals: everything else written inside:
+    // {i, scaled, total}.
+    // Params (all ReadInside locals/params): the differential ref check —
+    // total is REASSIGNED inside (total += i at line 71) -> ByRef; scaled and
+    // limit are never reassigned (declaration write only) -> plain in-params.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void Summarize_full_body_returns_trailing_message_and_flags_reassigned_total_as_ref()
+    {
+        var suggestion = ResolveAndClassify(68, 76);
+
+        Assert.Equal(new[] { "message" }, suggestion.Returns);
+        Assert.Equal(new[] { "i", "scaled", "total" }, suggestion.Locals);
+
+        var total = Assert.Single(suggestion.Params.Where(p => p.Name == "total"));
+        Assert.True(total.ByRef, "total is reassigned inside (total += i) -> the extraction owes a write-back");
+
+        var scaled = Assert.Single(suggestion.Params.Where(p => p.Name == "scaled"));
+        Assert.False(scaled.ByRef, "scaled is only declared inside, never reassigned");
+
+        var limit = Assert.Single(suggestion.Params.Where(p => p.Name == "limit"));
+        Assert.False(limit.ByRef);
+    }
+
+    // ---------------------------------------------------------------------
+    // return + extract-first isolation (Heaviest, full body 87..93).
+    // Returns: `return best;` is the last statement -> best (trailing-return
+    // rule again). Written inside but never read after: i -> local.
+    // Params: reads are best/count/i/widgets; best and i are reassigned
+    // inside -> ref. Nothing extract-first (widgets[i] indexer, Bigger
+    // invocation — em 03 territory) may leak into any bucket: the params list
+    // is exactly the four variable reads.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void Heaviest_full_body_returns_best_locals_i_and_leaks_no_extract_first_names()
+    {
+        var suggestion = ResolveAndClassify(87, 93);
+
+        Assert.Equal(new[] { "best" }, suggestion.Returns);
+        Assert.Equal(new[] { "i" }, suggestion.Locals);
+        Assert.Equal(new[] { "best", "count", "i", "widgets" }, suggestion.Params.Select(p => p.Name));
+        Assert.Equal(new[] { true, false, true, false }, suggestion.Params.Select(p => p.ByRef));
+    }
+
+    // ---------------------------------------------------------------------
+    // single-statement fallback (ScoreReads, line 58 alone: `return ...;`).
+    // The one-argument AnalyzeDataFlow overload is the live path for a single
+    // statement. The trailing return is `score + bonus` — an expression, not
+    // a simple name — so v1 reports no return candidate (composite return
+    // expressions are extract-first shape for em 03) and no locals.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void Single_statement_selection_uses_one_argument_overload_and_composite_trailing_return_is_not_nameable()
+    {
+        var (tree, compilation) = DemoFixture.Load();
+        var model = compilation.GetSemanticModel(tree);
+        var report = SelectionResolver.Resolve(tree, 58, 58);
+        Assert.True(report.Succeeded, report.Error);
+
+        Assert.True(DataFlowClassifier.AnalyzeSelectionFlow(model, report).Succeeded);
+
+        var suggestion = DataFlowClassifier.Classify(model, report);
+        Assert.Equal(new[] { "bonus", "score" }, suggestion.Params.Select(p => p.Name));
+        Assert.All(suggestion.Params, p => Assert.False(p.ByRef));
+        Assert.Empty(suggestion.Returns);
+        Assert.Empty(suggestion.Locals);
+    }
+
+    // ---------------------------------------------------------------------
+    // DEAD PATH (parent decision #3, pinned so the choice is self-explanatory
+    // to future readers): a synthetic BlockSyntax re-parents tree nodes that
+    // are not in the tree, and AnalyzeDataFlow refuses with
+    // ArgumentException("statements not within tree"). That is why the live
+    // path uses the two-argument overload (both endpoints are real tree
+    // nodes) — which also keeps symbol identity across selection and tail
+    // analyses, the prerequisite for the written-inside ∩ read-after
+    // intersection of the return bucket.
+    // ---------------------------------------------------------------------
+    [Fact]
+    public void Synthetic_block_data_flow_throws_not_within_tree_so_the_two_argument_overload_stays_live()
+    {
+        var (tree, compilation) = DemoFixture.Load();
+        var model = compilation.GetSemanticModel(tree);
+
+        var summarize = tree.GetRoot().DescendantNodes()
+            .OfType<MethodDeclarationSyntax>()
+            .Single(m => m.Identifier.ValueText == "Summarize");
+        var statements = summarize.Body!.Statements;
+
+        // Same node instances, new (synthetic) parent — this is the shape
+        // parent decision #3 proposed for multi-statement ranges and tails.
+        var syntheticBlock = SyntaxFactory.Block(statements[2], statements[3], statements[4]);
+
+        var ex = Assert.Throws<ArgumentException>(() => model.AnalyzeDataFlow(syntheticBlock));
+        Assert.Contains("not within tree", ex.Message, StringComparison.Ordinal);
+    }
+
+    private static ExtractionSuggestion ResolveAndClassify(int startLine, int endLine)
+    {
+        var (tree, compilation) = DemoFixture.Load();
+        var model = compilation.GetSemanticModel(tree);
+
+        var report = SelectionResolver.Resolve(tree, startLine, endLine);
+        Assert.True(report.Succeeded, report.Error);
+
+        return DataFlowClassifier.Classify(model, report);
+    }
+}
@@ -0,0 +1,32 @@
+using ExtractMethod.Tooling;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+
+namespace BeforeAfter.Tests.ExtractMethod;
+
+/// <summary>
+/// Shared entry point for the ExtractMethod tests: the checked-in Demo.cs
+/// fixture is DATA (excluded from this project's compilation, copied to the
+/// output dir) and must ALWAYS be loaded through the tool's own
+/// <see cref="CompilationLoader"/> so tests exercise the exact parse +
+/// scratch-compilation path the CLI uses.
+/// </summary>
+internal static class DemoFixture
+{
+    /// <summary>Where the fixture lands after the csproj copies it (link Fixtures/).</summary>
+    public static string Path =>
+        System.IO.Path.Combine(AppContext.BaseDirectory, "Fixtures", "Demo.cs");
+
+    /// <summary>Parse + scratch-compile the fixture once per caller.</summary>
+    public static (SyntaxTree Tree, CSharpCompilation Compilation) Load()
+    {
+        if (!File.Exists(Path))
+        {
+            throw new FileNotFoundException($"fixture missing at {Path}");
+        }
+
+        var tree = CompilationLoader.ParseFile(Path);
+        var compilation = CompilationLoader.CreateCompilation(tree, "DemoFixture");
+        return (tree, compilation);
+    }
+}
@@ -13,16 +13,7 @@ namespace BeforeAfter.Tests.ExtractMethod;
 /// </summary>
 public class DemoFixtureTests
 {
-    private static readonly string FixturePath =
-        Path.Combine(AppContext.BaseDirectory, "Fixtures", "Demo.cs");
-
-    private static (SyntaxTree Tree, CSharpCompilation Compilation) LoadFixture()
-    {
-        Assert.True(File.Exists(FixturePath), $"fixture missing at {FixturePath}");
-        var tree = CompilationLoader.ParseFile(FixturePath);
-        var compilation = CompilationLoader.CreateCompilation(tree, "DemoFixture");
-        return (tree, compilation);
-    }
+    private static (SyntaxTree Tree, CSharpCompilation Compilation) LoadFixture() => DemoFixture.Load();

    // ---------------------------------------------------------------------
    // (a) The fixture is pristine under the scratch compilation: if this
@@ -77,6 +68,9 @@ public class DemoFixtureTests

        var dataFlow = compilation.GetSemanticModel(tree).AnalyzeDataFlow(forStatement);

+        // AnalyzeDataFlow is nullable-annotated in Roslyn 5.x: assert non-null
+        // so the dereference below is provably safe (and test intent explicit).
+        Assert.NotNull(dataFlow);
        Assert.True(dataFlow.Succeeded);
    }