Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Jan 8, 2024

It turns out that the extractor "almost" works out of the box for params and default parameters for lambda expressions.
The implementation of "ordinary" lambdas are based on System.Func whereas the implementation of lambdas with parameter defaults or the use of params parameters 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.

class LambdaArgumentsTest
{
    void M1()
    {
        var l1 = (int x) => x + 1;
        var l2 = (int x, int y = 1) => x + y;
    }
}

Compiling and de-compiling yields

[CompilerGenerated]
internal delegate TResult <>f__AnonymousDelegate0<T1, T2, TResult>(T1 arg1, T2 arg2 = 1);
internal class LambdaArgumentsTest
{
    [Serializable]
    [CompilerGenerated]
    private sealed class <>c
    {
        public static readonly <>c <>9 = new <>c();

        public static Func<int, int> <>9__0_0;

        public static <>f__AnonymousDelegate0<int, int, int> <>9__0_1;

        internal int <M1>b__0_0(int x)
        {
            return x + 1;
        }

        internal int <M1>b__0_1(int x, int y = 1)
        {
            return x + y;
        }
    }

    private void M1()
    {
        Func<int, int> func = <>c.<>9__0_0 ?? (<>c.<>9__0_0 = new Func<int, int>(<>c.<>9.<M1>b__0_0));
        <>f__AnonymousDelegate0<int, int, int> <>f__AnonymousDelegate = <>c.<>9__0_1 ?? (<>c.<>9__0_1 = new <>f__AnonymousDelegate0<int, int, int>(<>c.<>9.<M1>b__0_1));
    }
}

In this PR

  • Add extractor support for creating methods from compiler generated delegates. The compiler generated code isn't identified as source code by the extractor and thus it won't be extracted correctly. A method IsCompilerGeneratedDelegate has been introduced to catch "exactly" these use-cases. As the Invoke method 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).
  • Add Code QL library support for getting the arguments provided by params for delegate like types (these are not considered static call targets and thus the implementation for getRuntimeArgumentFromParameter has been updated to handle this case).
  • Add test for lambda arguments.
  • Add test for lambda default parameters.

@github-actions github-actions bot added the C# label Jan 8, 2024
@michaelnebel michaelnebel force-pushed the csharp/lambdadefaultparams branch 2 times, most recently from b5dd8e2 to e16d568 Compare January 10, 2024 10:28
@michaelnebel michaelnebel changed the title C# 12: Support for lambda param arguments and default arguments. C# 12: Support for lambda param parameter and parameter defaults. Jan 10, 2024
@michaelnebel michaelnebel marked this pull request as ready for review January 10, 2024 13:19
@michaelnebel michaelnebel requested a review from a team as a code owner January 10, 2024 13:19
Copy link
Contributor

@tamasvajk tamasvajk left a 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 59 to 63
if (Symbol.MethodKind == MethodKind.DelegateInvoke &&
Symbol.ContainingType is INamedTypeSymbol nt)
{
return nt.TypeKind == TypeKind.Delegate && nt.IsImplicitlyDeclared;
}
Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Contributor

@hvitved hvitved left a 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants