Skip to content

C# 12: Support for lambda param parameter and parameter defaults. #15249

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

Merged
merged 8 commits into from
Jan 11, 2024

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 4 times, most recently from e16d568 to 51d8065 Compare January 10, 2024 12:31
@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the comment from Does not consider default arguments. to

   * Does not consider 
   *   - default arguments,
   *   - named arguments.

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!

hvitved
hvitved previously approved these changes Jan 10, 2024
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?

@michaelnebel
Copy link
Contributor Author

I will address the remaining review comments and re-run DCA as there appeared to be some alert wobble (but performance looked just fine).

@michaelnebel
Copy link
Contributor Author

IMO DCA looks good
(1) Performance looks good.
(2) There are some discrepancies in alert locations in generated code (and a small number of diff in non-security related queries - which I think we have seen before when two almost identical extractors have been used).

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

@michaelnebel michaelnebel merged commit 8b464fb into github:main Jan 11, 2024
@michaelnebel michaelnebel deleted the csharp/lambdadefaultparams branch January 11, 2024 15:18
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.

3 participants