Strings: a lazily materialised host string has a base class, instead of passing null to a string parameter

Jint.Tests.PublicInterface/LazyHostStringTests.cs

@@ -1,17 +1,36 @@
+using System;
 using System.Text;
 using Jint.Native;
 
 namespace Jint.Tests.PublicInterface;
 
 /// <summary>
-/// A host can expose its own string representation by deriving from <see cref="JsString"/> and passing a
-/// null backing value to the base constructor, materializing the flat .NET string only when something
-/// actually asks for it. These tests pin that contract for the engine paths a script can reach.
+/// A host can expose its own string representation by deriving from <see cref="LazyJsString"/>: it declares
+/// the length up front and implements one <c>Materialize()</c> method, and the flat .NET string is produced
+/// only when something actually asks for the characters. These tests pin that contract for the engine paths
+/// a script can reach.
+///
+/// <para>
+/// Before <see cref="LazyJsString"/> the same thing was written by passing <see langword="null"/> to
+/// <see cref="JsString(string)"/> — whose parameter is typed <see cref="string"/> — and overriding
+/// <c>ToString()</c>, <c>Length</c> and the indexer, with the memoization written by hand in each host. That
+/// still works and <see cref="RavenApiUsageTests"/> still exercises it; what it never had was a signature, a
+/// place for the contract to be written down, or anything that checked it.
+/// </para>
 /// </summary>
 public class LazyHostStringTests
 {
     private const string Text = "abc";
 
+    /// <summary>
+    /// Whether this run has host-contract verification on. Public and static so xUnit can read it for
+    /// <c>SkipUnless</c>.
+    /// </summary>
+    public static bool Verifying => HostContractVerificationSwitch.Enabled;
+
+    /// <inheritdoc cref="Verifying" />
+    public static bool NotVerifying => !Verifying;
+
     private static Engine CreateEngine(out LazyHostString host, string value = Text)
     {
         host = new LazyHostString(value);
@@ -67,7 +86,7 @@ public void ReportsLengthWithoutMaterializing()
 
         engine.Evaluate("host.length").AsNumber().Should().Be(3);
 
-        host.MaterializationCount.Should().Be(0, "an overridden Length must not force the flat value");
+        host.MaterializationCount.Should().Be(0, "the length was declared to the constructor, so nothing has to be produced to answer it");
     }
 
     [Fact]
@@ -153,42 +172,206 @@ public void SupportsCommonStringPrototypeMethods()
     }
 
     /// <summary>
-    /// Stands in for a host string backed by a native handle: the payload is kept encoded and only
-    /// decoded on demand. The backing value handed to the base constructor is null, so every member
-    /// that needs the text has to route through <see cref="ToString"/>.
+    /// The laziness itself, expressed as a count. Every script below is answered from the declared length
+    /// (or, for the indexer, from the host's own encoded buffer) and never asks for the text.
     /// </summary>
