Extend UseLambdaSyntax to emit statement lambdas - #3974
Conversation
Under UseLambdaSyntax, anonymous functions became lambdas only when an
expression body was possible; statement-bodied ones kept C# 2 delegate
syntax. Now every anonymous function whose parameter shape a lambda can
express uses lambda syntax; delegate syntax remains for ref/out/in and
params parameters and for pre-C# 3 language profiles.
Two latent issues surfaced by the wider lambda coverage: DeclareVariables
assumed an insertion point directly under a LambdaExpression is an
expression body it must convert to a block, which block-bodied lambdas
now violate; and anonymous methods declared without a parameter list
carry compiler-generated parameter names like '<p0>' that are not valid
identifiers, so the lambda's mandatory parameter list regenerates such
names from the parameter type: (object obj, EventArgs e) => ...
A side effect visible in fixtures: an explicit parameter list can make
a delegate-creation cast redundant that bare 'delegate' syntax needed
for overload resolution, e.g. new Thread((ThreadStart)delegate { })
becomes new Thread(() => { }).
Assisted-by: Claude:claude-fable-5:Claude Code
The parameter-list-less anonymous method form is compatible with any delegate signature, and C# code must rely on exactly that when a delegate's parameter types cannot be named at the use site: IL, unlike C#, permits a delegate signature to reference less accessible types. Expanding such an anonymous method into a lambda would force the unnameable type into a parameter list. Keep the delegate form, with its parameter list dropped, when the parameters are unused and one of their types is not accessible from the current context. Assisted-by: Claude:claude-fable-5:Claude Code
christophwille
left a comment
christophwille
left a comment
There was a problem hiding this comment.
Automated high-effort review (Claude Code, multi-agent find + adversarial-verify) of head 0116f24.
The direction looks good, and the head commit ("Keep 'delegate {}' when a parameter type is inaccessible") already resolves one issue the review had flagged against the earlier state: lambda expansion forcing unnameable delegate parameter types into an emitted parameter list. Six findings remain, posted inline:
- Correctness (confirmed):
DeclareVariables.ResolveCollisionscan emit invalid C# likex => int num = x;when the expression-lambda body preserved by the new stop condition is itself the matching assignment. Pre-existing on master, but this PR touches exactly that stop condition. - Output regression (confirmed): unused-parameter anonymous methods no longer collapse to
delegate {}when the parameter types are accessible, so synthetic-but-valid parameter names (ilasmA_0/A_1, obfuscators) now leak into output - visible in this PR's own Issue1038 expectation change. - Comment accuracy:
ref/out/inare expressible in explicitly-typed lambda parameter lists; the comment's "shapes a lambda cannot express" rationale is wrong for those modifiers. - Test coverage: the new invalid-name (
<p0>) branch inAssignVariableNamesis exercised by no fixture after the Bug971 rewrite (the newInaccessibleParameterTypes.iluses empty parameter names, which take thevariables[i].Namefallback instead). - Test coverage: the retained delegate-syntax fallback for
ref/out/in/paramsparameters has no fixture. - Simplification:
HasGeneratedName = truein the invalid-name branch is redundant; thecontinueis the load-bearing part.
| // will convert that body to a block); a point at a statement-bodied lambda's block | ||
| // itself must keep moving up into the enclosing scope. | ||
| while (!(v.InsertionPoint.nextNode.Parent is BlockStatement | ||
| || (v.InsertionPoint.nextNode.Parent is LambdaExpression && v.InsertionPoint.nextNode is Expression))) |
There was a problem hiding this comment.
Correctness (confirmed): when the expression-lambda body preserved by this stop condition is itself the matching assignment, the CombineDeclarationAndInitializer path replaces the lambda's body Expression with a VariableDeclarationStatement without first converting the body to a block, emitting invalid C# such as x => int num = x;.
Repro shape: IL stloc num(ldarg x); ldloc num; ret inside a delegate (a single return num = x;, with num a non-captured local of the lambda) decompiles to the expression lambda x => num = x, whose body node is the AssignmentExpression. IsMatchingAssignment then matches the body node itself and the replacement puts a statement in expression position.
This was already reachable on master for single-return expression lambdas, but this PR touches exactly this stop condition and the new comment documents the body-to-block conversion assumption, so it seems worth guarding here (e.g. do not stop at a lambda when the insertion point is the matching assignment itself, or convert the body to a block before combining).
| // and for unused parameters whose types the current context cannot name: | ||
| // only the parameter-list-less "delegate {}" form, which is compatible with | ||
| // any delegate signature, can legally occur in such code. | ||
| isLambda = true; |
There was a problem hiding this comment.
Output regression (confirmed): making isLambda unconditional here removes the parameterless delegate { } form for anonymous methods whose parameters are unused but whose types are accessible, so machine-generated-but-valid parameter names now leak into output. The IsValidName guard in AssignVariableNames only catches syntactically invalid names, and the accessibility guard on this branch only restores delegate {} when a parameter type cannot be named.
This PR's own Issue1038 expectation shows the effect: delegate { } becomes (TK A_0, TR A_1) => {}, surfacing ilasm's synthetic A_0/A_1 names (from unnamed Param rows) as if they were source-level names. Obfuscated assemblies get the same treatment for every unused-parameter anonymous method.
Consider also keeping delegate {} (or regenerating names) when all parameters are unused and their metadata names are absent or generated.
| // otherwise use lambda only if an expression lambda is possible | ||
| isLambda = (body.Statements.Count == 1 && body.Statements.Single() is ReturnStatement); | ||
| // Lambdas cover statement bodies too; anonymous method syntax remains for | ||
| // parameter shapes a lambda cannot express (ref/out/in and params modifiers) |
There was a problem hiding this comment.
Comment accuracy: explicitly-typed lambda parameters have supported ref/out modifiers since C# 3 ((ref int x) => { ... } is valid) and in since C# 7.2; only the omitted parameter list and params (before C# 13) are genuinely delegate-only. Keeping delegate syntax for these modifiers is a fine conservative choice, but the "shapes a lambda cannot express" justification is factually wrong for ref/out/in and could mislead a future change.
| isLambda = true; | ||
| } | ||
| else if (settings.UseLambdaSyntax && ame.Parameters.All(p => p.ParameterModifier == ReferenceKind.None && !p.IsParams)) | ||
| else if (settings.UseLambdaSyntax && ame.Parameters.All(p => p.ParameterModifier == ReferenceKind.None && !p.IsParams) |
There was a problem hiding this comment.
Test coverage: after this PR no fixture exercises the retained delegate-syntax path for ref/out/in/params parameters - every delegate occurrence in TestCases was converted to lambda syntax, and no fixture contains an anonymous method with a modifier-carrying parameter list. (The new InaccessibleParameterTypes fixture covers the delegate {} / Parameters.Clear() fallback, but the modifier condition of this branch is untested.) A small Pretty fixture with e.g. a delegate type taking a ref parameter would pin the behavior this branch claims to preserve.
| if (variables.TryGetValue(i, out var v)) | ||
| variableMapping[v] = name; | ||
| } | ||
| else if (!IsValidName(name)) |
There was a problem hiding this comment.
Test coverage: this new branch is exercised by no fixture. The only fixture that produced <p0>-style names, Bug971_DelegateWithoutParameterList, was rewritten in this same PR from delegate { } to (object obj) => { }, so no test now compiles an anonymous method declared without a parameter list against a parameterful delegate. The new InaccessibleParameterTypes.il does not cover it either: its anonymous-method parameters have empty names, which take the variables[i].Name fallback above instead of reaching this IsValidName check. An ILPretty fixture with a literally '<p0>'-named parameter would keep this branch pinned.
| // declared without a parameter list) are not valid C# identifiers; generate | ||
| // a fresh name from the parameter type instead of escaping the raw name. | ||
| if (variables.TryGetValue(i, out var invalidlyNamed)) | ||
| invalidlyNamed.HasGeneratedName = true; |
There was a problem hiding this comment.
Simplification: setting HasGeneratedName = true here is redundant - AssignName already regenerates any name that fails IsValidName (it checks v.HasGeneratedName || !IsValidName(newName)), so the load-bearing part of this branch is the continue that skips name reservation/mapping. Dropping the assignment and keeping only the skip would express the fix exactly; as written, a reader may assume the flag is what triggers regeneration and simplify away the continue.
2 participants
With
UseLambdaSyntaxenabled, anonymous functions were emitted as lambdas only when an expression body was possible; every statement-bodied anonymous function still decompiled to C# 2delegatesyntax. This PR extends the setting to emit statement lambdas: any anonymous function whose parameter shape a lambda can express now uses lambda syntax.delegatesyntax remains forref/out/inandparamsparameters, and for pre-C# 3 language profiles (SetLanguageVersionstill disables the setting there).Two latent issues surfaced by the wider lambda coverage and are fixed here:
DeclareVariables.ResolveCollisionsstopped its insertion-point walk at any node whose parent is aLambdaExpression, assuming an expression body that declaration insertion then converts to a block. A statement-bodied lambda's own block now reaches that path and tripped aDebug.Assert; the stop now applies only to actual expression bodies.<p0>,<sender>) that are not valid C# identifiers. The old output hid them by omitting the parameter list; a lambda cannot, soAssignVariableNamesnow treats an invalid metadata name like a missing one and regenerates it from the parameter type:(object obj, EventArgs e) => {...}.A visible side effect captured in the fixtures: an explicit parameter list can make a delegate-creation cast redundant that bare
delegateneeded for overload resolution -new Thread((ThreadStart)delegate { })becomesnew Thread(() => { }).Verification: full decompiler suite green (3317 passed). Differential check with decompdiff (master vs. this branch, fresh Release builds) over 12 real-world assemblies / 2341 types - Newtonsoft.Json 9, EntityFramework 6.2, Autofac, Castle.Core, Moq, NLog, log4net, Dapper, RestSharp, SharpZipLib, Mono.Cecil, NUnit 3.5: 68 types changed, every changed line is the delegate-to-lambda rewrite (parameter names from metadata are preserved), 0 new errors, and no deltas in lines, gotos,
//IL_warnings, or compiler-generated name leaks.This PR description was written by the Claude Code agent session that authored the change.
🤖 Generated with Claude Code