Rewrite the code comments in plainer language

The comments explained the change in Jint's own vocabulary - inline caches,
shape mode, descriptors, lanes - which is not vocabulary this repository
uses. Say what each change does and why it is worth it instead, and name a
Jint concept only where the reader has to look it up anyway.

No behaviour change: comments and XML docs only, plus one short comment on
the enum conversion option.

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/ContentWrapper/ContentFieldObject.cs

@@ -134,10 +134,10 @@ public sealed class ContentFieldObject : ObjectInstance
 
     protected override OwnPropertyProbe ProbeOwnProperty(JsValue property)
     {
-        // Deliberately mirrors GetOwnProperty above, minus the descriptor: the flags are on the descriptor
-        // itself, so an existence or enumerability question is answered without ever reading CustomValue,
-        // which is what maps the JSON value to a JsValue. The engine trusts the answer without verifying it,
-        // so the two must stay in step.
+        // Answers whether a key exists without converting its value, which reading the property would do.
+        // Used by "in", hasOwnProperty, Object.keys, spread and JSON.stringify. Must give the same answer as
+        // GetOwnProperty above, which Jint does not check at runtime, only in tests (see
+        // JintHostContractVerification), so the two methods are kept identical apart from the return value.
         EnsurePropertiesInitialized();
 
         var propertyName = property.AsString();

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintExtensions.cs

@@ -74,9 +74,8 @@ public static class JintExtensions
         {
             foreach (var (key, item) in vars)
             {
-                // Deferred instead of Engine.SetValue, which maps every variable now. The global itself is
-                // installed eagerly, so existence checks and enumeration see the name without materializing
-                // anything; only the mapping waits for the first read of the value.
+                // Sets the value, but runs the conversion only when the script reads it for the first time.
+                // The name is added right away, so enumeration and "in" checks work as before.
                 engine.Advanced.AddLazyGlobal(key, e => MapVariable(e, item));
             }
         }
@@ -87,8 +86,8 @@ public static class JintExtensions
     }
 
     /// <summary>