-    private sealed class LazyHostString : JsString
+    [Theory]
+    [InlineData("host.length")]
+    [InlineData("for (var i = 0, n = 0; i < host.length; i++) n++;")]
+    [InlineData("typeof host")]
+    [InlineData("!!host")]
+    [InlineData("host ? 1 : 0")]
+    [InlineData("host[0]")]
+    [InlineData("host.charAt(1)")]
+    [InlineData("host.charCodeAt(2)")]
+    [InlineData("host === host")]
+    [InlineData("host === 'abcd'")]
+    [InlineData("host !== 'ab'")]
+    [InlineData("host == 'abcd'")]
+    public void NothingThatOnlyNeedsTheLengthMaterializes(string script)
+    {
+        var engine = CreateEngine(out var host);
+
+        engine.Evaluate(script);
+
+        host.MaterializationCount.Should().Be(0);
+    }
+
+    [Fact]
+    public void ComparingTwoLazyStringsOfDifferentLengthsMaterializesNeither()
+    {
+        var shorter = new LazyHostString("abc");
+        var longer = new LazyHostString("abcd");
+        var engine = new Engine();
+        engine.SetValue("a", shorter);
+        engine.SetValue("b", longer);
+
+        engine.Evaluate("a === b").AsBoolean().Should().BeFalse();
+
+        shorter.MaterializationCount.Should().Be(0, "string equality compares lengths first, and both answer that from the declared length");
+        longer.MaterializationCount.Should().Be(0);
+    }
+
+    [Fact]
+    public void ReadingTheTextMaterializesExactlyOnceAndStaysThere()
+    {
+        var engine = CreateEngine(out var host);
+
+        engine.Evaluate("String(host)").AsString().Should().Be(Text);
+        host.MaterializationCount.Should().Be(1);
+
+        engine.Evaluate("host.toUpperCase() + host.slice(1) + JSON.stringify(host) + (host === 'abd')");
+        host.MaterializationCount.Should().Be(1, "the base class memoizes what Materialize() returned");
+    }
+
+    /// <summary>
+    /// The other half of the count, and the part a host has to plan for: three script-visible operations
+    /// genuinely need the characters and therefore materialize. Using the value as a **property key** is the
+    /// one that surprises — a key is a string plus its ordinal hash, and neither can be answered from a
+    /// length — so <c>host in obj</c>, <c>obj[host] = …</c> and a computed key in an object literal all
+    /// produce the text. So does anything that writes it out: <c>JSON.stringify</c>, string concatenation,
+    /// and every <c>String.prototype</c> method that reads more than one character.
+    /// </summary>
+    [Theory]
+    [InlineData("host in { abc: 1 }")]
+    [InlineData("host in { zzz: 1 }")]
+    [InlineData("var o = {}; o[host] = 1;")]
+    [InlineData("({ [host]: 1 })")]
+    [InlineData("({ abc: 7 })[host]")]
+    [InlineData("new Map().set(host, 1)")]
+    [InlineData("JSON.stringify(host)")]
+    [InlineData("JSON.stringify({ value: host })")]
+    [InlineData("JSON.stringify([host])")]
+    [InlineData("host + ''")]
+    [InlineData("`${host}`")]
+    [InlineData("host === 'abd'")]
+    [InlineData("host.toUpperCase()")]
+    [InlineData("String(host)")]
+    public void TheOperationsThatNeedTheCharactersMaterializeExactlyOnce(string script)
+    {
+        var engine = CreateEngine(out var host);
+
+        engine.Evaluate(script);
+
+        host.MaterializationCount.Should().Be(1);
+    }
+
+    [Fact]
+    public void MaterializeIsCalledOncePerInstanceAcrossEveryRead()
+    {
+        var engine = CreateEngine(out var host);
+
+        engine.Evaluate("var o = {}; o[host] = 1; JSON.stringify(o) + host.toUpperCase() + (host === 'abd') + (host + '!')");
+
+        host.MaterializationCount.Should().Be(1);
+    }
+
+    [Fact]
+    public void ADeclaredLengthThatIsNotAStringLengthIsRefusedAtConstruction()
     {
-        // ASCII only, which keeps the encoded length equal to the character length so that Length
-        // can be answered without decoding - the whole point of holding the payload encoded.
+        Invoking(() => new FixedLengthString(-1, "abc")).Should().Throw<ArgumentOutOfRangeException>()
+            .WithParameterName("length");
+
+        Invoking(() => new FixedLengthString(int.MaxValue, "abc")).Should().Throw<ArgumentOutOfRangeException>()
+            .WithParameterName("length");
+    }
+
+    [Fact]
+    public void AMaterializeThatReturnsNullIsRefusedByName()
+    {
+        var engine = new Engine();
+        engine.SetValue("host", new NullMaterializingString());
+
+        Invoking(() => engine.Evaluate("String(host)")).Should().Throw<InvalidOperationException>()
+            .WithMessage("*NullMaterializingString*Materialize*null*");
+    }
+
+    /// <summary>
+    /// The declared length is what every length-first shortcut answers from, so a host that declares one
+    /// length and produces another breaks them silently: the value compares unequal to a string it equals,
+    /// and an index the engine considers in range reads past the end. Host-contract verification catches the
+    /// disagreement at the moment both answers exist.
+    /// </summary>
+    [Fact(Skip = "host-contract verification is off in this run", SkipUnless = nameof(Verifying))]
+    public void ADeclaredLengthThatContradictsTheTextIsCaughtWhenVerifying()
+    {
+        var engine = new Engine();
+        engine.SetValue("host", new FixedLengthString(3, "abcdef"));
+
+        Invoking(() => engine.Evaluate("String(host)")).Should().Throw<InvalidOperationException>()
+            .WithMessage("*FixedLengthString*3*6*");
+    }
+
+    /// <inheritdoc cref="ADeclaredLengthThatContradictsTheTextIsCaughtWhenVerifying" />
+    /// <remarks>
+    /// The other side of the same gate: with verification off the check is not merely skipped, it is not
+    /// emitted, so the lie goes through and the host sees the broken behaviour the verifier describes.
+    /// </remarks>
+    [Fact(Skip = "host-contract verification is on in this run", SkipUnless = nameof(NotVerifying))]
+    public void ADeclaredLengthThatContradictsTheTextIsNotCheckedWhenNotVerifying()
+    {
+        var engine = new Engine();
+        engine.SetValue("host", new FixedLengthString(3, "abcdef"));
+
+        engine.Evaluate("String(host)").AsString().Should().Be("abcdef");
+        engine.Evaluate("host.length").AsNumber().Should().Be(3);
+    }
+
+    /// <summary>
+    /// Stands in for a host string backed by a native handle: the payload is kept encoded and only decoded
+    /// on demand. The only members it declares are the length (to the base constructor),
+    /// <see cref="Materialize"/>, and the indexer — which it overrides because reading one byte out of the
+    /// buffer is cheaper than decoding the whole value.
+    /// </summary>
+    private sealed class LazyHostString : LazyJsString
+    {
+        // ASCII only, which keeps the encoded length equal to the character length so that the declared
+        // length is knowable without decoding - the whole point of holding the payload encoded.
         private readonly byte[] _encoded;
-        private string _materialized;
         private int _materializationCount;
 
-        public LazyHostString(string value) : base(null)
+        public LazyHostString(string value) : base(value.Length)
         {
             _encoded = Encoding.ASCII.GetBytes(value);
         }
 
         /// <summary>
-        /// How many times the flat value had to be produced. Used by the tests to assert that
-        /// members which can answer lazily really do.
+        /// How many times the flat value had to be produced. Used by the tests to assert that members
+        /// which can answer lazily really do, and that the base class memoizes the one that cannot.
         /// </summary>
         public int MaterializationCount => _materializationCount;
 
-        public override string ToString()
+        protected override string Materialize()
         {
-            if (_materialized is null)
-            {
-                _materializationCount++;
-                _materialized = Encoding.ASCII.GetString(_encoded);
-            }
+            _materializationCount++;
+            return Encoding.ASCII.GetString(_encoded);
+        }
+
+        public override char this[int index] => (char) _encoded[index];
+    }
+
+    /// <summary>
+    /// Declares whatever length it is told to, so the constructor's validation and the length-agreement
+    /// verifier can both be reached from a test.
+    /// </summary>
+    private sealed class FixedLengthString : LazyJsString
+    {
+        private readonly string _text;
 
-            return _materialized;
+        public FixedLengthString(int declaredLength, string text) : base(declaredLength)
+        {
+            _text = text;
         }
 
-        public override int Length => _encoded.Length;
+        protected override string Materialize() => _text;
+    }
+
+    private sealed class NullMaterializingString : LazyJsString
+    {
+        public NullMaterializingString() : base(3)
+        {
+        }
 
-        public override char this[int index] => (char) _encoded[index];
+        protected override string Materialize() => null;
     }
 }

