Support more well-known attributes in local functions by RikkiGibson · Pull Request #41299 · dotnet/roslyn

RikkiGibson · 2020-01-30T00:26:33Z

Related to #38801

cston · 2020-01-30T16:29:56Z

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

        // Note that this doesn't affect BaseMethodWrapperSymbol for example because the implementation has no locals.
        public sealed override bool AreLocalsZeroed => !(BaseMethod is SourceMethodSymbol sourceMethod) || sourceMethod.AreLocalsZeroed;

+        internal override bool RequiresSecurityObject => InheritsBaseMethodAttributes && BaseMethod.RequiresSecurityObject;


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

sealed for each override. #Closed

cston · 2020-01-30T16:42:28Z

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

+line: 13
+";
+
+            var compilation = CreateCompilationWithMscorlib45(source, options: TestOptions.ReleaseExe, parseOptions: TestOptions.RegularPreview);


CreateCompilationWithMscorlib45 [](start = 30, length = 31)

CreateCompilation? #Closed

cston · 2020-01-30T16:43:06Z

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

+
+            var compilation = CreateCompilationWithMscorlib45(
+                source,
+                references: new MetadataReference[] { SystemRef },


CreateCompilation(source, ... here and in other added tests? #Closed

cston · 2020-01-30T16:52:39Z

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

+}";
+
+            var expected = @"
+name: LocalFunctionCaller


LocalFunctionCaller [](start = 6, length = 19)

~~Should this report "local1" instead?~~
This is consistent with the name used when calling a member method with [CallerMemberName] from a local function. #Closed

cston · 2020-01-30T16:58:59Z

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

+    }
+}
+";
+            var compilation = CreateCompilationWithMscorlib40(source, options: TestOptions.ReleaseDll, parseOptions: TestOptions.RegularPreview);


CreateCompilationWithMscorlib40 [](start = 30, length = 31)

CreateCompilation? #Resolved

cston · 2020-01-30T17:09:40Z

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

        }

+        [Fact]
+        public void LocalFunctionParamsArray_ParamArrayAttribute()


ParamArrayAttribute [](start = 45, length = 19)

There is no ParamArrayAttribute in the test. If that's intentional, consider removing _ParamArrayAttribute from the test name. #Resolved

AlekseyTs · 2020-01-30T20:44:44Z

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

                get { return ImmutableArray<CustomModifier>.Empty; }
            }
+
+            internal override MarshalPseudoCustomAttributeData MarshallingInformation


internal override MarshalPseudoCustomAttributeData MarshallingInformation [](start = 12, length = 73)

It is not clear what motivated this change. #Closed

Because I deleted the implementation in SynthesizedParameterSymbolBase, it was necessary to provide an implementation in each subtype.

In reply to: 373183931 [](ancestors = 373183931)

AlekseyTs · 2020-01-30T20:57:17Z

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

                    p.Name,
-                    attributes: inheritAttributes ? p.GetAttributes() : default,
-                    hasEnumeratorCancellationAttribute: inheritAttributes && p.IsSourceParameterWithEnumeratorCancellationAttribute()));
+                    p.RefCustomModifiers,


p.RefCustomModifiers [](start = 20, length = 20)

Should these be substituted? It looks like we didn't pass these before. Do we have a test with a scenario where this change makes a difference? #Closed

AlekseyTs · 2020-01-30T21:03:23Z

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

        {
            Debug.Assert(!refCustomModifiers.IsDefault);
-            Debug.Assert(!attributes.IsDefault);
+            Debug.Assert(!refCustomModifiers.IsEmpty || baseParameterForAttributes is object);


!refCustomModifiers.IsEmpty || baseParameterForAttributes is object [](start = 25, length = 67)

It is not obvious why is this the right thing to assert #Closed

If this condition is false, it means there was no good reason to create this as a SynthesizedComplexParameterSymbol instead of a SynthesizedParameterSymbol.

In reply to: 373192152 [](ancestors = 373192152)

AlekseyTs · 2020-01-30T21:09:51Z

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

+                    oldParam.RefKind,
+                    oldParam.Name,
+                    oldParam.RefCustomModifiers,
+                    (oldParam as SynthesizedComplexParameterSymbol)?.BaseParameterForAttributes));


(oldParam as SynthesizedComplexParameterSymbol) [](start = 20, length = 47)

Are there scenarios when this is not null? Do we have a test "backing" this change? #Closed

I don't think any test is affected by this change. This is used for SynthesizedEntryPointSymbol, SynthesizedImplementationMethod, and SynthesizedSealedPropertyAccessor. Actually, this seems like a behavior change where for example a synthesized sealed accessor may start to contain attributes from a base accessor from source, when it didn't before. So perhaps the right thing to do here is to just revert the change.

In reply to: 373194840 [](ancestors = 373194840)

