Module-related debugger improvements (#1374)

* Fixed undefined bindings in module scope

Implemented DebugScopeType.Module

* Bonus: No step through return point of implicit class constructor

Improved and cleaned up debugger StepFlowTests

Jint.Tests/Runtime/Debugger/ScopeTests.cs

@@ -332,6 +332,38 @@ namespace Jint.Tests.Runtime.Debugger
             });
         }
 
+        [Fact]
+        public void HasCorrectScopeChainForModule()
+        {
+            string imported = @"
+            function add(a, b)
+            {
+                debugger;
+                return a + b;
+            }
+            
+            export { add };";
+
+            string main = @"
+            import { add } from 'imported-module';
+            const x = 1;
+            const y = 2;
+            add(x, y);";
+            TestHelpers.TestAtBreak(engine =>
+            {
+                engine.AddModule("imported-module", imported);
+                engine.AddModule("main", main);
+                engine.ImportModule("main");
+            },
+            info =>
+            {
+                Assert.Collection(info.CurrentScopeChain,
+                    scope => AssertScope(scope, DebugScopeType.Local, "arguments", "a", "b"),
+                    scope => AssertScope(scope, DebugScopeType.Module, "add"),
+                    scope => AssertScope(scope, DebugScopeType.Global));
+            });
+        }
+
         [Fact]
         public void HasCorrectScopeChainForNestedBlock()
         {
@@ -501,5 +533,22 @@ namespace Jint.Tests.Runtime.Debugger
                 );
             });
         }
+
+        [Fact]
+        public void InspectsModuleScopedBindings()
+        {
+            string main = @"const x = 1; debugger;";
+            TestHelpers.TestAtBreak(engine =>
+            {
+                engine.AddModule("main", main);
+                engine.ImportModule("main");
+            },
+            info =>
+            {
+                // No need for imports - main module is module scoped too, duh.
+                var value = AssertOnlyScopeContains(info.CurrentScopeChain, "x", DebugScopeType.Module);
+                Assert.Equal(1, value.AsInteger());
+            });
+        }
     }
 }

Jint.Tests/Runtime/Debugger/StepFlowTests.cs

