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.

Sign up for GitHub

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

Jump to bottom

Use lambda expression and method group signature in type inference #55786

Merged
merged 25 commits into from Aug 28, 2021
Merged

Use lambda expression and method group signature in type inference #55786

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2572169
Use lambda signatures and method group signatures in type inference
cston Jul 8, 2021
3b3f294
Misc.
cston Aug 24, 2021
9424c62
Misc.
cston Aug 25, 2021
b3176ff
Update method type inference
cston Aug 25, 2021
5da002f
Variance
cston Aug 25, 2021
763f5b5
Resolve PROTOTYPE comments
cston Aug 25, 2021
7b21e1a
Misc.
cston Aug 26, 2021
3fefaa3
Formatting
cston Aug 26, 2021
dca307d
Avoid holding onto lambda cache
cston Aug 26, 2021
a0f8caf
PR feedback
cston Aug 26, 2021
821e1bc
PR feedback
cston Aug 26, 2021
6aa9504
PR feedback
cston Aug 26, 2021
aaf8881
PR feedback
cston Aug 26, 2021
7223b1b
Misc.
cston Aug 27, 2021
f2f885d
Rename FunctionSignature -> FunctionTypeSymbol.Lazy, GetSignatureAsTy…
cston Aug 27, 2021
450a4ac
PR feedback
cston Aug 27, 2021
a9ea853
PR feedback
cston Aug 27, 2021
af7cdd5
Revert change
cston Aug 27, 2021
5875469
Fix tests
cston Aug 27, 2021
5a2911a
PR feedback
cston Aug 27, 2021
df2a2d9
Rename file
cston Aug 27, 2021
772d7ed
PR feedback
cston Aug 27, 2021
24e29e9
More tests
cston Aug 27, 2021
890e3f3
Test Expression<T> not derived from Expression
cston Aug 27, 2021
63f5795
Update Breaking Changes document
cston Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
137 changes: 59 additions & 78 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Expand Up @@ -97,6 +97,11 @@ internal partial class Binder
return CreateAnonymousFunctionConversion(syntax, source, conversion, isCast: isCast, conversionGroupOpt, destination, diagnostics);
}

if (conversion.Kind == ConversionKind.FunctionType)
{
return CreateFunctionTypeConversion(syntax, source, conversion, isCast: isCast, conversionGroupOpt, destination, diagnostics);
}

if (conversion.IsStackAlloc)
{
return CreateStackAllocConversion(syntax, source, conversion, isCast, conversionGroupOpt, destination, diagnostics);
Expand Down Expand Up @@ -546,69 +551,67 @@ private BoundExpression ConvertSwitchExpression(BoundUnconvertedSwitchExpression
return finalConversion;
}

private BoundExpression CreateAnonymousFunctionConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, BindingDiagnosticBag diagnostics)
private BoundExpression CreateFunctionTypeConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, BindingDiagnosticBag diagnostics)
Copy link
Member

@jcouv jcouv Aug 26, 2021 •
edited

Choose a reason for hiding this comment

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

conversion

Is it intentional that we're not using this parameter's value? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't realized the parameter value was not used, but it makes sense since the conversion does include any additional state. I've kept the parameter for consistency with other Create*Conversion() methods and added an assert for conversion.Kind.

