Synthesize delegate types for lambdas with parameter types or return type that are not valid type arguments by cston · Pull Request #57295 · dotnet/roslyn

cston · 2021-10-21T06:45:37Z

Synthesize delegate types for lambda expressions and method groups with parameter types or return type that are not valid type arguments.

For delegates where all parameter types and return type are valid as type arguments, the synthesized delegate type is a generic type where the types are the type arguments That allows a synthesized delegate type to be shared across all delegates with the same number of parameters and same ref kinds.

For instance the delegate type for (ref int p) => p is emitted as <>F{00000001}<int, int> with the generic type internal delegate TResult <>F{00000001}<T1, TResult>(ref T1).

For delegates where one or more of the parameter types or return type are not valid as type arguments (this PR), the synthesized delegate type is non-generic with fixed parameter and return types. The synthesized delegate type is still shared across all delegates with the exact signature however.

For instance the delegate type for (int* p) => *p is emitted as internal delegate int <>f__AnonymousDelegate0(int*).

The PR includes changes to edit-and-continue where the new delegate types are matched across edits by comparing delegate type names and signatures, and where a signature change, which should be rare, is considered a rude edit.

Fixes #55217.

…type that are not valid type arguments

cston · 2021-12-27T18:08:48Z

@dotnet/roslyn-compiler, please review, thanks.

tmat · 2021-12-27T19:46:14Z

Why not use hash of the signature display string instead of an index in the synthesized name?

In reply to: 1001717281

cston · 2021-12-28T04:43:19Z

Why not use hash of the signature display string instead of an index in the synthesized name?

Using an index (based on source order) matches the naming for anonymous types, and it seemed straightforward and sufficient to cover the scenarios.

tmat · 2021-12-28T06:33:30Z

Yeah, but it does not work with EnC.

In reply to: 1001893183

AlekseyTs · 2021-12-31T16:16:12Z

src/Compilers/CSharp/Portable/Emitter/EditAndContinue/CSharpSymbolMatcher.cs

-                            return (NamedTypeSymbol?)value.Delegate?.GetInternalSymbol();
+                            if (delegateTemplate.HasFixedTypes)
+                            {
+                                TryFindAnonymousDelegateWithFixedType(delegateTemplate, out var value);


TryFindAnonymousDelegateWithFixedType

Was the logic backwards before? It looks like the condition didn't change its meaning, but the branches got flipped. #Closed

The branches were flipped because HasFixedTypes == !IsParameterizedDelegateType.

AlekseyTs

LGTM (commit 16)

jcouv · 2022-01-06T02:35:39Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

+                    template.Construct(typeParameters);
+            }
+
+            static bool allValidTypeArguments(AnonymousTypeDescriptor typeDescr)


The name of the local function doesn't seem to match its purpose #Closed

jcouv · 2022-01-06T02:49:50Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

            {
-                typeArgumentsBuilder.Add(fields[i].TypeWithAnnotations);
+                var xOwner = x.ContainingSymbol!;
+                var yOwner = y.ContainingSymbol!;


var yOwner = y.ContainingSymbol!;

Can remove the nullable suppressions? #Closed

jcouv · 2022-01-06T03:40:43Z

...p/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.DelegateTemplateSymbol.cs

+                TypeSymbol? voidReturnTypeOpt,
+                int parameterCount,
+                RefKindVector refKinds)
+                : base(manager, Location.None) // Location is not needed since NameAndIndex is set explicitly below.


Would it make sense to split the base constructor (one that takes a NameAndIndex and one that takes a Location)? #Closed

It's not ideal that the two constructors here have different ways to set NameAndIndex, but it might be better to leave that difference here rather than change the base type.

jcouv · 2022-01-06T03:40:44Z

...p/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.DelegateTemplateSymbol.cs

