From 9b3692eda27a7b607842b4bbbf5672af14d459a7 Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Fri, 27 Sep 2019 22:13:07 +0200 Subject: [PATCH 1/4] Use .NET Core 3.0 final SDK on Azure Pipelines --- azure-pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 707a6de24..3c1755b9a 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -39,7 +39,7 @@ jobs: - task: DotNetCoreInstaller@0 inputs: - version: '3.0.100-preview9-014004' + version: '3.0.100' - powershell: .\BuildTools\pipelines-install.ps1 displayName: Install From 115f8ab0d9485f61a31bbf3dd989070147bb84d0 Mon Sep 17 00:00:00 2001 From: Andreas Weizel Date: Fri, 27 Sep 2019 22:28:40 +0200 Subject: [PATCH 2/4] AddIn: Pass /navigateTo:none to ILSpy as command-line parameter --- ILSpy.AddIn/ILSpyInstance.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ILSpy.AddIn/ILSpyInstance.cs b/ILSpy.AddIn/ILSpyInstance.cs index 0cc23f773..bd8c56cb7 100644 --- a/ILSpy.AddIn/ILSpyInstance.cs +++ b/ILSpy.AddIn/ILSpyInstance.cs @@ -42,7 +42,8 @@ namespace ICSharpCode.ILSpy.AddIn var process = new Process() { StartInfo = new ProcessStartInfo() { FileName = GetILSpyPath(), - UseShellExecute = false + UseShellExecute = false, + Arguments = "/navigateTo:none" } }; process.Start(); From b45f21e71430d2fb6921a643989fe5b936c3b37a Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 27 Sep 2019 22:33:49 +0200 Subject: [PATCH 3/4] #1675: Fix converting static method to method group --- .../Ugly/NoExtensionMethods.Expected.cs | 10 ++++++ .../TestCases/Ugly/NoExtensionMethods.cs | 10 ++++++ .../Ugly/NoExtensionMethods.opt.roslyn.il | 24 +++++++++++++ .../Ugly/NoExtensionMethods.roslyn.il | 36 +++++++++++++++++++ ICSharpCode.Decompiler/CSharp/CallBuilder.cs | 30 ++++++++++++++-- 5 files changed, 108 insertions(+), 2 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.Expected.cs b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.Expected.cs index dfe0e184b..72d6c3b3e 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.Expected.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.Expected.cs @@ -19,5 +19,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly { return value; } + + internal static Func ExtensionMethodAsStaticFunc() + { + return Return; + } + + internal static Func ExtensionMethodBoundToNull() + { + return new Func(null, __ldftn(Return)); + } } } diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.cs b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.cs index 61b9b43bb..294b71938 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.cs @@ -13,5 +13,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly { return value; } + + internal static Func ExtensionMethodAsStaticFunc() + { + return Return; + } + + internal static Func ExtensionMethodBoundToNull() + { + return ((object)null).Return; + } } } diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.opt.roslyn.il b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.opt.roslyn.il index 41b3e7b87..12d81400b 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.opt.roslyn.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.opt.roslyn.il @@ -61,6 +61,30 @@ IL_0001: ret } // end of method NoExtensionMethods::Return + .method assembly hidebysig static class [mscorlib]System.Func`2 + ExtensionMethodAsStaticFunc() cil managed + { + // Code size 13 (0xd) + .maxstack 8 + IL_0000: ldnull + IL_0001: ldftn !!0 ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods::Return(!!0) + IL_0007: newobj instance void class [mscorlib]System.Func`2::.ctor(object, + native int) + IL_000c: ret + } // end of method NoExtensionMethods::ExtensionMethodAsStaticFunc + + .method assembly hidebysig static class [mscorlib]System.Func`1 + ExtensionMethodBoundToNull() cil managed + { + // Code size 13 (0xd) + .maxstack 8 + IL_0000: ldnull + IL_0001: ldftn !!0 ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods::Return(!!0) + IL_0007: newobj instance void class [mscorlib]System.Func`1::.ctor(object, + native int) + IL_000c: ret + } // end of method NoExtensionMethods::ExtensionMethodBoundToNull + } // end of class ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.roslyn.il b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.roslyn.il index 60af94216..a740ada26 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.roslyn.il +++ b/ICSharpCode.Decompiler.Tests/TestCases/Ugly/NoExtensionMethods.roslyn.il @@ -73,6 +73,42 @@ IL_0006: ret } // end of method NoExtensionMethods::Return + .method assembly hidebysig static class [mscorlib]System.Func`2 + ExtensionMethodAsStaticFunc() cil managed + { + // Code size 18 (0x12) + .maxstack 2 + .locals init (class [mscorlib]System.Func`2 V_0) + IL_0000: nop + IL_0001: ldnull + IL_0002: ldftn !!0 ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods::Return(!!0) + IL_0008: newobj instance void class [mscorlib]System.Func`2::.ctor(object, + native int) + IL_000d: stloc.0 + IL_000e: br.s IL_0010 + + IL_0010: ldloc.0 + IL_0011: ret + } // end of method NoExtensionMethods::ExtensionMethodAsStaticFunc + + .method assembly hidebysig static class [mscorlib]System.Func`1 + ExtensionMethodBoundToNull() cil managed + { + // Code size 18 (0x12) + .maxstack 2 + .locals init (class [mscorlib]System.Func`1 V_0) + IL_0000: nop + IL_0001: ldnull + IL_0002: ldftn !!0 ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods::Return(!!0) + IL_0008: newobj instance void class [mscorlib]System.Func`1::.ctor(object, + native int) + IL_000d: stloc.0 + IL_000e: br.s IL_0010 + + IL_0010: ldloc.0 + IL_0011: ret + } // end of method NoExtensionMethods::ExtensionMethodBoundToNull + } // end of class ICSharpCode.Decompiler.Tests.TestCases.Ugly.NoExtensionMethods diff --git a/ICSharpCode.Decompiler/CSharp/CallBuilder.cs b/ICSharpCode.Decompiler/CSharp/CallBuilder.cs index 7b6c1b2bf..f7e2b5a67 100644 --- a/ICSharpCode.Decompiler/CSharp/CallBuilder.cs +++ b/ICSharpCode.Decompiler/CSharp/CallBuilder.cs @@ -1235,12 +1235,38 @@ namespace ICSharpCode.Decompiler.CSharp default: throw new ArgumentException($"Unknown instruction type: {func.OpCode}"); } - if (method.IsStatic && !method.IsExtensionMethod) { + if (CanUseDelegateConstruction(method, thisArg, inst.Method.DeclaringType.GetDelegateInvokeMethod())) { + return HandleDelegateConstruction(inst.Method.DeclaringType, method, expectedTargetDetails, thisArg, inst); + } else { var argumentList = BuildArgumentList(expectedTargetDetails, null, inst.Method, 0, inst.Arguments, null); return HandleConstructorCall(new ExpectedTargetDetails { CallOpCode = OpCode.NewObj }, null, inst.Method, argumentList).WithILInstruction(inst); } - return HandleDelegateConstruction(inst.Method.DeclaringType, method, expectedTargetDetails, thisArg, inst); + } + + private bool CanUseDelegateConstruction(IMethod targetMethod, ILInstruction thisArg, IMethod invokeMethod) + { + if (targetMethod.IsStatic) { + // If the invoke method is known, we can compare the parameter counts to figure out whether the + // delegate is static or binds the first argument + if (invokeMethod != null) { + if (invokeMethod.Parameters.Count == targetMethod.Parameters.Count) { + return thisArg.MatchLdNull(); + } else if (targetMethod.IsExtensionMethod && invokeMethod.Parameters.Count == targetMethod.Parameters.Count - 1) { + return true; + } else { + return false; + } + } else { + // delegate type unknown: + return thisArg.MatchLdNull() || targetMethod.IsExtensionMethod; + } + } else { + // targetMethod is instance method + if (invokeMethod != null && invokeMethod.Parameters.Count != targetMethod.Parameters.Count) + return false; + return true; + } } internal TranslatedExpression Build(LdVirtDelegate inst) From c32361d46440112815d18cb1a672652550ff0d4f Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Fri, 27 Sep 2019 23:37:32 +0200 Subject: [PATCH 4/4] #1691: Avoid replacing string.Concat() with operator+ when the evaluation order depends on the compiler version. --- .../CorrectnessTestRunner.cs | 6 ++ .../ICSharpCode.Decompiler.Tests.csproj | 1 + .../TestCases/Correctness/StringConcat.cs | 57 ++++++++++++++ .../ReplaceMethodCallsWithOperators.cs | 78 ++++++++++++++++++- 4 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/TestCases/Correctness/StringConcat.cs diff --git a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs index 6d75516da..4ad298c7c 100644 --- a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs @@ -280,6 +280,12 @@ namespace ICSharpCode.Decompiler.Tests RunCS(options: options); } + [Test] + public void StringConcat([ValueSource("defaultOptions")] CompilerOptions options) + { + RunCS(options: options); + } + [Test] public void MiniJSON([ValueSource("defaultOptions")] CompilerOptions options) { diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 1b7aedd7e..dad38279b 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -79,6 +79,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/StringConcat.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/StringConcat.cs new file mode 100644 index 000000000..229c9ef6a --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/StringConcat.cs @@ -0,0 +1,57 @@ +using System; + +namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness +{ + class StringConcat + { + private class C + { + readonly int i; + + public C(int i) + { + Console.WriteLine(" new C(" + i + ")"); + this.i = i; + } + + public override string ToString() + { + Console.WriteLine(" C(" + i + ").ToString()"); + return i.ToString(); + } + } + + static string Space() + { + Console.WriteLine(" Space()"); + return " "; + } + + static void Main() + { + Console.WriteLine("string + C:"); + Console.WriteLine(Space() + new C(1)); + + Console.WriteLine("C + string:"); + Console.WriteLine(new C(2) + Space()); + + Console.WriteLine("C + string + C:"); + Console.WriteLine(new C(3) + Space() + new C(4)); + + Console.WriteLine("string + C + C:"); + Console.WriteLine(Space() + new C(5) + new C(6)); + + Console.WriteLine("string.Concat(C, string, C):"); + Console.WriteLine(string.Concat(new C(10), Space(), new C(11))); + + Console.WriteLine("string.Concat(string.Concat(C, string), C):"); + Console.WriteLine(string.Concat(string.Concat(new C(15), Space()), new C(16))); + + Console.WriteLine("string.Concat(C, string.Concat(string, C)):"); + Console.WriteLine(string.Concat(new C(20), string.Concat(Space(), new C(21)))); + + Console.WriteLine("string.Concat(C, string) + C:"); + Console.WriteLine(string.Concat(new C(30), Space()) + new C(31)); + } + } +} diff --git a/ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs b/ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs index 9d9838d71..d7a081e99 100644 --- a/ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs +++ b/ICSharpCode.Decompiler/CSharp/Transforms/ReplaceMethodCallsWithOperators.cs @@ -23,7 +23,9 @@ using System.Reflection; using System.Text; using ICSharpCode.Decompiler.CSharp.Syntax; using ICSharpCode.Decompiler.CSharp.Syntax.PatternMatching; +using ICSharpCode.Decompiler.Semantics; using ICSharpCode.Decompiler.TypeSystem; +using ICSharpCode.Decompiler.Util; namespace ICSharpCode.Decompiler.CSharp.Transforms { @@ -56,7 +58,7 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms var arguments = invocationExpression.Arguments.ToArray(); // Reduce "String.Concat(a, b)" to "a + b" - if (method.Name == "Concat" && method.DeclaringType.FullName == "System.String" && CheckArgumentsForStringConcat(arguments)) { + if (IsStringConcat(method) && CheckArgumentsForStringConcat(arguments)) { invocationExpression.Arguments.Clear(); // detach arguments from invocationExpression Expression expr = arguments[0]; for (int i = 1; i < arguments.Length; i++) { @@ -174,9 +176,77 @@ namespace ICSharpCode.Decompiler.CSharp.Transforms if (arguments.Length < 2) return false; - return !arguments.Any(arg => arg is NamedArgumentExpression) && - (arguments[0].GetResolveResult().Type.IsKnownType(KnownTypeCode.String) || - arguments[1].GetResolveResult().Type.IsKnownType(KnownTypeCode.String)); + if (arguments.Any(arg => arg is NamedArgumentExpression)) + return false; + + // The evaluation order when the object.ToString() calls happen is a mess: + // The C# spec says the evaluation for order for each individual string + should be: + // * evaluate left argument + // * evaluate right argument + // * call ToString() on object argument + // What actually happens pre-VS2019.3: + // * evaluate all arguments in chain of + operators from left to right + // * call ToString() on all object arguments from left to right + // What happens in VS2019.3: + // * for each argument in chain of + operators fom left to right: + // * evaluate argument + // * call ToString() on object argument + // See https://github.com/dotnet/roslyn/issues/38641 for details. + // To ensure the decompiled code's behavior matches the original IL behavior, + // no matter which compiler is used to recompile it, we require that all + // implicit ToString() calls except for the last are free of side effects. + foreach (var arg in arguments.SkipLast(1)) { + if (!ToStringIsKnownEffectFree(arg.GetResolveResult().Type)) { + return false; + } + } + foreach (var arg in arguments) { + if (arg.GetResolveResult() is InvocationResolveResult rr && IsStringConcat(rr.Member)) { + // Roslyn + mcs also flatten nested string.Concat() invocations within a operator+ use, + // which causes it to use the incorrect evaluation order despite the code using an + // explicit string.Concat() call. + // This problem is avoided if the outer call remains string.Concat() as well. + return false; + } + } + + // One of the first two arguments must be string, otherwise the + operator + // won't resolve to a string concatenation. + return arguments[0].GetResolveResult().Type.IsKnownType(KnownTypeCode.String) + || arguments[1].GetResolveResult().Type.IsKnownType(KnownTypeCode.String); + } + + private bool IsStringConcat(IParameterizedMember member) + { + return member is IMethod method + && method.Name == "Concat" + && method.DeclaringType.IsKnownType(KnownTypeCode.String); + } + + static bool ToStringIsKnownEffectFree(IType type) + { + type = NullableType.GetUnderlyingType(type); + switch (type.GetDefinition()?.KnownTypeCode) { + case KnownTypeCode.Boolean: + case KnownTypeCode.Char: + case KnownTypeCode.SByte: + case KnownTypeCode.Byte: + case KnownTypeCode.Int16: + case KnownTypeCode.UInt16: + case KnownTypeCode.Int32: + case KnownTypeCode.UInt32: + case KnownTypeCode.Int64: + case KnownTypeCode.UInt64: + case KnownTypeCode.Single: + case KnownTypeCode.Double: + case KnownTypeCode.Decimal: + case KnownTypeCode.IntPtr: + case KnownTypeCode.UIntPtr: + case KnownTypeCode.String: + return true; + default: + return false; + } } static BinaryOperatorType? GetBinaryOperatorTypeFromMetadataName(string name)