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
C# 12: Support for lambda param parameter and parameter defaults.
#15249
base: main
Are you sure you want to change the base?
Conversation
2eaa4e5
to
ec0fefd
Compare
b5dd8e2
to
e16d568
Compare
e16d568
to
51d8065
Compare
param arguments and default arguments.param parameter and parameter defaults.
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.
LGTM, added some questions.
| @@ -182,12 +182,20 @@ class Call extends DotNet::Call, Expr, @call { | |||
| /** | |||
| * Gets the argument that corresponds to parameter `p` of a potential | |||
| * run-time target of this call. | |||
| * | |||
| * Does not consider default arguments. | |||
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.
Does this predicate consider named arguments? (This question is not related to delegates.) It might be worth doing this change in a separate PR.
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.
It does not. I also noted that difference, but didn't want to make any changes related to that as it is not possible to use named arguments for lambdas.
| if (Symbol.MethodKind == MethodKind.DelegateInvoke && | ||
| Symbol.ContainingType is INamedTypeSymbol nt) | ||
| { | ||
| return nt.TypeKind == TypeKind.Delegate && nt.IsImplicitlyDeclared; | ||
| } |
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.
Is the Symbol itself IsImplicitlyDeclared for the cases that we're interested in? There's probably no point in checking the ContainingType.TypeKind, we already know that Symbol is a DelegateInvoke, so its container can't be anything else then a delegate, can it?
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.
The Symbol itself is implicitly declared, but the same also holds for the Invoke method call symbol "ordinary" lambdas. I tried to be as narrow and conservative as possible in terms of detecting when we deal with a compiler generated delegate type.
Good point about the typekind check. I will fix that!
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.
LGTM. Did you run DCA already?
It turns out that the extractor "almost" works out of the box for
paramsand default parameters for lambda expressions.The implementation of "ordinary" lambdas are based on
System.Funcwhereas the implementation of lambdas with parameter defaults or the use ofparamsparameters are based on compiler generated delegates (as these features are already support by delegates).Below is an example of some C# code that is compiled and de-compiled to illustrate the difference.
Compiling and de-compiling yields
In this PR
IsCompilerGeneratedDelegatehas been introduced to catch "exactly" these use-cases. As theInvokemethod doesn't have any location, we use the containing types location (which appears to re-use the source location of the lambda even though itself is compiler generated).getRuntimeArgumentFromParameterhas been updated to handle this case).