@@ -26,7 +26,7 @@ namespace Jint.Tests.Runtime.Debugger
         [Fact]
         public void StepsThroughWhileLoop()
         {
-            string script = @"
+            var script = @"
                 let x = 0;
                 while (x < 2)
                 {
@@ -50,7 +50,7 @@ namespace Jint.Tests.Runtime.Debugger
         [Fact]
         public void StepsThroughDoWhileLoop()
         {
-            string script = @"
+            var script = @"
                 let x = 0;
                 do
                 {
@@ -74,7 +74,7 @@ namespace Jint.Tests.Runtime.Debugger
         [Fact]
         public void StepsThroughForLoop()
         {
-            string script = @"
+            var script = @"
                 for (let x = 0; x < 2; x++)
                 {
                     'dummy';
@@ -87,10 +87,10 @@ namespace Jint.Tests.Runtime.Debugger
                 node => Assert.IsType<ForStatement>(node),        // for ...
                 node => Assert.IsType<VariableDeclaration>(node), // let x = 0
                 node => Assert.IsType<BinaryExpression>(node),    // x < 2
-                node => Assert.IsType<ExpressionStatement>(node), // 'dummy';
+                node => Assert.True(node.IsLiteral("dummy")),     // 'dummy';
                 node => Assert.IsType<UpdateExpression>(node),    // x++;
                 node => Assert.IsType<BinaryExpression>(node),    // x < 2
-                node => Assert.IsType<ExpressionStatement>(node), // 'dummy';
+                node => Assert.True(node.IsLiteral("dummy")),     // 'dummy';
                 node => Assert.IsType<UpdateExpression>(node),    // x++;
                 node => Assert.IsType<BinaryExpression>(node)     // x < 2 (false)
             );
@@ -99,7 +99,7 @@ namespace Jint.Tests.Runtime.Debugger
         [Fact]
         public void StepsThroughForOfLoop()
         {
-            string script = @"
+            var script = @"
                 const arr = [1, 2];
                 for (const item of arr)
                 {
@@ -113,16 +113,16 @@ namespace Jint.Tests.Runtime.Debugger
                 node => Assert.IsType<VariableDeclaration>(node), // let arr = [1, 2];
                 node => Assert.IsType<ForOfStatement>(node),      // for ...
                 node => Assert.IsType<VariableDeclaration>(node), // item
-                node => Assert.IsType<ExpressionStatement>(node), // 'dummy';
+                node => Assert.True(node.IsLiteral("dummy")),     // 'dummy';
                 node => Assert.IsType<VariableDeclaration>(node), // item
-                node => Assert.IsType<ExpressionStatement>(node)  // 'dummy';
+                node => Assert.True(node.IsLiteral("dummy"))      // 'dummy';
             );
         }
 
         [Fact]
         public void StepsThroughForInLoop()
         {
-            string script = @"
+            var script = @"
                 const obj = { x: 1, y: 2 };
                 for (const key in obj)
                 {
@@ -142,11 +142,36 @@ namespace Jint.Tests.Runtime.Debugger
             );
         }
 
+        [Fact]
+        public void StepsThroughConstructor()
+        {
+            var script = @"
+                class Test
+                {
+                    constructor()
+                    {
+                        'in constructor';
+                    }
+                }
+                new Test();
+                'after construction';
+            ";
+
+            var nodes = CollectStepNodes(script);
+
+            Assert.Collection(nodes,
+                node => Assert.IsType<ClassDeclaration>(node),          // class Test
+                node => Assert.IsType<ExpressionStatement>(node),       // new Test();
+                node => Assert.True(node.IsLiteral("in constructor")),  // 'in constructor()'
+                node => Assert.Null(node),                              // return point
+                node => Assert.True(node.IsLiteral("after construction"))
+            );
+        }
 
         [Fact]
         public void SkipsFunctionBody()
         {
-            string script = @"
+            var script = @"
                 function test()
                 {
                     'dummy';
@@ -159,9 +184,28 @@ namespace Jint.Tests.Runtime.Debugger
             Assert.Collection(nodes,
                 node => Assert.IsType<FunctionDeclaration>(node), // function(test) ...;
                 node => Assert.IsType<ExpressionStatement>(node), // test();
-                node => Assert.IsType<Directive>(node),           // 'dummy';
+                node => Assert.True(node.IsLiteral("dummy")),     // 'dummy';
                 node => Assert.Null(node)                         // return point
             );
         }
+
+        [Fact]
+        public void SkipsReturnPointOfImplicitConstructor()
+        {
+            var script = @"
+                class Test
+                {
+                }
+                new Test();
+                'dummy';
+            ";
+
+            var nodes = CollectStepNodes(script);
+            Assert.Collection(nodes,
+                node => Assert.IsType<ClassDeclaration>(node),    // class Test
+                node => Assert.IsType<ExpressionStatement>(node), // new Test();
+                node => Assert.True(node.IsLiteral("dummy"))      // 'dummy';
+            );
+        }
     }
 }

Jint.Tests/Runtime/Debugger/TestHelpers.cs

@@ -24,9 +24,9 @@ namespace Jint.Tests.Runtime.Debugger
         /// Initializes engine in debugmode and executes script until debugger statement,
         /// before calling stepHandler for assertions. Also asserts that a break was triggered.
         /// </summary>
-        /// <param name="script">Script that is basis for testing</param>
+        /// <param name="initialization">Action to initialize and execute scripts</param>
         /// <param name="breakHandler">Handler for assertions</param>
-        public static void TestAtBreak(string script, Action<Engine, DebugInformation> breakHandler)
+        public static void TestAtBreak(Action<Engine> initialization, Action<Engine, DebugInformation> breakHandler)
         {
             var engine = new Engine(options => options
                 .DebugMode()
@@ -41,11 +41,28 @@ namespace Jint.Tests.Runtime.Debugger
                 return StepMode.None;
             };
 
-            engine.Execute(script);
+            initialization(engine);
 
             Assert.True(didBreak, "Test script did not break (e.g. didn't reach debugger statement)");
         }
 
+        /// <inheritdoc cref="TestAtBreak()"/>
+        public static void TestAtBreak(Action<Engine> initialization, Action<DebugInformation> breakHandler)
+        {
+            TestAtBreak(engine => initialization(engine), (engine, info) => breakHandler(info));
+        }
+
+        /// <summary>
+        /// Initializes engine in debugmode and executes script until debugger statement,
+        /// before calling stepHandler for assertions. Also asserts that a break was triggered.
+        /// </summary>
+        /// <param name="script">Script that is basis for testing</param>
+        /// <param name="breakHandler">Handler for assertions</param>
+        public static void TestAtBreak(string script, Action<Engine, DebugInformation> breakHandler)
+        {
+            TestAtBreak(engine => engine.Execute(script), breakHandler);
+        }
+
         /// <inheritdoc cref="TestAtBreak()"/>
         public static void TestAtBreak(string script, Action<DebugInformation> breakHandler)
         {

Jint/Native/Function/ClassDefinition.cs

@@ -14,7 +14,7 @@ namespace Jint.Native.Function
         private static readonly MethodDefinition _superConstructor;
         internal static CallExpression _defaultSuperCall;
 
-        private static readonly MethodDefinition _emptyConstructor;
+        internal static readonly MethodDefinition _emptyConstructor;
 
         internal readonly string? _className;
         private readonly Expression? _superClass;

Jint/Native/Function/ScriptFunctionInstance.cs

@@ -159,7 +159,11 @@ namespace Jint.Native.Function
                     var result = _functionDefinition.EvaluateBody(context, this, arguments);
 
                     // The DebugHandler needs the current execution context before the return for stepping through the return point
-                    if (context.DebugMode && result.Type != CompletionType.Throw)
+                    // We exclude the empty constructor generated for classes without an explicit constructor.
+                    bool isStep = context.DebugMode &&
+                        result.Type != CompletionType.Throw &&
+                        _functionDefinition.Function != ClassDefinition._emptyConstructor.Value;
+                    if (isStep)
                     {
                         // We don't have a statement, but we still need a Location for debuggers. DebugHandler will infer one from
                         // the function body:

Jint/Runtime/Debugger/DebugScope.cs

@@ -57,7 +57,7 @@ namespace Jint.Runtime.Debugger
         /// <returns>Value of the binding</returns>
         public JsValue? GetBindingValue(string name)
         {
-            _record.TryGetBindingValue(name, strict: true, out var result);
+            _record.TryGetBinding(new EnvironmentRecord.BindingName(name), strict: true, out _, out var result);
             return result;
         }
 

Jint/Runtime/Debugger/DebugScopeType.cs

@@ -79,7 +79,6 @@ namespace Jint.Runtime.Debugger
         /// <summary>
         /// Module scope bindings.
         /// </summary>
-        /// <remarks>Not currently implemented by Jint.</remarks>
         Module,
 
         /// <summary>

Jint/Runtime/Debugger/DebugScopes.cs

@@ -55,6 +55,9 @@ namespace Jint.Runtime.Debugger
                         // If an ObjectEnvironmentRecord is not a GlobalEnvironmentRecord, it's With
                         AddScope(DebugScopeType.With, record);
                         break;
+                    case ModuleEnvironmentRecord:
+                        AddScope(DebugScopeType.Module, record);
+                        break;
                     case DeclarativeEnvironmentRecord der:
                         if (der._catchEnvironment)
                         {

Jint/Runtime/Environments/DeclarativeEnvironmentRecord.cs

@@ -124,17 +124,6 @@ namespace Jint.Runtime.Environments
             return null!;
         }
 
-        internal sealed override bool TryGetBindingValue(string name, bool strict, [NotNullWhen(true)] out JsValue? value)
-        {
-            if (_dictionary is not null && _dictionary.TryGetValue(name, out var binding) && binding.IsInitialized())
-            {
-                value = binding.Value;
-                return true;
-            }
-            value = null;
-            return false;
-        }
-
         [MethodImpl(MethodImplOptions.NoInlining)]
         private void ThrowUninitializedBindingError(string name)
         {

Jint/Runtime/Environments/EnvironmentRecord.cs

@@ -72,17 +72,6 @@ namespace Jint.Runtime.Environments
         /// <return>The value of an already existing binding from an environment record.</return>
         public abstract JsValue GetBindingValue(string name, bool strict);
 
-        /// <summary>
-        /// Returns the value of an already existing binding from an environment record. Unlike <see cref="GetBindingValue(string, bool)"/>
-        /// this does not throw an exception for uninitialized bindings, but instead returns false and sets <paramref name="value"/> to null.
-        /// </summary>
-        /// <param name="name">The identifier of the binding</param>
-        /// <param name="strict">Strict mode</param>
-        /// <param name="value">The value of an already existing binding from an environment record.</param>
-        /// <returns>True if the value is initialized, otherwise false.</returns>
-        /// <remarks>This is used for debugger inspection. Note that this will currently still throw if the binding cannot be retrieved (e.g. because it doesn't exist).</remarks>
-        internal abstract bool TryGetBindingValue(string name, bool strict, [NotNullWhen(true)] out JsValue? value);
-
         /// <summary>
         /// Delete a binding from an environment record. The String value N is the text of the bound name If a binding for N exists, remove the binding and return true. If the binding exists but cannot be removed return false. If the binding does not exist return true.
         /// </summary>

Jint/Runtime/Environments/GlobalEnvironmentRecord.cs

@@ -241,37 +241,6 @@ namespace Jint.Runtime.Environments
             return ObjectInstance.UnwrapJsValue(desc, _global);
         }
 
-        internal override bool TryGetBindingValue(string name, bool strict, [NotNullWhen(true)] out JsValue? value)
-        {
-            if (_declarativeRecord.HasBinding(name))
-            {
-                return _declarativeRecord.TryGetBindingValue(name, strict, out value);
-            }
-
-            // see ObjectEnvironmentRecord.TryGetBindingValue
-            var desc = PropertyDescriptor.Undefined;
-            if (_globalObject is not null)
-            {
-                if (_globalObject._properties?.TryGetValue(name, out desc) == false)
-                {
-                    desc = PropertyDescriptor.Undefined;
-                }
-            }
-            else
-            {
-                desc = _global.GetProperty(name);
-            }
-
-            if (strict && desc == PropertyDescriptor.Undefined)
-            {
-                value = null;
-                return false;
-            }
-
-            value = ObjectInstance.UnwrapJsValue(desc, _global);
-            return true;
-        }
-
         public override bool DeleteBinding(string name)
         {
             if (_declarativeRecord.HasBinding(name))

Jint/Runtime/Environments/ObjectEnvironmentRecord.cs

@@ -166,19 +166,6 @@ namespace Jint.Runtime.Environments
             return ObjectInstance.UnwrapJsValue(desc, _bindingObject);
         }
 
-        internal override bool TryGetBindingValue(string name, bool strict, [NotNullWhen(true)] out JsValue? value)
-        {
-            var desc = _bindingObject.GetProperty(name);
-            if (strict && desc == PropertyDescriptor.Undefined)
-            {
-                value = null;
-                return false;
-            }
-
-            value = ObjectInstance.UnwrapJsValue(desc, _bindingObject);
-            return true;
-        }
-
         public override bool DeleteBinding(string name)
         {
             return _bindingObject.Delete(name);