{
// We have a successful anonymous function conversion; rather than producing a node
// which is a conversion on top of an unbound lambda, replace it with the bound
// lambda.
Debug.Assert(source.Kind is BoundKind.MethodGroup or BoundKind.UnboundLambda);
Debug.Assert(syntax.IsFeatureEnabled(MessageID.IDS_FeatureInferredDelegateType));

// UNDONE: Figure out what to do about the error case, where a lambda
// UNDONE: is converted to a delegate that does not match. What to surface then?
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var delegateType = source.GetInferredDelegateType(ref useSiteInfo);
Debug.Assert(delegateType is { });

var unboundLambda = (UnboundLambda)source;
if ((destination.SpecialType == SpecialType.System_Delegate || destination.IsNonGenericExpressionType()) &&
syntax.IsFeatureEnabled(MessageID.IDS_FeatureInferredDelegateType))
if (source.Kind == BoundKind.UnboundLambda &&
destination.IsNonGenericExpressionType())
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var delegateType = unboundLambda.InferDelegateType(ref useSiteInfo);
BoundLambda boundLambda;
if (delegateType is { })
{
bool isExpressionTree = destination.IsNonGenericExpressionType();
if (isExpressionTree)
{
delegateType = Compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T).Construct(delegateType);
delegateType.AddUseSiteInfo(ref useSiteInfo);
}
boundLambda = unboundLambda.Bind(delegateType, isExpressionTree);
}
else
{
diagnostics.Add(ErrorCode.ERR_CannotInferDelegateType, syntax.GetLocation());
delegateType = CreateErrorType();
boundLambda = unboundLambda.BindForErrorRecovery();
}
diagnostics.AddRange(boundLambda.Diagnostics);
var expr = createAnonymousFunctionConversion(syntax, source, boundLambda, conversion, isCast, conversionGroup, delegateType);
conversion = Conversions.ClassifyConversionFromExpression(expr, destination, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);
return CreateConversion(syntax, expr, conversion, isCast, conversionGroup, destination, diagnostics);
delegateType = Compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T).Construct(delegateType);
Copy link
Contributor

@AlekseyTs AlekseyTs Aug 27, 2021 •
edited

Choose a reason for hiding this comment

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

Compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T)

Why do we assume there is a relationship between the two types? #Closed

Copy link
Member Author

@cston cston Aug 27, 2021 •
edited

Choose a reason for hiding this comment

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

Added tests where Expression<T> is not derived from Expression.

delegateType.AddUseSiteInfo(ref useSiteInfo);
}

conversion = Conversions.ClassifyConversionFromExpression(source, delegateType, ref useSiteInfo);
BoundExpression expr;
if (!conversion.Exists)
{
GenerateImplicitConversionError(diagnostics, syntax, conversion, source, delegateType);
expr = new BoundConversion(syntax, source, conversion, @checked: false, explicitCastInCode: isCast, conversionGroup, constantValueOpt: ConstantValue.NotAvailable, type: delegateType, hasErrors: true) { WasCompilerGenerated = source.WasCompilerGenerated };
}
else
{
#if DEBUG
// Test inferring a delegate type for all callers.
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
_ = unboundLambda.InferDelegateType(ref discardedUseSiteInfo);
#endif
var boundLambda = unboundLambda.Bind((NamedTypeSymbol)destination, isExpressionTree: destination.IsGenericOrNonGenericExpressionType(out _));
diagnostics.AddRange(boundLambda.Diagnostics);
return createAnonymousFunctionConversion(syntax, source, boundLambda, conversion, isCast, conversionGroup, destination);
expr = CreateConversion(syntax, source, conversion, isCast, conversionGroup, delegateType, diagnostics);
}

static BoundConversion createAnonymousFunctionConversion(SyntaxNode syntax, BoundExpression source, BoundLambda boundLambda, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination)
conversion = Conversions.ClassifyConversionFromExpression(expr, destination, ref useSiteInfo);
if (!conversion.Exists)
{
return new BoundConversion(
syntax,
boundLambda,
conversion,
@checked: false,
explicitCastInCode: isCast,
conversionGroup,
constantValueOpt: ConstantValue.NotAvailable,
type: destination)
{ WasCompilerGenerated = source.WasCompilerGenerated };
GenerateImplicitConversionError(diagnostics, syntax, conversion, source, destination);
}

diagnostics.Add(syntax, useSiteInfo);
return CreateConversion(syntax, expr, conversion, isCast, conversionGroup, destination, diagnostics);
}

private BoundExpression CreateAnonymousFunctionConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, BindingDiagnosticBag diagnostics)
{
// We have a successful anonymous function conversion; rather than producing a node
// which is a conversion on top of an unbound lambda, replace it with the bound
// lambda.

// UNDONE: Figure out what to do about the error case, where a lambda
// UNDONE: is converted to a delegate that does not match. What to surface then?

var unboundLambda = (UnboundLambda)source;

var boundLambda = unboundLambda.Bind((NamedTypeSymbol)destination, isExpressionTree: destination.IsGenericOrNonGenericExpressionType(out _));
diagnostics.AddRange(boundLambda.Diagnostics);
return new BoundConversion(
syntax,
boundLambda,
conversion,
@checked: false,
explicitCastInCode: isCast,
conversionGroup,
constantValueOpt: ConstantValue.NotAvailable,
type: destination)
{ WasCompilerGenerated = source.WasCompilerGenerated };
}

