Fix warning message for indexing into lists, display fully qualified classname in warnings (#9861)

aparajit-pratap · web-flow · commit 078692133d68 · 2019-07-29T19:30:42.000-04:00
* fix warning message for indexing into lists, display fully qualified classname in warnings

* display fully qualified class names in type mismatch and method resolution warnings

* add resource strings for null reference exceptions with lists, strings and dictionaries

* add comments
diff --git a/src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs b/src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs
@@ -355,6 +355,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogSemanticError(ex.InnerException.Message);
                 }
+
                 dsi.LogSemanticError(ex.Message);
             }
             catch (System.Reflection.TargetException ex)
@@ -363,6 +364,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);
                 }
+
                 dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);
             }
             catch (System.Reflection.TargetInvocationException ex)
@@ -374,9 +376,9 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                     if (exception != null)
                     {
                         var innerMessage = string.Format(Resources.ArgumentNullException, exception.ParamName);
-                        var msg = string.Format(Resources.OperationFailType2, 
-                            ReflectionInfo.DeclaringType.Name, 
-                            ReflectionInfo.Name, 
+                        var msg = string.Format(Resources.OperationFailType2,
+                            ReflectionInfo.DeclaringType.Name,
+                            ReflectionInfo.Name,
                             innerMessage);
 
                         dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, msg);
@@ -391,7 +393,11 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                     }
                     else if (exc is System.NullReferenceException)
                     {
-                        dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(null));
+                        dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(exc));
+                    }
+                    else if (exc is BuiltinNullReferenceException)
+                    {
+                        dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(exc));
                     }
                     else if (exc is StringOverIndexingException)
                     {
@@ -417,6 +423,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);
                 }
+
                 dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);
             }
             catch (System.MethodAccessException ex)
@@ -425,6 +432,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);
                 }
+
                 dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);
             }
             catch (System.InvalidOperationException ex)
@@ -433,6 +441,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);
                 }
+
                 dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);
             }
             catch (System.NotSupportedException ex)
@@ -441,6 +450,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
                 {
                     dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);
                 }
+
                 dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);
             }
             catch (ArgumentNullException ex)
