Handle overflow in DesignScript and core nodes (#11075)

mmisol · web-flow · commit b06a168e9699 · 2020-08-31T18:15:28.000-04:00
Operations that may result in an integer overflow in DesignScript code
or core nodes now display a warning when one occurs. The result is
maintained as it was, in order to avoid introducing incompatibilities.
diff --git a/src/DocumentationBrowserViewExtension/Docs/IntegerOverflow.html b/src/DocumentationBrowserViewExtension/Docs/IntegerOverflow.html
diff --git a/src/DocumentationBrowserViewExtension/DocumentationBrowserViewExtension.csproj b/src/DocumentationBrowserViewExtension/DocumentationBrowserViewExtension.csproj
@@ -151,6 +151,9 @@
   <ItemGroup>
     <Folder Include="Resources\" />
   </ItemGroup>
+  <ItemGroup>
+    <EmbeddedResource Include="Docs\IntegerOverflow.html" />
+  </ItemGroup>
   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
   <Target Name="AfterBuild">
     <ItemGroup>
diff --git a/src/Engine/ProtoCore/DSASM/Executive.cs b/src/Engine/ProtoCore/DSASM/Executive.cs
@@ -3530,8 +3530,9 @@ private void ADD_Handler(Instruction instruction)
             // Need to optmize these if-elses to a table. 
             if (opdata1.IsInteger && opdata2.IsInteger)
             {
-                opdata2 = StackValue.BuildInt(opdata1.IntegerValue + opdata2.IntegerValue);
-
+                opdata2 = StackValue.BuildInt(HandleOverflow(
+                    () => checked(opdata1.IntegerValue + opdata2.IntegerValue),
+                    () => opdata1.IntegerValue + opdata2.IntegerValue));
             }
             else if (opdata1.IsNumeric && opdata2.IsNumeric)
             {
@@ -3563,15 +3564,38 @@ private void ADD_Handler(Instruction instruction)
             rmem.Push(opdata2);
             ++pc;
         }
-        
+
+        /// <summary>
+        /// Handles possible overflows from a checked operation. If it works, its result is
+        /// returned, otherwise the result of the unchecked operation is returned and a warning
+        /// is logged.
+        /// </summary>
+        /// <param name="checkedOperation">Checked operation to be attempted first</param>
+        /// <param name="uncheckedOperation">Unchecked operation to be perfomed when the checked operation overflowed</param>
+        /// <returns>The result of the first succesful operation</returns>
+        private long HandleOverflow(Func<long> checkedOperation, Func<long> uncheckedOperation)
+        {
+            try
+            {
+                return checkedOperation();
+            }
+            catch (OverflowException)
+            {
+                runtimeCore.RuntimeStatus.LogWarning(WarningID.IntegerOverflow, string.Format($"{Resources.IntegerOverflow}href=IntegerOverflow.html"));
+                return uncheckedOperation();
+            }
+        }
+
         private void SUB_Handler(Instruction instruction)
         {
             StackValue opdata1 = rmem.Pop();
             StackValue opdata2 = rmem.Pop();
 
             if (opdata1.IsInteger && opdata2.IsInteger)
             {
-                opdata2 = StackValue.BuildInt(opdata2.IntegerValue - opdata1.IntegerValue);
+                opdata2 = StackValue.BuildInt(HandleOverflow(
+                    () => checked(opdata2.IntegerValue - opdata1.IntegerValue),
+                    () => opdata2.IntegerValue - opdata1.IntegerValue));
             }
             else if (opdata1.IsNumeric && opdata2.IsNumeric)
             {
@@ -3595,7 +3619,9 @@ private void MUL_Handler(Instruction instruction)
 
             if (opdata1.IsInteger && opdata2.IsInteger)
             {
-                opdata2 = StackValue.BuildInt(opdata1.IntegerValue * opdata2.IntegerValue);
+                opdata2 = StackValue.BuildInt(HandleOverflow(
+                    () => checked(opdata1.IntegerValue * opdata2.IntegerValue),
+                    () => opdata1.IntegerValue * opdata2.IntegerValue));
             }
             else if (opdata1.IsNumeric && opdata2.IsNumeric)
             {
@@ -3691,7 +3717,9 @@ private void NEG_Handler(Instruction instruction)
             StackValue opdata1 = rmem.Pop();
             if (opdata1.IsInteger)
             {
-                opdata1 = StackValue.BuildInt(-opdata1.IntegerValue);
+                opdata1 = StackValue.BuildInt(HandleOverflow(
+                    () => checked(-opdata1.IntegerValue),
+                    () => -opdata1.IntegerValue));
             }
             else if (opdata1.IsDouble)
             {
diff --git a/src/Engine/ProtoCore/Properties/Resources.Designer.cs b/src/Engine/ProtoCore/Properties/Resources.Designer.cs
diff --git a/src/Engine/ProtoCore/Properties/Resources.en-US.resx b/src/Engine/ProtoCore/Properties/Resources.en-US.resx
@@ -941,4 +941,7 @@ Parameter name: {0}. href=ArgumentNullException.html</value>
   <data name="ListMethodDeprecated" xml:space="preserve">
     <value>Method '{0}' has been deprecated, please use method '{1}' instead with Dictionary type</value>
   </data>
+  <data name="IntegerOverflow" xml:space="preserve">
+    <value>The operation resulted in an integer overflow. Its result may be unexpected.</value>
+  </data>
 </root>
diff --git a/src/Engine/ProtoCore/Properties/Resources.resx b/src/Engine/ProtoCore/Properties/Resources.resx
@@ -944,4 +944,7 @@ Parameter name: {0} href-ArgumentNullException.html</value>
   <data name="ListMethodDeprecated" xml:space="preserve">
     <value>Method '{0}' has been deprecated, please use method '{1}' instead with Dictionary type</value>
   </data>
+  <data name="IntegerOverflow" xml:space="preserve">
+    <value>The operation resulted in an integer overflow. Its result may be unexpected.</value>
+  </data>
 </root>
diff --git a/src/Engine/ProtoCore/RuntimeStatus.cs b/src/Engine/ProtoCore/RuntimeStatus.cs
@@ -37,6 +37,7 @@ public enum WarningID
             MoreThanOneDominantList,
             RunOutOfMemory,
             InvalidArrayIndexType,
+            IntegerOverflow
         }
 
         public struct WarningEntry
diff --git a/src/Libraries/CoreNodes/Math.cs b/src/Libraries/CoreNodes/Math.cs
@@ -4,6 +4,7 @@
 using System.Linq;
 using Autodesk.DesignScript.Runtime;
 using DSCore.Properties;
+using DynamoServices;
 using NCalc;
 using ProtoCore.Utils;
 using CSMath = System.Math;
@@ -280,7 +281,8 @@ public static double Atan2(double numerator, double denominator)
         /// <search>ceiling,round</search>
         public static long Ceiling(double number)
         {
-            return (long)CSMath.Ceiling(number);
+            var ceiling = CSMath.Ceiling(number);
+            return DoCheckedCast(ceiling);
         }
 
         /// <summary>
@@ -360,7 +362,8 @@ public static double Exp(double number)
         /// <search>round</search>
         public static long Floor(double number)
         {
-            return (long)CSMath.Floor(number);
+            var floor = CSMath.Floor(number);
+            return DoCheckedCast(floor);
         }
         
         [IsVisibleInDynamoLibrary(false)]
@@ -643,5 +646,24 @@ public static bool Xor(bool a, bool b)
         }
 
         private static readonly Random mRandom = new Random();
+
+        /// <summary>
+        /// Performs a cast to long in a checked context. If the operation produces an overflow,
+        /// then a warning is produced and the overflowed result is returned.
+        /// </summary>
+        /// <param name="value">Operation to be performed</param>
+        /// <returns>The result of the operation</returns>
+        private static long DoCheckedCast(double value)
+        {
+            try
+            {
+                return checked((long)value);
+            }
+            catch (OverflowException)
+            {
+                LogWarningMessageEvents.OnLogWarningMessage(string.Format($"{Properties.Resources.IntegerOverflow}href=IntegerOverflow.html"));
+                return (long)value;
+            }
+        }
     }
 }
diff --git a/src/Libraries/CoreNodes/Properties/Resources.Designer.cs b/src/Libraries/CoreNodes/Properties/Resources.Designer.cs
diff --git a/src/Libraries/CoreNodes/Properties/Resources.en-US.resx b/src/Libraries/CoreNodes/Properties/Resources.en-US.resx
@@ -156,6 +156,9 @@
   <data name="FromObjectObsolete" xml:space="preserve">
     <value>This node is obsolete, please use "String from Object"</value>
   </data>
+  <data name="IntegerOverflow" xml:space="preserve">
+    <value>The operation resulted in an integer overflow. Its result may be unexpected.</value>
+  </data>
   <data name="InvalidKeysErrorMessage" xml:space="preserve">
     <value>One or more input types are not matching. Lists as keys are not supported.</value>
   </data>
diff --git a/src/Libraries/CoreNodes/Properties/Resources.resx b/src/Libraries/CoreNodes/Properties/Resources.resx
@@ -156,6 +156,9 @@
   <data name="FromObjectObsolete" xml:space="preserve">
     <value>This node is obsolete, please use "String from Object"</value>
   </data>
+  <data name="IntegerOverflow" xml:space="preserve">
+    <value>The operation resulted in an integer overflow. Its result may be unexpected.</value>
+  </data>
   <data name="InvalidKeysErrorMessage" xml:space="preserve">
     <value>One or more input types are not matching. Lists as keys are not supported.</value>
   </data>
diff --git a/test/DynamoCoreTests/CodeBlockNodeTests.cs b/test/DynamoCoreTests/CodeBlockNodeTests.cs
@@ -1256,7 +1256,68 @@ public void TestEscapeSequenceAtEnd()
             int outportBrokenNodeCount = helloBrokenNode.OutPorts.Count;
             Assert.IsFalse(outportCount > 0);
         }
