diff --git a/tests/BeforeAfter.Tests/ExtractMethod/DataFlowClassificationTests.cs b/tests/BeforeAfter.Tests/ExtractMethod/DataFlowClassificationTests.cs index c3f14d4..3ea81ef 100644 --- a/tests/BeforeAfter.Tests/ExtractMethod/DataFlowClassificationTests.cs +++ b/tests/BeforeAfter.Tests/ExtractMethod/DataFlowClassificationTests.cs @@ -63,7 +63,8 @@ public class DataFlowClassificationTests 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[] { "total" }, suggestion.Returns.Select(r => r.Name)); + Assert.Equal(new[] { "int" }, suggestion.Returns.Select(r => r.Type)); Assert.Equal(new[] { "i" }, suggestion.Locals); // limit is read in the for-header (68..72) — a plain by-value in-param. @@ -86,7 +87,7 @@ public class DataFlowClassificationTests { var suggestion = ResolveAndClassify(68, 76); - Assert.Equal(new[] { "message" }, suggestion.Returns); + Assert.Equal(new[] { "message" }, suggestion.Returns.Select(r => r.Name)); Assert.Equal(new[] { "i", "scaled", "total" }, suggestion.Locals); var total = Assert.Single(suggestion.Params.Where(p => p.Name == "total")); @@ -113,7 +114,7 @@ public class DataFlowClassificationTests { var suggestion = ResolveAndClassify(87, 93); - Assert.Equal(new[] { "best" }, suggestion.Returns); + Assert.Equal(new[] { "best" }, suggestion.Returns.Select(r => r.Name)); 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)); diff --git a/tests/BeforeAfter.Tests/ExtractMethod/ExtractFirstTests.cs b/tests/BeforeAfter.Tests/ExtractMethod/ExtractFirstTests.cs new file mode 100644 index 0000000..d6a0c5a --- /dev/null +++ b/tests/BeforeAfter.Tests/ExtractMethod/ExtractFirstTests.cs @@ -0,0 +1,143 @@ +using ExtractMethod.Tooling; +using Microsoft.CodeAnalysis; + +namespace BeforeAfter.Tests.ExtractMethod; + +/// +/// Tests for yak em 03: the extract-first scan over the checked-in Demo.cs +/// fixture. Line anchors are the exact fixture lines pinned by em 01/02: +/// ScoreReads body 56..58, Summarize body 68..76, Heaviest body 87..93, +/// RepeatReads body 105..108, Casts body 116 (the em 03 additions at the +/// end of the fixture; nothing before them may shift — see Demo.cs header). +/// +/// Extract-first = read expressions that are NOT simple local/parameter +/// references (parent bucket spec): member/field/property access, element +/// access, invocations, casts — deduped by symbol + text, occurrence- +/// counted, invocations flagged ("hoisting changes eval count"), own-class +/// field/property access marked optional. +/// +public class ExtractFirstTests +{ + // --------------------------------------------------------------------- + // optional marking (own-class FIELD): ScoreReads reads `_seed` once; + // `seed`, `score`, `bonus` are simple local/param names and must NOT + // appear (decision #6 puts them in the params bucket instead). + // --------------------------------------------------------------------- + [Fact] + public void ScoreReads_reports_own_class_field_once_and_optional() + { + var report = Compose(56, 58); + + var entry = Assert.Single(report.ExtractFirst); + Assert.Equal("_seed", entry.Text); + Assert.Equal("Demo._seed", entry.SymbolDisplay); + Assert.Equal(1, entry.Occurrences); + Assert.False(entry.IsInvocation); + Assert.True(entry.Optional, "own-class field reads are optional to hoist"); + Assert.Equal("field", entry.OwnMemberKind); + } + + // --------------------------------------------------------------------- + // optional marking (own-class PROPERTY): Summarize reads `Scale` once + // (line 74). The binary expressions around it (`total * Scale`, + // `"sum=" + scaled`) are composites of locals/params and literals — + // exactly the shapes the parent spec does NOT list, so they stay out. + // --------------------------------------------------------------------- + [Fact] + public void Summarize_reports_own_class_property_once_and_optional() + { + var report = Compose(68, 76); + + var entry = Assert.Single(report.ExtractFirst); + Assert.Equal("Scale", entry.Text); + Assert.Equal("Demo.Scale", entry.SymbolDisplay); + Assert.Equal(1, entry.Occurrences); + Assert.False(entry.IsInvocation); + Assert.True(entry.Optional, "own-class property reads are optional to hoist"); + Assert.Equal("property", entry.OwnMemberKind); + } + + // --------------------------------------------------------------------- + // invocation flag + element access (Heaviest, full body 87..93): + // `widgets[0]` and `widgets[i]` are two DIFFERENT indexer reads (same + // indexer symbol, different text -> separate entries); the call + // `best.Bigger(widgets[i])` is flagged because hoisting it changes how + // often it is evaluated. The callee `best.Bigger` must NOT be reported + // separately (the invocation covers it), and `best`/`i`/`widgets`/ + // `count` are simple local/param names — invisible to this bucket. + // None of Heaviest's candidates is own-class (Widget is a nested record, + // the indexer belongs to List) -> nothing optional here. + // --------------------------------------------------------------------- + [Fact] + public void Heaviest_flags_the_invocation_and_lists_both_indexer_reads() + { + var report = Compose(87, 93); + + Assert.Equal( + new[] { "widgets[0]", "best.Bigger(widgets[i])", "widgets[i]" }, + report.ExtractFirst.Select(e => e.Text)); + Assert.All(report.ExtractFirst, e => Assert.Equal(1, e.Occurrences)); + Assert.All(report.ExtractFirst, e => Assert.False(e.Optional)); // none of Heaviest's reads is own-class + + var invocation = Assert.Single(report.ExtractFirst, e => e.IsInvocation); + Assert.Equal("best.Bigger(widgets[i])", invocation.Text); + } + + // --------------------------------------------------------------------- + // dedupe + occurrence count (RepeatReads, body 105..108): `_seed` is + // read TWICE (lines 105, 106) and must dedupe to ONE entry with + // Occurrences == 2. The field WRITE `_seed = a;` (line 107) is a + // non-variable assignment LHS — the reads-only scan skips it, so it + // neither adds an occurrence nor changes the count (decision 5), and + // the report must carry the decision-5 limitation note. + // --------------------------------------------------------------------- + [Fact] + public void RepeatReads_dedupes_the_field_read_to_count_two_and_skips_the_field_write() + { + var report = Compose(105, 108); + + var entry = Assert.Single(report.ExtractFirst); + Assert.Equal("_seed", entry.Text); + Assert.Equal(2, entry.Occurrences); + Assert.True(entry.Optional); + Assert.Equal("field", entry.OwnMemberKind); + + // Two notes apply to this selection: the decision-5 write limitation + // AND the composite trailing return (`return a + b;`). + Assert.Equal(2, report.Notes.Count); + Assert.Contains(report.Notes, n => n.Contains("assignment left-hand sides", StringComparison.Ordinal)); + Assert.Contains(report.Notes, n => n.Contains("composite return expression (a + b)", StringComparison.Ordinal)); + } + + // --------------------------------------------------------------------- + // cast (Casts, body 116): `(double)total` is an extract-first candidate + // (parent spec lists casts). A conversion is not a symbol, so the entry + // keys on its text alone ("unbound" display) and carries no flags. + // --------------------------------------------------------------------- + [Fact] + public void Casts_reports_the_cast_expression_and_the_field_read() + { + var report = Compose(116, 116); + + Assert.Equal( + new[] { "(double)total", "_seed" }, + report.ExtractFirst.Select(e => e.Text)); + + var cast = report.ExtractFirst[0]; + Assert.Equal(1, cast.Occurrences); + Assert.False(cast.IsInvocation); + Assert.False(cast.Optional); + Assert.Equal("", cast.SymbolDisplay); + } + + private static ExtractionReport Compose(int startLine, int endLine) + { + var (tree, compilation) = DemoFixture.Load(); + var model = compilation.GetSemanticModel(tree); + + var resolved = SelectionResolver.Resolve(tree, startLine, endLine); + Assert.True(resolved.Succeeded, resolved.Error); + + return ExtractionReporter.Compose(model, resolved); + } +} diff --git a/tests/BeforeAfter.Tests/ExtractMethod/Fixtures/Demo.cs b/tests/BeforeAfter.Tests/ExtractMethod/Fixtures/Demo.cs index 5d2711b..df5fa1a 100644 --- a/tests/BeforeAfter.Tests/ExtractMethod/Fixtures/Demo.cs +++ b/tests/BeforeAfter.Tests/ExtractMethod/Fixtures/Demo.cs @@ -92,4 +92,27 @@ public class Demo return best; } + + /// + /// Bucket (em 03): the field read _seed appears TWICE — the + /// extract-first scan must dedupe it to one entry with an occurrence + /// count of 2. The field WRITE below (_seed = a;) is a + /// non-variable assignment left-hand side: the reads-only scan skips it + /// (decision 5) and the report notes the limitation. + /// + public int RepeatReads(int n) + { + int a = _seed + n; + int b = _seed * n; + _seed = a; + return a + b; + } + + /// Bucket (em 03): a cast is an extract-first candidate too + /// (parent spec lists casts); its bound symbol is null — conversions are + /// not symbols — so the entry keys on its text alone. + public double Casts(int total) + { + return (double)total / _seed; + } } \ No newline at end of file diff --git a/tests/BeforeAfter.Tests/ExtractMethod/ReportTests.cs b/tests/BeforeAfter.Tests/ExtractMethod/ReportTests.cs new file mode 100644 index 0000000..385775a --- /dev/null +++ b/tests/BeforeAfter.Tests/ExtractMethod/ReportTests.cs @@ -0,0 +1,106 @@ +using ExtractMethod.Tooling; +using Microsoft.CodeAnalysis; + +namespace BeforeAfter.Tests.ExtractMethod; + +/// +/// Tests for yak em 03: the signature promotion (SignatureBuilder) and the +/// formatted report (ReportFormatter) over the checked-in Demo.cs fixture. +/// Line anchors are the exact fixture lines pinned by em 01/02: ScoreReads +/// body 56..58, Summarize 68..76 (partial selection 68..72), Heaviest 87..93. +/// +public class ReportTests +{ + // --------------------------------------------------------------------- + // Signature promotion, the case em 02's comment promises: in Summarize + // lines 68..72, `total` sits in params[ref] AND returns (declared inside, + // reassigned inside, read after), and `i` sits in params AND locals (the + // for-header declares it inside). Promotion: both drop out of the params, + // total becomes the plain return value -> `int Extract(int limit)`. + // --------------------------------------------------------------------- + [Fact] + public void Signature_for_Summarize_selection_promotes_declared_inside_variables() + { + var (report, formatted) = Compose(68, 72); + + Assert.Equal(new[] { "limit" }, report.Signature.Params.Select(p => p.Name)); + Assert.Equal("int", report.Signature.ReturnType); + Assert.Contains("suggested signature: int Extract(int limit)", formatted); + } + + // --------------------------------------------------------------------- + // Heaviest full body: `best` (declared inside, ref in the raw params, + // trailing return candidate) promotes to the return; the for-var `i` + // drops from the params; widgets/count stay as plain in-params. + // --------------------------------------------------------------------- + [Fact] + public void Signature_for_Heaviest_returns_the_trailing_candidate_and_drops_declared_inside_params() + { + var (_, formatted) = Compose(87, 93); + + // Parameter order follows the classifier's ordinal-by-name order — + // the same order the params bucket above prints (em 02 determinism). + Assert.Contains("suggested signature: Widget Extract(int count, List widgets)", formatted); + } + + // --------------------------------------------------------------------- + // No nameable return -> void (ScoreReads ends with `return score + bonus;`, + // a composite expression v1 cannot name — the report notes this); locals + // declared inside (seed, score) drop from the params; only the enclosing + // method's own parameter survives. + // --------------------------------------------------------------------- + [Fact] + public void Signature_for_ScoreReads_is_void_and_carries_only_the_enclosing_parameter() + { + var (report, formatted) = Compose(56, 58); + + Assert.Equal(new[] { "bonus" }, report.Signature.Params.Select(p => p.Name)); + Assert.Equal("void", report.Signature.ReturnType); + Assert.Contains("suggested signature: void Extract(int bonus)", formatted); + + // The void-ness has an explanation in the report: the composite + // trailing return is not nameable in v1. + Assert.Contains(report.Notes, n => n.Contains("composite return expression (score + bonus)", StringComparison.Ordinal)); + } + + // --------------------------------------------------------------------- + // The report the CLI prints, pinned EXACTLY for one fixture selection + // (acceptance: "CLI report on the fixture matches the tests"). Heaviest + // full body is the richest case: raw buckets (params include the + // declared-inside best/i — the promotion happens only in the signature), + // extract-first with the invocation flag, no notes, signature last. + // --------------------------------------------------------------------- + [Fact] + public void Formatted_report_for_Heaviest_matches_the_CLI_output_exactly() + { + var (_, formatted) = Compose(87, 93); + + var expected = + "3 statement(s) selected, lines 87..93 in Heaviest(): LocalDeclarationStatement, ForStatement, ReturnStatement\n" + + "params: best (Widget) [ref]\n" + + "params: count (int) [in]\n" + + "params: i (int) [ref]\n" + + "params: widgets (List) [in]\n" + + "returns: best (Widget)\n" + + "locals: i\n" + + "extract-first:\n" + + " - widgets[0] — List.this[int] ×1\n" + + " - best.Bigger(widgets[i]) — Widget.Bigger(Widget) ×1 [hoisting changes eval count]\n" + + " - widgets[i] — List.this[int] ×1\n" + + "suggested signature: Widget Extract(int count, List widgets)\n"; + + Assert.Equal(expected, formatted); + } + + private static (ExtractionReport Report, string Formatted) Compose(int startLine, int endLine) + { + var (tree, compilation) = DemoFixture.Load(); + var model = compilation.GetSemanticModel(tree); + + var resolved = SelectionResolver.Resolve(tree, startLine, endLine); + Assert.True(resolved.Succeeded, resolved.Error); + + var report = ExtractionReporter.Compose(model, resolved); + return (report, ReportFormatter.Format(report)); + } +} diff --git a/tools/ExtractMethod/Program.cs b/tools/ExtractMethod/Program.cs index 76f0b09..4a87268 100644 --- a/tools/ExtractMethod/Program.cs +++ b/tools/ExtractMethod/Program.cs @@ -38,25 +38,22 @@ if (compilation.GetDiagnostics().Any(d => d.Severity == DiagnosticSeverity.Error } // 3. snap the range to whole statements, report cleanly otherwise -var report = SelectionResolver.Resolve(tree, startLine, endLine); -if (!report.Succeeded) +var resolved = SelectionResolver.Resolve(tree, startLine, endLine); +if (!resolved.Succeeded) { - Console.Error.WriteLine($"error: {report.Error}"); + Console.Error.WriteLine($"error: {resolved.Error}"); return SelectionResolver.ExitError; } -Console.WriteLine( - $"{report.Count} statement(s) selected, lines {report.StartLine}..{report.EndLine} " + - $"in {report.Method?.Identifier.ValueText}(): " + - string.Join(", ", report.Kinds)); - -// 4. classify the data flow into the raw buckets (em 02: no pretty report yet) -// A classification failure is a semantic resolution error: same exit code as -// the resolver, message on stderr, but never a stack trace. -ExtractionSuggestion suggestion; +// 4. compose the full suggestion (em 02 buckets + em 03 extract-first and +// signature) and print the report. A composition failure is a semantic +// resolution error: same exit code as the resolver, message on stderr, +// but never a stack trace. +var model = compilation.GetSemanticModel(tree); try { - suggestion = DataFlowClassifier.Classify(compilation.GetSemanticModel(tree), report); + var extraction = ExtractionReporter.Compose(model, resolved); + Console.Write(ReportFormatter.Format(extraction)); } catch (Exception e) when (e is InvalidOperationException or ArgumentException) { @@ -64,31 +61,4 @@ catch (Exception e) when (e is InvalidOperationException or ArgumentException) return SelectionResolver.ExitError; } -foreach (var param in suggestion.Params) -{ - Console.WriteLine($"params: {param.Name} ({param.Type}){(param.ByRef ? " [ref]" : " [in]")}"); -} -if (suggestion.Params.Count == 0) -{ - Console.WriteLine("params: (none)"); -} - -foreach (var name in suggestion.Returns) -{ - Console.WriteLine($"returns: {name}"); -} -if (suggestion.Returns.Count == 0) -{ - Console.WriteLine("returns: (none)"); -} - -foreach (var name in suggestion.Locals) -{ - Console.WriteLine($"locals: {name}"); -} -if (suggestion.Locals.Count == 0) -{ - Console.WriteLine("locals: (none)"); -} - return 0; \ No newline at end of file diff --git a/tools/ExtractMethod/Tooling/DataFlowClassifier.cs b/tools/ExtractMethod/Tooling/DataFlowClassifier.cs index c9a891c..d466019 100644 --- a/tools/ExtractMethod/Tooling/DataFlowClassifier.cs +++ b/tools/ExtractMethod/Tooling/DataFlowClassifier.cs @@ -21,19 +21,27 @@ namespace ExtractMethod.Tooling; /// the precise v1 rule. public sealed record ParamSuggestion(string Name, string Type, bool ByRef); +/// One return candidate of the extracted method (v1: plain strings). +/// Variable name as it appears in the source. +/// Type as a display string — the signature line of em 03 +/// prints it as the extracted method's return type. +public sealed record ReturnSuggestion(string Name, string Type); + /// The classified buckets of one selection (v1: symbol names as plain strings). /// Variables the selection READS and the new method therefore /// receives: read locals + the enclosing method's parameters (parent decision #6). /// A variable may also appear in and/or — -/// v1 reports per-bucket and em 03 dedupes into a coherent signature. +/// v1 reports per-bucket; em 03's signature builder dedupes into a coherent +/// signature (a variable DECLARED inside the selection cannot be a parameter). /// Variables written inside AND read after the selection /// (tail data flow), plus the value of a trailing return X; when X is a -/// simple name — they must flow out of the extraction. +/// simple name — they must flow out of the extraction. Typed since em 03: the +/// signature line needs the return type, not just the name. /// Variables written inside and never read after: scratch /// locals of the new method. public sealed record ExtractionSuggestion( IReadOnlyList Params, - IReadOnlyList Returns, + IReadOnlyList Returns, IReadOnlyList Locals); /// @@ -146,20 +154,28 @@ public static class DataFlowClassifier // ReadInside is ImmutableArray (not a set): fine for Contains lookups. var readAfter = AnalyzeTailFlow(model, selection)?.ReadInside ?? ImmutableArray.Empty; - var trailingReturn = TrailingReturnName(selection); - // ---- returns: written inside ∧ read after, plus trailing return X ---- - var returnNames = writtenInside + // Typed since em 03 (the signature line needs the return type). The + // symbols come straight from the flow sets, so types are exact; the + // trailing-return candidate resolves its own identifier below. + var returnSymbols = writtenInside .Where(v => readAfter.Contains(v)) - .Select(v => v.Name) - .Concat(trailingReturn is { } name ? new[] { name } : Array.Empty()) - .Distinct(StringComparer.Ordinal) - .OrderBy(n => n, StringComparer.Ordinal) + .ToDictionary(v => v.Name, VariableTypeString, StringComparer.Ordinal); + + var trailingReturn = TrailingReturnSuggestion(model, selection); + if (trailingReturn is { } candidate) + { + returnSymbols.TryAdd(candidate.Name, candidate.Type); + } + + var returnSuggestions = returnSymbols + .Select(kv => new ReturnSuggestion(kv.Key, kv.Value)) + .OrderBy(r => r.Name, StringComparer.Ordinal) .ToList(); // ---- locals: written inside, never read after -> scratch locals ---- var localNames = writtenInside - .Where(v => !returnNames.Contains(v.Name, StringComparer.Ordinal)) + .Where(v => !returnSymbols.ContainsKey(v.Name)) .Select(v => v.Name) .OrderBy(n => n, StringComparer.Ordinal) .ToList(); @@ -171,22 +187,22 @@ public static class DataFlowClassifier .OrderBy(v => v.Name, StringComparer.Ordinal) .Select(v => new ParamSuggestion( v.Name, - ParamTypeString(v), + VariableTypeString(v), IsReassignedInside(model, v, selection.Statements))) .ToList(); - return new ExtractionSuggestion(paramSuggestions, returnNames, localNames); + return new ExtractionSuggestion(paramSuggestions, returnSuggestions, localNames); } /// - /// Display string of the variable's type. ISymbol has no Type member; - /// only locals and parameters carry one (the params bucket is restricted - /// to exactly those kinds, hence the match). + /// Display string of a variable's type. ISymbol has no Type member; only + /// locals and parameters carry one. Used for the params bucket AND (since + /// em 03) the returns bucket — the signature line prints it. /// - private static string ParamTypeString(ISymbol variable) => variable switch + private static string VariableTypeString(ISymbol variable) => variable switch { - ILocalSymbol local => local.Type?.ToDisplayString() ?? "unknown", - IParameterSymbol parameter => parameter.Type?.ToDisplayString() ?? "unknown", + ILocalSymbol local => local.Type?.ToDisplayString(Displays.TypeFormat) ?? "unknown", + IParameterSymbol parameter => parameter.Type?.ToDisplayString(Displays.TypeFormat) ?? "unknown", _ => "unknown", }; @@ -264,14 +280,37 @@ public static class DataFlowClassifier /// /// Parent spec: "if the selection ends with `return X;`, X is the candidate." - /// Returns the simple-name candidate, or null when the returned expression - /// is not a simple name — an expression like `score + bonus` is not nameable - /// as a v1 string (composite return expressions are extract-first shape for - /// em 03). Empty return ('return;') has no candidate. + /// Returns the simple-name candidate with its type, or null when the + /// returned expression is not a simple name — an expression like + /// `score + bonus` is not nameable as a v1 string (composite return + /// expressions are extract-first shape for em 03: the report suggests + /// extracting it into a local first). Empty return ('return;') has no + /// candidate. /// - private static string? TrailingReturnName(SelectionReport selection) + private static ReturnSuggestion? TrailingReturnSuggestion(SemanticModel model, SelectionReport selection) { var last = selection.Statements[^1]; - return last is ReturnStatementSyntax { Expression: IdentifierNameSyntax name } ? name.Identifier.ValueText : null; + if (last is not ReturnStatementSyntax { Expression: IdentifierNameSyntax name }) + { + return null; + } + + var type = model.GetSymbolInfo(name).Symbol switch + { + ILocalSymbol local => local.Type?.ToDisplayString(Displays.TypeFormat) ?? "unknown", + IParameterSymbol parameter => parameter.Type?.ToDisplayString(Displays.TypeFormat) ?? "unknown", + _ => "unknown", + }; + return new ReturnSuggestion(name.Identifier.ValueText, type); } + + /// + /// The trailing return statement when its expression is NOT a simple + /// name (and not absent) — the composite-return shape the em 03 report + /// flags as "extract a local first" (see ). + /// + public static ReturnStatementSyntax? CompositeTrailingReturn(SelectionReport selection) + => selection.Statements[^1] is ReturnStatementSyntax { Expression: not null and not IdentifierNameSyntax } ret + ? ret + : null; } \ No newline at end of file diff --git a/tools/ExtractMethod/Tooling/Displays.cs b/tools/ExtractMethod/Tooling/Displays.cs new file mode 100644 index 0000000..c73912b --- /dev/null +++ b/tools/ExtractMethod/Tooling/Displays.cs @@ -0,0 +1,53 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; + +namespace ExtractMethod.Tooling; + +/// +/// Compact symbol display for the suggestion report. The defaults are wrong +/// for a report a human reads next to their own source: +/// - ToDisplayString() fully qualifies every namespace +/// (System.Collections.Generic.List<Widget>), +/// - ToMinimalDisplayString() keeps containing types AND prefixes +/// members with their own type (int Demo._seed). +/// The report wants the shortest form that stays readable in context: +/// types as bare names (Widget, List<Widget>), members as +/// ContainingType.Member with parameter types. +/// +internal static class Displays +{ + /// + /// Type display for suggestion lines: shortest unambiguous-enough form + /// (no namespaces, no containing types, keyword spellings for special + /// types, nullable annotations kept). Built explicitly because + /// MinimallyQualifiedFormat keeps containing types for nested types + /// (Demo.Widget) and has no With-style override here. + /// + internal static readonly SymbolDisplayFormat TypeFormat = new( + globalNamespaceStyle: SymbolDisplayGlobalNamespaceStyle.Omitted, + typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameOnly, + genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters + | SymbolDisplayGenericsOptions.IncludeVariance, + miscellaneousOptions: SymbolDisplayMiscellaneousOptions.UseSpecialTypes + | SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers + | SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier); + + /// Compact member display for extract-first entries: + /// Demo._seed, Demo.Scale, Widget.Bigger(Widget), + /// List<Widget>.this[int]. + internal static string Member(ISymbol symbol) => symbol switch + { + IMethodSymbol method => + $"{Type(method.ContainingType)}.{method.Name}({Parameters(method.Parameters)})", + IPropertySymbol { IsIndexer: true } indexer => + $"{Type(indexer.ContainingType)}.this[{Parameters(indexer.Parameters)}]", + IFieldSymbol or IPropertySymbol or IEventSymbol => + $"{Type(symbol.ContainingType)}.{symbol.Name}", + _ => symbol.ToDisplayString(TypeFormat), + }; + + private static string Parameters(ImmutableArray parameters) + => string.Join(", ", parameters.Select(p => Type(p.Type))); + + private static string Type(ITypeSymbol? type) => type?.ToDisplayString(TypeFormat) ?? "?"; +} diff --git a/tools/ExtractMethod/Tooling/ExtractFirstScanner.cs b/tools/ExtractMethod/Tooling/ExtractFirstScanner.cs new file mode 100644 index 0000000..d95c177 --- /dev/null +++ b/tools/ExtractMethod/Tooling/ExtractFirstScanner.cs @@ -0,0 +1,316 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace ExtractMethod.Tooling; + +/// +/// One deduplicated "extract-first" candidate (parent bucket spec): a read +/// expression that is NOT a simple local/parameter reference, so the classic +/// refactoring sequence is to Extract-Variable it FIRST and re-run the tool — +/// the extraction then receives a clean variable instead of a nested +/// expression. plus identify +/// the candidate; is how often the same candidate +/// (same symbol + same source text) is read inside the selection. +/// +/// Source text of the expression (trivia-free). +/// The bound symbol, minimally qualified for the +/// report, or <unbound> when the expression resolves to no +/// symbol (e.g. a cast — conversions are not symbols). +/// How often this exact candidate is read. +/// True for method invocations: hoisting one into +/// a variable changes HOW OFTEN it is evaluated (pure calls are safe to +/// hoist, effectful ones are a behavior change — v1 cannot tell them apart, +/// so every invocation is flagged). +/// field or property when the +/// accessed member belongs to the class that owns the selection — hoisting +/// such an access is OPTIONAL because a same-class extracted method can still +/// see it (it is still listed: extraction often precedes moving the method to +/// another class, where the field would need to travel as a parameter). +public sealed record ExtractFirstEntry( + string Text, + string SymbolDisplay, + int Occurrences, + bool IsInvocation, + string? OwnMemberKind) +{ + /// Own-class field/property reads are optional to hoist (see ). + public bool Optional => OwnMemberKind is not null; +} + +/// +/// The extract-first scan (yak em 03, parent bucket spec): find the READ +/// expressions of the selection that are not simple local/parameter +/// references — member/property/field access, element access, invocations, +/// casts — dedupe them by semantic key (bound symbol + source text), count +/// occurrences, flag invocations, and mark own-class field/property access +/// as optional. +/// +/// WHY a hand-rolled recursive walk instead of one DescendantNodes query: +/// "which expression positions are READS" is exactly what data-flow analysis +/// answers for VARIABLES (em 02's ReadInside), but for nested expressions +/// the read/write distinction must come from the SYNTAX shape — and the scan +/// needs three shape rules a flat query cannot express: +/// 1. assignment LEFT sides are writes, skipped entirely (decision #5), +/// 2. the direct callee of an invocation (and the container of an element +/// access) is covered by that invocation/access — reporting `best.Bigger` +/// AND `best.Bigger(...)` would double-count one call, +/// 3. a member access's NAME is part of the access node, not an +/// independent expression — visiting it as an IdentifierName would +/// report the same access twice (e.g. `a.B.C` as both the member +/// access and a bare `C`). +/// +/// KNOWN v1 APPROXIMATIONS (documented, accepted): +/// - Reads-only: non-variable assignment left-hand sides (`arr[i] = 5`) are +/// skipped whole — including reads NESTED in them (`arr` in +/// `arr[i].X = 5`). Surfaced as a report note (see +/// ). +/// - Compound-assignment LHS also reads its old value (`obj.Count += 1`); +/// v1 skips it with the rest of the LHS. +/// - Chained accesses report each link (`a.B.C` lists `a.B` and `a.B.C`) +/// except the direct callee of a call, which the call covers. +/// - Null-conditional chains are approximated: `a?.B` reports the member +/// binding (`.B`) with its symbol; the receiver is not re-reported. +/// - The scan classifies expression NODES; a statement's descendant TYPE +/// references (`Widget best = ...`) are reached by the generic descent but +/// filtered out because they resolve to types, not values. +/// +public static class ExtractFirstScanner +{ + /// + /// Scans the selection and returns the deduplicated extract-first + /// candidates in first-occurrence (source) order. + /// + public static IReadOnlyList Scan(SemanticModel model, SelectionReport selection) + { + // The resolver guarantees Method is non-null on a successful report; + // its containing type is what "own-class" is measured against. + var enclosingType = (model.GetDeclaredSymbol(selection.Method!) as IMethodSymbol)?.ContainingType; + + var entries = new List(); + var indexByKey = new Dictionary(StringComparer.Ordinal); + + // Records one candidate occurrence. The semantic key is the bound + // symbol PLUS the source text: the same member read through different + // expressions (`widgets[0]` vs `widgets[i]`) stays separate, while an + // identical expression read twice (`_seed` twice in RepeatReads) + // dedupes into one entry with Occurrences > 1. + void Report(ExpressionSyntax expr, bool invocation) + { + var info = model.GetSymbolInfo(expr); + // Prefer the resolved symbol; on an ambiguous binding the first + // candidate still identifies the member better than nothing. + var symbol = info.Symbol ?? info.CandidateSymbols.FirstOrDefault(); + + var text = expr.ToString(); // trivia-free expression text + var key = $"{symbol?.ToDisplayString() ?? ""}|{text}"; + var display = symbol is null ? "" : Displays.Member(symbol); + var ownKind = OwnMemberKind(symbol, enclosingType); + + if (indexByKey.TryGetValue(key, out var index)) + { + entries[index] = entries[index] with { Occurrences = entries[index].Occurrences + 1 }; + } + else + { + indexByKey[key] = entries.Count; + entries.Add(new ExtractFirstEntry(text, display, 1, invocation, ownKind)); + } + } + + // Own-class data member → hoisting is optional (parent bucket spec). + // Exact containing-type match: base-class members would need a base + // reference in the extracted method — not v1's "optional" case. + static string? OwnMemberKind(ISymbol? symbol, INamedTypeSymbol? enclosingType) + { + if (enclosingType is null) + { + return null; + } + + return symbol switch + { + IFieldSymbol field when SymbolEqualityComparer.Default.Equals(field.ContainingType, enclosingType) + => "field", + IPropertySymbol property when SymbolEqualityComparer.Default.Equals(property.ContainingType, enclosingType) + => "property", + _ => null, + }; + } + + // Classifies a bare simple name. Simple names ARE reported when they + // bind to anything that is not a variable or a type/namespace — that + // is how the field read `_seed` and the property read `Scale` (both + // written as bare identifiers) enter this bucket (decision #6 keeps + // them out of the params bucket). + void ClassifyIdentifier(IdentifierNameSyntax id) + { + if (id.IsVar) + { + return; // the contextual `var` keyword is not a value read + } + + var symbol = model.GetSymbolInfo(id).Symbol; + switch (symbol) + { + case null: + return; // unresolvable: nothing sensible to report + case ILocalSymbol or IParameterSymbol: + return; // the params bucket (decision #6), not extract-first + case INamespaceSymbol or ITypeSymbol: + return; // a type/namespace mention (e.g. `Widget`), not a value read + case IDiscardSymbol: + return; // discards are write sinks, never reads + default: + Report(id, invocation: false); + break; + } + } + + /// nameof is spelled like a call but reads no member state. + static bool IsNameOf(InvocationExpressionSyntax inv) + => inv.Expression is IdentifierNameSyntax { Identifier.ValueText: "nameof" }; + + void Visit(SyntaxNode node, bool suppress) + { + switch (node) + { + // Decision #5: v1 is a READS-only scan. The whole LHS subtree + // is skipped — writes to non-variables are invisible (the + // report notes this when it applies; see + // HasNonVariableAssignmentLeftSide). Nested reads inside an + // LHS (the `arr` in `arr[i].X = 5`) are the documented loss. + case AssignmentExpressionSyntax assignment: + Visit(assignment.Right, suppress: false); + break; + + case InvocationExpressionSyntax inv when IsNameOf(inv): + foreach (var arg in inv.ArgumentList.Arguments) + { + Visit(arg.Expression, suppress: false); + } + + break; + + case InvocationExpressionSyntax inv: + // The invocation is the candidate; its direct callee is + // covered by it (suppress = true below). + if (!suppress) + { + Report(inv, invocation: true); + } + + Visit(inv.Expression, suppress: true); + foreach (var arg in inv.ArgumentList.Arguments) + { + Visit(arg.Expression, suppress: false); + } + + break; + + case ElementAccessExpressionSyntax elementAccess: + if (!suppress) + { + Report(elementAccess, invocation: false); + } + + // The indexed CONTAINER is covered by the access itself + // (same rule as the invocation callee above). + Visit(elementAccess.Expression, suppress: true); + foreach (var arg in elementAccess.ArgumentList.Arguments) + { + Visit(arg.Expression, suppress: false); + } + + break; + + case MemberAccessExpressionSyntax memberAccess: + if (!suppress) + { + Report(memberAccess, invocation: false); + } + + // The member NAME is part of this node; only the receiver + // is an independent read. Suppression never propagates + // here: it marks exactly ONE node (the direct callee), so + // `a.B.C(x)` still reports its `a.B` link. + Visit(memberAccess.Expression, suppress: false); + break; + + case MemberBindingExpressionSyntax binding: + // `a?.B`: the binding is the member access shape of a + // null-conditional chain. No children to visit (the name + // is part of this node). + if (!suppress) + { + Report(binding, invocation: false); + } + + break; + + case CastExpressionSyntax cast: + if (!suppress) + { + Report(cast, invocation: false); + } + + Visit(cast.Expression, suppress: false); + break; + + case IdentifierNameSyntax id: + if (!suppress) + { + ClassifyIdentifier(id); + } + + break; + + case ParenthesizedExpressionSyntax parenthesized: + // Parentheses must not change what "covered by the call" + // means: `(best.Bigger)(...)` keeps the callee covered. + Visit(parenthesized.Expression, suppress); + break; + + default: + // Generic descent (statement scaffolding, binaries, + // unary/conditional operators, interpolation holes, ...): + // none of these shapes is itself an extract-first + // candidate, but their operand expressions are. + foreach (var child in node.ChildNodes()) + { + Visit(child, suppress: false); + } + + break; + } + } + + foreach (var statement in selection.Statements) + { + Visit(statement, suppress: false); + } + + return entries; + } + + /// + /// True when the selection assigns to something that is not a local or a + /// parameter (a field, property, indexer, ...) — the decision-5 case the + /// report must flag, because the reads-only scan cannot see that write. + /// + public static bool HasNonVariableAssignmentLeftSide(SemanticModel model, SelectionReport selection) + { + foreach (var statement in selection.Statements) + { + foreach (var node in statement.DescendantNodesAndSelf().OfType()) + { + var symbol = model.GetSymbolInfo(node.Left).Symbol; + if (symbol is not ILocalSymbol and not IParameterSymbol) + { + return true; + } + } + } + + return false; + } +} diff --git a/tools/ExtractMethod/Tooling/ExtractionReporter.cs b/tools/ExtractMethod/Tooling/ExtractionReporter.cs new file mode 100644 index 0000000..37139a7 --- /dev/null +++ b/tools/ExtractMethod/Tooling/ExtractionReporter.cs @@ -0,0 +1,58 @@ +using Microsoft.CodeAnalysis; + +namespace ExtractMethod.Tooling; + +/// Everything the extract-method report is built from: the resolved +/// selection, the raw classification buckets (em 02), the extract-first +/// candidates (em 03), the promoted signature line (em 03) and the v1 +/// limitation notes. One record so the CLI and the tests compose EXACTLY the +/// same thing — the report cannot drift between them. +public sealed record ExtractionReport( + SelectionReport Selection, + ExtractionSuggestion Suggestion, + IReadOnlyList ExtractFirst, + SignatureSuggestion Signature, + IReadOnlyList Notes); + +/// +/// Composes the full extraction suggestion for a resolved selection: the em 02 +/// data-flow buckets, the em 03 extract-first scan, the promoted signature and +/// the limitation notes. Throws the same clean exceptions as +/// when analysis cannot bind. +/// +public static class ExtractionReporter +{ + public static ExtractionReport Compose(SemanticModel model, SelectionReport selection) + { + var suggestion = DataFlowClassifier.Classify(model, selection); + var extractFirst = ExtractFirstScanner.Scan(model, selection); + var signature = SignatureBuilder.Build(model, selection, suggestion); + return new ExtractionReport(selection, suggestion, extractFirst, signature, BuildNotes(model, selection)); + } + + /// + /// The v1 limitation notes (parent decision #5 and the em 02 contract): + /// each note is printed only when its limitation actually applies to the + /// selection, so a clean selection gets a clean report. + /// + private static IReadOnlyList BuildNotes(SemanticModel model, SelectionReport selection) + { + var notes = new List(); + + if (ExtractFirstScanner.HasNonVariableAssignmentLeftSide(model, selection)) + { + notes.Add( + "reads-only scan (decision 5): assignment left-hand sides are skipped, " + + "so writes to fields/indexers/properties are invisible to this report"); + } + + if (DataFlowClassifier.CompositeTrailingReturn(selection) is { } trailing) + { + notes.Add( + $"the selection ends with a composite return expression ({trailing.Expression!.ToFullString().Trim()}) — " + + "extract it into a local first to make the return value nameable"); + } + + return notes; + } +} diff --git a/tools/ExtractMethod/Tooling/ReportFormatter.cs b/tools/ExtractMethod/Tooling/ReportFormatter.cs new file mode 100644 index 0000000..67a58cc --- /dev/null +++ b/tools/ExtractMethod/Tooling/ReportFormatter.cs @@ -0,0 +1,80 @@ +using System.Text; + +namespace ExtractMethod.Tooling; + +/// +/// Renders an as the human-readable extract- +/// method suggestion report (parent spec): params (with ref), return +/// candidates, locals, extract-first candidates (deduped, with occurrence +/// counts and flags), the limitation notes, and the suggested signature line +/// LAST. Pure string shaping — no Roslyn — so tests can pin the exact output +/// the CLI prints. +/// +public static class ReportFormatter +{ + public static string Format(ExtractionReport report) + { + var sb = new StringBuilder(); + var selection = report.Selection; + var suggestion = report.Suggestion; + + // Header (same wording the CLI printed since em 01). + sb.AppendLine( + $"{selection.Count} statement(s) selected, lines {selection.StartLine}..{selection.EndLine} " + + $"in {selection.Method?.Identifier.ValueText}(): " + + string.Join(", ", selection.Kinds)); + + AppendBucket(sb, "params", suggestion.Params.Select(p => + $"{p.Name} ({p.Type}){(p.ByRef ? " [ref]" : " [in]")}")); + AppendBucket(sb, "returns", suggestion.Returns.Select(r => $"{r.Name} ({r.Type})")); + AppendBucket(sb, "locals", suggestion.Locals); + + if (report.ExtractFirst.Count == 0) + { + sb.AppendLine("extract-first: (none)"); + } + else + { + sb.AppendLine("extract-first:"); + foreach (var entry in report.ExtractFirst) + { + // Flags accumulate; occurrence count is always shown because + // "count occurrences" is the point of the dedupe. + var flags = new List(); + if (entry.IsInvocation) + { + flags.Add("hoisting changes eval count"); + } + + if (entry.Optional) + { + flags.Add($"optional: own-class {entry.OwnMemberKind}"); + } + + var flagText = flags.Count > 0 ? $" [{string.Join("; ", flags)}]" : string.Empty; + sb.AppendLine($" - {entry.Text} — {entry.SymbolDisplay} ×{entry.Occurrences}{flagText}"); + } + } + + foreach (var note in report.Notes) + { + sb.AppendLine($"note: {note}"); + } + + var signature = report.Signature; + var parameterList = string.Join(", ", signature.Params.Select(p => $"{(p.ByRef ? "ref " : "")}{p.Type} {p.Name}")); + sb.AppendLine($"suggested signature: {signature.ReturnType} {signature.MethodName}({parameterList})"); + + return sb.ToString(); + } + + private static void AppendBucket(StringBuilder sb, string name, IEnumerable items) + { + var enumerated = items.ToList(); + sb.AppendLine(enumerated.Count == 0 ? $"{name}: (none)" : $"{name}: {enumerated[0]}"); + for (var i = 1; i < enumerated.Count; i++) + { + sb.AppendLine($"{name}: {enumerated[i]}"); + } + } +} diff --git a/tools/ExtractMethod/Tooling/SignatureBuilder.cs b/tools/ExtractMethod/Tooling/SignatureBuilder.cs new file mode 100644 index 0000000..0c4cdc1 --- /dev/null +++ b/tools/ExtractMethod/Tooling/SignatureBuilder.cs @@ -0,0 +1,128 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace ExtractMethod.Tooling; + +/// The suggested signature line of the extraction (parent spec: +/// "ends with a suggested signature: int Extract(int i, Order order)"). +/// Return type, void when nothing must flow +/// out, or a tuple type when several declared-inside variables must. +/// v1 always suggests Extract. +/// The coherent parameter list (ref-ness preserved from +/// the classification). +public sealed record SignatureSuggestion( + string ReturnType, + string MethodName, + IReadOnlyList Params); + +/// +/// Turns the raw em 02 buckets into ONE coherent signature line. The buckets +/// over-report on purpose (parent spec: a variable can sit in several +/// buckets); building a signature is where the overlaps are resolved — the +/// "promotion" em 02's comment promises. +/// +/// RULES (v1): +/// - A variable DECLARED inside the selection cannot be a parameter of the +/// extracted method (it does not exist before the selection): drop it from +/// the params. If it is also a return candidate, it becomes the RETURN — +/// e.g. `total` (declared, reassigned, read after) in Summarize lands in +/// params[ref] ∧ returns raw, and promotes to the plain return value. +/// - A return candidate declared OUTSIDE the selection that the extraction +/// already receives by-ref (read inside ∧ written inside) keeps its ref +/// parameter: the write-back already flows the value out, and also +/// returning it would hand the caller the same value twice. +/// - A return candidate declared OUTSIDE that is NOT a param (written +/// inside, never read inside) becomes a return slot too: the caller +/// reassigns it from the extracted method's result. +/// - Trailing composite returns (`return a + b;`) are not nameable in v1 — +/// no return slot; the report's note suggests extracting a local first. +/// +/// Determinism: parameters keep the classifier's ordinal-by-name order and +/// return slots are ordered by name, so the line is stable for tests. +/// +public static class SignatureBuilder +{ + public static SignatureSuggestion Build(SemanticModel model, SelectionReport selection, ExtractionSuggestion suggestion) + { + var declaredInside = DeclaredInsideNames(model, selection); + var paramNames = suggestion.Params.Select(p => p.Name).ToHashSet(StringComparer.Ordinal); + + var parameters = suggestion.Params + .Where(p => !declaredInside.Contains(p.Name)) + .ToList(); + + var returnSlots = suggestion.Returns + .Where(r => declaredInside.Contains(r.Name) || !paramNames.Contains(r.Name)) + .OrderBy(r => r.Name, StringComparer.Ordinal) + .ToList(); + + var returnType = returnSlots.Count switch + { + 0 => "void", + 1 => returnSlots[0].Type, + // Branch-insensitive over-approximation can over-report returns + // (decision #4); the tuple keeps every candidate flowing rather + // than guessing which is real. + _ => $"({string.Join(", ", returnSlots.Select(r => r.Type))})", + }; + + return new SignatureSuggestion(returnType, "Extract", parameters); + } + + /// + /// Names of the locals DECLARED inside the selection. v1 covers the + /// declaration shapes the fixture uses — local declarations, for-loop + /// headers (`for (int i = ...)`) and foreach headers; other declaring + /// shapes (pattern declarations, using/fixed statements, local functions) + /// are not selected by the v1 resolver and would need their own cases. + /// + /// Names, not symbols, are matched against the bucket entries — sound + /// here because C# forbids a local from shadowing another local or a + /// parameter of the same method, so names are unique per method. + /// + public static HashSet DeclaredInsideNames(SemanticModel model, SelectionReport selection) + { + var names = new HashSet(StringComparer.Ordinal); + + foreach (var statement in selection.Statements) + { + foreach (var node in statement.DescendantNodesAndSelf()) + { + switch (node) + { + case LocalDeclarationStatementSyntax localDeclaration: + foreach (var variable in localDeclaration.Declaration.Variables) + { + if (model.GetDeclaredSymbol(variable) is ILocalSymbol) + { + names.Add(variable.Identifier.ValueText); + } + } + + break; + + case ForStatementSyntax forStatement when forStatement.Declaration is not null: + foreach (var variable in forStatement.Declaration.Variables) + { + if (model.GetDeclaredSymbol(variable) is ILocalSymbol) + { + names.Add(variable.Identifier.ValueText); + } + } + + break; + + case ForEachStatementSyntax forEach: + if (model.GetDeclaredSymbol(forEach) is ILocalSymbol) + { + names.Add(forEach.Identifier.ValueText); + } + + break; + } + } + } + + return names; + } +}