@@ -478,7 +488,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
 
         private string ErrorString(System.Exception ex)
         {
-            if (ex is System.InvalidOperationException)
+            if (ex is System.InvalidOperationException || ex is BuiltinNullReferenceException)
                 return ex.Message;
 
             string msg = (ex == null) ? "" : ex.Message;
diff --git a/src/Engine/ProtoCore/RuntimeStatus.cs b/src/Engine/ProtoCore/RuntimeStatus.cs
@@ -356,8 +356,6 @@ public void LogMethodResolutionWarning(FunctionGroup funcGroup,
                                                List<StackValue> arguments = null)
         {
             string message;
-            string propertyName;
-            Operator op;
 
             var qualifiedMethodName = methodName;
 
@@ -366,12 +364,22 @@ public void LogMethodResolutionWarning(FunctionGroup funcGroup,
 
             if (classScope != Constants.kGlobalScope)
             {
+                if (methodName == nameof(DesignScript.Builtin.Get.ValueAtIndex))
+                {
+                    if (arguments.Count == 2 && arguments[0].IsInteger && arguments[1].IsInteger)
+                    {
+                        LogWarning(WarningID.IndexOutOfRange, Resources.IndexIntoNonArrayObject);
+                        return;
+                    }
+                }
                 var classNode = runtimeCore.DSExecutable.classTable.ClassNodes[classScope];
                 className = classNode.Name;
                 classNameSimple = className.Split('.').Last();
                 qualifiedMethodName = classNameSimple + "." + methodName;
             }
 
+            Operator op;
+            string propertyName;
             if (CoreUtils.TryGetPropertyName(methodName, out propertyName))
             {
                 if (classScope != Constants.kGlobalScope)
@@ -408,7 +416,7 @@ public void LogMethodResolutionWarning(FunctionGroup funcGroup,
                 var argsJoined = string.Join(", ", arguments.Select(GetTypeName));
                 
                 var fep = funcGroup.FunctionEndPoints[0];
-                var formalParamsJoined = string.Join(", ", fep.FormalParams.Select(x => x.ToShortString()));
+                var formalParamsJoined = string.Join(", ", fep.FormalParams);
 
                 message = string.Format(Resources.NonOverloadMethodResolutionError, qualifiedMethodName, formalParamsJoined, argsJoined);
             }
diff --git a/src/Libraries/DesignScriptBuiltin/Builtin.cs b/src/Libraries/DesignScriptBuiltin/Builtin.cs
@@ -14,6 +14,11 @@ public static class Get
 
             public static object ValueAtIndex(Dictionary dictionary, string key)
             {
+                if (dictionary == null)
+                {
+                    throw new BuiltinNullReferenceException(DesignScriptBuiltin.NullReferenceExceptionMessage);
+                }
+
                 try
                 {
                     return dictionary.ValueAtKey(key);
@@ -56,6 +61,10 @@ public static object ValueAtIndexInForLoop(string stringList, int index)
 
             public static object ValueAtIndex(string stringList, int index)
             {
+                if (stringList == null)
+                {
+                    throw new BuiltinNullReferenceException(DesignScriptBuiltin.NullReferenceExceptionMessage);
+                }
                 while (index < 0)
                 {
                     var count = stringList.Length;
diff --git a/src/Libraries/DesignScriptBuiltin/IndexingExceptions.cs b/src/Libraries/DesignScriptBuiltin/IndexingExceptions.cs
@@ -25,4 +25,16 @@ public StringOverIndexingException(string message)
         {
         }
     }
+
+    /// <summary>
+    /// Null reference exception thrown with null DS builtin types:
+    /// lists, dictionaries and strings.
+    /// </summary>
+    public class BuiltinNullReferenceException : NullReferenceException
+    {
+        public BuiltinNullReferenceException(string message)
+            : base(message)
+        {
+        }
+    }
 }
diff --git a/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.Designer.cs b/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.Designer.cs
diff --git a/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.en-US.resx b/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.en-US.resx
@@ -123,4 +123,7 @@
   <data name="StringOverIndexingExceptionMessage" xml:space="preserve">
     <value>Index was out of range. If non-negative must be less than the size of the string.</value>
   </data>
+  <data name="NullReferenceExceptionMessage" xml:space="preserve">
+    <value>Cannot index into a null list, dictionary or string.</value>
+  </data>
 </root>
diff --git a/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.resx b/src/Libraries/DesignScriptBuiltin/Properties/DesignScriptBuiltin.resx
@@ -123,4 +123,7 @@
   <data name="StringOverIndexingExceptionMessage" xml:space="preserve">
     <value>Index was out of range. If non-negative must be less than the size of the string.</value>
   </data>
+  <data name="NullReferenceExceptionMessage" xml:space="preserve">
+    <value>Cannot index into a null list, dictionary or string.</value>
+  </data>
 </root>

Original file line number	Diff line number	Diff line change
`@@ -355,6 +355,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`355`	`355`	`{`
`356`	`356`	`dsi.LogSemanticError(ex.InnerException.Message);`
`357`	`357`	`}`
	`358`	`+`
`358`	`359`	`dsi.LogSemanticError(ex.Message);`
`359`	`360`	`}`
`360`	`361`	`catch (System.Reflection.TargetException ex)`
`@@ -363,6 +364,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`363`	`364`	`{`
`364`	`365`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);`
`365`	`366`	`}`
	`367`	`+`
`366`	`368`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);`
`367`	`369`	`}`
`368`	`370`	`catch (System.Reflection.TargetInvocationException ex)`
`@@ -374,9 +376,9 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`374`	`376`	`if (exception != null)`
`375`	`377`	`{`
`376`	`378`	`var innerMessage = string.Format(Resources.ArgumentNullException, exception.ParamName);`
`377`		`- var msg = string.Format(Resources.OperationFailType2,`
`378`		`- ReflectionInfo.DeclaringType.Name,`
`379`		`- ReflectionInfo.Name,`
	`379`	`+ var msg = string.Format(Resources.OperationFailType2,`
	`380`	`+ ReflectionInfo.DeclaringType.Name,`
	`381`	`+ ReflectionInfo.Name,`
`380`	`382`	`innerMessage);`
`381`	`383`
`382`	`384`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, msg);`
`@@ -391,7 +393,11 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`391`	`393`	`}`
`392`	`394`	`else if (exc is System.NullReferenceException)`
`393`	`395`	`{`
`394`		`- dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(null));`
	`396`	`+ dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(exc));`
	`397`	`+ }`
	`398`	`+ else if (exc is BuiltinNullReferenceException)`
	`399`	`+ {`
	`400`	`+ dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(exc));`
`395`	`401`	`}`
`396`	`402`	`else if (exc is StringOverIndexingException)`
`397`	`403`	`{`
`@@ -417,6 +423,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`417`	`423`	`{`
`418`	`424`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);`
`419`	`425`	`}`
	`426`	`+`
`420`	`427`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);`
`421`	`428`	`}`
`422`	`429`	`catch (System.MethodAccessException ex)`
`@@ -425,6 +432,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`425`	`432`	`{`
`426`	`433`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);`
`427`	`434`	`}`
	`435`	`+`
`428`	`436`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);`
`429`	`437`	`}`
`430`	`438`	`catch (System.InvalidOperationException ex)`
`@@ -433,6 +441,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`433`	`441`	`{`
`434`	`442`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);`
`435`	`443`	`}`
	`444`	`+`
`436`	`445`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);`
`437`	`446`	`}`
`438`	`447`	`catch (System.NotSupportedException ex)`
`@@ -441,6 +450,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`441`	`450`	`{`
`442`	`451`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.InnerException.Message);`
`443`	`452`	`}`
	`453`	`+`
`444`	`454`	`dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ex.Message);`
`445`	`455`	`}`
`446`	`456`	`catch (ArgumentNullException ex)`
`@@ -478,7 +488,7 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter`
`478`	`488`
`479`	`489`	`private string ErrorString(System.Exception ex)`
`480`	`490`	`{`
`481`		`- if (ex is System.InvalidOperationException)`
	`491`	`+ if (ex is System.InvalidOperationException \|\| ex is BuiltinNullReferenceException)`
`482`	`492`	`return ex.Message;`
`483`	`493`
`484`	`494`	`string msg = (ex == null) ? "" : ex.Message;`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,11 @@ public static class Get`
`14`	`14`
`15`	`15`	`public static object ValueAtIndex(Dictionary dictionary, string key)`
`16`	`16`	`{`
	`17`	`+ if (dictionary == null)`
	`18`	`+ {`
	`19`	`+ throw new BuiltinNullReferenceException(DesignScriptBuiltin.NullReferenceExceptionMessage);`
	`20`	`+ }`
	`21`	`+`
`17`	`22`	`try`
`18`	`23`	`{`
`19`	`24`	`return dictionary.ValueAtKey(key);`
`@@ -56,6 +61,10 @@ public static object ValueAtIndexInForLoop(string stringList, int index)`
`56`	`61`
`57`	`62`	`public static object ValueAtIndex(string stringList, int index)`
`58`	`63`	`{`
	`64`	`+ if (stringList == null)`
	`65`	`+ {`
	`66`	`+ throw new BuiltinNullReferenceException(DesignScriptBuiltin.NullReferenceExceptionMessage);`
	`67`	`+ }`
`59`	`68`	`while (index < 0)`
`60`	`69`	`{`
`61`	`70`	`var count = stringList.Length;`
Original file line number	Diff line number	Diff line change
`@@ -25,4 +25,16 @@ public StringOverIndexingException(string message)`
`25`	`25`	`{`
`26`	`26`	`}`
`27`	`27`	`}`
	`28`	`+`
	`29`	`+ /// <summary>`
	`30`	`+ /// Null reference exception thrown with null DS builtin types:`
	`31`	`+ /// lists, dictionaries and strings.`
	`32`	`+ /// </summary>`
	`33`	`+ public class BuiltinNullReferenceException : NullReferenceException`
	`34`	`+ {`
	`35`	`+ public BuiltinNullReferenceException(string message)`
	`36`	`+ : base(message)`
	`37`	`+ {`
	`38`	`+ }`
	`39`	`+ }`
`28`	`40`	`}`