Store nullable attribute constructor params in a field: #33668

chsienki · 2019-02-25T22:38:46Z

Add a derived nullable attribute symbol
Add a constructor symbol that allows assignment to a field
Generate the appropriate constructors for byte and byte[] versions of the attribute
Add tests

- Add a derived nullable attribute symbol - Add a constructor symbol that allows assignment to a field - Generate the appropriate constructors for byte and byte[] versions of the attribute - Add tests

chsienki · 2019-02-25T22:39:17Z

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

-                _constructors = getConstructors(compilation, this, diagnostics);
-            }
-            else
+            if (generateDefaultConstructors)


I hate this. Any ideas on a better approach most welcome.

Perhaps replace the _constructors field with an abstract Constructors property, and have two derived types, one that returns a single constructor with no parameters.

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

chsienki · 2019-02-26T01:27:39Z

@dotnet/roslyn-compiler for review please.

cston · 2019-02-27T00:03:54Z

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

+    {
+        private readonly ImmutableArray<FieldSymbol> _fields;
+
+        private readonly TypeSymbolWithAnnotations _byteType;


TypeSymbolWithAnnotations [](start = 25, length = 25)

Can be stored as TypeSymbol.

cston · 2019-02-27T00:07:49Z

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

+
+        internal sealed class SynthesizedEmbeddedAttributeConstructorWithBodySymbol : SynthesizedInstanceConstructor
+        {
+            ImmutableArray<ParameterSymbol> _parameters;


ImmutableArray [](start = 12, length = 14)

private readonly

cston · 2019-02-27T00:08:40Z

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

+        {
+            ImmutableArray<ParameterSymbol> _parameters;
+
+            readonly Action<SyntheticBoundNodeFactory, ArrayBuilder<BoundStatement>, ImmutableArray<ParameterSymbol>> _getConstructorBody;


readonly [](start = 12, length = 8)

private

cston · 2019-02-27T00:08:57Z

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

+            );
+        }
+
+        internal sealed class SynthesizedEmbeddedAttributeConstructorWithBodySymbol : SynthesizedInstanceConstructor


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

private

cston · 2019-02-27T00:30:12Z

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

+        }
+
+        [Fact]
+        public void Field_Contains_ConstuctorArugments_SingleByteConstructor()


Constuctor [](start = 35, length = 10)

Typo, here and test below.

cston · 2019-02-27T00:34:21Z

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

@@ -1686,6 +1689,107 @@ static void F(B<A<(object?, (object, object?), object, object?, object, object?,
                type.GetMember<PropertySymbol>("Property").ToTestDisplayString());
        }

+        [Fact]
+        public void Field_Exists()


Field_ [](start = 20, length = 6)

The test names are a little confusing since it's not clear what "Field" refers to (at least it won't be clear outside of this PR). Perhaps name these tests "NullableAttribute_" or "NullableAttributeField_" as approriate.

cston · 2019-02-27T00:34:58Z

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

+namespace Microsoft.CodeAnalysis.CSharp.Symbols
+{
+
+    internal class SynthesizedEmbeddedNullableAttributeSymbol : SynthesizedEmbeddedAttributeSymbol


class [](start = 13, length = 5)

sealed

jaredpar · 2019-02-27T18:45:58Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

@@ -305,42 +303,36 @@ private void CreateEmbeddedAttributeItselfIfNeeded(DiagnosticBag diagnostics)
        private void CreateEmbeddedAttributeIfNeeded(
            ref SynthesizedEmbeddedAttributeSymbol symbol,
            DiagnosticBag diagnostics,
-            AttributeDescription description,
-            Func<CSharpCompilation, NamedTypeSymbol, DiagnosticBag, ImmutableArray<MethodSymbol>> getConstructors = null)
+            AttributeDescription description)
        {
            if ((object)symbol == null)


symbol is null #Resolved

jaredpar · 2019-02-27T18:46:13Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

-                new SynthesizedEmbeddedAttributeConstructorSymbol(
-                    containingType,
-                    m => ImmutableArray.Create(SynthesizedParameterSymbol.Create(m, byteArray, 0, RefKind.None))));
+            if ((object)symbol == null)


