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
Fix up .NET method invocation with Optional arg #21387
base: master
Are you sure you want to change the base?
Conversation
Fix .NET method invocation with an argument that has an [Optional] attribute but no default value.
// If the method contains just [Optional] without a default value set then we cannot use | ||
// Type.Missing as a placeholder. Instead we use the default value for that type. Only | ||
// exception to this rule is when the parameter type is object. | ||
argExprs[i] = Expression.Default(parameterType); |
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.
Please reword the comment.
It seems your comment in the PR description is more clear about behavior with object
When it comes to the compiled assembly the ParameterInfo.DefaultValue for an argument with only [Optional] is System.Reflection.Missing but this value cannot be passed as an argument unless the type is object because it cannot be casted to the argument type. Instead using null or the default value for that type should be used.
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.
I've updated the comment, please let me know if there's any further improvements or clarifications it should have.
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.
Thanks!
Please remove unrelated style and formatting changes. Our docs ask to pull separate PRs such changes.
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.
Ah sorry, that last host I made this change on was set to format on save, will undo those.
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.
Done
Thanks for the review @iSazonov |
The Engine WG discussed this and agree with the change and it is ready to be reviewed |
This is a new feature and hence does not meet the bar for backport. It is also in a fairly risky area. |
PR Summary
Fix .NET method invocation with an argument that has an [Optional] attribute but no default value.
Fixes: #21372
PR Context
While not common .NET has the ability to set an optional argument but no explicit value https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments#optional-arguments
The docs for F# even recommend this attribute when it comes to F# assemblies that could be used in C# and other .NET languages https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/parameters-and-arguments#optional-parameters
When it comes to the compiled assembly the
ParameterInfo.DefaultValue
for an argument with only[Optional]
is System.Reflection.Missing but this value cannot be passed as an argument unless the type isobject
because it cannot be casted to the argument type. Instead usingnull
or thedefault
value for that type should be used.This is a good candidate to backport.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).