-        
+
+        /// <summary>
+        /// Tests a variety of DS operations and builtin nodes that can result in an
+        /// integer overflow.
+        /// </summary>
+        [Test]
+        public void IntegerOverflow()
+        {
+            OpenModel(Path.Combine(TestDirectory, @"core\dsevaluation\Overflow.dyn"));
+
+            var dsSum = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "DS Sum Overflow");
+            var dsSub = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "DS Sub Overflow");
+            var dsNeg = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "DS Neg Overflow");
+            var dsMul = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "DS Mul Overflow");
+            var mathFactorial = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "Math.Factorial Overflow");
+            var mathAbs = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "Math.Abs Overflow");
+            var mathFloor = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "Math.Floor Overflow");
+            var mathCeiling = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault(n => n.Name == "Math.Ceiling Overflow");
+
+            var expectedWarning = "The operation resulted in an integer overflow. Its result may be unexpected.href=IntegerOverflow.html";
+
+            Assert.IsNotNull(dsSum);
+            Assert.AreEqual(ElementState.Warning, dsSum.State);
+            Assert.AreEqual(expectedWarning, dsSum.ToolTipText);
+            AssertPreviewValue(dsSum.AstIdentifierGuid, -2);
+
+            Assert.IsNotNull(dsSub);
+            Assert.AreEqual(ElementState.Warning, dsSub.State);
+            Assert.AreEqual(expectedWarning, dsSub.ToolTipText);
+            AssertPreviewValue(dsSub.AstIdentifierGuid, 2);
+
+            Assert.IsNotNull(dsNeg);
+            Assert.AreEqual(ElementState.Warning, dsNeg.State);
+            Assert.AreEqual(expectedWarning, dsNeg.ToolTipText);
+            AssertPreviewValue(dsNeg.AstIdentifierGuid, long.MaxValue);
+
+            Assert.IsNotNull(dsMul);
+            Assert.AreEqual(ElementState.Warning, dsMul.State);
+            Assert.AreEqual(expectedWarning, dsMul.ToolTipText);
+            AssertPreviewValue(dsMul.AstIdentifierGuid, -2);
+
+            Assert.IsNotNull(mathFactorial);
+            Assert.AreEqual(ElementState.Warning, mathFactorial.State);
+            StringAssert.EndsWith("The return value of Math.Factorial is out of range.", mathFactorial.ToolTipText);
+            AssertPreviewValue(mathFactorial.AstIdentifierGuid, null);
+
+            Assert.IsNotNull(mathAbs);
+            Assert.AreEqual(ElementState.Warning, mathAbs.State);
+            StringAssert.EndsWith("Negating the minimum value of a twos complement number is invalid.", mathAbs.ToolTipText);
+            AssertPreviewValue(mathAbs.AstIdentifierGuid, null);
+
+            Assert.IsNotNull(mathFloor);
+            Assert.AreEqual(ElementState.Warning, mathFloor.State);
+            Assert.AreEqual(expectedWarning, mathFloor.ToolTipText);
+            AssertPreviewValue(mathFloor.AstIdentifierGuid, long.MinValue);
+
+            Assert.IsNotNull(mathCeiling);
+            Assert.AreEqual(ElementState.Warning, mathCeiling.State);
+            Assert.AreEqual(expectedWarning, mathCeiling.ToolTipText);
+            AssertPreviewValue(mathCeiling.AstIdentifierGuid, long.MinValue);
+        }
+
         #region CodeBlockUtils Specific Tests
         [Test]
         [Category("UnitTests")]
