Interop: a member filter that hides an indexer hides a wrapped collection's elements

Jint.Tests.PublicInterface/HostIndexerFilterTests.cs

@@ -1,13 +1,22 @@
+using System.Collections.Generic;
+using System.Reflection;
 using Jint.Native;
+using Jint.Runtime;
 using Jint.Runtime.Interop;
 
 namespace Jint.Tests.PublicInterface;
 
 /// <summary>
-/// A member filter that rejects an indexer must actually hide indexed access —
-/// the resolver's default-indexer fast path must consult the filter with the
-/// same polarity as the candidate scan below it.
+/// A member filter that rejects an indexer must actually hide indexed access — on a plain wrapped object
+/// and on a wrapped collection alike, and in every lane: reads, existence, writes, growth, deletion and the
+/// <c>Array.prototype</c> generics an array-like view attracts.
 /// </summary>
+/// <remarks>
+/// <see cref="TypeResolver.MemberFilter"/> is CLR-containment configuration: it is how a host says which
+/// members script may reach. Every engine below sets <c>AllowWrite = true</c>, because that is the
+/// configuration the containment question is actually asked in — with writes off, a write is refused for a
+/// reason that has nothing to do with the filter and proves nothing about it (#3558).
+/// </remarks>
 public class HostIndexerFilterTests
 {
     private sealed class IndexedHost
@@ -17,20 +26,35 @@ private sealed class IndexedHost
         public string Name => "host";
     }
 
+    /// <summary>
+    /// The filter the reproduction uses: everything except a property that takes index parameters.
+    /// </summary>
+    private static bool ExcludesIndexers(MemberInfo member)
+        => member is not PropertyInfo property || property.GetIndexParameters().Length == 0;
+
     private static Engine BuildEngine(bool allowIndexer)
     {
         var resolver = allowIndexer
             ? new TypeResolver()
-            : new TypeResolver
-            {
-                MemberFilter = static member => member is not System.Reflection.PropertyInfo property || property.GetIndexParameters().Length == 0,
-            };
+            : new TypeResolver { MemberFilter = ExcludesIndexers };
 
-        var engine = new Engine(options => options.Interop.TypeResolver = resolver);
+        var engine = new Engine(options =>
+        {
+            options.Interop.TypeResolver = resolver;
+            options.Interop.AllowWrite = true;
+        });
         engine.SetValue("host", new IndexedHost());
         return engine;
     }
 