+                RefKindVector refKinds)
+                : base(manager, Location.None) // Location is not needed since NameAndIndex is set explicitly below.
+            {
+                Debug.Assert(refKinds.IsNull || parameterCount == refKinds.Capacity - (voidReturnTypeOpt is { } ? 0 : 1));


voidReturnTypeOpt is { }

nit: consider extracting the various null checks on voidReturnTypeOpt in this method to a local (returnsVoid or some such). #Closed

I'll leave as is because this constructor was essentially just moved from SynthesizedDelegateSymbol.

davidwengier · 2022-01-06T03:34:06Z

src/Compilers/Core/Portable/Emit/EditAndContinue/EmitBaseline.cs

            Debug.Assert((ordinal == 0) == (initialBaseline == null));
            Debug.Assert((ordinal == 0) == (anonymousTypeMap == null));
-            Debug.Assert((ordinal == 0) == (synthesizedDelegates == null));
+            Debug.Assert((ordinal == 0) == (anonymousDelegates == null));


Nit: Should there be a corresponding assert here for anonymousDelegatesWithFixedTypes? #Resolved

davidwengier · 2022-01-06T03:40:55Z

src/Compilers/CSharp/Portable/Emitter/EditAndContinue/EmitHelpers.cs

                changedTypes: changedTypes.ToImmutableAndFree());
        }

+        private static bool ContainsAnonymousDelegates(


Nit: Naming seems off to me. This checks for changes to delegates, not just whether they exist, right? Or does the current name mean "Contains all existing anonymous delegates"? #Resolved

The method checks the new collection contains all existing anonymous delegates. Renamed to ContainsPreviousAnonymousDelegates to clarify.

jcouv · 2022-01-06T03:41:28Z

...p/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType.DelegateTemplateSymbol.cs

+                }
+            }
+
+            internal AnonymousDelegateTemplateSymbol(AnonymousTypeManager manager, AnonymousTypeDescriptor typeDescr, ImmutableArray<TypeParameterSymbol> typeParametersToSubstitute)


nit: xml doc on these constructors would be useful. This constructor is for non-generic case, and above is generic case #Closed

jcouv · 2022-01-06T03:41:33Z

src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs

