Add ImplementationAttributes to IMethodSymbol by Youssef1313 · Pull Request #50560 · dotnet/roslyn

Youssef1313 · 2021-01-16T18:01:01Z

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractMethodSymbol.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationMethodSymbol.cs

jcouv · 2021-01-20T19:12:44Z

Tagging @dotnet/roslyn-compiler: I think it's sensible to make the ImplementationAttributes API public. @mavasani had identified some scenarios where that's useful. Any concerns?
Manish, can you confirm those scenarios are still applicable (we didn't already find another solution)?

@Youssef1313 Assuming we can get agreement on design change, a PR that exposes new public APIs needs direct test coverage for those APIs.

333fred · 2021-01-20T19:38:00Z

Tagging @dotnet/roslyn-compiler: I think it's sensible to make the ImplementationAttributes API public. @mavasani had identified some scenarios where that's useful. Any concerns?

My main question is: why does this need special handling? It's an attribute, right? You should be able get it through the regular API?

333fred · 2021-01-20T19:38:17Z

(Didn't mean to close, sorry)

jcouv · 2021-01-20T22:44:54Z

@333fred Those are for metadata flags. You can put set them via attributes in source, but they are not emitted as attributes.
See PopulateMethodTableRows and ImplementationAttributes in PE symbols.
Here's a sharplab example to illustrate.

333fred · 2021-01-21T01:43:00Z

Thanks for the clarification Julien. So, the main concern here is symbols from metadata, since symbols from source could retrieve the info via the existing attributes APIs?

jcouv · 2021-01-21T05:38:19Z

There seems to be some additional value, even aside from metadata symbols.
Some methods get implementation flags for other reasons than having an MethodImplAttribute on them. See SourceMethodSymbolWithAttributes.ImplementationAttributes, SynthesizedInstanceConstructor and SynthesizedEventAccessorSymbol for example.

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

333fred · 2021-01-22T18:28:38Z

Done review pass (commit 2)

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractMethodSymbol.cs

Youssef1313 · 2021-02-17T14:36:52Z

@jcouv Thanks for the ping. I addressed @AlekseyTs's feedback. But not sure why some tests are failing. Failures happens for AggressiveInlining only.

AlekseyTs · 2021-02-17T23:06:10Z

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

        /// Misc implementation metadata flags (ImplFlags in metadata).
        /// </summary>
-        internal abstract System.Reflection.MethodImplAttributes ImplementationAttributes { get; }
+        internal abstract System.Reflection.MethodImplAttributes MethodImplementationFlags { get; }


MethodImplementationFlags [](start = 65, length = 25)

In my opinion this rename is unnecessary. #Closed

AlekseyTs · 2021-02-17T23:07:10Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

        [InlineData("[EnumeratorCancellation] ", "")]
        [InlineData("", "[EnumeratorCancellation] ")]
-        public void AsyncIteratorWithAwaitCompletedAndYield_WithEnumeratorCancellation_ExtendedPartialMethod(string definitionAttributes, string implementationAttributes)
+        public void AsyncIteratorWithAwaitCompletedAndYield_WithEnumeratorCancellation_ExtendedPartialMethod(string definitionAttributes, string MethodImplementationFlags)


MethodImplementationFlags [](start = 145, length = 25)

The rename looks suspicious. #Closed

AlekseyTs · 2021-02-17T23:10:30Z

src/Compilers/Test/Utilities/VisualBasic/MockSymbols.vb

    End Property

-    Friend Overrides ReadOnly Property ImplementationAttributes As MethodImplAttributes
+    Friend Overrides ReadOnly Property MethodImplementationFlags As MethodImplAttributes


MethodImplementationFlags [](start = 39, length = 25)

Similar to C# I think this rename is unnecessary. #Closed

AlekseyTs · 2021-02-17T23:14:47Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

 Microsoft.CodeAnalysis.GeneratorPostInitializationContext.AddSource(string hintName, string source) -> void
 Microsoft.CodeAnalysis.GeneratorPostInitializationContext.CancellationToken.get -> System.Threading.CancellationToken
 Microsoft.CodeAnalysis.GeneratorPostInitializationContext.GeneratorPostInitializationContext() -> void
+Microsoft.CodeAnalysis.IMethodSymbol.ImplementationAttributes.get -> System.Reflection.MethodImplAttributes


ImplementationAttributes [](start = 37, length = 24)

Is there an API like that. #Closed

AlekseyTs · 2021-02-17T23:20:48Z

src/Compilers/CSharp/Test/Symbol/Symbols/MethodImplementationFlagsTests.cs

+            {
+                var c = module.GlobalNamespace.GetMember<NamedTypeSymbol>("C");
+                var aggressiveInliningMethod = c.GetMember<MethodSymbol>("M_Aggressive");
+                Assert.Equal(MethodImplAttributes.AggressiveInlining, aggressiveInliningMethod.MethodImplementationFlags);


aggressiveInliningMethod.MethodImplementationFlags [](start = 70, length = 50)

It looks like this test doesn't actually test the added public API. Rename of the internal API made it hard to spot. I assume the other tests in this file have the same issue. #Closed

AlekseyTs · 2021-02-17T23:22:32Z

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/MethodImplementationFlagsTests.vb

+            Dim validator As Action(Of ModuleSymbol) = Sub([module])
+                                                           Dim c = [module].GlobalNamespace.GetMember(Of NamedTypeSymbol)("C")
+                                                           Dim aggressiveInliningMethod = c.GetMember(Of MethodSymbol)("M_Aggressive")
+                                                           Assert.Equal(MethodImplAttributes.AggressiveInlining, aggressiveInliningMethod.MethodImplementationFlags)


aggressiveInliningMethod.MethodImplementationFlags [](start = 113, length = 50)

Same comment as for C# tests. #Closed

AlekseyTs · 2021-02-17T23:33:45Z

Done with review pass (commit 17) #Closed

This reverts commit f1370bd.

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationMethodSymbol.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationConstructedMethodSymbol.cs

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractMethodSymbol.cs

...atures/Core/Portable/MetadataAsSource/AbstractMetadataAsSourceService.WrappedMethodSymbol.cs

…SourceService.WrappedMethodSymbol.cs

333fred

LGTM (commit 23), assuming tests pass.

333fred

These should fix the tests. The issue is that the default VB test framework is too old and doesn't have AggressiveInlining, so it doesn't compile.

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/MethodImplementationFlagsTests.vb

AlekseyTs · 2021-02-23T16:38:47Z

Done with review pass (commit 23), the CI is failing. #Closed

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

AlekseyTs

LGTM (commit 24)

AlekseyTs · 2021-02-24T01:52:54Z

@Youssef1313 Thanks for the contribution.

HelloKitty · 2021-06-06T02:45:52Z

What version is this shipped in? Currently getting: System.TypeLoadException: Method 'get_MethodImplementationFlags' in type 'Microsoft.CodeAnalysis.CodeGeneration.CodeGenerationMethodSymbol' from assembly 'Microsoft.CodeAnalysis.Workspaces, Version=3.9.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.

When referencing it was a transient property in a source generator.

333fred · 2021-06-06T03:41:07Z

Should be 3.10.

ghost added the Area-Compilers label Jan 16, 2021

Youssef1313 force-pushed the patch-29 branch from 48aa69d to 319387c Compare January 16, 2021 19:16

Youssef1313 marked this pull request as ready for review January 16, 2021 19:25

Youssef1313 requested review from a team as code owners January 16, 2021 19:25

Youssef1313 commented Jan 16, 2021

View reviewed changes

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractMethodSymbol.cs Outdated Show resolved Hide resolved

Add ImplementationAttributes to IMethodSymbol

a1a2646

Youssef1313 force-pushed the patch-29 branch from cff81dd to a1a2646 Compare January 16, 2021 19:34

Youssef1313 commented Jan 16, 2021

View reviewed changes

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationMethodSymbol.cs Outdated Show resolved Hide resolved

Update AbstractMetadataAsSourceService.WrappedMethodSymbol.cs

d1eb045

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 16, 2021

333fred closed this Jan 20, 2021

333fred reopened this Jan 20, 2021

333fred reviewed Jan 22, 2021

View reviewed changes

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits January 23, 2021 11:34

Doc comment

e8e4ac0

Test attempt

91a1758

Youssef1313 commented Jan 23, 2021

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs Outdated Show resolved Hide resolved

Youssef1313 and others added 2 commits January 25, 2021 20:08

Update SymbolEqualityTests.cs

678679b

Update tests

2c50bd2

333fred reviewed Jan 26, 2021

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs Outdated Show resolved Hide resolved

src/Compilers/CSharp/Test/Symbol/Symbols/SymbolEqualityTests.cs Outdated Show resolved Hide resolved

jasonmalinowski reviewed Jan 26, 2021

View reviewed changes

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs Outdated Show resolved Hide resolved

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationAbstractMethodSymbol.cs Outdated Show resolved Hide resolved

Youssef1313 and others added 2 commits January 30, 2021 21:08

Merge branch 'master' into patch-29

66db631

Use symbol validator in one test

0e09211

AlekseyTs reviewed Feb 17, 2021

View reviewed changes

Youssef1313 and others added 4 commits February 18, 2021 18:49

Revert "Rename to MethodImplementationFlags"

55b5fea

This reverts commit f1370bd.

Rename public api to MethodImplementationFlags

fe87018

Merge branch 'master' into patch-29

3e78098

Fix doc comment

183baa0

Youssef1313 commented Feb 18, 2021

View reviewed changes

Missing fixes in Workspaces layer

eea87d8

Youssef1313 commented Feb 18, 2021

View reviewed changes

...atures/Core/Portable/MetadataAsSource/AbstractMetadataAsSourceService.WrappedMethodSymbol.cs Outdated Show resolved Hide resolved

Update src/Features/Core/Portable/MetadataAsSource/AbstractMetadataAs…

c82381b

…SourceService.WrappedMethodSymbol.cs

333fred approved these changes Feb 18, 2021

View reviewed changes

333fred marked this pull request as ready for review February 18, 2021 18:05

333fred reviewed Feb 22, 2021

View reviewed changes

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/MethodImplementationFlagsTests.vb Outdated Show resolved Hide resolved

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/MethodImplementationFlagsTests.vb Outdated Show resolved Hide resolved

Apply suggestions from code review

58bd332

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

AlekseyTs approved these changes Feb 24, 2021

View reviewed changes

AlekseyTs merged commit 12fc3fc into dotnet:master Feb 24, 2021

ghost added this to the Next milestone Feb 24, 2021

Youssef1313 deleted the patch-29 branch February 24, 2021 04:40

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Conversation

Youssef1313 commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcouv commented Jan 20, 2021

Uh oh!

333fred commented Jan 20, 2021

Uh oh!

333fred commented Jan 20, 2021

Uh oh!

jcouv commented Jan 20, 2021

Uh oh!

333fred commented Jan 21, 2021

Uh oh!

jcouv commented Jan 21, 2021

Uh oh!

Uh oh!

333fred commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Youssef1313 commented Feb 17, 2021

Uh oh!

AlekseyTs Feb 17, 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!

AlekseyTs Feb 17, 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!

AlekseyTs Feb 17, 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!

AlekseyTs Feb 17, 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!

AlekseyTs Feb 17, 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!

AlekseyTs Feb 17, 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!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Feb 24, 2021

Uh oh!

HelloKitty commented Jun 6, 2021

Uh oh!

333fred commented Jun 6, 2021

Uh oh!

Reviewers

Youssef1313 commented Jan 16, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs Feb 17, 2021 •

edited

Loading

AlekseyTs commented Feb 17, 2021 •

edited

Loading

AlekseyTs commented Feb 23, 2021 •

edited

Loading