Improve error location reporting (#1273)

sebastienros · Aug 27, 2022 · 41309de · 41309de
1 parent 3e0902c
commit 41309de
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 30 deletions.
diff --git a/Jint.Tests/Runtime/EngineLimitTests.cs b/Jint.Tests/Runtime/EngineLimitTests.cs
@@ -10,9 +10,9 @@ public class EngineLimitTests
     public void ShouldAllowReasonableCallStackDepth()
     {
 #if RELEASE
-        const int FunctionNestingCount = 650;
+        const int FunctionNestingCount = 690;
 #else
-        const int FunctionNestingCount = 340;
+        const int FunctionNestingCount = 350;
 #endif
 
         // generate call tree

diff --git a/Jint.Tests/Runtime/ErrorTests.cs b/Jint.Tests/Runtime/ErrorTests.cs
@@ -423,6 +423,24 @@ static ParserOptions CreateParserOptions()
             EqualIgnoringNewLineDifferences(Expected, e.JavaScriptStackTrace);
         }
 
+        [Fact]
+        public void ShouldReportCorrectColumn()
+        {
+            var e = Assert.Throws<JavaScriptException>(() =>
+            {
+                var engine = new Engine();
+                engine.Execute(@"var $variable1 = 611;
+var _variable2 = 711;
+var variable3 = 678;
+
+$variable1 + -variable2 - variable3;");
+            });
+
+            Assert.Equal(5, e.Location.Start.Line);
+            Assert.Equal(14, e.Location.Start.Column);
+            Assert.Equal("   at <anonymous>:5:15", e.JavaScriptStackTrace);
+        }
+
         private static void EqualIgnoringNewLineDifferences(string expected, string actual)
         {
             expected = expected.Replace("\r\n", "\n");

diff --git a/Jint/Engine.Modules.cs b/Jint/Engine.Modules.cs
@@ -1,5 +1,4 @@
 using Esprima;
-using Esprima.Ast;
 using Jint.Native;
 using Jint.Native.Object;
 using Jint.Native.Promise;

diff --git a/Jint/Runtime/ExceptionHelper.cs b/Jint/Runtime/ExceptionHelper.cs
@@ -52,7 +52,8 @@ public static void ThrowReferenceNameError(Realm realm, string? name)
         [DoesNotReturn]
         public static void ThrowReferenceError(Realm realm, string? message)
         {
-            throw new JavaScriptException(realm.Intrinsics.ReferenceError, message);
+            var location = realm.GlobalObject.Engine.GetLastSyntaxElement()?.Location ?? default;
+            throw new JavaScriptException(realm.Intrinsics.ReferenceError, message).SetJavaScriptLocation(location);
         }
 
         [DoesNotReturn]
@@ -64,19 +65,22 @@ public static void ThrowTypeErrorNoEngine(string? message = null)
         [DoesNotReturn]
         public static void ThrowTypeError(Realm realm, string? message = null)
         {
-            throw new JavaScriptException(realm.Intrinsics.TypeError, message);
+            var location = realm.GlobalObject.Engine.GetLastSyntaxElement()?.Location ?? default;
+            throw new JavaScriptException(realm.Intrinsics.TypeError, message).SetJavaScriptLocation(location);
         }
 
         [DoesNotReturn]
         public static void ThrowRangeError(Realm realm, string? message = null)
         {
-            throw new JavaScriptException(realm.Intrinsics.RangeError, message);
+            var location = realm.GlobalObject.Engine.GetLastSyntaxElement()?.Location ?? default;
+            throw new JavaScriptException(realm.Intrinsics.RangeError, message).SetJavaScriptLocation(location);
         }
 
         [DoesNotReturn]
         public static void ThrowUriError(Realm realm)
         {
-            throw new JavaScriptException(realm.Intrinsics.UriError);
+            var location = realm.GlobalObject.Engine.GetLastSyntaxElement()?.Location ?? default;
+            throw new JavaScriptException(realm.Intrinsics.UriError).SetJavaScriptLocation(location);
         }
 
         [DoesNotReturn]

diff --git a/Jint/Runtime/Interpreter/JintStatementList.cs b/Jint/Runtime/Interpreter/JintStatementList.cs
@@ -1,5 +1,6 @@
 using Esprima.Ast;
 using Jint.Native;
+using Jint.Native.Error;
 using Jint.Runtime.Environments;
 using Jint.Runtime.Interpreter.Statements;
 
@@ -104,33 +105,36 @@ public Completion Execute(EvaluationContext context)
             }
             catch (JavaScriptException v)
             {
-                SyntaxElement source = s!._statement;
-                if (v.Location != default)
-                {
-                    source = EsprimaExtensions.CreateLocationNode(v.Location);
-                }
-                var completion = new Completion(CompletionType.Throw, v.Error, null, source);
-                return completion;
+                return CreateThrowCompletion(s, v);
             }
             catch (TypeErrorException e)
             {
-                var error = engine.Realm.Intrinsics.TypeError.Construct(new JsValue[]
-                {
-                    e.Message
-                });
-                return new Completion(CompletionType.Throw, error, null, s!._statement);
+                return CreateThrowCompletion(engine.Realm.Intrinsics.TypeError, e, s!);
             }
             catch (RangeErrorException e)
             {
-                var error = engine.Realm.Intrinsics.RangeError.Construct(new JsValue[]
-                {
-                    e.Message
-                });
-                c = new Completion(CompletionType.Throw, error, null, s!._statement);
+                return CreateThrowCompletion(engine.Realm.Intrinsics.RangeError, e, s!);
             }
             return new Completion(c.Type, lastValue ?? JsValue.Undefined, c.Target, c._source);
         }
 
+        private static Completion CreateThrowCompletion(ErrorConstructor errorConstructor, Exception e, JintStatement s)
+        {
+            var error = errorConstructor.Construct(new JsValue[] { e.Message });
+            return new Completion(CompletionType.Throw, error, null, s._statement);
+        }
+
+        private static Completion CreateThrowCompletion(JintStatement? s, JavaScriptException v)
+        {
+            SyntaxElement source = s!._statement;
+            if (v.Location != default)
+            {
+                source = EsprimaExtensions.CreateLocationNode(v.Location);
+            }
+
+            return new Completion(CompletionType.Throw, v.Error, null, source);
+        }
+
         /// <summary>
         /// https://tc39.es/ecma262/#sec-blockdeclarationinstantiation
         /// </summary>

diff --git a/Jint/Runtime/Interpreter/Statements/JintSwitchBlock.cs b/Jint/Runtime/Interpreter/Statements/JintSwitchBlock.cs
@@ -1,4 +1,3 @@
-using Esprima;
 using Esprima.Ast;
 using Jint.Native;
 using Jint.Runtime.Environments;

diff --git a/Jint/Runtime/JavaScriptException.cs b/Jint/Runtime/JavaScriptException.cs
@@ -61,21 +61,21 @@ public JavaScriptException SetJavaScriptLocation(Location location)
         return this;
     }
 
-    private class JavaScriptErrorWrapperException : JintException
+    private sealed class JavaScriptErrorWrapperException : JintException
     {
         private string? _callStack;
         private Location _location;
 
-        public JsValue Error { get; }
-
-        public ref readonly Location Location => ref _location;
-
         internal JavaScriptErrorWrapperException(JsValue error, string? message = null)
             : base(message ?? GetMessage(error))
         {
             Error = error;
         }
 
+        public JsValue Error { get; }
+
+        public ref readonly Location Location => ref _location;
+
         internal void SetLocation(Location location)
         {
             _location = location;