Avoid `virtual` implementations for `AddSynthesizedAttributes` by jcouv · Pull Request #81289 · dotnet/roslyn

jcouv · 2025-11-17T05:39:43Z

AlekseyTs · 2025-11-19T15:48:52Z

src/Compilers/CSharp/Portable/Symbols/TypeMap.cs

            foreach (var tp in oldTypeParameters)
            {
-                var newTp = synthesized ?
+                TypeParameterSymbol newTp = synthesized ?


TypeParameterSymbol

It looks like this is the only change in this file, and it is mostly stylistic. Consider reverting. #Closed

I see, the types no longer inherit from each other, a target type is necessary.

AlekseyTs · 2025-11-19T15:59:25Z

src/Compilers/CSharp/Portable/Symbols/SubstitutedTypeParameterSymbol.cs

 namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
-    internal class SubstitutedTypeParameterSymbol : WrappedTypeParameterSymbol
+    internal sealed class SubstitutedTypeParameterSymbol : SubstitutedTypeParameterSymbolBase


SubstitutedTypeParameterSymbol

Since we are keeping the name, we should check all type tests and conversions performed against this type and confirm that they are still good (i.e. shouldn't be changed to targeting SubstitutedTypeParameterSymbolBase instead). #Closed

There are no type tests or conversions using SubstitutedTypeParameterSymbol

AlekseyTs · 2025-11-19T16:06:35Z

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

    /// A type parameter for a synthesized class or method.
    /// </summary>
-    internal sealed class SynthesizedSubstitutedTypeParameterSymbol : SubstitutedTypeParameterSymbol
+    internal sealed class SynthesizedSubstitutedTypeParameterSymbol : SubstitutedTypeParameterSymbolBase


SynthesizedSubstitutedTypeParameterSymbol

I think we should change name of this type. I think it always represents a definition of a synthesized type parameter and "Substituted" in the name adds some confusion. I suggest removing it and use "SynthesizedTypeParameterSymbol" as the name for the class. #Closed

Then adding synthesized attributes would totally make sense for this symbol.

AlekseyTs · 2025-11-19T16:10:22Z

I suggest asserting the following condition in this constructor:

ContainingSymbol.OriginalDefinition != _underlyingTypeParameter.ContainingSymbol.OriginalDefinition
``` #Closed

---
Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedSubstitutedTypeParameterSymbol.cs:27 in 4b84820. [](commit_id = 4b8482084236881342c19851e822e9e2daf3359a, deletion_comment = False)

AlekseyTs · 2025-11-19T16:12:35Z

src/Compilers/CSharp/Portable/Symbols/SubstitutedTypeParameterSymbol.cs

+    {
+        internal SubstitutedTypeParameterSymbol(Symbol newContainer, TypeMap map, TypeParameterSymbol substitutedFrom, int ordinal)
+            : base(newContainer, map, substitutedFrom, ordinal)
+        {


{

I suggest asserting the following condition in this constructor:

ContainingSymbol.OriginalDefinition == _underlyingTypeParameter.ContainingSymbol.OriginalDefinition ``` #Closed

AlekseyTs · 2025-11-19T16:16:06Z

    public override TypeParameterSymbol OriginalDefinition

Since we are splitting types, I suggest to keep going and splitting implementation of this method as well by doing the following:

Making it abstract override here
Returning _underlyingTypeParameter.OriginalDefinition from implementation in SubstitutedTypeParameterSymbol
Returning this from implementation in SynthesizedSubstitutedTypeParameterSymbol #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/SubstitutedTypeParameterSymbol.cs:62 in 4b84820. [](commit_id = 4b84820, deletion_comment = False)

AlekseyTs · 2025-11-19T16:22:42Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedEventSymbol.cs

            }
        }
+
+        internal sealed override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<CSharpAttributeData> attributes)


sealed

Consider adding a comment saying that, if we need to un-seal this method, we should make it abstract. And in other similar places #Closed

AlekseyTs · 2025-11-19T16:25:57Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenLambdaOrLocalFunctionSymbol.cs

        }
