Skip to content

The delegate-conversion tests all miss the instance-keyed cache - #3566

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:test/delegate-conversion-same-engine-two-types
Sep 1, 2026
Merged

The delegate-conversion tests all miss the instance-keyed cache#3566
lahma merged 1 commit into
sebastienros:mainfrom
lahma:test/delegate-conversion-same-engine-two-types

Conversation

@lahma

@lahma lahma commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

#3521 keyed both delegate caches by the target type. HostDelegateConversionTests pins it through three shapes — two engines on one Prepared<Script>, the same in the other order, and the two alternating — and every one of them gives each engine a function of its own.

That means all three exercise the AST-keyed cache. None exercises the instance-keyed one: no test asks a single function instance for two different delegate types, which is the entire reason the outer ConditionalWeakTable<object, TypeKeyedCache<Delegate>> carries a TypeKeyedCache rather than a bare Delegate.

The gap, measured

Keying only the instance lane on a constant — so it behaves as it did before #3521 while the AST lane stays keyed by target type:

-if (!boundDelegates.TryGetValue(type, out var bound))
+if (!boundDelegates.TryGetValue(typeof(object), out var bound))
...
-bound = boundDelegates.GetOrAdd(type, targetBinder(functionInstance)!);
+bound = boundDelegates.GetOrAdd(typeof(object), targetBinder(functionInstance)!);
Passed!  - Failed: 0, Passed: 3

All three existing tests stay green. A simplification that folded the instance lane back to a plain weak table would have landed unnoticed.

The test added here fails under exactly that mutation, with the defect's own message:

Failed OneEngineConvertingOneFunctionToTwoDelegateTypes
  System.ArgumentException : Object of type 'Notify' cannot be converted to type 'Transform'.

and passes on main as it stands. It also converts back to the first type afterwards, so it says the two entries coexist rather than evict each other.

Test-only; no production change. Found while auditing process-wide caches for #3426 — I had an equivalent local fix and test predating #3521, and this is the only part of it main did not already have.

Verification

Jint.Tests.PublicInterface HostDelegateConversionTests: 4/0 on net472, net8.0 and net10.0.

🤖 Generated with Claude Code

https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S

…eparation tests cannot reach

`HostDelegateConversionTests` covers sebastienros#3434 through three shapes, and all three give each engine a function
of its own. That exercises the AST-keyed cache — the cross-engine half — and leaves the instance-keyed one
untested: no test asks a single function *instance* for two different delegate types, which is the case the
outer `ConditionalWeakTable<object, TypeKeyedCache<Delegate>>` exists to answer.

Measured rather than assumed. Keying only that lane on a constant, so the instance cache is un-keyed while
the AST cache stays keyed by target type, leaves all three existing tests green and fails nothing:

    Passed: 3, Failed: 0

The test added here fails under exactly that mutation, with the defect's own message —
`ArgumentException: Object of type 'Notify' cannot be converted to type 'Transform'` — so a future
simplification that folds the instance lane back to a plain weak table now has something that notices.

Test-only; no production change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S
@lahma
lahma merged commit 0073c25 into sebastienros:main Sep 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant