From 54871db3f6c92edfb23e4de16e9585d8eeb854cf Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Fri, 31 Jul 2026 15:12:43 +0300 Subject: [PATCH] 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. --- .../ContentWrapper/ContentFieldObject.cs | 8 +++--- .../Scripting/Internal/JintExtensions.cs | 9 +++---- .../Scripting/Internal/JintObjectConverter.cs | 12 ++++----- .../Scripting/Internal/JsonMapper.cs | 10 +++---- .../Scripting/JintScriptEngine.cs | 3 +++ .../Scripting/NullPropagation.cs | 26 +++++++------------ .../Scripting/WritableContext.cs | 10 +++---- .../Scripting/JintScriptEngineTests.cs | 5 ++-- .../Operations/Scripting/JsonMapperTests.cs | 7 +++-- .../JintHostContractVerification.cs | 18 ++++++------- 10 files changed, 48 insertions(+), 60 deletions(-) diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/ContentWrapper/ContentFieldObject.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/ContentWrapper/ContentFieldObject.cs index 5f05220a9..0095307e8 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/ContentWrapper/ContentFieldObject.cs +++ b/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(); diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintExtensions.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintExtensions.cs index 2dc19097d..b25920d26 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintExtensions.cs +++ b/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 } /// - /// The conversion performs, including its special case for - /// a CLR type, so deferring a variable cannot change what the script sees. + /// Converts a value exactly like does, including its + /// special case for types, so that a deferred variable cannot look different from an eager one. /// private static JsValue MapVariable(Engine engine, object? item) { diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintObjectConverter.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintObjectConverter.cs index e9dfd69ee..ac0cfad60 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JintObjectConverter.cs +++ b/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 { /// - /// 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. /// /// - /// Matching is by assignability, so 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 - /// . + /// 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 + /// 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. /// public static readonly Type[] HandledTypes = [ diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JsonMapper.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JsonMapper.cs index 3f0ed8ac1..00e9e7e56 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/Internal/JsonMapper.cs +++ b/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[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])); diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs index faf8c20cc..0181c43ad 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs +++ b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/JintScriptEngine.cs @@ -145,7 +145,10 @@ public sealed class JintScriptEngine(IMemoryCache cache, IOptions new CustomClrConverter(engine)); engineOptions.SetReferencesResolver(NullPropagation.Instance, NullPropagation.Interests); engineOptions.Strict(); diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/NullPropagation.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/NullPropagation.cs index 8e8d0c724..73cd0a536 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/NullPropagation.cs +++ b/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 { /// - /// The situations this resolver actually answers, declared so the engine keeps the fast paths for - /// everything else. + /// The cases this resolver actually handles. /// /// - /// Deliberately omitted are and - /// , 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. 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 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. /// public const ReferenceResolverInterests Interests = ReferenceResolverInterests.NullishPropertyBase | @@ -35,16 +31,12 @@ public sealed class NullPropagation : IReferenceResolver public static readonly NullPropagation Instance = new NullPropagation(); /// - /// 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. /// /// - /// Passing the reference base straight through hands script the engine's internal sentinel for the - /// unresolvable state - a reading [[Unresolvable]] - rather than - /// undefined, which is documented on and - /// on . That is what scripts have always seen here, so it is kept and pinned - /// by a test; assigning instead would be the tidier behaviour but a - /// breaking change for existing tenant scripts. + /// The returned base is not undefined here but an internal Jint marker string that reads + /// [[Unresolvable]]. That is what scripts have always seen, so it is kept as it is and covered by + /// a test. Returning undefined would be nicer, but would change behavior for existing scripts. /// public bool TryUnresolvableReference(Engine engine, Reference reference, out JsValue value) { diff --git a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/WritableContext.cs b/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/WritableContext.cs index 3ccfcab70..4c248a78e 100644 --- a/backend/src/Squidex.Domain.Apps.Core.Operations/Scripting/WritableContext.cs +++ b/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( diff --git a/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JintScriptEngineTests.cs b/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JintScriptEngineTests.cs index fec93fd7b..5ed63d5e2 100644 --- a/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JintScriptEngineTests.cs +++ b/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JintScriptEngineTests.cs @@ -729,9 +729,8 @@ public class JintScriptEngineTests : IClassFixture [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); "; diff --git a/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JsonMapperTests.cs b/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JsonMapperTests.cs index 2872ebcd8..40ce7b1b4 100644 --- a/backend/tests/Squidex.Domain.Apps.Core.Tests/Operations/Scripting/JsonMapperTests.cs +++ b/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)); } diff --git a/backend/tests/Squidex.Domain.Apps.Core.Tests/TestHelpers/JintHostContractVerification.cs b/backend/tests/Squidex.Domain.Apps.Core.Tests/TestHelpers/JintHostContractVerification.cs index 3fe8ea3b0..6bcce4278 100644 --- a/backend/tests/Squidex.Domain.Apps.Core.Tests/TestHelpers/JintHostContractVerification.cs +++ b/backend/tests/Squidex.Domain.Apps.Core.Tests/TestHelpers/JintHostContractVerification.cs @@ -10,19 +10,17 @@ using System.Runtime.CompilerServices; namespace Squidex.Domain.Apps.Core.TestHelpers; /// -/// Turns on Jint's host-contract verifiers for this test assembly. +/// Turns on Jint's self checks for this test assembly. /// /// -/// 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. /// -/// 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. /// /// internal static class JintHostContractVerification