If for all scenarios that can happen today oldParam is never a SynthesizedComplexParameterSymbol, I would add an assert to confirm this fact and pass null.

In reply to: 373204933 [](ancestors = 373204933,373194840)

AlekseyTs · 2020-01-30T21:15:23Z

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

                case DebugInfoInjector { Previous: var previous } injector:
                    var newPrevious = RemoveDynamicAnalysisInjectors(previous);
-                    return (object)newPrevious != previous ? new DebugInfoInjector(newPrevious) : injector;
+                    if ((object)newPrevious == Instrumenter.NoOp)


(object)newPrevious == Instrumenter.NoOp [](start = 24, length = 40)

I think it would be better to check if (object)newPrevious == previous first #Closed

AlekseyTs · 2020-01-30T21:20:41Z

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

+                Console.WriteLine(""name: "" + callerName);
+            }
+
+            log();


log(); [](start = 12, length = 6)

Consider also testing scenario with nesting into a lambda #Closed

AlekseyTs · 2020-01-30T21:28:45Z

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

+                Assert.Empty(localFn1.GetReturnTypeAttributes());
+
+                var param = localFn1.Parameters.Single();
+                Assert.Empty(param.GetAttributes());


Assert.Empty(param.GetAttributes()); [](start = 16, length = 36)

Is this intentional? Did we explicitly decide to not synthesize the attribute? #Closed

This is pre-existing behavior for local functions with params parameters. I'm not sure if this happened by accident or if it was a deliberate decision. /cc @agocke in case you recall handling this scenario in local functions.

In reply to: 373203102 [](ancestors = 373203102)

AlekseyTs · 2020-01-30T21:31:47Z

Done with review pass (iteration 6) #Closed

cston · 2020-01-30T23:09:14Z

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

-        private readonly ImmutableArray<CSharpAttributeData> _attributes;
+
+        // The parameter containing attributes to inherit into this synthesized parameter, if any.
+        internal SourceComplexParameterSymbol? BaseParameterForAttributes { get; }


internal [](start = 8, length = 8)

Could this be a private readonly field? #Resolved

AlekseyTs

LGTM (iteration 8)

RikkiGibson added 5 commits January 29, 2020 15:38

Reduce allocations in RemoveDynamicAnalysisInjectors

f57cd0c

Add tests for CallerInfo and ParamArray attributes

ac66327

Support MarshalAs attributes in local functions

109cf66

Handle some security attributes in local functions

dd9c986

Cleanup

7f6a1f8

RikkiGibson added the Area-Compilers label Jan 30, 2020

RikkiGibson marked this pull request as ready for review January 30, 2020 01:07

RikkiGibson requested a review from a team as a code owner January 30, 2020 01:07

cston reviewed Jan 30, 2020

View reviewed changes

Address feedback

7a008d2

cston approved these changes Jan 30, 2020

View reviewed changes

AlekseyTs reviewed Jan 30, 2020

View reviewed changes

Address feedback

c92ab46

cston reviewed Jan 30, 2020

View reviewed changes

Make baseParameterForAttributes a field

902c720

AlekseyTs approved these changes Jan 31, 2020

View reviewed changes

RikkiGibson merged commit 8422f4f into dotnet:features/local-function-attributes Jan 31, 2020

RikkiGibson deleted the lfa-misc-attributes branch January 31, 2020 02:55

RikkiGibson mentioned this pull request Jan 31, 2020

Test plan for "local function attributes" #38801

Closed

54 tasks

Conversation

RikkiGibson commented Jan 30, 2020

Uh oh!

cston Jan 30, 2020 • 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!

cston Jan 30, 2020 • 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!

cston Jan 30, 2020 • 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!

cston Jan 30, 2020 • 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!

cston Jan 30, 2020 • edited by RikkiGibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jan 30, 2020 • edited by RikkiGibson 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 Jan 30, 2020 • 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!

RikkiGibson Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 30, 2020 • 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 Jan 30, 2020 • 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!

RikkiGibson Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 30, 2020 • 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!

RikkiGibson Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 30, 2020 • 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 Jan 30, 2020 • 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 Jan 30, 2020 • 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!

RikkiGibson Jan 30, 2020 • 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 Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jan 30, 2020 • edited by RikkiGibson 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 left a comment

Choose a reason for hiding this comment

cston Jan 30, 2020 •

edited

Loading

cston Jan 30, 2020 •

edited

Loading

cston Jan 30, 2020 •

edited

Loading

cston Jan 30, 2020 •

edited

Loading

cston Jan 30, 2020 •

edited by RikkiGibson

Loading

cston Jan 30, 2020 •

edited by RikkiGibson

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

AlekseyTs Jan 30, 2020 •

edited

Loading

RikkiGibson Jan 30, 2020 •

edited

Loading

AlekseyTs commented Jan 30, 2020 •

edited

Loading

cston Jan 30, 2020 •

edited by RikkiGibson

Loading