Avoid `virtual` implementations for `AddSynthesizedAttributes` #81289

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).

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.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedSubstitutedTypeParameterSymbol.cs:27 in 4b84820. [](commit_id = 4b84820, 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

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

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

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

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

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

        }

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


class

sealed?

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

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

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

Done with review pass (commit 4)

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

Avoid virtual implementations for AddSynthesizedAttributes #81289

Are you sure you want to change the base?

Avoid virtual implementations for AddSynthesizedAttributes #81289

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!

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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 19, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Avoid `virtual` implementations for `AddSynthesizedAttributes` #81289

Avoid `virtual` implementations for `AddSynthesizedAttributes` #81289

AlekseyTs Nov 19, 2025 •

edited

Loading

AlekseyTs commented Nov 19, 2025 •

edited

Loading