symbol is null #Resolved

jaredpar · 2019-02-27T18:47:08Z

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

        private readonly ModuleSymbol _module;

+        protected ImmutableArray<MethodSymbol> _constructors;


protected values should be PascalCased and not _ prefixed.

jaredpar · 2019-02-27T18:48:08Z

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

+                new SynthesizedFieldSymbol(
+                    this,
+                    byteArrayType.TypeSymbol,
+                    "nullableFlags",


Given this is public it should be PascalCased

jaredpar · 2019-02-27T18:49:12Z

Done with review pass (iteration 1)

- Split SynthesizedEmbeddedAttribute into a base class and implementation - Rename nullableFlags to NullableFlags - Rename tests

jaredpar · 2019-02-27T19:12:00Z

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

+
+        /// <summary>
+        /// When implemented by a more derived type, gets a list of constructors to synthesize for the attribute
+        /// </summary>


Blank line below.

jaredpar

cston · 2019-02-27T19:21:00Z

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

+    /// 4) It has System.Runtime.CompilerServices.CompilerGeneratedAttribute
+    /// 5) It has a parameter-less constructor
+    /// </summary>
+    internal abstract class SynthesizedEmbeddedAttributeSymbolBase : NamedTypeSymbol


SynthesizedEmbeddedAttributeSymbolBase [](start = 28, length = 38)

Consider leaving this type in SynthesizedEmbeddedAttributeSymbol.cs, along with the derived type, so the changes in this PR are clear.

…arer

Store nullable attribute constructor params in a field:

0ecd009

- Add a derived nullable attribute symbol - Add a constructor symbol that allows assignment to a field - Generate the appropriate constructors for byte and byte[] versions of the attribute - Add tests

chsienki added the Area-Compilers label Feb 25, 2019

chsienki commented Feb 25, 2019

View reviewed changes

chsienki marked this pull request as ready for review February 26, 2019 01:27

chsienki requested a review from a team as a code owner February 26, 2019 01:27

jcouv added the New Language Feature - Nullable Reference Types Nullable Reference Types label Feb 26, 2019

jcouv added this to the 16.1.P1 milestone Feb 26, 2019

cston reviewed Feb 27, 2019

View reviewed changes

jaredpar reviewed Feb 27, 2019

View reviewed changes

PR Feedback:

69b5581

- Split SynthesizedEmbeddedAttribute into a base class and implementation - Rename nullableFlags to NullableFlags - Rename tests

jaredpar reviewed Feb 27, 2019

View reviewed changes

jaredpar approved these changes Feb 27, 2019

View reviewed changes

Remove dead summary text

d6b8f87

cston reviewed Feb 27, 2019

View reviewed changes

Move base attribute class into the original class to make changes cle…

3573a16

…arer

cston approved these changes Feb 27, 2019

View reviewed changes

chsienki merged commit 509277e into dotnet:master Feb 27, 2019

chsienki deleted the nullable_attribute_field branch February 27, 2019 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store nullable attribute constructor params in a field: #33668

Store nullable attribute constructor params in a field: #33668

chsienki commented Feb 25, 2019

chsienki Feb 25, 2019

cston Feb 27, 2019

chsienki commented Feb 26, 2019

cston Feb 27, 2019

cston Feb 27, 2019

cston Feb 27, 2019

cston Feb 27, 2019

cston Feb 27, 2019

cston Feb 27, 2019

cston Feb 27, 2019

jaredpar Feb 27, 2019 •

edited

Loading

jaredpar Feb 27, 2019 •

edited

Loading

jaredpar Feb 27, 2019

jaredpar Feb 27, 2019

jaredpar commented Feb 27, 2019

jaredpar Feb 27, 2019

jaredpar left a comment

cston Feb 27, 2019

chsienki Feb 27, 2019

		private readonly ModuleSymbol _module;

		protected ImmutableArray<MethodSymbol> _constructors;

Store nullable attribute constructor params in a field: #33668

Store nullable attribute constructor params in a field: #33668

Conversation

chsienki commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

jaredpar Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Feb 27, 2019

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Feb 27, 2019 •

edited

Loading

jaredpar Feb 27, 2019 •

edited

Loading