Local function attributes emit #39226

RikkiGibson · 2019-10-10T22:16:34Z

Related to #38801

This PR includes emitting the attributes in metadata, but not compiler recognition of most well-known attributes for local functions in source.

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

RikkiGibson · 2019-10-17T23:19:03Z

Could I please get a second sign-off @dotnet/roslyn-compiler #Closed

RikkiGibson · 2019-10-23T17:22:59Z

Ping @dotnet/roslyn-compiler #Closed

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs

agocke · 2019-10-23T20:54:37Z

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs

@@ -163,6 +165,10 @@ protected override ImmutableArray<TypeSymbol> ExtraSynthesizedRefParameters
        internal override bool IsExpressionBodied => false;
        internal MethodSymbol TopLevelMethod => _topLevelMethod;

+        public override ImmutableArray<CSharpAttributeData> GetAttributes() => _originalMethod.GetAttributes();


Hmm, I wonder if we should just override this for CCI. Maybe this should throw Unreachable? Who is asking for the attributes on a synthesized symbol? #Resolved

This is used when emitting the synthesized symbol's attributes. #Resolved

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs

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

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

AlekseyTs · 2019-10-24T22:34:03Z

It doesn't look like we are emitting attributes on type parameters. Also, what about other attributes normally synthesized by the compiler (dynamic, nullable, etc.) Do we emit those? Should we? #Closed

AlekseyTs · 2019-10-24T22:34:33Z

Done with review pass (Iteration 9) #Closed

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

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

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

…utes' into lfa-attrs-emit

RikkiGibson · 2019-11-07T00:32:06Z

I was depending on NamedTypeSymbol implementing INamedTypeSymbol in some of the tests, will fix. #Closed

RikkiGibson · 2019-11-12T21:41:25Z

Could I please get a second review @dotnet/roslyn-compiler #Closed

agocke · 2019-11-13T21:30:33Z

I'm still not clear on why we're overriding GetAttributes, which I view as a binding-level method, for retrieving attributes on synthesized symbols, instead of either IReference.GetAttributes(EmitContext) or GetAttributesToEmit.

I feel like any point where we are actually calling GetAttributes on a synthesized symbol is probably a bug, and if it wants to know the emit attributes it should call GetAttributesToEmit. #Resolved

AlekseyTs · 2019-11-13T21:41:30Z

@agocke

I'm still not clear on why we're overriding GetAttributes, which I view as a binding-level method

I am not sure why you would view things this way. The GetAttributes API doesn't imply any binding and we override it in many scenarios. Let's discuss this offline. #Closed

agocke · 2019-11-13T22:09:44Z

Talked with @AlekseyTs offline. The invariant that I thought should hold doesn't really hold. In that case, GetAttributes seems like the right thing to use. #Resolved

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs

agocke

LGTM

AlekseyTs · 2019-11-14T19:05:31Z

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

+        /// <summary>
+        /// Indicates whether synthesized methods derived from this method should inherit the attributes of this method, its parameters, return type, and type parameters.
+        /// </summary>
+        internal virtual bool SynthesizedMethodsInheritAttributes => false;


internal virtual bool SynthesizedMethodsInheritAttributes => false; [](start = 8, length = 67)

I find this approach very fragile and error prone. There could be multiple synthesized methods of different nature associated with the same "base" method. The property whether attributes should be inherited belongs to the specific synthesized method instead. Please make the change. #Closed

RikkiGibson · 2019-11-14T19:28:50Z

Follow up to #39226 (comment)

It doesn't look like we are emitting attributes on type parameters. Also, what about other attributes normally synthesized by the compiler (dynamic, nullable, etc.) Do we emit those? Should we?

It looks like there is still a gap for emitting the attributes of type parameters. Will make sure to address this in follow-up. #Resolved

AlekseyTs · 2019-11-14T19:43:50Z

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

+            comp.VerifyDiagnostics();
+            var verifier = CompileAndVerify(comp);
+
+            // PROTOTYPE: EnumeratorCancellation attribute should produce a combinedTokens field and modify codegen


PROTOTYPE [](start = 15, length = 9)

