Inject NonNullTypesAttribute by jcouv · Pull Request #29180 · dotnet/roslyn

jcouv · 2018-08-09T04:09:58Z

If no NonNullTypesAttribute type is available in source, we make one up.

cston · 2018-08-09T04:17:45Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

Just curious: Why pass a delegate to a local function rather than inlining the body of the local function as a lambda? #Resolved

I can't see what code this comment was for, and I couldn't find injectNamespaceOpt: injectNamespace);.
Is this comment still alive?

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

cston · 2018-08-09T04:31:38Z

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

private … Make...

jcouv · 2018-08-09T06:11:16Z

This is meant for design discussion, not for detailed review yet ;-) Thanks #Closed

jcouv · 2018-08-27T21:50:52Z

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

ReportUseSiteDiagnostics [](start = 19, length = 24)

ReportUseSiteDiagnostics [](start = 19, length = 24)

📝 I wasn't able to find scenarios for use-site diagnostics on AttributeTargets or the constructor of AttributeUsage, but I'm including those checks anyways. #Closed

cston · 2018-09-07T16:28:33Z

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

+        var obj2 = new TestReference.TestType2();
+        var obj3 = new TestReference.TestType3();
+    }
+}";


Remove if not needed. #Resolved

AlekseyTs · 2018-09-07T17:22:42Z

Consider splitting referenced assembly and added module cases. Will simply adding a module declaring the type cause an error, or it is reported only if the attribute is used? #Closed

Refers to: docs/compilers/CSharp/Compiler Breaking Changes - VS2019.md:11 in f4a2147. [](commit_id = f4a2147, deletion_comment = False)

AlekseyTs · 2018-09-07T17:25:03Z

Also, I think the term "referenced" is somewhat confusing in this context. Would it be clearer to say something like: "we allowed to refer to a System.Runtime.CompilerServices.NonNullTypesAttribute type declared in a referenced assembly or an added module." BTW, I think one can still refer to a System.Runtime.CompilerServices.NonNullTypesAttribute type declared in a referenced assembly by using an extern alias. Adding a test for this scenario would be good.

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

Refers to: docs/compilers/CSharp/Compiler Breaking Changes - VS2019.md:11 in bba7cd2. [](commit_id = bba7cd293a3deacbf276165a0f93fae80ff9426a, deletion_comment = False)

AlekseyTs · 2018-09-07T17:36:04Z

docs/compilers/CSharp/Compiler Breaking Changes - VS2019.md

+*Breaks are formatted with a monotonically increasing numbered list to allow them to referenced via shorthand (i.e., "known break #1").
+Each entry should include a short description of the break, followed by either a link to the issue describing the full details of the break or the full details of the break inline.*
+
+1. Previously, we allowed the `Microsoft.CodeAnalysis.EmbeddedAttribute` to be referenced from a source module.


to be referenced from [](start = 73, length = 21)

declared in source? #Closed

No, it can still be declared in source. The scenario that changed is with a module added that declares that type.

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

No, it can still be declared in source. The scenario that changed is with a module added that declares that type.

So this is not about a source module, this is a bout an added module. Also consider the other comment about using "referenced" term.

In reply to: 216041844 [](ancestors = 216041844,216033962)

AlekseyTs · 2018-09-07T17:38:04Z

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

+            => new AttributeUsageInfo(validTargets: AttributeTargets.All, allowMultiple: false, inherited: false);
+
+        internal override DiagnosticBag GetDiagnostics()
+            => _diagnostics;


=> _diagnostics [](start = 12, length = 15)

Consider avoiding exposing "private" DiagnosticBag to external consumers. That can be achieved by adding a DiagnosticBag parameter where the diagnostics should be added. #Closed

AlekseyTs · 2018-09-07T18:31:34Z

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

