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

Ensure HasAnonymousFunctionConversion returns without binding lambda body for explicitly-typed lambda #69093

Closed
sharwell opened this issue Jul 18, 2023 · 6 comments · Fixed by #69695
Assignees
Milestone

Comments

@sharwell
Copy link
Member

sharwell commented Jul 18, 2023

Warning: The assumptions made in this post appear to be incorrect. See #69093 (comment).

81.1% of the CPU time spent on the IntelliSense thread in #67926 is calling HasAnonymousFunctionConversion.

This issue relates to the following code:

case BoundKind.UnboundLambda:
if (HasAnonymousFunctionConversion(sourceExpression, destination))
{
return Conversion.AnonymousFunction;
}
break;

This code should return without binding the contents of the lambda if the lambda explicitly declares all types related to its signature. For example, the following lambda declares its parameter types and its return value explicitly, and therefore can be evaluated for having a conversion without further binding.

SomeInvocation(int (string arg) => { /* none of this matters */ });

While this guarantee doesn't automatically fix the problem described in #67926, it provides the user with a viable workaround in nested lambda cases where they can explicitly declare the types and avoid further costs.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 18, 2023
@sharwell
Copy link
Member Author

sharwell commented Jul 18, 2023

It looks like the error cases for return type and parameter types are handled here:

if (anonymousFunction.HasExplicitReturnType(out var refKind, out var returnType))
{
if (invokeMethod.RefKind != refKind ||
!invokeMethod.ReturnType.Equals(returnType.Type, TypeCompareKind.AllIgnoreOptions))
{
return LambdaConversionResult.MismatchedReturnType;
}
}

if (anonymousFunction.HasExplicitlyTypedParameterList)
{
for (int p = 0; p < delegateParameters.Length; ++p)
{
if (delegateParameters[p].RefKind != anonymousFunction.RefKind(p) ||
!delegateParameters[p].Type.Equals(anonymousFunction.ParameterType(p), TypeCompareKind.AllIgnoreOptions))
{
return LambdaConversionResult.MismatchedParameterType;
}
}

However, the success case still depends on binding:

// Ensure the body can be converted to that delegate type
var bound = anonymousFunction.Bind(delegateType, isTargetExpressionTree);
if (ErrorFacts.PreventsSuccessfulDelegateConversion(bound.Diagnostics.Diagnostics))
{
return LambdaConversionResult.BindingFailed;
}
return LambdaConversionResult.Success;

Is there a reason we need to use PreventsSuccessfulDelegateConversion on this path if both the return type and the parameter types were explicitly given and isTargetExpressionTree is false?

@cston
Copy link
Member

cston commented Jul 18, 2023

There may be edge cases where skipping binding the lambda body might lead to a breaking change, with captured variables for instance.

For example, the following binds to the extension method rather than the instance method, because binding the lambda body fails for the instance method (see sharplab.io).

using System;

class A
{
    static void Main()
    {
        object x = 1;
        var a = new A();
        a.F1(x, y => F2(int () => y));
    }

    void F1<T>(T x, Func<T, int> f) { }
    static int F2(Func<int> f) => f();
}

static class B
{
    public static void F1(this A a, object x, Func<int, int> f) { }
}

@jaredpar
Copy link
Member

@cston your example though uses an implicitly typed lambda where as the issue is about explicitly typed lambdas. This is how I would explictly type out that lambda

a.F1(x, int (int y) => F2(int () => y));

At that point why do we need to look at the lambda body here? The instance method is just going to fail at the generic inference phase.

@sharwell
Copy link
Member Author

@jaredpar I believe the failure from @cston example is during type inference for the outer lambda. For inferring the type of y, we can't assume int () => y can be converted to the target delegate based on int () => alone.

@cston
Copy link
Member

cston commented Aug 9, 2023

For the example above, it looks like there would not be a breaking change if the compiler ignored the lambda body when determining whether the inner lambda expression is convertible to the delegate type.

The reason is because the outer lambda is an argument to an overloaded method, and even if we skip binding the body of the inner lambda when determining convertibility to the delegate parameter in F2, and therefore consider F2 applicable, we'll still bind the inner lambda body as a part of determining the convertibility of the outer lambda to the delegate parameter in F1 since the outer lambda is not explicitly typed.

@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 9, 2023
@jaredpar jaredpar added this to the 17.8 milestone Aug 9, 2023
@jaredpar
Copy link
Member

jaredpar commented Aug 9, 2023

Discussed this in LDM today and we confirmed that we want to change behavior here. For a lambda with a full explicit signature (return type and parameters) then for purposes of conversion we will only consider that signature. There is no need to bind the lambda body to see if it matches that signature for the purposes of determining if a conversion is legal.

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