diff --git a/test/core/dsevaluation/Overflow.dyn b/test/core/dsevaluation/Overflow.dyn

Original file line number	Diff line number	Diff line change
`@@ -3530,8 +3530,9 @@ private void ADD_Handler(Instruction instruction)`
`3530`	`3530`	`// Need to optmize these if-elses to a table.`
`3531`	`3531`	`if (opdata1.IsInteger && opdata2.IsInteger)`
`3532`	`3532`	`{`
`3533`		`- opdata2 = StackValue.BuildInt(opdata1.IntegerValue + opdata2.IntegerValue);`
`3534`		`-`
	`3533`	`+ opdata2 = StackValue.BuildInt(HandleOverflow(`
	`3534`	`+ () => checked(opdata1.IntegerValue + opdata2.IntegerValue),`
	`3535`	`+ () => opdata1.IntegerValue + opdata2.IntegerValue));`
`3535`	`3536`	`}`
`3536`	`3537`	`else if (opdata1.IsNumeric && opdata2.IsNumeric)`
`3537`	`3538`	`{`
`@@ -3563,15 +3564,38 @@ private void ADD_Handler(Instruction instruction)`
`3563`	`3564`	`rmem.Push(opdata2);`
`3564`	`3565`	`++pc;`
`3565`	`3566`	`}`
`3566`		`-`
	`3567`	`+`
	`3568`	`+ /// <summary>`
	`3569`	`+ /// Handles possible overflows from a checked operation. If it works, its result is`
	`3570`	`+ /// returned, otherwise the result of the unchecked operation is returned and a warning`
	`3571`	`+ /// is logged.`
	`3572`	`+ /// </summary>`
	`3573`	`+ /// <param name="checkedOperation">Checked operation to be attempted first</param>`
	`3574`	`+ /// <param name="uncheckedOperation">Unchecked operation to be perfomed when the checked operation overflowed</param>`
	`3575`	`+ /// <returns>The result of the first succesful operation</returns>`
	`3576`	`+ private long HandleOverflow(Func<long> checkedOperation, Func<long> uncheckedOperation)`
	`3577`	`+ {`
	`3578`	`+ try`
	`3579`	`+ {`
	`3580`	`+ return checkedOperation();`
	`3581`	`+ }`
	`3582`	`+ catch (OverflowException)`
	`3583`	`+ {`
	`3584`	`+ runtimeCore.RuntimeStatus.LogWarning(WarningID.IntegerOverflow, string.Format($"{Resources.IntegerOverflow}href=IntegerOverflow.html"));`
	`3585`	`+ return uncheckedOperation();`
	`3586`	`+ }`
	`3587`	`+ }`
	`3588`	`+`
