-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DefaultBinder named parameter support skipping default parameters #71013
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/area-system-reflection Issue DetailsDefaultBinder supports named parameters and parameters with defaults could be ignored, but only if the named parameters provided in order Example: public static class Sample
{
public static void SampleMethod(string param1, int param2 = 0, bool param3 = false) { }
}
public void Test( )
{
var methods = typeof(Sample).GetMethods();
var flags = BindingFlags.Public | BindingFlags.Static;
// Binding the named parameters in order works fine.
var methodArgs = new object[] { "value", 2 };
var paramNames = new string[] {"param1", "param2"}
var method = Type.DefaultBinder.BindToMethod(flags, methods, ref methodArgs, null, null, paramNames , out var _);
// Skipping an optional parameter `param2` causes IndexOutOfRangeException exception
methodArgs = new object[] { "value", true };
paramNames = new string[] {"param1", "param3"}
var method = Type.DefaultBinder.BindToMethod(flags, methods, ref methodArgs, null, null, paramNames , out var _); The PR fixes the above issue and also a case where Fixes #66237
|
Test failures unrelated dotnet/winforms#8829 and reported #13757 |
@@ -60,7 +60,7 @@ internal partial class DefaultBinder : Binder | |||
if (names == null) | |||
{ | |||
// Default mapping | |||
for (j = 0; j < args.Length; j++) | |||
for (j = 0; j < par.Length; j++) | |||
paramOrder[i][j] = j; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paramOrder[i][j]
array keeps track of real(defined) position of parameters, in case args
count is less than defined parameters its not tracked, in order to support any number of named parameters defined in any order we need to keep track of all parameters
if (pCls == argTypes[paramOrder[i][j]]) | ||
continue; | ||
int index = paramOrder[i][j]; | ||
if (index < args.Length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case only 1st and 3rd named arguments provided from the 4 parameters the order in paramOrder[i]
will be {0, 2, 1, 3} and argTypes[paramOrder[i][1]]
will cause IndexOutOfBoundsException
, therefore first need to check if current parameter is within provided arguments range
if (argTypes[paramOrder[i][j]] == null) | ||
continue; | ||
|
||
if (!pCls.IsAssignableFrom(argTypes[paramOrder[i][j]])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until this row nothing is changed except using index
I defined above: int index = paramOrder[i][j];
instead of each paramOrder[i][j]
access
continue; | ||
|
||
if (!pCls.IsAssignableFrom(argTypes[paramOrder[i][j]])) | ||
if (defaultValueBinding && par[j].HasDefaultValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case index
not within the provided arguments range (index >= args.Length
) allow it to continue only if the parameter has default value and BindingFlags.OptionalParamBinding
is set
objs[i] = Array.CreateInstance(paramArrayTypes[0], 0); // create an empty array for the | ||
|
||
else | ||
objs[i] = parms[i].DefaultValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the provided args count is less than defined parameters the missing arguments filled with defaults, but it was not supporting skipped arguments, the update fixes that
|
||
for (int i = 0; i < vars.Length; i++) | ||
vars[i] = varsCopy[paramOrder[i]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method reorders named arguments in parameter order, again it was not supporting if any of the arguments skipped
src/libraries/System.Runtime/tests/System/Reflection/DefaultBinderTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/DefaultBinderTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/DefaultBinder.cs
Outdated
Show resolved
Hide resolved
Failure looks unrelated, filed an issue, merging |
DefaultBinder supports named parameters and parameters with defaults could be ignored, but only if the named parameters provided in order Example:
The PR fixes the above issue and also a case where
all the unspecified parameters have default values
and further makes sure the binding updates work withType.InvokeMember(...)
Fixes #66237