#62 step 1: the replaceable Dynamitey uses, replaced - #72
Merged
Merged
Conversation
Three of the five go; what is left is the two that cannot be reimplemented, only matched or dropped, which is what the major is for. - Dynamic.InvokeConstructor becomes DynamicConstructor.Invoke: a call site built from Binder.InvokeConstructor and cached per (type, argument count), so the constructor is still chosen by the arguments' runtime types. Not Activator.CreateInstance, whose overload resolution differs - a difference the suite already pins. - Dynamic.GenericDelegateType becomes a local pair of Func/Action tables, same arity rules and the same errors past them. - BinderHash.cs and BareBonesList.cs are deleted. Nothing referenced them. #62 called BinderHash public API and so a break to remove; it is internal, so it is neither. ActLikeMaker no longer imports Dynamitey at all. The remaining coupling is ActLikeProxy's IEquivalentType/AggreType and Util's InvokeContext, both type identity. 190 tests on net8.0 and net10.0, 196 on net47, 101 on browser-wasm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
It has one caller, Create<TTarget, TInterface>(params object[]), and no reason to be visible - adding public API in the same change that removes some for being needless surface was not the intent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
Member
Author
|
Made |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Named constructor arguments are not preserved, and replacement paths lack coverage for the claimed behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Replaces removable Dynamitey usage with cached constructor binding and local delegate tables.
Changes:
- Adds cached DLR-based constructor invocation.
- Inlines
Func/Actiondelegate lookup. - Removes unused internal binder and list helpers.
| File | Summary and review findings |
|---|---|
ImpromptuInterface/src/Optimization/DynamicConstructor.cs |
Adds cached constructor binding. Critical (1 vote): named InvokeArg metadata is dropped. Nit (1 vote): add coverage for overload, optional/named arguments, and value-type construction. |
ImpromptuInterface/src/Optimization/BinderHash.cs |
Removes unused internal binder hashing code. |
ImpromptuInterface/src/Optimization/BareBonesList.cs |
Removes unused internal list implementation. |
ImpromptuInterface/src/EmitProxy/ActLikeMaker.cs |
Uses the new constructor helper and local delegate tables. Critical (2 votes): named arguments are not preserved. Nit (1 vote): add public-path coverage for dispatch, optional/value-type handling, and caching. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public TInterface Create<TTarget, TInterface>(params object[] args) where TInterface : class | ||
| { | ||
| return this.ActLike(Dynamic.InvokeConstructor(typeof(TTarget), args)); | ||
| return this.ActLike(DynamicConstructor.Invoke(typeof(TTarget), args)); |
Comment on lines
+67
to
+70
| for (var i = 0; i < count; i++) | ||
| { | ||
| infos.Add(CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null)); | ||
| arguments.Add(Expression.ArrayIndex(argsParameter, Expression.Constant(i))); |
This was referenced Sep 22, 2026
This was referenced Sep 24, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1 of #62 — a minor-release step, no behaviour change. Three of the five uses go; the two that remain are the type-identity ones the major exists for.
Dynamic.InvokeConstructor(ActLikeMaker.cs:60)DynamicConstructor.InvokeDynamic.GenericDelegateType(ActLikeMaker.cs:1708)Func/ActiontablesString_OR_InvokeMemberName(BinderHash.cs)IEquivalentType/AggreType(ActLikeProxy.cs)InvokeContext(Util.cs)ActLikeMakerno longer importsDynamiteyat all.The constructor
DynamicConstructorbuilds a call site fromBinder.InvokeConstructorand caches it per (type, argument count) — the shape is what a site is compiled for, the arguments' runtime types are what it dispatches on. SoCreate<TTarget, TInterface>(params object[])still picks the constructor the same way C#'snew T(dynamicArg)does.Deliberately not
Activator.CreateInstance: its overload resolution is not the same, and the suite already pins a case where the two differ (SingleMethodInvoke.cs:198). The value-type parameterless case keeps theActivatorpath, because a dynamic invocation does not see that constructor — the same carve-out Dynamitey has.The delegate tables
GenericDelegateTypewas a lookup into two static arrays of openFunc/Actiontypes. It is now the same lookup, locally, with the same arity rules and the same exceptions past them. Only arities under 16 reach it;GenerateFullDelegatealready covers the rest.A correction to #62
That issue lists
BinderHash.csas public API, and therefore a breaking change to remove. It is not —BinderHash,GenericBinderHashBaseandBareBonesListare allinternal. So deleting them is neither breaking nor something to defer to the major, and both files are gone here. I will correct the issue.Verified: 190 tests on net8.0 and net10.0, 196 on net47, 101 on browser-wasm;
CI=truesolution build clean.🤖 Generated with Claude Code
https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg