From 3c31e100ad0ffba099799e4a4352141c674b0773 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Mon, 18 Sep 2017 23:22:11 +0200 Subject: [PATCH] [nullables] Fix nullable transform being applied incorrectly. --- .../CorrectnessTestRunner.cs | 6 ++ .../ICSharpCode.Decompiler.Tests.csproj | 2 + .../TestCases/Correctness/NullableTests.cs | 58 ++++++++++++++++ .../Util/BitSetTests.cs | 68 +++++++++++++++++++ .../IL/Transforms/NullableLiftingTransform.cs | 55 ++++++++++++--- ICSharpCode.Decompiler/Util/BitSet.cs | 30 +++++++- 6 files changed, 206 insertions(+), 13 deletions(-) create mode 100644 ICSharpCode.Decompiler.Tests/TestCases/Correctness/NullableTests.cs create mode 100644 ICSharpCode.Decompiler.Tests/Util/BitSetTests.cs diff --git a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs index d313764b0..6d635593f 100644 --- a/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs +++ b/ICSharpCode.Decompiler.Tests/CorrectnessTestRunner.cs @@ -95,6 +95,12 @@ namespace ICSharpCode.Decompiler.Tests RunCS(options: options); } + [Test] + public void NullableTests([ValueSource("defaultOptions")] CompilerOptions options) + { + RunCS(options: options); + } + [Test] public void Generics([ValueSource("defaultOptions")] CompilerOptions options) { diff --git a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj index 6eb934819..e1624ad4f 100644 --- a/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj +++ b/ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj @@ -56,6 +56,7 @@ + @@ -97,6 +98,7 @@ + diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/NullableTests.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/NullableTests.cs new file mode 100644 index 000000000..f4a3ed8d7 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/NullableTests.cs @@ -0,0 +1,58 @@ +// Copyright (c) 2017 Daniel Grunwald +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System; + +namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness +{ + class NullableTests + { + static void Main() + { + Console.WriteLine("MayThrow:"); + Console.WriteLine(MayThrow(10, 2, 3)); + Console.WriteLine(MayThrow(10, 0, null)); + + Console.WriteLine("NotUsingAllInputs:"); + Console.WriteLine(NotUsingAllInputs(5, 3)); + Console.WriteLine(NotUsingAllInputs(5, null)); + + Console.WriteLine("UsingUntestedValue:"); + Console.WriteLine(NotUsingAllInputs(5, 3)); + Console.WriteLine(NotUsingAllInputs(5, null)); + } + + static int? MayThrow(int? a, int? b, int? c) + { + // cannot be lifted without changing the exception behavior + return a.HasValue & b.HasValue & c.HasValue ? a.GetValueOrDefault() / b.GetValueOrDefault() + c.GetValueOrDefault() : default(int?); + } + + static int? NotUsingAllInputs(int? a, int? b) + { + // cannot be lifted because the value differs if b == null + return a.HasValue & b.HasValue ? a.GetValueOrDefault() + a.GetValueOrDefault() : default(int?); + } + + static int? UsingUntestedValue(int? a, int? b) + { + // cannot be lifted because the value differs if b == null + return a.HasValue ? a.GetValueOrDefault() + b.GetValueOrDefault() : default(int?); + } + } +} diff --git a/ICSharpCode.Decompiler.Tests/Util/BitSetTests.cs b/ICSharpCode.Decompiler.Tests/Util/BitSetTests.cs new file mode 100644 index 000000000..66cf10636 --- /dev/null +++ b/ICSharpCode.Decompiler.Tests/Util/BitSetTests.cs @@ -0,0 +1,68 @@ +// Copyright (c) 2017 Daniel Grunwald +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using ICSharpCode.Decompiler.Util; +using NUnit.Framework; + +namespace ICSharpCode.Decompiler.Tests.Util +{ + [TestFixture] + public class BitSetTests + { + [Test] + public void SetRange() + { + var bitset = new BitSet(302); + bitset.Set(2, 300); + Assert.IsFalse(bitset[0]); + Assert.IsFalse(bitset[1]); + for (int i = 2; i < 300; ++i) { + Assert.IsTrue(bitset[i]); + } + Assert.IsFalse(bitset[301]); + } + + [Test] + public void ClearRange() + { + var bitset = new BitSet(300); + bitset.Set(0, 300); + bitset.Clear(1, 299); + Assert.IsTrue(bitset[0]); + for (int i = 1; i < 299; ++i) { + Assert.IsFalse(bitset[i]); + } + Assert.IsTrue(bitset[299]); + } + + [Test] + public void AllInRange() + { + var bitset = new BitSet(300); + bitset.Set(1, 299); + Assert.IsTrue(bitset.All(1, 299)); + Assert.IsTrue(bitset.All(10, 290)); + Assert.IsTrue(bitset.All(100, 200)); + Assert.IsFalse(bitset.All(0, 200)); + Assert.IsFalse(bitset.All(0, 1)); + Assert.IsFalse(bitset.All(1, 300)); + bitset[200] = false; + Assert.IsFalse(bitset.All(1, 299)); + } + } +} diff --git a/ICSharpCode.Decompiler/IL/Transforms/NullableLiftingTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/NullableLiftingTransform.cs index 0af2e6357..db6c07dd6 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/NullableLiftingTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/NullableLiftingTransform.cs @@ -20,6 +20,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; using ICSharpCode.Decompiler.TypeSystem; +using ICSharpCode.Decompiler.Util; namespace ICSharpCode.Decompiler.IL.Transforms { @@ -153,7 +154,12 @@ namespace ICSharpCode.Decompiler.IL.Transforms }; } else { context.Step("NullableLiftingTransform.DoLift", ifInst); - lifted = DoLift(exprToLift); + BitSet bits; + (lifted, bits) = DoLift(exprToLift); + if (lifted != null && !bits.All(0, nullableVars.Count)) { + // don't lift if a nullableVar doesn't contribute to the result + lifted = null; + } } if (lifted != null) { Debug.Assert(lifted is ILiftableInstruction liftable && liftable.IsLifted @@ -164,22 +170,39 @@ namespace ICSharpCode.Decompiler.IL.Transforms // Lifts the specified instruction. // Creates a new lifted instruction without modifying the input instruction. - // If lifting fails, returns null. - ILInstruction DoLift(ILInstruction inst) + // Returns (new lifted instruction, bitset of nullableVars that will cause the expression to evaluate to null). + // If lifting fails, returns (null, null). + (ILInstruction, BitSet) DoLift(ILInstruction inst) { - if (MatchGetValueOrDefault(inst, out ILVariable inputVar) && nullableVars.Contains(inputVar)) { + if (MatchGetValueOrDefault(inst, out ILVariable inputVar)) { // n.GetValueOrDefault() lifted => n. - return new LdLoc(inputVar) { ILRange = inst.ILRange }; + BitSet foundIndices = new BitSet(nullableVars.Count); + for (int i = 0; i < nullableVars.Count; i++) { + if (nullableVars[i] == inputVar) { + foundIndices[i] = true; + } + } + if (foundIndices.Any()) + return (new LdLoc(inputVar) { ILRange = inst.ILRange }, foundIndices); + else + return (null, null); } else if (inst is Conv conv) { - var arg = DoLift(conv.Argument); + var (arg, bits) = DoLift(conv.Argument); if (arg != null) { - return new Conv(arg, conv.InputType, conv.InputSign, conv.TargetType, conv.CheckForOverflow, isLifted: true) { + if (conv.HasFlag(InstructionFlags.MayThrow) && !bits.All(0, nullableVars.Count)) { + // Cannot execute potentially-throwing instruction unless all + // the nullableVars are arguments to the instruction + // (thus causing it not to throw when any of them is null). + return (null, null); + } + var newInst = new Conv(arg, conv.InputType, conv.InputSign, conv.TargetType, conv.CheckForOverflow, isLifted: true) { ILRange = conv.ILRange }; + return (newInst, bits); } } else if (inst is BinaryNumericInstruction binary) { - var left = DoLift(binary.Left); - var right = DoLift(binary.Right); + var (left, leftBits) = DoLift(binary.Left); + var (right, rightBits) = DoLift(binary.Right); if (left != null && right == null && SemanticHelper.IsPure(binary.Right.Flags)) { // Embed non-nullable pure expression in lifted expression. right = binary.Right.Clone(); @@ -189,7 +212,16 @@ namespace ICSharpCode.Decompiler.IL.Transforms left = binary.Left.Clone(); } if (left != null && right != null) { - return new BinaryNumericInstruction( + var bits = leftBits ?? rightBits; + if (rightBits != null) + bits.UnionWith(rightBits); + if (binary.HasFlag(InstructionFlags.MayThrow) && !bits.All(0, nullableVars.Count)) { + // Cannot execute potentially-throwing instruction unless all + // the nullableVars are arguments to the instruction + // (thus causing it not to throw when any of them is null). + return (null, null); + } + var newInst = new BinaryNumericInstruction( binary.Operator, left, right, binary.LeftInputType, binary.RightInputType, binary.CheckForOverflow, binary.Sign, @@ -197,9 +229,10 @@ namespace ICSharpCode.Decompiler.IL.Transforms ) { ILRange = binary.ILRange }; + return (newInst, bits); } } - return null; + return (null, null); } #endregion diff --git a/ICSharpCode.Decompiler/Util/BitSet.cs b/ICSharpCode.Decompiler/Util/BitSet.cs index 8a00021e6..a9e9deddf 100644 --- a/ICSharpCode.Decompiler/Util/BitSet.cs +++ b/ICSharpCode.Decompiler/Util/BitSet.cs @@ -80,7 +80,33 @@ namespace ICSharpCode.Decompiler.Util } return false; } - + + /// + /// Gets whether all bits in the specified range are set. + /// + public bool All(int startIndex, int endIndex) + { + Debug.Assert(startIndex <= endIndex); + if (startIndex >= endIndex) { + return true; + } + int startWordIndex = WordIndex(startIndex); + int endWordIndex = WordIndex(endIndex - 1); + ulong startMask = Mask << startIndex; + ulong endMask = Mask >> -endIndex; // same as (Mask >> (64 - (endIndex % 64))) + if (startWordIndex == endWordIndex) { + return (words[startWordIndex] & (startMask & endMask)) == (startMask & endMask); + } else { + if ((words[startWordIndex] & startMask) != startMask) + return false; + for (int i = startWordIndex + 1; i < endWordIndex; i++) { + if (words[i] != ulong.MaxValue) + return false; + } + return (words[endWordIndex] & endMask) == endMask; + } + } + /// /// Gets whether both bitsets have the same content. /// @@ -174,7 +200,7 @@ namespace ICSharpCode.Decompiler.Util } else { words[startWordIndex] |= startMask; for (int i = startWordIndex + 1; i < endWordIndex; i++) { - words[i] = 0; + words[i] = ulong.MaxValue; } words[endWordIndex] |= endMask; }