`3567`	`3589`	`private void SUB_Handler(Instruction instruction)`
`3568`	`3590`	`{`
`3569`	`3591`	`StackValue opdata1 = rmem.Pop();`
`3570`	`3592`	`StackValue opdata2 = rmem.Pop();`
`3571`	`3593`
`3572`	`3594`	`if (opdata1.IsInteger && opdata2.IsInteger)`
`3573`	`3595`	`{`
`3574`		`- opdata2 = StackValue.BuildInt(opdata2.IntegerValue - opdata1.IntegerValue);`
	`3596`	`+ opdata2 = StackValue.BuildInt(HandleOverflow(`
	`3597`	`+ () => checked(opdata2.IntegerValue - opdata1.IntegerValue),`
	`3598`	`+ () => opdata2.IntegerValue - opdata1.IntegerValue));`
`3575`	`3599`	`}`
`3576`	`3600`	`else if (opdata1.IsNumeric && opdata2.IsNumeric)`
`3577`	`3601`	`{`
`@@ -3595,7 +3619,9 @@ private void MUL_Handler(Instruction instruction)`
`3595`	`3619`
`3596`	`3620`	`if (opdata1.IsInteger && opdata2.IsInteger)`
`3597`	`3621`	`{`
`3598`		`- opdata2 = StackValue.BuildInt(opdata1.IntegerValue * opdata2.IntegerValue);`
	`3622`	`+ opdata2 = StackValue.BuildInt(HandleOverflow(`
	`3623`	`+ () => checked(opdata1.IntegerValue * opdata2.IntegerValue),`
	`3624`	`+ () => opdata1.IntegerValue * opdata2.IntegerValue));`
`3599`	`3625`	`}`
`3600`	`3626`	`else if (opdata1.IsNumeric && opdata2.IsNumeric)`
`3601`	`3627`	`{`
`@@ -3691,7 +3717,9 @@ private void NEG_Handler(Instruction instruction)`
`3691`	`3717`	`StackValue opdata1 = rmem.Pop();`
`3692`	`3718`	`if (opdata1.IsInteger)`
`3693`	`3719`	`{`
`3694`		`- opdata1 = StackValue.BuildInt(-opdata1.IntegerValue);`
	`3720`	`+ opdata1 = StackValue.BuildInt(HandleOverflow(`
	`3721`	`+ () => checked(-opdata1.IntegerValue),`
	`3722`	`+ () => -opdata1.IntegerValue));`
`3695`	`3723`	`}`
`3696`	`3724`	`else if (opdata1.IsDouble)`
`3697`	`3725`	`{`
Original file line number	Diff line number	Diff line change
`@@ -37,6 +37,7 @@ public enum WarningID`
`37`	`37`	`MoreThanOneDominantList,`
`38`	`38`	`RunOutOfMemory,`
`39`	`39`	`InvalidArrayIndexType,`
	`40`	`+ IntegerOverflow`
`40`	`41`	`}`
`41`	`42`
`42`	`43`	`public struct WarningEntry`