Browse Source

Address review: keep while-loop instead of headless for for ref locals

Per @siegfriedpammer: rather than rescue the hoisted for-initializer in
DeclareVariables, don't form the for-loop at all. TransformFor now bails when the
loop variable is a by-ref-like local used after the loop, leaving the while-loop
(matching source); the ref decl keeps its initializer, no CS8174. Reverts the
DeclareVariables change; test now expects the while form.

Assisted-by: Copilot:claude-opus-4.8:GitHub Copilot CLI
pull/3863/head
Sebastien Lebreton 1 week ago committed by Siegfried Pammer
parent
commit
44ac0dd05d
  1. 9
      ICSharpCode.Decompiler.Tests/TestCases/Pretty/RefLocalsAndReturns.cs
  2. 59
      ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs
  3. 16
      ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs

9
ICSharpCode.Decompiler.Tests/TestCases/Pretty/RefLocalsAndReturns.cs

@ -168,17 +168,18 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty @@ -168,17 +168,18 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
public static Issue3827Node RefLocalUsedAfterForLoop(Issue3827Node[] buckets, int hash, Issue3827Node newNode)
{
// The ref local is used after the loop, so its declaration is hoisted in front of the
// for-statement; the for-initializer ref-assignment must stay on the declaration, because
// a ref local cannot be declared without an initializer.
// The ref local is used after the loop, so its declaration is hoisted in front of the loop.
// The loop is kept as a while-loop (rather than converted to a headless for-loop) so the
// ref-assignment stays on the declaration -- a ref local cannot be declared without an initializer.
ref Issue3827Node reference = ref buckets[hash & 3];
for (; reference != null; reference = ref reference.next)
while (reference != null)
{
if (reference.value == hash)
{
reference = newNode;
break;
}
reference = ref reference.next;
}
if (reference == null)
{

59
ICSharpCode.Decompiler/CSharp/Transforms/DeclareVariables.cs

@ -588,39 +588,6 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms @@ -588,39 +588,6 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
&& identExpr.TypeArguments.Count == 0;
}
/// <summary>
/// Matches the case where the variable's only initialization is the first initializer of a
/// for-statement (`for (v = expr; ...)`) and the declaration has to be placed in front of the
/// for-statement (e.g. because the variable is used after the loop). Used to keep a ref local's
/// initializer on its declaration (a ref local may not be declared without one).
/// </summary>
bool IsMatchingForInitializerAssignment(VariableToDeclare v, [NotNullWhen(true)] out ForStatement? forStatement,
[NotNullWhen(true)] out Statement? initializerStatement, [NotNullWhen(true)] out AssignmentExpression? assignment)
{
forStatement = null;
initializerStatement = null;
assignment = null;
if (v.InsertionPoint.nextNode is not ForStatement { Parent: BlockStatement } fs)
return false;
if (fs.Initializers.FirstOrDefault() is not ExpressionStatement { Expression: AssignmentExpression a } firstInit)
return false;
if (a.Operator != AssignmentOperatorType.Assign
|| a.Left is not IdentifierExpression identExpr
|| identExpr.Identifier != v.Name
|| identExpr.TypeArguments.Count != 0)
{
return false;
}
forStatement = fs;
initializerStatement = firstInit;
assignment = a;
return true;
}
bool CombineDeclarationAndInitializer(VariableToDeclare v, TransformContext context)
{
if (v.Type.IsByRefLike)
@ -674,32 +641,6 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms @@ -674,32 +641,6 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
return vds;
}, "Combine variable declaration with initializer"));
}
else if (v.Type.IsByRefLike && IsMatchingForInitializerAssignment(v, out var forStatement, out var forInitializerStatement, out var forInitAssignment))
{
// A by-ref-like local that is used after the loop has its declaration hoisted in
// front of the for-statement, but its only initialization is the for-initializer
// ref-assignment. A ref local must be declared with an initializer (CS8174), so move
// the ref-assignment's value up into the declaration and drop the for-initializer.
AstType type = context.TypeSystemAstBuilder.ConvertType(v.Type);
if (v.ILVariable.IsRefReadOnly && type is ComposedType { HasRefSpecifier: true } composedType)
{
composedType.HasReadOnlySpecifier = true;
}
var vds = new VariableDeclarationStatement(type, v.Name, forInitAssignment.Right.Detach());
var init = vds.Variables.Single();
init.AddAnnotation(forInitAssignment.Left.GetResolveResult());
foreach (object annotation in forInitAssignment.Left.Annotations.Concat(forInitAssignment.Annotations))
{
if (annotation is not ResolveResult)
{
init.AddAnnotation(annotation);
}
}
forInitializerStatement.Remove();
forStatement.Parent!.InsertChildBefore(forStatement, vds, Slots.Statement);
}
else if (CanBeDeclaredAsOutVariable(v, out var dirExpr))
{
// 'T v; SomeCall(out v);' can be combined to 'SomeCall(out T v);'

16
ICSharpCode.Decompiler/CSharp/Transforms/PatternStatementTransform.cs

@ -203,6 +203,12 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms @@ -203,6 +203,12 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
if (variable != m3.Get<IdentifierExpression>("ident").Single().GetILVariable())
return null;
WhileStatement loop = (WhileStatement)next;
// Cannot convert to for loop, if the iteration variable is a ref local used after the loop: its
// declaration is hoisted in front, leaving a headless `for (; cond; v = ref ...)` whose only
// initialization is the for-initializer ref-assignment -- which can't be split from a ref local
// (CS8174). Keeping it a while-loop matches the source and keeps the initializer on the decl.
if (variable != null && variable.Type.IsByRefLike && IsVariableUsedAfter(loop, variable))
return null;
// Cannot convert to for loop, if any variable that is used in the "iterator" part of the pattern,
// will be declared in the body of the while-loop.
var iteratorStatement = m3.Get<Statement>("iterator").Single();
@ -247,6 +253,16 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms @@ -247,6 +253,16 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms
return false;
}
bool IsVariableUsedAfter(Statement loop, IL.ILVariable variable)
{
for (AstNode? sibling = loop.NextSibling; sibling != null; sibling = sibling.NextSibling)
{
if (sibling.DescendantsAndSelf.OfType<IdentifierExpression>().Any(ie => ie.GetILVariable() == variable))
return true;
}
return false;
}
bool IteratorVariablesDeclaredInsideLoopBody(Statement iteratorStatement)
{
foreach (var id in iteratorStatement.DescendantsAndSelf.OfType<IdentifierExpression>())

Loading…
Cancel
Save