Support field-targeted attributes on auto-properties (7.3) by jcouv · Pull Request #22664 · dotnet/roslyn

jcouv · 2017-10-11T22:41:36Z

This is a small feature for C# 7.3.

In the following example, the field-targeted attribute is applied to the backing field instead of the property itself.

[Serializable]
public class Foo {
    [field: NonSerialized]
    public string MySecret { get; set; }
}

This used to be a warning: warning CS0657: 'field' is not a valid attribute location for this declaration. Valid attribute locations for this declaration are 'property'. All attributes in this block will be ignored.

📝 The two failing tests are expected, as this PR needs be be rebased to get the new LanguageVersion.

📝 No VB change. The field target doesn't exist in VB (only assembly and module are supported as explicit targets).

Fixes #22512
Implements dotnet/csharplang#42
Spec https://github.com/dotnet/csharplang/blob/master/proposals/auto-prop-field-attrs.md

jcouv · 2017-10-30T16:34:38Z

📝 This is a warning (rather than an error) since such code currently produces a warning.
[field: SomeAttribute] public int prop { get; set; } #Resolved

jcouv · 2017-10-30T17:35:28Z

@AlekseyTs @dotnet/roslyn-compiler This PR is ready for review. The first commit adds support for attributes on backing fields. The second commit adds LangVersion 7.3.
This PR will need to be retargeted to the appropriate branch for 7.3 (once we know what branch that is). #Resolved

cston · 2017-10-30T19:05:47Z

+        private void CheckForFieldTargetedAttribute(bool isAutoProperty, BasePropertyDeclarationSyntax syntax, DiagnosticBag diagnostics)
+        {
+            var languageVersion = this.DeclaringCompilation.LanguageVersion;
+            if (!isAutoProperty || languageVersion.AllowAttributesOnBackingFields())


Consider having the caller check _isAutoProperty and remove the parameter. #Resolved

AlekseyTs · 2017-10-31T22:32:00Z


+        /// <summary>
+        /// C# language version 7.3
+        /// </summary>


CSharp7_3 = 703 [](start = 8, length = 15)

CSharp7_3 = 703 [](start = 8, length = 15)

It is probably worth separating addition of a language version into its own PR. #Closed

AlekseyTs · 2017-10-31T22:53:25Z

            }
        }

+        internal virtual Location ErrorLocation => throw ExceptionUtilities.Unreachable;


internal virtual Location ErrorLocation => throw ExceptionUtilities.Unreachable; [](start = 8, length = 80)

In general, we prefer abstract API to virtual APIs with default implementation that throws. Perhaps this API can be moved to specific derived type?

AlekseyTs · 2017-10-31T23:16:05Z

            AddSynthesizedAttribute(ref attributes, compilation.SynthesizeDebuggerBrowsableNeverAttribute());
        }
+
+        internal sealed override bool IsNotSerialized


IsNotSerialized [](start = 38, length = 15)

IsNotSerialized [](start = 38, length = 15)

It feels like a lot of code duplicated here, I think we should find a way to share it instead. Perhaps we should stop inheriting from SynthesizedFieldSymbolBase and introduce a new base class, from which SourceFieldSymbol and SynthesizedBackingFieldSymbol can inherit. #Closed

If we do that, then we'll end up duplicating some logic related to synthesized symbols.
I can give a try and see how it looks... #Resolved

That worked out better indeed. Thanks Aleksey #Resolved

AlekseyTs · 2017-11-07T21:51:28Z

SynthesizedFieldSymbolBase.AddSynthesizedFieldAttributes(ref attributes, this, suppressDynamicAttribute: false); [](start = 12, length = 112)

It feels like, if we move SourceFieldSymbol.AddSynthesizedAttributes to the new base, we wouldn't have to add this call. #Closed

AlekseyTs · 2017-11-07T21:52:41Z

AddSynthesizedFieldAttributes [](start = 29, length = 29)

It feels like, if we move SourceFieldSymbol.AddSynthesizedAttributes to the new base, we wouldn't have to make any changes in this file. #Closed

AlekseyTs · 2017-11-07T22:01:46Z

// (7,16): error CS0625: 'Test.P': instance field types marked with StructLayout(LayoutKind.Explicit) must have a FieldOffset attribute [](start = 16, length = 135)

Is this message missing 'in' - "instance field in type marked ..."? #Closed

AlekseyTs · 2017-11-07T22:04:52Z

// TODO [](start = 16, length = 7)

TODO? #Closed

AlekseyTs · 2017-11-07T22:15:11Z

Done with review pass (iteration 3). #Closed

AlekseyTs · 2017-11-07T22:16:58Z

It looks like there are some test failures. #Resolved

jcouv · 2017-11-07T22:31:54Z

That's expected (two tests related to language version). I didn't want to rebase mid-review.

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

AlekseyTs · 2017-11-08T01:12:42Z

if (!this.ContainingType.IsImplicitlyDeclared) [](start = 12, length = 46)

I do not think this condition can ever be true. Consider replacing the if with assert. #Closed

It turns out this can be reached (InteractiveSessionTests.ScriptMemberAccessFromNestedClass). I'll restore the original code.

AlekseyTs · 2017-11-08T04:45:37Z

AssertEx.SetEqual(fieldAttributesExpected, GetAttributeStrings(field1.GetAttributes())); [](start = 20, length = 88)

Is the attribute converted to a constant value on the field? #Closed

AlekseyTs · 2017-11-08T04:45:51Z

