em 03: extract-first scan + full suggestion report
- ExtractFirstScanner: reads-only scan of non-local/param read expressions (member/field/property access, element access, invocation, cast), deduped by symbol+text with occurrence counts; invocations flagged (hoisting changes eval count); own-class field/property reads marked optional. Assignment LHS skipped whole (decision 5) — surfaced as a report note. - SignatureBuilder: promotes bucket overlaps into one coherent signature (declared-inside vars drop from params; declared-inside return candidates become the return; outside candidates already covered by their ref write-back stay ref params). Trailing composite returns stay void with a note suggesting an extract-variable first. - ExtractionReporter + ReportFormatter: single composition shared by CLI and tests; report = header, params (ref), returns, locals, extract-first (count + flags), notes, suggested-signature line last. - DataFlowClassifier: Returns now carry types (ReturnSuggestion) — the signature line needs them. - Displays: compact symbol/type formatting for the report. - Fixture: RepeatReads (dedupe ×2 + skipped field write), Casts (cast candidate) appended at the end so all pinned line numbers stay put.
This commit is contained in:
@@ -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;
|
||||
@@ -21,19 +21,27 @@ namespace ExtractMethod.Tooling;
|
||||
/// the precise v1 rule.</param>
|
||||
public sealed record ParamSuggestion(string Name, string Type, bool ByRef);
|
||||
|
||||
/// <summary>One return candidate of the extracted method (v1: plain strings).</summary>
|
||||
/// <param name="Name">Variable name as it appears in the source.</param>
|
||||
/// <param name="Type">Type as a display string — the signature line of em 03
|
||||
/// prints it as the extracted method's return type.</param>
|
||||
public sealed record ReturnSuggestion(string Name, string Type);
|
||||
|
||||
/// <summary>The classified buckets of one selection (v1: symbol names as plain strings).</summary>
|
||||
/// <param name="Params">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 <see cref="Locals"/> and/or <see cref="Returns"/> —
|
||||
/// v1 reports per-bucket and em 03 dedupes into a coherent signature.</param>
|
||||
/// v1 reports per-bucket; em 03's signature builder dedupes into a coherent
|
||||
/// signature (a variable DECLARED inside the selection cannot be a parameter).</param>
|
||||
/// <param name="Returns">Variables written inside AND read after the selection
|
||||
/// (tail data flow), plus the value of a trailing <c>return X;</c> when X is a
|
||||
/// simple name — they must flow out of the extraction.</param>
|
||||
/// simple name — they must flow out of the extraction. Typed since em 03: the
|
||||
/// signature line needs the return type, not just the name.</param>
|
||||
/// <param name="Locals">Variables written inside and never read after: scratch
|
||||
/// locals of the new method.</param>
|
||||
public sealed record ExtractionSuggestion(
|
||||
IReadOnlyList<ParamSuggestion> Params,
|
||||
IReadOnlyList<string> Returns,
|
||||
IReadOnlyList<ReturnSuggestion> Returns,
|
||||
IReadOnlyList<string> Locals);
|
||||
|
||||
/// <summary>
|
||||
@@ -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<ISymbol>.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<string>())
|
||||
.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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="TrailingReturnSuggestion"/>).
|
||||
/// </summary>
|
||||
public static ReturnStatementSyntax? CompositeTrailingReturn(SelectionReport selection)
|
||||
=> selection.Statements[^1] is ReturnStatementSyntax { Expression: not null and not IdentifierNameSyntax } ret
|
||||
? ret
|
||||
: null;
|
||||
}
|
||||
@@ -0,0 +1,53 @@
|
||||
using System.Collections.Immutable;
|
||||
using Microsoft.CodeAnalysis;
|
||||
|
||||
namespace ExtractMethod.Tooling;
|
||||
|
||||
/// <summary>
|
||||
/// Compact symbol display for the suggestion report. The defaults are wrong
|
||||
/// for a report a human reads next to their own source:
|
||||
/// - <c>ToDisplayString()</c> fully qualifies every namespace
|
||||
/// (<c>System.Collections.Generic.List<Widget></c>),
|
||||
/// - <c>ToMinimalDisplayString()</c> keeps containing types AND prefixes
|
||||
/// members with their own type (<c>int Demo._seed</c>).
|
||||
/// The report wants the shortest form that stays readable in context:
|
||||
/// types as bare names (<c>Widget</c>, <c>List<Widget></c>), members as
|
||||
/// <c>ContainingType.Member</c> with parameter types.
|
||||
/// </summary>
|
||||
internal static class Displays
|
||||
{
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// (<c>Demo.Widget</c>) and has no With-style override here.
|
||||
/// </summary>
|
||||
internal static readonly SymbolDisplayFormat TypeFormat = new(
|
||||
globalNamespaceStyle: SymbolDisplayGlobalNamespaceStyle.Omitted,
|
||||
typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameOnly,
|
||||
genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters
|
||||
| SymbolDisplayGenericsOptions.IncludeVariance,
|
||||
miscellaneousOptions: SymbolDisplayMiscellaneousOptions.UseSpecialTypes
|
||||
| SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers
|
||||
| SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier);
|
||||
|
||||
/// <summary>Compact member display for extract-first entries:
|
||||
/// <c>Demo._seed</c>, <c>Demo.Scale</c>, <c>Widget.Bigger(Widget)</c>,
|
||||
/// <c>List<Widget>.this[int]</c>.</summary>
|
||||
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<IParameterSymbol> parameters)
|
||||
=> string.Join(", ", parameters.Select(p => Type(p.Type)));
|
||||
|
||||
private static string Type(ITypeSymbol? type) => type?.ToDisplayString(TypeFormat) ?? "?";
|
||||
}
|
||||
@@ -0,0 +1,316 @@
|
||||
using Microsoft.CodeAnalysis;
|
||||
using Microsoft.CodeAnalysis.CSharp.Syntax;
|
||||
|
||||
namespace ExtractMethod.Tooling;
|
||||
|
||||
/// <summary>
|
||||
/// 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. <see cref="Text"/> plus <see cref="SymbolDisplay"/> identify
|
||||
/// the candidate; <see cref="Occurrences"/> is how often the same candidate
|
||||
/// (same symbol + same source text) is read inside the selection.
|
||||
/// </summary>
|
||||
/// <param name="Text">Source text of the expression (trivia-free).</param>
|
||||
/// <param name="SymbolDisplay">The bound symbol, minimally qualified for the
|
||||
/// report, or <c><unbound></c> when the expression resolves to no
|
||||
/// symbol (e.g. a cast — conversions are not symbols).</param>
|
||||
/// <param name="Occurrences">How often this exact candidate is read.</param>
|
||||
/// <param name="IsInvocation">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).</param>
|
||||
/// <param name="OwnMemberKind"><c>field</c> or <c>property</c> 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).</param>
|
||||
public sealed record ExtractFirstEntry(
|
||||
string Text,
|
||||
string SymbolDisplay,
|
||||
int Occurrences,
|
||||
bool IsInvocation,
|
||||
string? OwnMemberKind)
|
||||
{
|
||||
/// <summary>Own-class field/property reads are optional to hoist (see <see cref="OwnMemberKind"/>).</summary>
|
||||
public bool Optional => OwnMemberKind is not null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="HasNonVariableAssignmentLeftSide"/>).
|
||||
/// - 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.
|
||||
/// </summary>
|
||||
public static class ExtractFirstScanner
|
||||
{
|
||||
/// <summary>
|
||||
/// Scans the selection and returns the deduplicated extract-first
|
||||
/// candidates in first-occurrence (source) order.
|
||||
/// </summary>
|
||||
public static IReadOnlyList<ExtractFirstEntry> 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<ExtractFirstEntry>();
|
||||
var indexByKey = new Dictionary<string, int>(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() ?? "<unbound>"}|{text}";
|
||||
var display = symbol is null ? "<unbound>" : 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;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>nameof is spelled like a call but reads no member state.</summary>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public static bool HasNonVariableAssignmentLeftSide(SemanticModel model, SelectionReport selection)
|
||||
{
|
||||
foreach (var statement in selection.Statements)
|
||||
{
|
||||
foreach (var node in statement.DescendantNodesAndSelf().OfType<AssignmentExpressionSyntax>())
|
||||
{
|
||||
var symbol = model.GetSymbolInfo(node.Left).Symbol;
|
||||
if (symbol is not ILocalSymbol and not IParameterSymbol)
|
||||
{
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,58 @@
|
||||
using Microsoft.CodeAnalysis;
|
||||
|
||||
namespace ExtractMethod.Tooling;
|
||||
|
||||
/// <summary>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.</summary>
|
||||
public sealed record ExtractionReport(
|
||||
SelectionReport Selection,
|
||||
ExtractionSuggestion Suggestion,
|
||||
IReadOnlyList<ExtractFirstEntry> ExtractFirst,
|
||||
SignatureSuggestion Signature,
|
||||
IReadOnlyList<string> Notes);
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="DataFlowClassifier.Classify"/> when analysis cannot bind.
|
||||
/// </summary>
|
||||
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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
private static IReadOnlyList<string> BuildNotes(SemanticModel model, SelectionReport selection)
|
||||
{
|
||||
var notes = new List<string>();
|
||||
|
||||
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;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,80 @@
|
||||
using System.Text;
|
||||
|
||||
namespace ExtractMethod.Tooling;
|
||||
|
||||
/// <summary>
|
||||
/// Renders an <see cref="ExtractionReport"/> 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.
|
||||
/// </summary>
|
||||
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<string>();
|
||||
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<string> 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]}");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,128 @@
|
||||
using Microsoft.CodeAnalysis;
|
||||
using Microsoft.CodeAnalysis.CSharp.Syntax;
|
||||
|
||||
namespace ExtractMethod.Tooling;
|
||||
|
||||
/// <summary>The suggested signature line of the extraction (parent spec:
|
||||
/// "ends with a suggested signature: <c>int Extract(int i, Order order)</c>").</summary>
|
||||
/// <param name="ReturnType">Return type, <c>void</c> when nothing must flow
|
||||
/// out, or a tuple type when several declared-inside variables must.</param>
|
||||
/// <param name="MethodName">v1 always suggests <c>Extract</c>.</param>
|
||||
/// <param name="Params">The coherent parameter list (ref-ness preserved from
|
||||
/// the classification).</param>
|
||||
public sealed record SignatureSuggestion(
|
||||
string ReturnType,
|
||||
string MethodName,
|
||||
IReadOnlyList<ParamSuggestion> Params);
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public static HashSet<string> DeclaredInsideNames(SemanticModel model, SelectionReport selection)
|
||||
{
|
||||
var names = new HashSet<string>(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;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user