-    /// The conversion <see cref="Engine.SetValue(string, object)"/> performs, including its special case for
-    /// a CLR type, so deferring a variable cannot change what the script sees.
+    /// Converts a value exactly like <see cref="Engine.SetValue(string, object)"/> does, including its
+    /// special case for types, so that a deferred variable cannot look different from an eager one.
     /// </summary>
     private static JsValue MapVariable(Engine engine, object? item)
     {

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintObjectConverter.cs

@@ -22,14 +22,14 @@ namespace Squidex.Domain.Apps.Core.Scripting.Internal;
 public sealed class JintObjectConverter : IObjectConverter
 {
     /// <summary>
-    /// The CLR types this converter answers for, declared at registration so the engine can keep its
-    /// compiled interop member-read lane for members whose declared type can never reach this converter.
+    /// The types this converter handles, passed to Jint when the converter is registered.
     /// </summary>
     /// <remarks>
-    /// Matching is by assignability, so <see cref="IUser"/> covers every implementation. Registering the
-    /// converter without this set makes every wrapped CLR member read in the engine take the slow lane.
-    /// Enums are not listed: they are handled natively through
-    /// <see cref="Options.InteropOptions.EnumConversion"/>.
+    /// Without this list Jint has to offer every property of every .NET object to this converter and cannot
+    /// use its faster property reader for any of them. Base types and interfaces count, so
+    /// <see cref="IUser"/> covers all implementations. Keep the list in sync with the switch below - a type
+    /// that is converted but not listed here fails a test (see JintHostContractVerification). Enums are
+    /// missing on purpose, they are converted by Jint itself, see EnumConversion in JintScriptEngine.
     /// </remarks>
     public static readonly Type[] HandledTypes =
     [

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JsonMapper.cs

@@ -55,9 +55,9 @@ public static class JsonMapper
 
     private static JsObject FromObject(JsonObject obj, Engine engine)
     {
-        // Built through the hidden class machinery, so JSON objects sharing a key sequence - every content
-        // item of the same schema does - share one hidden class and keep a script reading them monomorphic.
-        // A bare ObjectInstance subclass can never be in shape mode and is outside the read caches entirely.
+        // Objects that are created this way and have the same keys - all content items of a schema do -
+        // share one description of their layout, like a class. Reading a property is then a lot faster than
+        // with a custom ObjectInstance class, where every single object gets its own property dictionary.
         var entries = new KeyValuePair<string, JsValue>[obj.Count];
 
         var index = 0;
@@ -119,8 +119,8 @@ public static class JsonMapper
 
             var result = new JsonArray((int)length);
 
-            // The indexed accessor reads the dense backing directly, where a string key would allocate one
-            // key per element and route through the full property lookup.
+            // The indexer reads the array storage directly. The old version converted the index to a string
+            // and did a full property lookup for every element.
             for (var i = 0u; i < length; i++)
             {
                 result.Add(Map(a[i]));

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs

@@ -145,7 +145,10 @@ public sealed class JintScriptEngine(IMemoryCache cache, IOptions<JintScriptOpti
         {
             engineOptions.AddObjectConverter(JintObjectConverter.Instance, JintObjectConverter.HandledTypes);
             engineOptions.AllowClrWrite(!options.Readonly);
+
+            // Converts enums to their name, e.g. "Published". This was done by JintObjectConverter before.
             engineOptions.Interop.EnumConversion = EnumConversionMode.String;
+
             engineOptions.SetTypeConverter(engine => new CustomClrConverter(engine));
             engineOptions.SetReferencesResolver(NullPropagation.Instance, NullPropagation.Interests);
             engineOptions.Strict();

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/NullPropagation.cs

@@ -15,17 +15,13 @@ namespace Squidex.Domain.Apps.Core.Scripting;
 public sealed class NullPropagation : IReferenceResolver
 {
     /// <summary>
-    /// The situations this resolver actually answers, declared so the engine keeps the fast paths for
-    /// everything else.
+    /// The cases this resolver actually handles.
     /// </summary>
     /// <remarks>
-    /// Deliberately omitted are <see cref="ReferenceResolverInterests.ObjectPropertyBase"/> and
-    /// <see cref="ReferenceResolverInterests.PrimitivePropertyBase"/>, the pair that disables the
-    /// non-computed member-read inline caches, the dense-array indexed-read lane and the member-call callee
-    /// lane engine-wide. <see cref="TryPropertyReference"/> declines every base that is not null or
-    /// undefined, so those are situations where the engine consulting this resolver could never change the
-    /// result. Interests are a subscription filter and not a promise: a situation not subscribed to behaves
-    /// exactly as if no resolver were registered.
+    /// Without this list Jint has to assume that we want to see every property read and turns off its read
+    /// caches for the whole engine. But <see cref="TryPropertyReference"/> only ever does something when the
+    /// value is null or undefined, so the other cases can be left to Jint. Behavior does not change: for a
+    /// case that is not listed here Jint behaves as if no resolver was registered at all.
     /// </remarks>
     public const ReferenceResolverInterests Interests =
         ReferenceResolverInterests.NullishPropertyBase |
@@ -35,16 +31,12 @@ public sealed class NullPropagation : IReferenceResolver
     public static readonly NullPropagation Instance = new NullPropagation();
 
     /// <summary>
-    /// Answers a read of a name that resolves to no binding, so that an unknown name does not throw a
-    /// reference error.
+    /// Called when a name does not exist, so that reading an unknown variable does not throw.
     /// </summary>
     /// <remarks>
-    /// Passing the reference base straight through hands script the engine's internal sentinel for the
-    /// unresolvable state - a <see cref="JsString"/> reading <c>[[Unresolvable]]</c> - rather than
-    /// <c>undefined</c>, which is documented on <see cref="IReferenceResolver.TryUnresolvableReference"/> and
-    /// on <see cref="Reference.Base"/>. That is what scripts have always seen here, so it is kept and pinned
-    /// by a test; assigning <see cref="JsValue.Undefined"/> instead would be the tidier behaviour but a
-    /// breaking change for existing tenant scripts.
+    /// The returned base is not <c>undefined</c> here but an internal Jint marker string that reads
+    /// <c>[[Unresolvable]]</c>. That is what scripts have always seen, so it is kept as it is and covered by
+    /// a test. Returning <c>undefined</c> would be nicer, but would change behavior for existing scripts.
     /// </remarks>
     public bool TryUnresolvableReference(Engine engine, Reference reference, out JsValue value)
     {

backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/WritableContext.cs

@@ -21,12 +21,10 @@ internal sealed class WritableContext : ObjectInstance
     {
         this.vars = vars;
 
-        // Scripts touch a fraction of the variables, but mapping one is not always cheap: a content data
-        // variable builds a wrapper, a user variable walks and groups every claim. The descriptors are
-        // installed eagerly - so key order, enumeration and existence checks are exactly what they were -
-        // and only the mapping waits for the first read of a value. Once it has run the descriptor drops
-        // back to an ordinary data property and rejoins the write inline cache, which is what a
-        // hand-written CustomJsValue descriptor cannot do.
+        // Adds the value, but runs the conversion only when the script reads it for the first time. Most
+        // scripts use a few of these variables and some of them are expensive, e.g. the user variable walks
+        // and groups all claims. The properties themselves are added right away, so key order, enumeration
+        // and "in" checks stay the same.
         foreach (var (key, item) in vars)
         {
             SetOwnProperty(key, PropertyDescriptor.CreateLazy(

backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JintScriptEngineTests.cs

@@ -729,9 +729,8 @@ public class JintScriptEngineTests : IClassFixture<TranslationsFixture>
     [Fact]
     public void Should_not_throw_if_reading_undeclared_identifier()
     {
-        // The null propagation resolver answers an unresolvable reference with the reference base, which is
-        // Jint's internal sentinel. The value is odd, but it is what scripts have always seen and the point
-        // of the test is that the read does not throw a reference error.
+        // Reading an unknown name does not throw, it returns an internal Jint marker string. That is odd,
+        // but it is what scripts have always seen here, see NullPropagation.TryUnresolvableReference.
         const string script = @"
                 String(unknownName) + '|' + (typeof unknownName);
             ";

backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JsonMapperTests.cs

@@ -22,10 +22,9 @@ public class JsonMapperTests
         var mapped = (ObjectInstance)JsonMapper.Map(CreateJson(), engine);
         var nested = (ObjectInstance)mapped.Get("nested");
 
-        // A shared shape is what keeps a script reading a batch of content items monomorphic. It is a
-        // performance property and never a correctness one, but it is silent when it regresses: building
-        // these objects as a host ObjectInstance subclass again would put them back in the per-object
-        // dictionary with no test noticing.
+        // Sharing the layout is what makes reading properties of many content items fast. It only affects
+        // performance and never behavior, which is why it is asserted here: building these objects with a
+        // custom ObjectInstance class again would silently undo it and no other test would notice.
         Assert.True(engine.Advanced.HasSharedShape(mapped));
         Assert.True(engine.Advanced.HasSharedShape(nested));
     }

backend/tests/Squidex.Domain.Apps.Core.Tests/TestHelpers/JintHostContractVerification.cs

@@ -10,19 +10,17 @@ using System.Runtime.CompilerServices;
 namespace Squidex.Domain.Apps.Core.TestHelpers;
 
 /// <summary>
-/// Turns on Jint's host-contract verifiers for this test assembly.
+/// Turns on Jint's self checks for this test assembly.
 /// </summary>
 /// <remarks>
-/// The scripting integration defines several Jint extension points - the ContentWrapper objects override
-/// GetOwnProperty and ProbeOwnProperty, and the engine trusts both without re-verifying them on the hot
-/// path. A hook that contradicts another therefore fails silently in production: a key vanishes from every
-/// enumeration, or a read resolves on the prototype for a property that exists. With the switch on, Jint
-/// recomputes the answer the fast paths exist to avoid and throws on the first disagreement, so these tests
-/// are the checker.
+/// Our ContentWrapper classes and the object converter implement Jint extension points where Jint relies on
+/// our answers being consistent, without checking them - checking would cost as much as the shortcut saves.
+/// A mistake there is silent in production: a key can disappear from Object.keys, or a converted type can be
+/// skipped. With this switch on Jint verifies the answers and throws on the first mismatch, so a mistake
+/// fails a test instead.
 /// <para>
-/// It has to be set before the first use of any Jint type - the flag is read once at type initialization -
-/// which is what the module initializer guarantees. Never turn it on in production: the verifiers
-/// deliberately redo the work they check.
+/// The switch has to be set before the first Jint type is used, which is what the module initializer
+/// guarantees. It stays off in production, where the checks would only cost performance.
 /// </para>
 /// </remarks>
 internal static class JintHostContractVerification