+
+            class InjectedNamespaceSymbol : NamespaceSymbol
+            {
+                private ModuleSymbol _module;


private ModuleSymbol _module; [](start = 16, length = 29)

I think this field is not necessary. It looks like it is used only for Extent and I think we can safely return extent for _containingNamespace #Closed

AlekseyTs · 2018-09-07T18:33:00Z

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

+            class InjectedNamespaceSymbol : NamespaceSymbol
+            {
+                private ModuleSymbol _module;
+                private NamespaceSymbol _containingNamespace;


private NamespaceSymbol _containingNamespace; [](start = 16, length = 45)

readonly? #Closed

jcouv · 2018-09-07T18:37:40Z

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

+    internal sealed class InjectedNonNullTypesAttributeSymbol : InjectedAttributeSymbol
+    {
+        private ImmutableArray<CSharpAttributeData> _lazyCustomAttributes;
+        private SymbolCompletionState _state;


_state [](start = 38, length = 6)

I will remove the completion state (and left-over logic that relates to it) #Resolved

AlekseyTs · 2018-09-07T18:41:09Z

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

+                public override string Name 
+                    => _name;
+
+                public override AssemblySymbol ContainingAssembly


public override AssemblySymbol ContainingAssembly [](start = 16, length = 49)

I think we can rely on implementation in base class. #Closed

There is no implementation (just the abstract declaration).

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

AlekseyTs · 2018-09-07T18:42:49Z

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

+                        : ImmutableArray<NamedTypeSymbol>.Empty;
+                }
+
+                internal override LexicalSortKey GetLexicalSortKey()


internal override LexicalSortKey GetLexicalSortKey() [](start = 16, length = 52)

I think we can rely on implementation in base class. #Closed

AlekseyTs · 2018-09-07T18:50:38Z

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

+                internal override NamespaceExtent Extent
+                    => new NamespaceExtent(_module);
+
+                internal override ImmutableArray<Symbol> GetMembersUnordered()


internal override ImmutableArray GetMembersUnordered() [](start = 16, length = 62)

I think we can rely on implementation in base class. #Closed

AlekseyTs · 2018-09-07T18:50:54Z

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

+                    return _nameToTypeMembersMap;
+                }
+
+                internal override ImmutableArray<NamedTypeSymbol> GetTypeMembersUnordered()


internal override ImmutableArray GetTypeMembersUnordered() [](start = 16, length = 75)

I think we can rely on implementation in base class. #Closed

AlekseyTs · 2018-09-07T18:58:48Z

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

        NamedTypeSymbolAll = NamedTypeSymbolWithLocationAll | MembersCompleted,

+        // For injected symbols
+        InjectedSymbolAll = Attributes,


InjectedSymbolAll = Attributes, [](start = 8, length = 31)

Is this still used? Consider reverting the file otherwise. #Closed

AlekseyTs · 2018-09-07T19:00:23Z

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

+            _diagnostics = diagnostics;
+        }
+
+        public override ImmutableArray<Location> Locations


public override ImmutableArray Locations [](start = 7, length = 51)

Can we rely on implementation from base? #Closed

AlekseyTs · 2018-09-07T21:15:09Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

-#else
-            return result;
-#endif
+            return GetMembers().ConditionallyDeOrder();


return GetMembers().ConditionallyDeOrder(); [](start = 12, length = 43)

Is base implementation different? #Closed

AlekseyTs · 2018-09-07T21:30:42Z

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

+                return ImmutableArray<CSharpAttributeData>.Empty;
+            }
+
+            Binder.ReportUseSiteDiagnostics(ctor, _diagnostics, Location.None);


Binder.ReportUseSiteDiagnostics(ctor, _diagnostics, Location.None); [](start = 12, length = 67)

Doesn't Binder.GetWellKnownTypeMember take care of this? #Closed

I didn't manage to hit this with a test, but I fixed this anyways.
The few ways I know to generate use-site diagnostics didn't work:

Obsolete is not reported for well-known types

I couldn't create a missing type because we're dealing with corlib

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

AlekseyTs · 2018-09-07T21:37:23Z

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

