-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Synthesize delegate types for lambda expressions and method groups #55203
Conversation
92765a5
to
3f61108
Compare
if (returnsVoid && returnRefKind != RefKind.None) | ||
{ | ||
// Invalid return type. | ||
return null; |
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.
Do we want to consider diagnostics for the cases we're returning null
? It would be better to have specific error messages for those cases, I think. #Closed
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.
A specific error shouldn't be necessary. There are two cases where we return null
: the case here of a ref void
return type which will have been reported as a declaration error; and the case below where a parameter or return type cannot be used as a type argument. The second case should be resolved in #55217.
src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedDelegateSymbol.cs
Show resolved
Hide resolved
|
||
internal int Capacity => _bits.Capacity / 2; | ||
|
||
internal IEnumerable<ulong> Words() => _bits.Words(); |
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.
I'm surprised dynamic tests weren't affected by the change from BitVector
to RefKindVector
. MakeDynamicCallSiteDelegateName
uses those "words" as part of the name for the synthesized delegate type.
Never mind, they were #Closed
static bool isValidTypeArgument(TypeSymbol? type) | ||
{ | ||
return type is { } && | ||
!type.IsPointerOrFunctionPointer() && |
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.
nit: Consider including a test with an array of pointer types (that's an example to illustrate the difference between IsPointerOrFunctionPointer
and IsUnsafe
) #Closed
var delegateType = ((INamedTypeSymbol)local.Type); | ||
Assert.Equal(Accessibility.Internal, delegateType.DeclaredAccessibility); | ||
Assert.Equal(expectedInvokeMethod, delegateType.DelegateInvokeMethod.ToTestDisplayString()); | ||
} |
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.
Let's include a test with more types: dynamic, tuples with names, nint or reference types with nullability. #Resolved
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.
To add to the list, arrays of function pointer types where the parameters/return type of the function pointer have refkinds.
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.
I also think we should have public API verification, ie what are we returning from GetSymbolInfo
or GetTypeInfo
for these synthesized symbols.
} | ||
static void M2<T>(T t) where T : struct | ||
{ | ||
var d = (ref T t) => t; |
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.
Consider varying parameter name here #Closed
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.
Done with review pass(iteration 5)
} | ||
|
||
int parameterCount = delegateSignature.Length - (returnsVoid ? 0 : 1); | ||
Debug.Assert(_factory.CompilationState.ModuleBuilderOpt is { }); | ||
int generation = _factory.CompilationState.ModuleBuilderOpt.CurrentGenerationOrdinal; | ||
var synthesizedType = _factory.Compilation.AnonymousTypeManager.SynthesizeDelegate(parameterCount, byRefs, returnsVoid, generation); | ||
return synthesizedType.Construct(delegateSignature); | ||
|
||
// The distinction between by-ref kinds is dropped. |
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.
Why? #Resolved
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.
Not sure, perhaps to allow greater sharing of delegate types. But I believe this preserves the original behavior.
} | ||
} | ||
|
||
public bool Equals(RefKindVector other) |
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.
Can we use record struct
s yet?
Done review pass (commit 5) |
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 Thanks (iteration 6)
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 (commit 6)
Approved to merge |
Synthesize delegate types for lambda expressions and method groups with inferred type that cannot be represented with
System.Func<>
orSystem.Action<>
types.Uses the existing compiler infrastructure for delegates created at
dynamic
call-sites.The PR does not include support for parameter or return types that cannot be used as generic type arguments: pointer types,
ref struct
types, etc. (see #55217). Those cases will be addressed separately.Proposal: lambda-improvements.md
Test plan: #52192