+@".class public A`1<T>
+{
+  .method public hidebysig specialname rtspecialname instance void .ctor() cil managed { ret }
+  .method public static void F1(int32* modopt(class A`1<!T>) i) { ret }


Consider expanding this test to observe the modopt in the synthesized delegate type

The modopt is used to test that the resulting delegate is generic only (the type parameter is referenced in the modopt only), but the resulting delegate does not include a modopt.

jcouv · 2022-01-06T03:41:39Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedDelegateSymbol.cs

-        internal override bool HasPossibleWellKnownCloneMethod() => false;
-    }
-
    internal sealed class SynthesizedDelegateConstructor : SynthesizedInstanceConstructor


Given that SynthesizedDelegateConstructor is the only type left in this file, it'd be good to rename the file #Closed

I'm reluctant to rename the file with changes in case that makes it difficult to track the history.

jcouv · 2022-01-06T04:03:10Z

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/SymbolMatcherTests.cs

            Assert.Equal("<>9__0_2", field3.Name);

-            var matcher = new CSharpSymbolMatcher(null, synthesizedDelegates0, compilation1.SourceAssembly, emitContext, peAssemblySymbol0);
+            var matcher = new CSharpSymbolMatcher(null, synthesizedDelegates0, null, compilation1.SourceAssembly, emitContext, peAssemblySymbol0);


Feels like we should add some SymbolMatcher tests for delegates with fixed types #Closed

jcouv · 2022-01-06T04:05:15Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

+                    if (ReferenceEquals(template.Manager, this) && !template.HasFixedTypes)
                    {
-                        builder.Add(template.Delegate);
+                        builder.Add(template);


nit: Consider adding an assertion that template.SmallestLocation is Location.None (which explains why we use the SynthesizedDelegateSymbolComparer comparer below) #Closed

There's actually no requirement that SmallestLocation is Location.None. But we are able to compare by MetadataName because the name is based only on the signature of the delegate type, not it's source order.

jcouv · 2022-01-06T04:05:26Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

        }

-        internal IReadOnlyDictionary<CodeAnalysis.Emit.SynthesizedDelegateKey, CodeAnalysis.Emit.SynthesizedDelegateValue> GetSynthesizedDelegates()
+        internal IReadOnlyDictionary<CodeAnalysis.Emit.SynthesizedDelegateKey, CodeAnalysis.Emit.SynthesizedDelegateValue> GetAnonymousDelegates()


nit: consider renaming GetAnonymousDelegatesWithoutFixedTypes (and propagate up into PEDeltaAssemblyBuilder and EmitBaseline) #Closed

I'd considered that but slightly preferred having a simpler name for the more common delegate types.

jcouv

Done with review pass (iteration 16)

jcouv

LGTM Thanks (iteration 19)

ghost added the Area-Compilers label Oct 21, 2021

runfoapp bot mentioned this pull request Oct 21, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

cston force-pushed the 55217 branch 2 times, most recently from f2e1ae5 to b28620d Compare November 2, 2021 15:14

runfoapp bot mentioned this pull request Nov 3, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpFindReferences.FindReferencesToCtor #57552

Closed

cston force-pushed the 55217 branch from b28620d to 1fd8a88 Compare November 4, 2021 21:58

cston force-pushed the 55217 branch from 1fd8a88 to 070f1b8 Compare November 13, 2021 17:45

runfoapp bot mentioned this pull request Nov 15, 2021

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

cston force-pushed the 55217 branch from 070f1b8 to 6a56427 Compare December 1, 2021 19:27

runfoapp bot mentioned this pull request Dec 2, 2021

Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.CommandLineTests.ArgumentParsing is flaky #58077

Open

cston force-pushed the 55217 branch from 2045c9b to 7f60001 Compare December 8, 2021 07:32

This was referenced Dec 8, 2021

Transient failure in test AsyncIteratorWithCustomCode #34207

Closed

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

cston force-pushed the 55217 branch from 499b227 to dbe8cc6 Compare December 10, 2021 19:13

runfoapp bot mentioned this pull request Dec 14, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

cston added 3 commits December 15, 2021 21:19

Synthesize delegate types for lambdas with parameter types or return …

a9d0780

…type that are not valid type arguments

EnC

0e85bab

More EnC

30f7ab3

cston force-pushed the 55217 branch 2 times, most recently from 18b3e97 to d141ba4 Compare December 17, 2021 22:57

Index anonymous delegates by name

1016863

cston force-pushed the 55217 branch from 9ca12fc to 1016863 Compare December 18, 2021 00:07

cston marked this pull request as ready for review December 18, 2021 00:14

cston requested a review from a team as a code owner December 18, 2021 00:14

cston requested review from davidwengier and tmat December 18, 2021 00:14

AlekseyTs reviewed Dec 31, 2021

View reviewed changes

More tests

029cfc9

jcouv self-assigned this Jan 4, 2022

AlekseyTs approved these changes Jan 5, 2022

View reviewed changes

jcouv reviewed Jan 6, 2022

View reviewed changes

davidwengier approved these changes Jan 6, 2022

View reviewed changes

jcouv reviewed Jan 6, 2022

View reviewed changes

cston changed the base branch from main to release/dev17.1 January 6, 2022 06:28

cston added 2 commits January 5, 2022 22:33

Merge remote-tracking branch 'upstream/release/dev17.1' into 55217

c697f9e

PR feedback

d9b0a52

cston force-pushed the 55217 branch from 590072b to d9b0a52 Compare January 6, 2022 07:14

cston added 2 commits January 6, 2022 10:48

Add CSharpSymbolMatcher test

2609a7b

Merge remote-tracking branch 'upstream/release/dev17.1' into 55217

15afc79

jcouv approved these changes Jan 6, 2022

View reviewed changes

cston changed the base branch from release/dev17.1 to main January 7, 2022 23:34

cston merged commit 09af9e4 into dotnet:main Jan 10, 2022

ghost added this to the Next milestone Jan 10, 2022

cston deleted the 55217 branch January 10, 2022 18:42

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

tmat mentioned this pull request Mar 9, 2023

dotnet watch is not picking up changes on .cshtml files using Razor Pages #67243

Closed

Conversation

cston commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Dec 27, 2021

Uh oh!

tmat commented Dec 27, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Dec 28, 2021

Uh oh!

tmat commented Dec 28, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

var yOwner = y.ContainingSymbol!;

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Jan 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Jan 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cston commented Oct 21, 2021 •

edited

Loading

tmat commented Dec 27, 2021 •

edited by cston

Loading

tmat commented Dec 28, 2021 •

edited by cston

Loading

AlekseyTs Dec 31, 2021 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

davidwengier Jan 6, 2022 •

edited by cston

Loading

davidwengier Jan 6, 2022 •

edited by cston

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading

jcouv Jan 6, 2022 •

edited

Loading