We usually add feature name after the "PROTOTYPE" word in parens. I.e. PROTOTYPE(<feature name here>). #Closed

AlekseyTs · 2019-11-14T19:51:41Z

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs

@@ -130,6 +137,20 @@ private ImmutableArray<ParameterSymbol> MakeParameters()
            return builder.ToImmutableAndFree();
        }

+        public override ImmutableArray<CSharpAttributeData> GetAttributes()


override [](start = 15, length = 8)

sealed? #Closed

AlekseyTs · 2019-11-14T19:51:49Z

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs

+                : ImmutableArray<CSharpAttributeData>.Empty;
+        }
+
+        public override ImmutableArray<CSharpAttributeData> GetReturnTypeAttributes()


override [](start = 15, length = 8)

sealed? #Closed

AlekseyTs · 2019-11-14T20:35:10Z

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

+
+                var localFn4 = cClass.GetMethod("<M>g__local4|0_3");
+                var attrs4 = localFn4.TypeParameters.Single().GetAttributes();
+                Assert.Equal("A", attrs4.Single().AttributeClass.Name);


attrs4.Single() [](start = 34, length = 15)

I don't see any change that would enable inheritance of attributes for type parameters. It looks like this is done unconditionally and unintentionally (we do want this for local functions, but not for any other case). #Closed

AlekseyTs · 2019-11-14T20:36:02Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs

+        [Theory]
+        [MemberData(nameof(OptimizationLevelTheoryData))]
+        [WorkItem(431, "https://github.com/dotnet/roslyn/issues/431")]
+        public void BaseMethodWrapper_DoNotInheritAttributes(OptimizationLevel optimizationLevel)


BaseMethodWrapper_DoNotInheritAttributes [](start = 20, length = 40)

Please include coverage for attributes on type parameters as well. #Closed

AlekseyTs · 2019-11-14T20:37:54Z

Done with review pass (iteration 16)

RikkiGibson added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 10, 2019

RikkiGibson added 4 commits October 16, 2019 14:25