Assert.Empty(GetAttributeStrings(field3.GetAttributes())); [](start = 20, length = 58)

Is the attribute converted to a constant value on the field? #Closed

AlekseyTs

LGTM (iteration 5) with minor comments.

jcouv · 2017-11-08T16:54:35Z

Awesome. Thanks!
@dotnet/roslyn-compiler for a second review

cston · 2017-11-13T05:26:59Z

Looks like the method returns a non-null value in all cases.

jcouv · 2017-11-13T20:13:27Z

test ubuntu_14_debug_prtest please

jcouv added the Area-Compilers label Oct 11, 2017

jcouv added this to the 15.later milestone Oct 11, 2017

jcouv self-assigned this Oct 11, 2017

dnfclas added the cla-already-signed label Oct 11, 2017

jcouv changed the title ~~Support field-targeted attributes on auto-properties (post dev15.5)~~ Support field-targeted attributes on auto-properties (15.later) Oct 11, 2017

jcouv force-pushed the field-attribute branch from c33b872 to 24d780c Compare October 12, 2017 01:54

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 12, 2017

jcouv force-pushed the field-attribute branch 4 times, most recently from 76b1da1 to 65062f6 Compare October 13, 2017 19:18

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 13, 2017

jcouv force-pushed the field-attribute branch from 65062f6 to f50a8d3 Compare October 13, 2017 21:50

jcouv changed the base branch from master to post-dev15.5-contrib October 13, 2017 21:51

jcouv force-pushed the field-attribute branch 3 times, most recently from ab1986f to c6050c0 Compare October 15, 2017 20:08

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 27, 2017

jcouv force-pushed the field-attribute branch 2 times, most recently from 905d79e to 891f24b Compare October 28, 2017 00:22

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 28, 2017

jcouv changed the title ~~Support field-targeted attributes on auto-properties (15.later)~~ Support field-targeted attributes on auto-properties (7.3) Oct 30, 2017

jcouv commented Oct 30, 2017

View reviewed changes

jcouv force-pushed the field-attribute branch from 891f24b to b0d6259 Compare October 30, 2017 16:56

jcouv changed the base branch from post-dev15.5-contrib to features/compiler October 30, 2017 18:48

cston reviewed Oct 30, 2017

View reviewed changes

AlekseyTs reviewed Oct 31, 2017

View reviewed changes

AlekseyTs reviewed Nov 7, 2017

View reviewed changes

jcouv mentioned this pull request Nov 7, 2017

SourceFieldSymbol.HasRuntimeSpecialName should check that containing type is enum #23058

Open

AlekseyTs reviewed Nov 8, 2017

View reviewed changes

AlekseyTs approved these changes Nov 8, 2017

View reviewed changes

cston reviewed Nov 13, 2017

View reviewed changes

cston approved these changes Nov 13, 2017

View reviewed changes

jcouv and others added 7 commits November 13, 2017 11:24

Support field-targeted attributes on auto-properties (C#)

0789c10

Gate feature on language version

4e9f094

Address PR feedback from Aleksey

9748c3b

Address PR feedback from Aleksey

7d14c30

Fix regression

8b4ecf4

Address PR feedback from Aleksey

03d858b

Address PR feedback from Chuck

d468b14

jcouv force-pushed the field-attribute branch from 6baae7c to d468b14 Compare November 13, 2017 19:27

Use LanguageVersion.CSharp7_3 following rebasing

9d1888c

Fix regression

0c6e6c5

jcouv merged commit 3d2101e into dotnet:features/compiler Nov 13, 2017

jcouv deleted the field-attribute branch November 13, 2017 21:24

jcouv modified the milestones: 15.6, 15.7 Jan 9, 2018

Conversation

jcouv commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Oct 30, 2017 • 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 commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Oct 30, 2017 • edited by jcouv 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 Oct 31, 2017 • 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 Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 31, 2017 • 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 1, 2017 • 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 6, 2017 • 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 7, 2017 • 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 7, 2017 • 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 7, 2017 • 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 7, 2017 • 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 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Nov 7, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Nov 7, 2017

Uh oh!

AlekseyTs Nov 8, 2017 • 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 13, 2017

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 8, 2017 • 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 8, 2017 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Nov 8, 2017

Uh oh!

cston Nov 13, 2017

Choose a reason for hiding this comment

jcouv commented Oct 11, 2017 •

edited

Loading

jcouv Oct 30, 2017 •

edited

Loading

jcouv commented Oct 30, 2017 •

edited

Loading

cston Oct 30, 2017 •

edited by jcouv

Loading

AlekseyTs Oct 31, 2017 •

edited

Loading

AlekseyTs Oct 31, 2017 •

edited

Loading

jcouv Nov 1, 2017 •

edited

Loading

jcouv Nov 6, 2017 •

edited

Loading

AlekseyTs Nov 7, 2017 •

edited

Loading

AlekseyTs Nov 7, 2017 •

edited

Loading

AlekseyTs Nov 7, 2017 •

edited

Loading

AlekseyTs Nov 7, 2017 •

edited

Loading

AlekseyTs commented Nov 7, 2017 •

edited

Loading

AlekseyTs commented Nov 7, 2017 •

edited by jcouv

Loading

AlekseyTs Nov 8, 2017 •

edited

Loading

AlekseyTs Nov 8, 2017 •

edited

Loading

AlekseyTs Nov 8, 2017 •

edited

Loading