Add support for named argument with .NET Methods#21487
Add support for named argument with .NET Methods#21487jborean93 wants to merge 38 commits intoPowerShell:masterfrom
Conversation
|
After testing it seems like the existing COM code can handle named arguments already it just needed to be plumbed through. I've updated the logic for populating the |
I can definitely understand the pros of making it case insensitive. If the pwsh team favours that over case sensitivity I can update the PR to be so but we would have to determine what happens when such a method is encountered, e.g.
I do think the probabilities of such a scenario happening are pretty small so it's worth considering whether to make it case sensitive, we just need to make sure we don't paint ourselves into a corner here. |
|
Looks like it is possible to have reflection build a method with arguments of the same name so we'll have to solve the name conflict problem in any case. |
|
I've just pushed a commit that makes it case insensitive as it looked like we needed to deal with collisions anyway so having it fit in with PowerShell's case insensitivity made more sense. When there is a conflict the overload selector will just append the Add-Type -TypeDefinition @'
using System;
public class TestClass
{
public static string Method(string arg, string Arg, string aRg) => $"{arg}{Arg}{aRg}";
}
'@
[TestClass]::Method(arg: 'foo', arg_: 'bar', arg__: 'test')I did not touch COM as that's complicated as heck. It only works because of the code copied from .NET already supported named args from the binder, so that stays case sensitive. I'll update the summary to include this assumption and the behaviour around conflicts. |
|
I give up on trying to deal with CI, will rebase when #21463 is merged but that's the last remaining failure which is unrelated to the changes here. |
|
@jborean93 I've made a PR with some change suggestions for the parsing here: jborean93#2 feel free to take a look. |
|
I've added completion for this in my own branch here: https://github.com/MartinGC94/PowerShell/tree/NamedArgCompletion that people can test out if they want to. I'll create a PR for it once this one gets merged.
|
|
Awesome, I'll have to try it out and see how I go. It's a bit hard at the moment to really test out console things on Linux as PowerShell is still tracking an old preview release so suffers from a problem rendering on the console once you reach 80 or so chars. |
vexx32
left a comment
There was a problem hiding this comment.
I really don't think I have any notes on this one. This looks really damn good. Fantastic job @jborean93 💜
|
Just an FYI, the Engine WG discussed this today but we need more time now that we've fully dug into the meat of it, and will continue discussions in the next WG meeting. |
There was a problem hiding this comment.
In general, I think this looks good.
Regarding naming: I think we can come up with a better name than LabeledExpession.
Compare where we have a NamedAttributeArgumentAst. Label makes me think of goto :).
How about NamedMethodArgumentExpressionAst?
Do we need to add to add an AstVisitor3 and ICustomAstVisitor3?
Edit: I read again and saw the calls to Default(...). We should be good.
|
@powercode thanks for having a look through the PR!
While right now the Ast parser only creates this for named arguments my thinking was that this Ast could be re-used in the future for any other expression location where we might want to support a name/label. I went with label here because we somewhat already use it as a term for loops where they can have a label attached to the loop itself If we wanted to restrict this Ast to method argument expressions only then I’m happy with your suggestion |
|
I think they are semantically different and that you want separate asts for different semantics. Consider an AstSearcher, where you want to find certain expressions. Reused asts then becomes a hindrance, not a help. Overall, however, I concur with Rain - great work |
a0c1c2d to
b1fd19e
Compare
|
Rebased again onto the latest changes in
Especially since 7.7 won't be an LTS release, even more time to make sure that any regressions that may be present are found and fixed before the next LTS. |
SeeminglyScience
left a comment
There was a problem hiding this comment.
Isn't this a great time to merge this PR?
Loads of time until next release!
Definitely agree, I've recently been trying to make my way this PR as it'd be awesome to get it into 7.7 as early as possible. I'm about halfway through and will submit the pending feedback I have so far (though almost entirely small nits or just praise. Excellent work thus far @jborean93 ❤️)
| // G array-literal-expression is not allowed - the comma is used | ||
| // G to separate argument-expressions. | ||
| // G argument-label-expression: | ||
| // G simple-name ':' |
There was a problem hiding this comment.
Appreciate the attention to detail getting these updated as well ❤️
| CheckMemberAccess(memberExpressionAst); | ||
| if (memberExpressionAst.Arguments is not null) | ||
| { | ||
| var existingArguments = new HashSet<string>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
| var existingArguments = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |
| HashSet<string>? existingArguments = null; |
Can be lazily inited to save an allocation in the common case of no named arguments. Unlikely to be all that impactful so open to ignoring if you think the resulting code is too complex
| foreach (ExpressionAst argument in memberExpressionAst.Arguments) | ||
| { | ||
| if (argument is NamedMethodArgumentAst namedArgumentExpression) | ||
| { |
There was a problem hiding this comment.
| { | |
| { | |
| existingArguments ??= new(StringComparer.OrdinalIgnoreCase); |
if previous suggestion taken
| namedArgumentExpression.Label.Value); | ||
| } | ||
| } | ||
| else if (existingArguments.Count > 0) |
There was a problem hiding this comment.
| else if (existingArguments.Count > 0) | |
| else if (existingArguments is { Count: > 0 }) |
if previous suggestion taken (handles null case)
There was a problem hiding this comment.
reminder to myself to check that this handles non-null defaults. I assume it does and I just haven't gotten there yet (presumably does not treat the argument value as null, but rather because there's no dynamic meta object at all, it's treated differently


PR Summary
Adds support for using named arguments when calling .NET methods.
For example:
Through named args we can provide optional parameters without relying on the position or by specifying all preceding defaults:
What is not in scope of this PR
Support for named arguments for other method binders (COM)- COM is already piped in, other binder can work already if they supportCallInfo.ArgumentNames.PSScriptMethodcould be expanded in the future to support passing in args through a parameter namePSCodeMethodcould be expanded in the future to support the same optional argument passingPSMethod, this aligns to how Generics are handled for these method invocationsPR Context
Issue to track this has closed due to no activity but it is still relevant #13307 and #7506 (sans splatting).
The changes here are done through
LabeledExpressionAstwhich is only parsed on method invocationsAssumptions Made
Named labels only support the simple name rules in PowerShell
A simple label supports the a subset of the rules as a C# identifier.
A quick test shows chars in the
Nl,Pc,Mn,Mc, andCfUnicode categories might be invalid.Future work could be introduced to expand support these invalid chars through an escape char like
\u0000,\U00000000, or the backtick ```u{}`` syntax but considering the rarity of these identifiers I'm not sure it's worth the effort.Named labels cannot be following by unnamed/positional argument
When specifying a named argument, any subsequent args must also be named.
While C# allows named arguments before positional arguments it is only if the name also aligns with the position.
Supporting such a scenario is not feasible in PowerShell as:
CallInfo.ArgumentNamesonly allows names at the end of the argumentsdynamicclass doesn't allow this syntax due to the aboveNamed argument can be in any order but positional takes priority
Further to the above, the order of the named args do not matter but they will only apply after the positional arguments are checked.
For example the overload
The above will not work because
file.txtwill be provided topath,Openwill be set tomode, making the next named argument invalid becausemodeis already specified.Will work as the first positional argument is set to the
pathargument while the rest are all named and relate to the remaining args.Named arguments are case insensitive
Edit: This was originally case sensitive but was changed in a recent commit base on feedback.
This follows the normal standard in PowerShell where most things are case insensitive allowing you to do
This only applies to the .NET binder, COM will be case sensitive as well as any other customer binder that is used as the original case is preserved in the AST and it's up to the binder to decide on how to treat them.
Unnamed arguments in .NET cannot be used with a name
Edit: originally an automatic name based on the index like
arg$idxwas used but based on feedback the logic was removed.It is possible to use reflection to define a method where each argument has no name.
Such method parameters need to be invoked positionally.
Name conflicts have the first one wins logic
Edit: originally a suffix like setup was used to ensure a unique name but based on feedback the logic was removed.
While rare it is possible to have a collision with the argument names when the method uses arguments that differ in case only, or the method was defined using
MethodBuilderand reflection with the same nameThese scenarios would be quite rare but it's still something that should be considered.
In the case of a conflict the named entries apply after the positional binding is done and from there the first match wins.
For example this C# signature shows 2 arguments that are the same when treated case insensitively
This is how the logic works based on this PR
Things are more complex for reflection based methods where there can be more than 2 matches but the logic is the same.
The named argument will be bound to the first entry after positional binding is done.
Tests have been added for this to ensure no regression in the logic but it is a very rare edge case due to how hard it is to build such methods.
A named param argument must be set as an array if multiple values are specified
When specifying a value for a params argument through a named arg the value supplied can only be provided through the one argument value.
Just like a normal param specification this value can be a single value which is casted to the array at runtime or as the array itself.
This behaviour is the same as how a positional params argument can be supplied when only as one argument.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).