+    private static (Engine Engine, List<long> List) BuildListEngine(bool allowIndexer)
+    {
+        var engine = BuildEngine(allowIndexer);
+        var list = new List<long> { 1, 2, 3 };
+        engine.SetValue("list", list);
+        return (engine, list);
+    }
+
     [Test]
     public void AMemberFilterExcludingTheIndexerHidesIndexedReads()
     {
@@ -58,25 +82,172 @@ public void TheDefaultConfigurationServesTheIndexer()
 
     [Test]
     public void AMemberFilterExcludingTheIndexerBlocksIndexedWrites()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        engine.Evaluate("list[0] = 42;");
+
+        list[0].Should().Be(1, "a filter-excluded indexer must not be written through");
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerBlocksTheStringSpellingOfAnIndexedWrite()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        engine.Evaluate("list['0'] = 42;");
+
+        list[0].Should().Be(1, "x[0] and x['0'] are one property key, so one filter decision answers both");
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerBlocksGrowth()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        engine.Evaluate("list[3] = 42;");
+
+        list.Should().HaveCount(3, "a write past the end reaches the collection through the same hidden indexer");
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerRefusesAnIndexedWriteInStrictMode()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        Invoking(() => engine.Evaluate("'use strict'; list[0] = 42;"))
+            .Should().Throw<JavaScriptException>("a refused [[Set]] is a TypeError in strict mode");
+
+        list[0].Should().Be(1);
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerHidesCollectionElements()
+    {
+        var (engine, _) = BuildListEngine(allowIndexer: false);
+
+        engine.Evaluate("list[0]").Should().Be(JsValue.Undefined);
+        engine.Evaluate("list['0']").Should().Be(JsValue.Undefined);
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerLeavesNoElementProperties()
+    {
+        var (engine, _) = BuildListEngine(allowIndexer: false);
+
+        // "in" is defined in terms of [[GetOwnProperty]], so these three may not disagree
+        engine.Evaluate("0 in list").AsBoolean().Should().BeFalse();
+        engine.Evaluate("list.hasOwnProperty(0)").AsBoolean().Should().BeFalse();
+        engine.Evaluate("Object.keys(list).length").AsNumber().Should().Be(0);
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerLeavesNothingToDelete()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        engine.Evaluate("delete list[0]").AsBoolean().Should().BeTrue("deleting an absent property succeeds");
+
+        list[0].Should().Be(1, "and it must not reach the collection to zero the slot");
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerBlocksTheArrayPrototypeGenerics()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: false);
+
+        Invoking(() => engine.Evaluate("Array.prototype.push.call(list, 9);"))
+            .Should().Throw<JavaScriptException>("push writes through the element lane the filter closed");
+
+        list.Should().Equal(1, 2, 3);
+    }
+
+    [Test]
+    public void AMemberFilterExcludingTheIndexerBlocksASortFromReachingTheCollection()
     {
         var engine = BuildEngine(allowIndexer: false);
-        var list = new System.Collections.Generic.List<long> { 1, 2, 3 };
+        var list = new List<long> { 3, 1, 2 };
         engine.SetValue("list", list);
 
         try
         {
-            engine.Evaluate("list[0] = 42;");
+            engine.Evaluate("Array.prototype.sort.call(list);");
         }
-        catch (Jint.Runtime.JavaScriptException)
+        catch (JavaScriptException)
         {
-            // a script-level rejection is fine
+            // a script-level refusal is the shape a hidden element lane owes a mutating generic
         }
-        catch (InvalidOperationException)
+
+        list.Should().Equal(new long[] { 3, 1, 2 }, "sort reorders through the element lane the filter closed");
+    }
+
+    [Test]
+    public void AFixedSizeArrayIsCoveredByTheSameDecision()
+    {
+        // LiveView, so the array crosses as a wrapper rather than as the JsArray copy the default
+        // ArrayConversion makes — a copy is a conversion of the value, not an access to a member, and no
+        // member filter speaks for it
+        var resolver = new TypeResolver { MemberFilter = ExcludesIndexers };
+        var engine = new Engine(options =>
         {
-            // the wrapper's pre-existing surface for writes that resolve to nothing;
-            // what this test pins is that the filtered-out indexer is never written through
-        }
+            options.Interop.TypeResolver = resolver;
+            options.Interop.AllowWrite = true;
+            options.Interop.ArrayConversion = ArrayConversionMode.LiveView;
+        });
 
-        list[0].Should().Be(1, "a filter-excluded indexer must not be written through");
+        var array = new long[] { 1, 2, 3 };
+        engine.SetValue("array", array);
+
+        engine.Evaluate("array[0]").Should().Be(JsValue.Undefined);
+        engine.Evaluate("array[0] = 42;");
+
+        array[0].Should().Be(1, "a CLR array declares no indexer of its own, so the decision is the one its IList indexer gets");
+    }
+
+    [Test]
+    public void AReadOnlyExposureIsCoveredByTheSameDecision()
+    {
+        var engine = BuildEngine(allowIndexer: false);
+        IReadOnlyList<long> view = new List<long> { 1, 2, 3 };
+        engine.SetValue("view", view);
+
+        engine.Evaluate("view[0]").Should().Be(JsValue.Undefined);
+    }
+
+    [Test]
+    public void TheDefaultConfigurationServesCollectionElements()
+    {
+        var (engine, list) = BuildListEngine(allowIndexer: true);
+
+        engine.Evaluate("list[0]").AsNumber().Should().Be(1);
+        engine.Evaluate("0 in list").AsBoolean().Should().BeTrue();
+
+        engine.Evaluate("list[0] = 42;");
+        list[0].Should().Be(42, "nothing here may cost an unfiltered engine its element lane");
+
+        engine.Evaluate("list[3] = 7;");
+        list.Should().Equal(42, 2, 3, 7);
+
+        engine.Evaluate("Array.prototype.push.call(list, 9);");
+        list.Should().Equal(42, 2, 3, 7, 9);
+    }
+
+    [Test]
+    public void AFilterThatKeepsTheIndexerKeepsTheElementLane()
+    {
+        // a filter that rejects something else entirely must not cost the collection its elements
+        var resolver = new TypeResolver { MemberFilter = static member => !string.Equals(member.Name, "Capacity", StringComparison.Ordinal) };
+        var engine = new Engine(options =>
+        {
+            options.Interop.TypeResolver = resolver;
+            options.Interop.AllowWrite = true;
+        });
+
+        var list = new List<long> { 1, 2, 3 };
+        engine.SetValue("list", list);
+
+        engine.Evaluate("list[0]").AsNumber().Should().Be(1);
+        engine.Evaluate("list[0] = 42;");
+        list[0].Should().Be(42);
     }
 }

Jint/Native/Array/ArrayOperations.cs

@@ -68,7 +68,11 @@ public static ArrayOperations For(ObjectInstance instance, bool forWrite)
             return new ArrayLikeObjectOperations(arrayLikeObject);
         }
 
