-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Add support for named argument with .NET Methods #21487
base: master
Are you sure you want to change the base?
Conversation
Looks like this affects COM, will have to test it out a bit more to ensure that unnamed argument continue to work as before. |
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. |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
488d140
to
d1dd481
Compare
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
.PSScriptMethod
could be expanded in the future to support passing in args through a parameter namePSCodeMethod
could 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
LabeledExpressionAst
which 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
, andCf
Unicode 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.ArgumentNames
only allows names at the end of the argumentsdynamic
class 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.txt
will be provided topath
,Open
will be set tomode
, making the next named argument invalid becausemode
is already specified.Will work as the first positional argument is set to the
path
argument 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.
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 are called argx
It is possible to use reflection to define a method where each argument has no name.
The current code will automatically translate that to
arg$idx
where$idx
corresponds to the parameter index (starting from 0).This allows labels to still be used for this code like
Name conflicts are resolved by adding
_
suffixWhile rare it is possible to have a collision with the argument names when
arg$idx
calculation conflicts with an explicitly named oneThese scenarios would be quite rare but it's still something that should be considered.
In the case of a conflict the code will keep on adding a
_
suffix until the argument name is unique enough.For example in the below case where reflection would be used to create this definition where all args are named
arg
in the metata:PowerShell can call this with
For case sensitive conflicts the below would apply
As arguments are parsed left to right, the leftmost one will have no suffix.
For example with the below where the first argument is unnamed (
...
) in the metadata while the second arg uses the automatic name value the code here will use:PowerShell can call this with
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
,.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).