Jint.Tests.PublicInterface/RavenApiUsageTests.cs

@@ -235,6 +235,14 @@ function output(doc) {
     }
 }
 
+/// <summary>
+/// A lazy host string written the pre-<see cref="LazyJsString"/> way: derived straight from
+/// <see cref="JsString"/>, with <see langword="null"/> passed to a constructor whose parameter is typed
+/// <see cref="string"/>, and <c>ToString()</c>, <c>Length</c> and the indexer overridden by hand. This is the
+/// shape RavenDB has in its own tree, and it is kept exactly as it is on purpose: <see cref="LazyJsString"/>
+/// is the supported spelling now, but the old one has to keep working, and this is what proves it does.
+/// <see cref="LazyHostStringTests"/> covers the new one.
+/// </summary>
 file sealed class CustomString : JsString
 {
     private readonly string _value;

Jint.Tests.PublicInterface/Verify/PublicApiTest_net10.0.verified.txt

@@ -1243,6 +1243,14 @@ namespace Jint.Native
         public static bool operator !=(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
         public static bool operator ==(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
     }
+    public abstract class LazyJsString : Jint.Native.JsString
+    {
+        protected LazyJsString(int length) { }
+        public override char this[int index] { get; }
+        public override int Length { get; }
+        protected abstract string Materialize();
+        public override sealed string ToString() { }
+    }
     public delegate Jint.Native.JsValue LazySlotFactory(Jint.Native.JsObject instance, object? state);
     public abstract class Prototype : Jint.Native.Object.ObjectInstance { }
 }

Jint.Tests.PublicInterface/Verify/PublicApiTest_net472.verified.txt

@@ -1094,6 +1094,14 @@ namespace Jint.Native
         public static bool operator !=(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
         public static bool operator ==(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
     }
+    public abstract class LazyJsString : Jint.Native.JsString
+    {
+        protected LazyJsString(int length) { }
+        public override char this[int index] { get; }
+        public override int Length { get; }
+        protected abstract string Materialize();
+        public override sealed string ToString() { }
+    }
     public delegate Jint.Native.JsValue LazySlotFactory(Jint.Native.JsObject instance, object? state);
     public abstract class Prototype : Jint.Native.Object.ObjectInstance { }
 }

Jint.Tests.PublicInterface/Verify/PublicApiTest_net8.0.verified.txt

@@ -1243,6 +1243,14 @@ namespace Jint.Native
         public static bool operator !=(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
         public static bool operator ==(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
     }
+    public abstract class LazyJsString : Jint.Native.JsString
+    {
+        protected LazyJsString(int length) { }
+        public override char this[int index] { get; }
+        public override int Length { get; }
+        protected abstract string Materialize();
+        public override sealed string ToString() { }
+    }
     public delegate Jint.Native.JsValue LazySlotFactory(Jint.Native.JsObject instance, object? state);
     public abstract class Prototype : Jint.Native.Object.ObjectInstance { }
 }

Jint.Tests.PublicInterface/Verify/PublicApiTest_netstandard2.0.verified.txt

@@ -1094,6 +1094,14 @@ namespace Jint.Native
         public static bool operator !=(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
         public static bool operator ==(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
     }
+    public abstract class LazyJsString : Jint.Native.JsString
+    {
+        protected LazyJsString(int length) { }
+        public override char this[int index] { get; }
+        public override int Length { get; }
+        protected abstract string Materialize();
+        public override sealed string ToString() { }
+    }
     public delegate Jint.Native.JsValue LazySlotFactory(Jint.Native.JsObject instance, object? state);
     public abstract class Prototype : Jint.Native.Object.ObjectInstance { }
 }

Jint.Tests.PublicInterface/Verify/PublicApiTest_netstandard2.1.verified.txt

@@ -1094,6 +1094,14 @@ namespace Jint.Native
         public static bool operator !=(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
         public static bool operator ==(Jint.Native.JsValue? a, Jint.Native.JsValue? b) { }
     }
+    public abstract class LazyJsString : Jint.Native.JsString
+    {
+        protected LazyJsString(int length) { }
+        public override char this[int index] { get; }
+        public override int Length { get; }
+        protected abstract string Materialize();
+        public override sealed string ToString() { }
+    }
     public delegate Jint.Native.JsValue LazySlotFactory(Jint.Native.JsObject instance, object? state);
     public abstract class Prototype : Jint.Native.Object.ObjectInstance { }
 }