+        {
+            if (_lazyCustomAttributes.IsDefault)
+            {
+                ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, MakeAttributes());


ImmutableInterlocked.InterlockedInitialize(ref _lazyCustomAttributes, MakeAttributes()); [](start = 16, length = 88)

It looks like there is a chance of duplicating diagnostics when more than one thread is executing this line at the same time. #Closed

AlekseyTs · 2018-09-07T21:40:23Z

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

+            internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
+            {
+                var containingType = (InjectedNonNullTypesAttributeSymbol)ContainingType;
+                GenerateMethodBodyCore(compilationState, containingType._diagnostics);


GenerateMethodBodyCore(compilationState, containingType._diagnostics); [](start = 16, length = 70)

Let's assume this call produces diagnostics. Every time we try to emit, will we add it again? So, each subsequent attempt will have more and more duplicates? #Closed

AlekseyTs · 2018-09-07T22:08:44Z

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

+
+            void addInjectedAttribute(WellKnownType wellKnownType, bool canUseFromSource)
+            {
+                NamedTypeSymbol attribute = Compilation.GetWellKnownType(wellKnownType);


Compilation.GetWellKnownType [](start = 44, length = 28)

Can this return a symbol from an added module? #Closed

Added a test (crash in emit). Thanks

Filed a follow-up issue for this and also duplicate diagnostic issues: #29732

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

AlekseyTs · 2018-09-07T22:13:52Z

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

 }";

            CreateCompilation(code, references: new[] { reference }, assemblyName: "Source").VerifyDiagnostics(
+                // error CS0101: The namespace 'Microsoft.CodeAnalysis' already contains a definition for 'EmbeddedAttribute'


// error CS0101: The namespace 'Microsoft.CodeAnalysis' already contains a definition for 'EmbeddedAttribute' [](start = 16, length = 109)

The breaking change description says:
"

Previously, we allowed to refer to a Microsoft.CodeAnalysis.EmbeddedAttribute type declared in an added module.
In Visual Studio 2019, this produces a collision with the injected declaration of that type.
"

However, it doesn't look like this scenario refers to the type, yet it is still an error. Consider adjusting description of the breaking change. It isn't really about referring, it is about having this type declared in an added module. #Closed

AlekseyTs · 2018-09-07T22:25:14Z

Done with review pass (iteration 19). Let's discuss offline what outstanding issues can be followed up separately. #Closed

AlekseyTs · 2018-09-07T22:45:26Z

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

+
+            // NonNullTypesAttribute from referenced assembly is used via extern alias
+            var comp = CreateCompilation(code, references: new[] { reference.ToMetadataReference(aliases: ImmutableArray.Create("Reference")) });
+            comp.VerifyDiagnostics();


VerifyDiagnostics [](start = 17, length = 17)

Let's also check that .NonNullTypes is true on the SourceModule #Closed

AlekseyTs

LGTM (iteration 20)

jcouv · 2018-09-10T19:52:24Z

Awesome. Thanks a lot for the review!

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Aug 9, 2018

jcouv added this to the 16.0 milestone Aug 9, 2018

jcouv self-assigned this Aug 9, 2018

jcouv requested a review from a team as a code owner August 9, 2018 04:09

cston reviewed Aug 9, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedNamespaceSymbol.cs Outdated

Copy link

Contributor

cston Aug 9, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private … Make...

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 10, 2018

jcouv force-pushed the inject-nnt branch 9 times, most recently from 98eb2c4 to 07d7944 Compare August 27, 2018 21:48

jcouv commented Aug 27, 2018

View reviewed changes

jcouv force-pushed the inject-nnt branch 4 times, most recently from e4455c1 to 39614a0 Compare August 28, 2018 06:24

jcouv requested a review from a team as a code owner August 28, 2018 06:24

jcouv force-pushed the inject-nnt branch 5 times, most recently from b18dba6 to 6407e6d Compare August 29, 2018 17:16

jcouv requested a review from a team as a code owner August 29, 2018 21:44

cston reviewed Sep 7, 2018

View reviewed changes

AlekseyTs reviewed Sep 7, 2018

View reviewed changes

jcouv commented Sep 7, 2018

View reviewed changes

AlekseyTs reviewed Sep 7, 2018

View reviewed changes

Address more PR feedback

2c7cbc2

AlekseyTs reviewed Sep 7, 2018

View reviewed changes

jcouv mentioned this pull request Sep 8, 2018

Injected NonNullTypesAttribute: Duplicate diagnostics and tightening a module scenario #29732

Closed

Tweak compat break doc. Improve tests.

93b0ac7

jcouv force-pushed the inject-nnt branch from b04900e to 93b0ac7 Compare September 8, 2018 02:14

AlekseyTs approved these changes Sep 10, 2018

View reviewed changes

jcouv merged commit 3e64c13 into dotnet:features/NullableReferenceTypes Sep 10, 2018

jcouv deleted the inject-nnt branch September 10, 2018 19:52

Conversation

jcouv commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Aug 9, 2018 • 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!

jcouv Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

cston Aug 9, 2018 • 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 Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs commented Sep 7, 2018

Uh oh!

AlekseyTs Sep 7, 2018 • 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 Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018 • 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 Sep 7, 2018 • 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 Aug 9, 2018 •

edited

Loading

cston Aug 9, 2018 •

edited by jcouv

Loading

cston Aug 9, 2018 •

edited

Loading

jcouv commented Aug 9, 2018 •

edited

Loading

jcouv Aug 27, 2018 •

edited

Loading

cston Sep 7, 2018 •

edited by jcouv

Loading

AlekseyTs commented Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

jcouv Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading

AlekseyTs commented Sep 7, 2018 •

edited

Loading

AlekseyTs Sep 7, 2018 •

edited

Loading