-        if (instance is ArrayLikeWrapper arrayWrapper)
+        // HasIndexedElements rather than the type alone: a view whose indexer the host's member filter
+        // rejects has no elements to offer this lane, and falls through to ObjectOperations for the same
+        // reason a Queue<T> does — the generics honour the wrapper's length, read undefined at each index
+        // and take the wrapper's own [[Set]] refusal for each write (#3558).
+        if (instance is ArrayLikeWrapper { HasIndexedElements: true } arrayWrapper)
         {
             return new ArrayLikeOperations(arrayWrapper);
         }

Jint/Runtime/Interop/AGENTS.md

@@ -75,7 +75,7 @@ rest of the list is split across the files indexed from the repository-root [`AG
 - **A wrapper's prototype comes from the engine's *running* realm, so building it is realm-scoped work.** `JsValue.FromObject` reaches `ObjectInstance`'s base constructor, `TypeReference.CreateTypeReference` reaches `TypeReferencePrototype`, `DelegateWrapper` reads it outright — all three take `engine.Realm.Intrinsics`, which is `ExecutionContext.Realm` and therefore whichever realm happens to be current at the call. That is right for `Engine.SetValue`, where the realm being written *is* the current one, and wrong for anything projecting into a second realm: `ShadowRealm.SetValue` converted against the principal realm and installed on the shadow realm's global, so a host object handed to a realm was not `instanceof Object` inside it ([#3325](https://github.com/sebastienros/jint/issues/3325)). Jint has exactly one realm-scoped construction path — push that realm's `ExecutionContext` for the duration (`ShadowRealm.EnterRealm`, and `ShadowRealmImportValue` before it) — and no lighter one; `Engine._realmInConstruction` is not it, being neither nestable nor exception-safe. Three host-facing base constructors deliberately do **not** follow the running realm: `ClrFunction(Engine, …)`, `HostFunction` and `Constructor(Engine, string)` pin `engine._originalIntrinsics`, because a function a host builds against an `Engine` belongs to the realm the surrounding script can reach whatever was running when it was built ([#2893](https://github.com/sebastienros/jint/pull/2893)). The two rules do not conflict — one is about a realm named by the caller, the other about a realm that merely happened to be current — but a member built through those constructors *inside* a wrapper (its `toJSON`, its `Symbol.dispose`) still lands in the principal realm, which realm-scoping the wrapper does not change.
 - **Array-like is not indexable, and `TypeDescriptor.IsArrayLike` is the weaker of the two.** It means the target has a `Count`, nothing more: `ICollection`, `ICollection<T>` and `IReadOnlyCollection<T>` are count-and-copy contracts with no index in them, so `Queue<T>`, `Stack<T>`, `LinkedList<T>`, `SortedSet<T>` and `HashSet<T>` are all array-like with no element at index 0. Any lane that *reads by index* must gate on `ObjectWrapper.HasIndexedElements` instead — `ArrayOperations.For` gated on array-likeness and a bare `ICollection` and handed `IndexWrappedOperations` a target it then hard-cast to `IList`, which is [#3302](https://github.com/sebastienros/jint/issues/3302): a raw `InvalidCastException` out of `Evaluate` for every `Array.prototype` generic over a `Queue<T>`. Falling through to `ObjectOperations` is not a consolation prize — it asks the *object*, so a host collection that really does have an integer indexer still gets its elements through the reflected accessor, and only a genuinely index-less one reads `undefined` per index, which is what an array-like with no index properties means. The same rule governs `ObjectWrapper.HasOriginalIterator`: a wrapper's `Symbol.iterator` is never the array iterator (it enumerates the CLR target), so the index-reading fast path it enables for array destructuring may only stand in for `GetIterator` where index reads reproduce what enumeration yields — which is exactly `HasIndexedElements`.
 - **Overload scoring's last rule is the converter's own answer, and it has to be.** `InteropHelper.CalculateMethodParameterScore` rates an argument against a parameter with a dozen structural rules, and everything they did not recognize scored a blanket 100 — "will rarely succeed". `FindBestMatch` discards only a *negative* score, so 100 is a match, and a candidate the argument can never bind to is the *best* one whenever it is the only one. That is survivable where the caller retries — `MethodInfoFunction.TryCall` asks `converter.TryConvert` per candidate and moves on when it declines — and not where the caller takes the first match and stops, which `JintBinaryExpression.TryOperatorOverloading` and `TypeReference`'s constructor selection both do: `'s' + v` selected `op_Addition(T, T)` and died converting the string instead of concatenating, for any host type whose only `+` is `(T, T)` ([#3407](https://github.com/sebastienros/jint/issues/3407)). The last rule now asks the installed `ClrTypeConverter` — the very one `MethodDescriptor.Call` will use — so the score cannot claim a conversion the call then fails to perform, and only what it *confirms* keeps the 100. Three things follow. **What the 100 was protecting is three shapes no structural rule can see**: a JS function to a delegate parameter, an enum parameter given a number outside its defined members, and a conversion operator declared on the **target** type — the operator scan above it reads the *argument's* type only, while `DefaultTypeConverter.TryCastWithOperators` reads both. Each is pinned in `Jint.Tests.PublicInterface/HostOverloadScoringTests.cs`, and deleting the probe fails exactly those three and nothing else in the suite. A parameter type still carrying **open** type parameters (`T`, `Func<T, bool>`) is the one thing the converter cannot be asked — the closed type does not exist until `MethodInfoFunction.ResolveMethod` builds it, and handing an open one over is an `ArgumentException` rather than an answer — so those keep the undecided 100. And the probe *performs* the conversion rather than predicting it, so a user-defined `op_Implicit` on a candidate that is then selected runs twice; the `CanChangeType` rule above it already converts speculatively, so that is this function's established cost rather than a new one, but it is why nothing here may assume a conversion happens once.
-- **An index-shaped key on a wrapped bounded collection is the wrapper's to answer, in every lane.** The reflected indexer parses an index out of whatever key it is handed and takes it to the collection, so an out-of-range `x[3] = 9` was the CLR's own `ArgumentOutOfRangeException` out of `Evaluate` — invisible to a script `try`/`catch` and to a host `catch (JavaScriptException)` alike. `ArrayLikeWrapper` owns those keys now ([#3384](https://github.com/sebastienros/jint/issues/3384)), *including* the descriptor lane ([#3423](https://github.com/sebastienros/jint/issues/3423)): `Get`, `Set`, `HasProperty`, `Delete`, `GetOwnProperty`, `ProbeOwnProperty`, `DefineOwnProperty` and both key enumerations all answer from `Length`, and a lane added later that does not is a lane where `in` and `hasOwnProperty` contradict each other — which they may not, `OrdinaryHasProperty` being defined in terms of `[[GetOwnProperty]]`. A **plain** `ObjectWrapper` over a bounded target has no view and reads the target's own count instead ([#3422](https://github.com/sebastienros/jint/issues/3422)); that check is deliberately conditioned on two facts at once, because the same lane serves `Dictionary<int, string>`, where `d[99] = "x"` is a legitimate add, and a string-keyed indexer on a collection, where `x["3"]` names a key rather than a position. `ObjectWrapper.ClassifyElementKey` is the one definition of "index-shaped key" and lives on the base class for that reason; a second copy is the thing that would drift.
+- **An index-shaped key on a wrapped bounded collection is the wrapper's to answer, in every lane.** The reflected indexer parses an index out of whatever key it is handed and takes it to the collection, so an out-of-range `x[3] = 9` was the CLR's own `ArgumentOutOfRangeException` out of `Evaluate` — invisible to a script `try`/`catch` and to a host `catch (JavaScriptException)` alike. `ArrayLikeWrapper` owns those keys now ([#3384](https://github.com/sebastienros/jint/issues/3384)), *including* the descriptor lane ([#3423](https://github.com/sebastienros/jint/issues/3423)): `Get`, `Set`, `HasProperty`, `Delete`, `GetOwnProperty`, `ProbeOwnProperty`, `DefineOwnProperty` and both key enumerations all answer from `Length`, and a lane added later that does not is a lane where `in` and `hasOwnProperty` contradict each other — which they may not, `OrdinaryHasProperty` being defined in terms of `[[GetOwnProperty]]`. A **plain** `ObjectWrapper` over a bounded target has no view and reads the target's own count instead ([#3422](https://github.com/sebastienros/jint/issues/3422)); that check is deliberately conditioned on two facts at once, because the same lane serves `Dictionary<int, string>`, where `d[99] = "x"` is a legitimate add, and a string-keyed indexer on a collection, where `x["3"]` names a key rather than a position. `ObjectWrapper.ClassifyElementKey` is the one definition of "index-shaped key" and lives on the base class for that reason; a second copy is the thing that would drift. Owning the key also means owning the **containment** decision the reflected lane used to make for free: a view resolves no member per access, so `TypeResolver.MemberFilter` has to be asked about the indexer the lanes stand for, once per (resolver, type), and `ArrayLikeWrapper._elementsExposed` is that answer ([#3558](https://github.com/sebastienros/jint/issues/3558)). It is asked **before** `CanWrite`/`IsFixedSize`, because containment decides whether there is a property at all and writability only what may be done to one — a fixed-size array whose indexer is hidden must report "no such property", not the `TypeError` naming its bounds. `HasIndexedElements` carries it, so a hidden element lane routes every `Array.prototype` generic to `ObjectOperations` exactly as a `Queue<T>` is routed. What the filter does *not* speak for, and must not be extended to without a decision: `length` (produced from `Count`, filtered separately), iteration (`GetEnumerator`), and `ArrayConversionMode.Copy`, which converts a `T[]` before any member is touched.
 
 ### `[JsAccessible]`: the generated lane, and why it is equivalent rather than merely fast