From ffcd468d22f492bbdcc4c4045a2a74df4a7bf308 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sun, 2 Mar 2025 21:41:20 +0100 Subject: [PATCH] Fix #1572: parameters of lambdas and local functions are renamed, if there are with names from outer scopes collisions. --- .../PrettyTestRunner.cs | 56 ++++----- .../TestCases/ILPretty/GuessAccessors.cs | 4 +- .../TestCases/Pretty/DelegateConstruction.cs | 33 ++++++ .../TestCases/VBPretty/Async.cs | 12 +- .../CSharp/CSharpDecompiler.cs | 2 +- .../CSharp/ExpressionBuilder.cs | 2 +- .../IL/Transforms/AssignVariableNames.cs | 112 +++++++++++------- 7 files changed, 143 insertions(+), 78 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/PrettyTestRunner.cs b/ICSharpCode.Decompiler.Tests/PrettyTestRunner.cs index f4ff7c914..cfe0c420b 100644 --- a/ICSharpCode.Decompiler.Tests/PrettyTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/PrettyTestRunner.cs @@ -233,22 +233,21 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task ExceptionHandling([ValueSource(nameof(defaultOptions))] CompilerOptions cscOptions) { - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: new DecompilerSettings { - NullPropagation = false, + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => { + settings.NullPropagation = false; // legacy csc generates a dead store in debug builds - RemoveDeadStores = (cscOptions == CompilerOptions.None), - FileScopedNamespaces = false, + settings.RemoveDeadStores = (cscOptions == CompilerOptions.None); + settings.FileScopedNamespaces = false; }); } [Test] public async Task Switch([ValueSource(nameof(defaultOptions))] CompilerOptions cscOptions) { - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: new DecompilerSettings { + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => { // legacy csc generates a dead store in debug builds - RemoveDeadStores = (cscOptions == CompilerOptions.None), - SwitchExpressions = false, - FileScopedNamespaces = false, + settings.RemoveDeadStores = (cscOptions == CompilerOptions.None); + settings.SwitchExpressions = false; }); } @@ -267,7 +266,10 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task DelegateConstruction([ValueSource(nameof(defaultOptionsWithMcs))] CompilerOptions cscOptions) { - await RunForLibrary(cscOptions: cscOptions); + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => { + settings.QueryExpressions = false; + settings.NullableReferenceTypes = false; + }); } [Test] @@ -293,9 +295,9 @@ namespace ICSharpCode.Decompiler.Tests { await RunForLibrary( cscOptions: cscOptions, - decompilerSettings: new DecompilerSettings { - UseEnhancedUsing = false, - FileScopedNamespaces = false, + configureDecompiler: settings => { + settings.UseEnhancedUsing = false; + settings.FileScopedNamespaces = false; } ); } @@ -327,11 +329,11 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task Loops([ValueSource(nameof(defaultOptionsWithMcs))] CompilerOptions cscOptions) { - DecompilerSettings settings = Tester.GetSettings(cscOptions); - // legacy csc generates a dead store in debug builds - settings.RemoveDeadStores = (cscOptions == CompilerOptions.None); - settings.UseExpressionBodyForCalculatedGetterOnlyProperties = false; - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: settings); + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => { + // legacy csc generates a dead store in debug builds + settings.RemoveDeadStores = (cscOptions == CompilerOptions.None); + settings.UseExpressionBodyForCalculatedGetterOnlyProperties = false; + }); } [Test] @@ -440,9 +442,7 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task VariableNamingWithoutSymbols([ValueSource(nameof(defaultOptions))] CompilerOptions cscOptions) { - var settings = Tester.GetSettings(cscOptions); - settings.UseDebugSymbols = false; - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: settings); + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => settings.UseDebugSymbols = false); } [Test] @@ -474,7 +474,7 @@ namespace ICSharpCode.Decompiler.Tests { await RunForLibrary( cscOptions: cscOptions, - decompilerSettings: new DecompilerSettings { UseEnhancedUsing = false, FileScopedNamespaces = false } + configureDecompiler: settings => { settings.UseEnhancedUsing = false; } ); } @@ -499,7 +499,7 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task FileScopedNamespaces([ValueSource(nameof(roslyn4OrNewerOptions))] CompilerOptions cscOptions) { - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: new DecompilerSettings()); + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => settings.FileScopedNamespaces = true); } [Test] @@ -601,7 +601,7 @@ namespace ICSharpCode.Decompiler.Tests [Test] public async Task Issue1080([ValueSource(nameof(roslynOnlyOptions))] CompilerOptions cscOptions) { - await RunForLibrary(cscOptions: cscOptions, decompilerSettings: new DecompilerSettings(CSharp.LanguageVersion.CSharp6)); + await RunForLibrary(cscOptions: cscOptions, configureDecompiler: settings => settings.SetLanguageVersion(CSharp.LanguageVersion.CSharp6)); } [Test] @@ -712,12 +712,12 @@ namespace ICSharpCode.Decompiler.Tests await RunForLibrary(cscOptions: cscOptions); } - async Task RunForLibrary([CallerMemberName] string testName = null, AssemblerOptions asmOptions = AssemblerOptions.None, CompilerOptions cscOptions = CompilerOptions.None, DecompilerSettings decompilerSettings = null) + async Task RunForLibrary([CallerMemberName] string testName = null, AssemblerOptions asmOptions = AssemblerOptions.None, CompilerOptions cscOptions = CompilerOptions.None, Action configureDecompiler = null) { - await Run(testName, asmOptions | AssemblerOptions.Library, cscOptions | CompilerOptions.Library, decompilerSettings); + await Run(testName, asmOptions | AssemblerOptions.Library, cscOptions | CompilerOptions.Library, configureDecompiler); } - async Task Run([CallerMemberName] string testName = null, AssemblerOptions asmOptions = AssemblerOptions.None, CompilerOptions cscOptions = CompilerOptions.None, DecompilerSettings decompilerSettings = null) + async Task Run([CallerMemberName] string testName = null, AssemblerOptions asmOptions = AssemblerOptions.None, CompilerOptions cscOptions = CompilerOptions.None, Action configureDecompiler = null) { var csFile = Path.Combine(TestCasePath, testName + ".cs"); var exeFile = TestsAssemblyOutput.GetFilePath(TestCasePath, testName, Tester.GetSuffix(cscOptions) + ".exe"); @@ -739,7 +739,9 @@ namespace ICSharpCode.Decompiler.Tests } // 2. Decompile - var decompiled = await Tester.DecompileCSharp(exeFile, decompilerSettings ?? Tester.GetSettings(cscOptions)).ConfigureAwait(false); + var settings = Tester.GetSettings(cscOptions); + configureDecompiler?.Invoke(settings); + var decompiled = await Tester.DecompileCSharp(exeFile, settings).ConfigureAwait(false); // 3. Compile CodeAssert.FilesAreEqual(csFile, decompiled, Tester.GetPreprocessorSymbols(cscOptions).Append("EXPECTED_OUTPUT").ToArray()); diff --git a/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/GuessAccessors.cs b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/GuessAccessors.cs index c9e0f56e0..423ad4119 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/GuessAccessors.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/GuessAccessors.cs @@ -15,8 +15,8 @@ namespace ClassLibrary1 //IL_0007: Expected O, but got Unknown UnknownClass val = new UnknownClass(); int? unknownProperty = val.UnknownProperty; - int? num2 = (val.UnknownProperty = unknownProperty.GetValueOrDefault()); - int? num3 = num2; + int? num = (val.UnknownProperty = unknownProperty.GetValueOrDefault()); + int? num3 = num; List list = new List { val[unknownProperty.Value] ?? "", val.NotProperty, diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs index fd76c9e23..d7566dced 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs @@ -19,15 +19,33 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading; #if CS100 using System.Threading.Tasks; + #endif namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty.DelegateConstruction { public static class DelegateConstruction { + internal class Dummy + { + public int baz; + + public List more; + } + + [CompilerGenerated] + internal class Helper + { + internal bool HelpMe(Dummy dum) + { + return true; + } + } + private class InstanceTests { public struct SomeData @@ -643,6 +661,21 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty.DelegateConstruction { del(x); } + + public void Issue1572(DelegateConstruction.Dummy dum) + { +#if EXPECTED_OUTPUT + DelegateConstruction.Helper CS_0024_003C_003E8__locals0 = new DelegateConstruction.Helper(); + DelegateConstruction.Dummy dummy = dum.more.Where((DelegateConstruction.Dummy dummy2) => true).Where((DelegateConstruction.Dummy dummy2) => true).FirstOrDefault(); + Console.WriteLine(); + dummy.baz++; +#else + DelegateConstruction.Helper h = new DelegateConstruction.Helper(); + DelegateConstruction.Dummy localDummy = dum.more.Where(h.HelpMe).Where(h.HelpMe).FirstOrDefault(); + Console.WriteLine(); + localDummy.baz++; +#endif + } } [AttributeUsage(AttributeTargets.All)] diff --git a/ICSharpCode.Decompiler.Tests/TestCases/VBPretty/Async.cs b/ICSharpCode.Decompiler.Tests/TestCases/VBPretty/Async.cs index 6cb502dd0..7c2c76025 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/VBPretty/Async.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/VBPretty/Async.cs @@ -40,9 +40,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.VBPretty public async void AwaitDefaultYieldAwaitable() { #if LEGACY_VBC || (OPTIMIZE && !ROSLYN4) - YieldAwaitable yieldAwaitable = default(YieldAwaitable); - YieldAwaitable yieldAwaitable2 = yieldAwaitable; - await yieldAwaitable2; + YieldAwaitable yieldAwaitable2 = default(YieldAwaitable); + YieldAwaitable yieldAwaitable = yieldAwaitable2; + await yieldAwaitable; #else await default(YieldAwaitable); #endif @@ -51,9 +51,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.VBPretty public async void AwaitDefaultHopToThreadPool() { #if LEGACY_VBC || (OPTIMIZE && !ROSLYN4) - HopToThreadPoolAwaitable hopToThreadPoolAwaitable = default(HopToThreadPoolAwaitable); - HopToThreadPoolAwaitable hopToThreadPoolAwaitable2 = hopToThreadPoolAwaitable; - await hopToThreadPoolAwaitable2; + HopToThreadPoolAwaitable hopToThreadPoolAwaitable2 = default(HopToThreadPoolAwaitable); + HopToThreadPoolAwaitable hopToThreadPoolAwaitable = hopToThreadPoolAwaitable2; + await hopToThreadPoolAwaitable; #else await default(HopToThreadPoolAwaitable); #endif diff --git a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs index 86f6ed66e..b9559dba1 100644 --- a/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs +++ b/ICSharpCode.Decompiler/CSharp/CSharpDecompiler.cs @@ -1282,7 +1282,7 @@ namespace ICSharpCode.Decompiler.CSharp { if (string.IsNullOrWhiteSpace(parameter.Name) && !parameter.Type.IsArgList()) { - // needs to be consistent with logic in ILReader.CreateILVarable + // needs to be consistent with logic in ILReader.CreateILVariable parameter.Name = "P_" + i; } i++; diff --git a/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs b/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs index e95134ed4..90df1890a 100644 --- a/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs +++ b/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs @@ -2554,7 +2554,7 @@ namespace ICSharpCode.Decompiler.CSharp } if (string.IsNullOrEmpty(pd.Name) && !pd.Type.IsArgList()) { - // needs to be consistent with logic in ILReader.CreateILVarable(ParameterDefinition) + // needs to be consistent with logic in ILReader.CreateILVariable pd.Name = "P_" + i; } if (settings.AnonymousTypes && parameter.Type.ContainsAnonymousType()) diff --git a/ICSharpCode.Decompiler/IL/Transforms/AssignVariableNames.cs b/ICSharpCode.Decompiler/IL/Transforms/AssignVariableNames.cs index 0d1acd261..24f7079fc 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/AssignVariableNames.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/AssignVariableNames.cs @@ -179,7 +179,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms string name = p.Name; if (string.IsNullOrWhiteSpace(name) && p.Type != SpecialType.ArgList) { - // needs to be consistent with logic in ILReader.CreateILVarable + // needs to be consistent with logic in ILReader.CreateILVariable name = "P_" + i; } if (variables.TryGetValue(i, out var v)) @@ -221,6 +221,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms if (variables.TryGetValue(i, out var v)) variableMapping[v] = p.Name; } + else if (variables.TryGetValue(i, out var v)) + { + v.HasGeneratedName = true; + } } } } @@ -287,6 +291,13 @@ namespace ICSharpCode.Decompiler.IL.Transforms return loopCounters.Contains(v) || (parentScope?.IsLoopCounter(v) == true); } + public string AssignNameIfUnassigned(ILVariable v) + { + if (variableMapping.TryGetValue(v, out var name)) + return name; + return AssignName(v); + } + public string AssignName(ILVariable v) { // variable has no valid name @@ -432,57 +443,65 @@ namespace ICSharpCode.Decompiler.IL.Transforms function.AcceptVisitor(this, null); } - protected override Unit Default(ILInstruction inst, VariableScope context) + Unit VisitChildren(ILInstruction inst, VariableScope context) { foreach (var child in inst.Children) { child.AcceptVisitor(this, context); } - if (inst is not IInstructionWithVariableOperand { Variable: var v }) - return default; + return default; + } - // if there is already a valid name for the variable slot, just use it - string name = context.TryGetExistingName(v); - if (!string.IsNullOrEmpty(name)) + protected override Unit Default(ILInstruction inst, VariableScope context) + { + if (inst is IInstructionWithVariableOperand { Variable: var v }) { - v.Name = name; - return default; - } + // if there is already a valid name for the variable slot, just use it + string name = context.TryGetExistingName(v); + if (!string.IsNullOrEmpty(name)) + { + v.Name = name; + return VisitChildren(inst, context); + } - switch (v.Kind) - { - case VariableKind.Parameter when !v.HasGeneratedName && v.Function.Kind == ILFunctionKind.TopLevelFunction: - // Parameter names of top-level functions are handled in ILReader.CreateILVariable - // and CSharpDecompiler.FixParameterNames - break; - case VariableKind.InitializerTarget: // keep generated names - case VariableKind.NamedArgument: - context.ReserveVariableName(v.Name); - break; - case VariableKind.DisplayClassLocal: - v.Name = context.NextDisplayClassLocal(); - break; - case VariableKind.Local when v.Index != null: - name = context.TryGetExistingName(v.Function, v.Index.Value); - if (name != null) - { - // make sure all local ILVariables that refer to the same slot in the locals signature - // are assigned the same name. - v.Name = name; - } - else - { + switch (v.Kind) + { + case VariableKind.Parameter when !v.HasGeneratedName && v.Function.Kind == ILFunctionKind.TopLevelFunction: + // Parameter names of top-level functions are handled in ILReader.CreateILVariable + // and CSharpDecompiler.FixParameterNames + break; + case VariableKind.InitializerTarget: // keep generated names + case VariableKind.NamedArgument: + context.ReserveVariableName(v.Name); + break; + case VariableKind.UsingLocal when v.AddressCount == 0 && v.LoadCount == 0: + // using variables that are not read, will not be declared in source source + break; + case VariableKind.DisplayClassLocal: + v.Name = context.NextDisplayClassLocal(); + break; + case VariableKind.Local when v.Index != null: + name = context.TryGetExistingName(v.Function, v.Index.Value); + if (name != null) + { + // make sure all local ILVariables that refer to the same slot in the locals signature + // are assigned the same name. + v.Name = name; + } + else + { + v.Name = context.AssignName(v); + context.AssignNameToLocalSignatureIndex(v.Function, v.Index.Value, v.Name); + } + break; + default: v.Name = context.AssignName(v); - context.AssignNameToLocalSignatureIndex(v.Function, v.Index.Value, v.Name); - } - break; - default: - v.Name = context.AssignName(v); - break; + break; + } } - return default; + return VisitChildren(inst, context); } protected internal override Unit VisitILFunction(ILFunction function, VariableScope context) @@ -515,7 +534,18 @@ namespace ICSharpCode.Decompiler.IL.Transforms context.Add((MethodDefinitionHandle)function.ReducedMethod.MetadataToken, newName); } - return base.VisitILFunction(function, new VariableScope(function, this.context, context)); + var nestedContext = new VariableScope(function, this.context, context); + base.VisitILFunction(function, nestedContext); + + if (function.Kind != ILFunctionKind.TopLevelFunction) + { + foreach (var p in function.Variables.Where(v => v.Kind == VariableKind.Parameter && v.Index >= 0)) + { + p.Name = nestedContext.AssignNameIfUnassigned(p); + } + } + + return default; } protected internal override Unit VisitCall(Call inst, VariableScope context)