Add test (doesn't read the local function correctly)

9ac8af5

Add basic emit test

2a0d830

Local function attributes emit

0dc3d2b

Add a simple test with an attribute argument

31b8a27

RikkiGibson force-pushed the lfa-attrs-emit branch from b4a8560 to 31b8a27 Compare October 16, 2019 21:49

RikkiGibson added the Area-Compilers label Oct 16, 2019

RikkiGibson added 2 commits October 16, 2019 15:15

Fix crash for synthesized parameters with custom ref modifiers

bd6d018

Simplify GetAttributes

1453260

RikkiGibson commented Oct 16, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson added 2 commits October 16, 2019 16:06

Add test

cbd5f78

Add VerifyTypeIL check

6d9bac5

RikkiGibson removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 16, 2019

RikkiGibson marked this pull request as ready for review October 16, 2019 23:21

RikkiGibson requested a review from a team as a code owner October 16, 2019 23:21

RikkiGibson added this to the Compiler.Next milestone Oct 16, 2019

cston reviewed Oct 17, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

cston approved these changes Oct 17, 2019

View reviewed changes

Address feedback. Split TypeIL out into Desktop-only test

a9695b9

RikkiGibson requested a review from a team October 17, 2019 18:15

agocke reviewed Oct 23, 2019

View reviewed changes

AlekseyTs reviewed Oct 24, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 24, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 24, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 24, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Oct 24, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs Outdated Show resolved Hide resolved

cston reviewed Nov 6, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs Outdated Show resolved Hide resolved

cston reviewed Nov 6, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs Outdated Show resolved Hide resolved

cston reviewed Nov 6, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs Outdated Show resolved Hide resolved

cston reviewed Nov 6, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs Outdated Show resolved Hide resolved

RikkiGibson added 2 commits November 6, 2019 16:18

Address more feedback

aaafd27

Merge remote-tracking branch 'upstream/features/local-function-attrib…

6ed4fea

…utes' into lfa-attrs-emit

RikkiGibson added 2 commits November 12, 2019 11:13

Fix build errors

cca8786

Make some methods static

e6acb3d

cston approved these changes Nov 12, 2019

View reviewed changes

agocke reviewed Nov 13, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs Outdated Show resolved Hide resolved

agocke reviewed Nov 13, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureMethod.cs Outdated Show resolved Hide resolved

Move GetAttributes() up and add assert

22da0fa

agocke approved these changes Nov 14, 2019

View reviewed changes

RikkiGibson merged commit 6af5960 into dotnet:features/local-function-attributes Nov 14, 2019

RikkiGibson deleted the lfa-attrs-emit branch November 14, 2019 00:43

AlekseyTs reviewed Nov 14, 2019

View reviewed changes

RikkiGibson mentioned this pull request Nov 14, 2019

Follow-up to "Local function attributes emit" #39830

Merged

AlekseyTs reviewed Nov 14, 2019

View reviewed changes

RikkiGibson mentioned this pull request Jan 27, 2020

Don't emit synthesized attributes on closure methods #41235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local function attributes emit #39226

Local function attributes emit #39226

RikkiGibson commented Oct 10, 2019 •

edited

Loading

RikkiGibson commented Oct 17, 2019 •

edited

Loading

RikkiGibson commented Oct 23, 2019 •

edited

Loading

agocke Oct 23, 2019 •

edited by RikkiGibson

Loading

RikkiGibson Nov 12, 2019 •

edited

Loading

AlekseyTs commented Oct 24, 2019 •

edited by RikkiGibson

Loading

AlekseyTs commented Oct 24, 2019 •

edited by RikkiGibson

Loading

RikkiGibson commented Nov 7, 2019 •

edited

Loading

RikkiGibson commented Nov 12, 2019 •

edited

Loading

agocke commented Nov 13, 2019 •

edited by RikkiGibson

Loading

AlekseyTs commented Nov 13, 2019 •

edited

Loading

agocke commented Nov 13, 2019 •

edited by RikkiGibson

Loading

agocke left a comment

AlekseyTs Nov 14, 2019 •

edited

Loading

RikkiGibson commented Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs commented Nov 14, 2019

Local function attributes emit #39226

Local function attributes emit #39226

Conversation

RikkiGibson commented Oct 10, 2019 • edited Loading

RikkiGibson commented Oct 17, 2019 • edited Loading

RikkiGibson commented Oct 23, 2019 • edited Loading

agocke Oct 23, 2019 • edited by RikkiGibson Loading

Choose a reason for hiding this comment

RikkiGibson Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Oct 24, 2019 • edited by RikkiGibson Loading

AlekseyTs commented Oct 24, 2019 • edited by RikkiGibson Loading

RikkiGibson commented Nov 7, 2019 • edited Loading

RikkiGibson commented Nov 12, 2019 • edited Loading

agocke commented Nov 13, 2019 • edited by RikkiGibson Loading

AlekseyTs commented Nov 13, 2019 • edited Loading

agocke commented Nov 13, 2019 • edited by RikkiGibson Loading

agocke left a comment

Choose a reason for hiding this comment

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented Nov 14, 2019 • edited Loading

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 14, 2019

RikkiGibson commented Oct 10, 2019 •

edited

Loading

RikkiGibson commented Oct 17, 2019 •

edited

Loading

RikkiGibson commented Oct 23, 2019 •

edited

Loading

agocke Oct 23, 2019 •

edited by RikkiGibson

Loading

RikkiGibson Nov 12, 2019 •

edited

Loading

AlekseyTs commented Oct 24, 2019 •

edited by RikkiGibson

Loading

AlekseyTs commented Oct 24, 2019 •

edited by RikkiGibson

Loading

RikkiGibson commented Nov 7, 2019 •

edited

Loading

RikkiGibson commented Nov 12, 2019 •

edited

Loading

agocke commented Nov 13, 2019 •

edited by RikkiGibson

Loading

AlekseyTs commented Nov 13, 2019 •

edited

Loading

agocke commented Nov 13, 2019 •

edited by RikkiGibson

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

RikkiGibson commented Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading

AlekseyTs Nov 14, 2019 •

edited

Loading