private BoundExpression CreateMethodGroupConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, BindingDiagnosticBag diagnostics)
Expand All @@ -627,29 +630,7 @@ private BoundExpression CreateMethodGroupConversion(SyntaxNode syntax, BoundExpr
hasErrors = true;
}

if (destination.SpecialType == SpecialType.System_Delegate &&
syntax.IsFeatureEnabled(MessageID.IDS_FeatureInferredDelegateType))
{
// https://github.com/dotnet/roslyn/issues/52869: Avoid calculating the delegate type multiple times during conversion.
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var delegateType = GetMethodGroupDelegateType(group, ref useSiteInfo);
var expr = createMethodGroupConversion(syntax, group, conversion, isCast, conversionGroup, delegateType!, hasErrors);
conversion = Conversions.ClassifyConversionFromExpression(expr, destination, ref useSiteInfo);
diagnostics.Add(syntax, useSiteInfo);
return CreateConversion(syntax, expr, conversion, isCast, conversionGroup, destination, diagnostics);
}

#if DEBUG
// Test inferring a delegate type for all callers.
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
_ = GetMethodGroupDelegateType(group, ref discardedUseSiteInfo);
#endif
return createMethodGroupConversion(syntax, group, conversion, isCast, conversionGroup, destination, hasErrors);

static BoundConversion createMethodGroupConversion(SyntaxNode syntax, BoundMethodGroup group, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, bool hasErrors)
{
return new BoundConversion(syntax, group, conversion, @checked: false, explicitCastInCode: isCast, conversionGroup, constantValueOpt: ConstantValue.NotAvailable, type: destination, hasErrors: hasErrors) { WasCompilerGenerated = group.WasCompilerGenerated };
}
return new BoundConversion(syntax, group, conversion, @checked: false, explicitCastInCode: isCast, conversionGroup, constantValueOpt: ConstantValue.NotAvailable, type: destination, hasErrors: hasErrors) { WasCompilerGenerated = group.WasCompilerGenerated };
}