+
+        internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<CSharpAttributeData> attributes)
+        {


{

This implementation looks suspicious, unless we can throw unreachable in it #Closed

AlekseyTs · 2025-11-19T16:32:05Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenMethodSymbol.cs

        }

-        protected class RewrittenMethodParameterSymbol : RewrittenParameterSymbol
+        protected class RewrittenMethodParameterSymbol : RewrittenMethodParameterSymbolBase


class

sealed? #Closed

AlekseyTs · 2025-11-19T16:32:20Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenMethodSymbol.cs

+            }
+
+            internal sealed override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<CSharpAttributeData> attributes)
+            {


{

This implementation looks suspicious, unless we can throw unreachable in it #Closed

AlekseyTs · 2025-11-19T16:33:18Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenMethodSymbol.cs

        }

-        protected class RewrittenMethodParameterSymbol : RewrittenParameterSymbol
+        protected class RewrittenMethodParameterSymbol : RewrittenMethodParameterSymbolBase


RewrittenMethodParameterSymbol

Similar suggestion for type checks/conversions since we are splitting the type and keeping the name #Closed

Also no type tests or conversions using this type

AlekseyTs · 2025-11-19T16:34:41Z

Done with review pass (commit 4) #Closed

AlekseyTs · 2025-11-25T23:45:36Z

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

@@ -14,17 +14,19 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
    /// <summary>
    /// A type parameter for a synthesized class or method.
    /// </summary>
-    internal sealed class SynthesizedSubstitutedTypeParameterSymbol : SubstitutedTypeParameterSymbol
+    internal sealed class SynthesizedTypeParameterSymbol : SubstitutedTypeParameterSymbolBase


SynthesizedTypeParameterSymbol

Consider renaming the file as well

Renamed in last commit

AlekseyTs

LGTM (commit 5)

jcouv · 2025-11-26T01:09:13Z

@dotnet/roslyn-compiler for second review. Thanks

…eParameterSymbol.cs

Julien Couvreur added 3 commits November 16, 2025 20:44

Switch to abstract/sealed AddSynthesizedAttributes on Wrapped symbols

066b356

throw

207ff67

Unused usings

5d99ca8

jcouv self-assigned this Nov 17, 2025

jcouv added the Area-Compilers label Nov 17, 2025

throw

4b84820

This was referenced Nov 17, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

[android-arm64] The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#6408

Open

jcouv marked this pull request as ready for review November 19, 2025 09:33

jcouv requested a review from a team as a code owner November 19, 2025 09:33

AlekseyTs reviewed Nov 19, 2025

View reviewed changes

Address feedback

3274845

jcouv requested a review from AlekseyTs November 25, 2025 20:16

AlekseyTs reviewed Nov 25, 2025

View reviewed changes

AlekseyTs approved these changes Nov 25, 2025

View reviewed changes

jjonescz approved these changes Nov 26, 2025

View reviewed changes

Rename SynthesizedSubstitutedTypeParameterSymbol.cs to SynthesizedTyp…

4412f49

…eParameterSymbol.cs

jcouv enabled auto-merge (squash) November 29, 2025 17:36

jcouv merged commit c14edd1 into dotnet:main Nov 30, 2025
28 of 29 checks passed

dotnet-policy-service bot added this to the Next milestone Nov 30, 2025

dotnet-maestro bot mentioned this pull request Dec 1, 2025

[release/10.0.2xx] Source code updates from dotnet/roslyn dotnet/dotnet#3598

Merged

dotnet-bot mentioned this pull request Dec 1, 2025

[Automated] PRs inserted in VS build main-11301.14 #81494

Closed

dotnet-bot mentioned this pull request Dec 9, 2025

[Automated] PRs inserted in VS build feature.debugger.stackAnalysis-11308.44 #81588

Closed

dotnet-maestro bot mentioned this pull request Dec 18, 2025

[main] Source code updates from dotnet/roslyn dotnet/dotnet#3957

Merged

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026

Conversation

jcouv commented Nov 17, 2025

Uh oh!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 19, 2025 • 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 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025 • 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!

AlekseyTs Nov 19, 2025 • 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 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs commented Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs commented Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs commented Nov 19, 2025 •

edited

Loading