private BoundExpression CreateStackAllocConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, ConversionGroup? conversionGroup, TypeSymbol destination, BindingDiagnosticBag diagnostics)
Expand Down Expand Up @@ -814,6 +795,7 @@ private BoundMethodGroup FixMethodGroupWithTypeOrValue(BoundMethodGroup group, C
group.LookupSymbolOpt,
group.LookupError,
group.Flags,
group.Signature,
receiverOpt, //only change
group.ResultKind);
}
Expand Down Expand Up @@ -1226,13 +1208,12 @@ static ErrorCode getRefMismatchErrorCode(TypeKind type)
TypeSymbol delegateOrFuncPtrType,
BindingDiagnosticBag diagnostics)
{
Debug.Assert(delegateOrFuncPtrType.SpecialType == SpecialType.System_Delegate || delegateOrFuncPtrType.TypeKind == TypeKind.Delegate || delegateOrFuncPtrType.TypeKind == TypeKind.FunctionPointer);

Debug.Assert(Conversions.IsAssignableFromMulticastDelegate(delegateOrFuncPtrType) || delegateOrFuncPtrType.TypeKind == TypeKind.Delegate || delegateOrFuncPtrType.TypeKind == TypeKind.FunctionPointer);
Debug.Assert(conversion.Method is object);
MethodSymbol selectedMethod = conversion.Method;

var location = syntax.Location;
if (delegateOrFuncPtrType.SpecialType != SpecialType.System_Delegate)
Copy link
Member

@jcouv jcouv Aug 27, 2021 •
edited

Choose a reason for hiding this comment

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

I assume you hunted down all the System_Delegate checks that were added earlier as part of this feature. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I've reviewed all cases where System_Delegate checks were added earlier.

if (!Conversions.IsAssignableFromMulticastDelegate(delegateOrFuncPtrType))
{
if (!MethodIsCompatibleWithDelegateOrFunctionPointer(receiverOpt, isExtensionMethod, selectedMethod, delegateOrFuncPtrType, location, diagnostics) ||
MemberGroupFinalValidation(receiverOpt, selectedMethod, syntax, diagnostics, isExtensionMethod))
Expand Down
44 changes: 17 additions & 27 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Expand Up @@ -396,18 +396,15 @@ internal BoundExpression BindToNaturalType(BoundExpression expression, BindingDi
private BoundExpression BindToInferredDelegateType(BoundExpression expr, BindingDiagnosticBag diagnostics)
{
var syntax = expr.Syntax;
CheckFeatureAvailability(syntax, MessageID.IDS_FeatureInferredDelegateType, diagnostics);
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var delegateType = expr switch
{
UnboundLambda unboundLambda => unboundLambda.InferDelegateType(ref useSiteInfo),
BoundMethodGroup methodGroup => GetMethodGroupDelegateType(methodGroup, ref useSiteInfo),
_ => throw ExceptionUtilities.UnexpectedValue(expr),
};
var delegateType = expr.GetInferredDelegateType(ref useSiteInfo);
Copy link
Member

@jcouv jcouv Aug 27, 2021 •
edited

Choose a reason for hiding this comment

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

Consider adding assertion here (only BoundMethodGroup or UnboundLambda) #Closed

diagnostics.Add(syntax, useSiteInfo);
if (delegateType is null)
{
diagnostics.Add(ErrorCode.ERR_CannotInferDelegateType, syntax.GetLocation());
if (CheckFeatureAvailability(syntax, MessageID.IDS_FeatureInferredDelegateType, diagnostics))
{
diagnostics.Add(ErrorCode.ERR_CannotInferDelegateType, syntax.GetLocation());
}
delegateType = CreateErrorType();
}
return GenerateConversionForAssignment(delegateType, expr, diagnostics);
Expand Down Expand Up @@ -2656,28 +2653,20 @@ private static string GetName(ExpressionSyntax syntax)

// Given a list of arguments, create arrays of the bound arguments and the names of those
// arguments.
private void BindArgumentsAndNames(ArgumentListSyntax argumentListOpt, BindingDiagnosticBag diagnostics, AnalyzedArguments result, bool allowArglist = false, bool isDelegateCreation = false)
private void BindArgumentsAndNames(BaseArgumentListSyntax argumentListOpt, BindingDiagnosticBag diagnostics, AnalyzedArguments result, bool allowArglist = false, bool isDelegateCreation = false)
{
if (argumentListOpt != null)
{
BindArgumentsAndNames(argumentListOpt.Arguments, diagnostics, result, allowArglist, isDelegateCreation: isDelegateCreation);
}
}

private void BindArgumentsAndNames(BracketedArgumentListSyntax argumentListOpt, BindingDiagnosticBag diagnostics, AnalyzedArguments result)
{
if (argumentListOpt != null)
{
BindArgumentsAndNames(argumentListOpt.Arguments, diagnostics, result, allowArglist: false);
}
}

private void BindArgumentsAndNames(
SeparatedSyntaxList<ArgumentSyntax> arguments,
BindingDiagnosticBag diagnostics,
AnalyzedArguments result,
bool allowArglist,
bool isDelegateCreation = false)
bool isDelegateCreation)
Copy link
Member

@jcouv jcouv Aug 26, 2021 •
edited

Choose a reason for hiding this comment

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

nit: we could inline this method now (only one usage above from a small method) #Closed

{
// Only report the first "duplicate name" or "named before positional" error,
// so as to avoid "cascading" errors.
Expand Down Expand Up @@ -2729,7 +2718,7 @@ private bool RefMustBeObeyed(bool isDelegateCreation, ArgumentSyntax argumentSyn
ref bool hadLangVersionError,
ArgumentSyntax argumentSyntax,
bool allowArglist,
bool isDelegateCreation = false)
bool isDelegateCreation)
{
RefKind origRefKind = argumentSyntax.RefOrOutKeyword.Kind().GetRefKind();
// The old native compiler ignores ref/out in a delegate creation expression.
Expand Down Expand Up @@ -4405,7 +4394,7 @@ private BoundExpression BindDelegateCreationExpression(SyntaxNode node, NamedTyp
{
var boundMethodGroup = new BoundMethodGroup(
argument.Syntax, default, WellKnownMemberNames.DelegateInvokeName, ImmutableArray.Create(sourceDelegate.DelegateInvokeMethod),
sourceDelegate.DelegateInvokeMethod, null, BoundMethodGroupFlags.None, argument, LookupResultKind.Viable);
sourceDelegate.DelegateInvokeMethod, null, BoundMethodGroupFlags.None, signature: null, argument, LookupResultKind.Viable);
if (!Conversions.ReportDelegateOrFunctionPointerMethodGroupDiagnostics(this, boundMethodGroup, type, diagnostics))
{
// If we could not produce a more specialized diagnostic, we report
Expand Down Expand Up @@ -6486,7 +6475,8 @@ private BoundExpression MakeMemberAccessValue(BoundExpression expr, BindingDiagn
rightName,
lookupResult.Symbols.All(s => s.Kind == SymbolKind.Method) ? lookupResult.Symbols.SelectAsArray(s_toMethodSymbolFunc) : ImmutableArray<MethodSymbol>.Empty,
lookupResult,
flags);
flags,
this);

if (!boundMethodGroup.HasErrors && typeArgumentsSyntax.Any(SyntaxKind.OmittedTypeArgument))
{
Expand Down Expand Up @@ -6636,6 +6626,7 @@ private BoundExpression BindMemberAccessBadResult(BoundMethodGroup node)
methods.Length == 1 ? methods[0] : null,
lookupError,
flags: BoundMethodGroupFlags.None,
signature: null,
receiverOpt: boundLeft,
resultKind: lookupKind,
hasErrors: true);
Expand Down Expand Up @@ -8512,10 +8503,11 @@ private ErrorPropertySymbol CreateErrorPropertySymbol(ImmutableArray<PropertySym
}

#nullable enable
internal NamedTypeSymbol? GetMethodGroupDelegateType(BoundMethodGroup node, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
internal NamedTypeSymbol? GetMethodGroupDelegateType(BoundMethodGroup node, out MethodSymbol? method)
{
if (GetUniqueSignatureFromMethodGroup(node) is { } method &&
GetMethodGroupOrLambdaDelegateType(method.RefKind, method.ReturnsVoid ? default : method.ReturnTypeWithAnnotations, method.ParameterRefKinds, method.ParameterTypesWithAnnotations, ref useSiteInfo) is { } delegateType)
method = GetUniqueSignatureFromMethodGroup(node);
if (method is { } &&
GetMethodGroupOrLambdaDelegateType(method.RefKind, method.ReturnsVoid ? default : method.ReturnTypeWithAnnotations, method.ParameterRefKinds, method.ParameterTypesWithAnnotations) is { } delegateType)
{
return delegateType;
}
Expand Down Expand Up @@ -8603,8 +8595,7 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
RefKind returnRefKind,
TypeWithAnnotations returnTypeOpt,
ImmutableArray<RefKind> parameterRefKinds,
ImmutableArray<TypeWithAnnotations> parameterTypes,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
ImmutableArray<TypeWithAnnotations> parameterTypes)
{
Debug.Assert(parameterRefKinds.IsDefault || parameterRefKinds.Length == parameterTypes.Length);
Debug.Assert(returnTypeOpt.Type?.IsVoidType() != true); // expecting !returnTypeOpt.HasType rather than System.Void
Expand Down Expand Up @@ -8636,7 +8627,6 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
if (wkDelegateType != WellKnownType.Unknown)
{
var delegateType = Compilation.GetWellKnownType(wkDelegateType);
delegateType.AddUseSiteInfo(ref useSiteInfo);
Copy link
Contributor

@AlekseyTs AlekseyTs Aug 27, 2021 •
edited

Choose a reason for hiding this comment

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

delegateType.AddUseSiteInfo(ref useSiteInfo);

It is not obvious why we don't want to do this. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller of GetMethodGroupOrLambdaDelegateType() is responsible for checking any use-site diagnostics on the return type. Added a comment.

if (typeArguments.Length == 0)
{
return delegateType;
Expand Down