Add [MemberNotNull] and [MemberNotNullWhen] support #85

kevinchalet · 2020-10-06T22:38:50Z

Fixes #84.

kevinchalet · 2020-10-06T22:39:24Z

TunnelVisionLabs.ReferenceAssemblyAnnotator/NullableAttributes.vb

@@ -94,7 +94,7 @@ Namespace Global.System.Diagnostics.CodeAnalysis

    ''' <summary>Specifies that the method will not return if the associated Boolean parameter is passed the specified value.</summary>
    <AttributeUsage(AttributeTargets.Parameter, Inherited:=False)>
-    Friend NotInheritable Class DoesNotReturnAttribute
+    Friend NotInheritable Class DoesNotReturnIfAttribute


I believe it's an actual bug, as DoesNotReturnAttribute already appears earlier in the file.

Nice find. We have a test hole. @sharwell

kevinchalet · 2020-10-06T22:39:55Z

TunnelVisionLabs.ReferenceAssemblyAnnotator/NullableAttributes.vb

@@ -110,4 +110,65 @@ Namespace Global.System.Diagnostics.CodeAnalysis
        Public ReadOnly Property ParameterValue As Boolean
    End Class

+    ''' <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary>
+    <AttributeUsage(AttributeTargets.Method Or AttributeTargets.Property, Inherited:=False, AllowMultiple:=True)>
+    Friend NotInheritable Class MemberNotNullAttribute


I'm by no means a VB.NET expert, so please double check (and forgive any rookie mistake 😅)

kevinchalet · 2020-10-06T22:40:32Z

...elVisionLabs.ReferenceAssemblyAnnotator/WellKnownTypes+MemberNotNullAttributeProvidedType.cs

+
+            protected override void ImplementAttribute(ModuleDefinition module, TypeDefinition attribute, WellKnownTypes wellKnownTypes, CustomAttributeFactory attributeFactory)
+            {
+                var constructor1 = MethodFactory.Constructor(wellKnownTypes.TypeSystem);


It's the first time I play with Cecil, so you'll likely want to triple-check I'm not doing anything stupid 😄

kevinchalet · 2020-10-06T22:50:56Z

@jnm2 @sharwell PTAL 😄

jnm2

Thanks so much for this, we really appreciate it!

@sharwell I'm less familiar with the ProvidedAttributeType part. Does it need custom attributes to be added on the string array like NotNullIfNotNullAttributeProvidedType does, and could we follow up after this PR with a test that would fail if this is needed and missing?

ReferenceAssemblyAnnotator/TunnelVisionLabs.ReferenceAssemblyAnnotator/WellKnownTypes+NotNullIfNotNullAttributeProvidedType.cs

Lines 18 to 27 in 05b952d

    
           protected override void ImplementAttribute(ModuleDefinition module, TypeDefinition attribute, WellKnownTypes wellKnownTypes, CustomAttributeFactory attributeFactory) 
        
           { 
        
               attribute.CustomAttributes.Add(attributeFactory.NullableContext(1)); 
        
               attribute.CustomAttributes.Add(attributeFactory.Nullable(0)); 
        
               attribute.CustomAttributes.Add(attributeFactory.AttributeUsage(AttributeTargets.Property | AttributeTargets.Parameter | AttributeTargets.ReturnValue, allowMultiple: true, inherited: false)); 
        
               var constructor = MethodFactory.Constructor(wellKnownTypes.TypeSystem); 
        
               constructor.Parameters.Add(new ParameterDefinition("parameterName", ParameterAttributes.None, wellKnownTypes.TypeSystem.String)); 
        
               attribute.Methods.Add(constructor); 
        
           }

jnm2 · 2020-10-06T23:21:12Z

TunnelVisionLabs.ReferenceAssemblyAnnotator/NullableAttributes.cs

+
+    /// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary>
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+    internal class MemberNotNullAttribute : Attribute


I think these should be declared as sealed. Could you update the URL at the top to https://github.com/dotnet/runtime/blob/v5.0.0-rc.1.20451.14/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs and then compare against it?

Nice catch! Looks like I accidentally removed it when copying the definition 😅

Fixed.

…es.cs URL

kevinchalet · 2020-10-06T23:58:09Z

Wooops, looks like I broke something. I'll take a look 😅

kevinchalet · 2020-10-07T00:39:11Z

I tried to add [ParamArray] to the members parameters to see if it could help, but it didn't.

I'm out of ideas. I'll wait for @sharwell to magically find the root cause in 3 seconds 🤣

jnm2 · 2020-10-07T00:45:56Z

Hmm, I can confirm that this repros locally for me on member_not_null but not on master:

msbuild /restore /verbosity:minimal
git clean -xdf tests
dotnet msbuild -restore tests/MultiTFM -warnaserror -nr:false -v:m

…ibute/MemberNotNullWhenAttribute's definition

kevinchalet · 2020-10-07T00:50:23Z

What's super weird is that the error ("Mono.Cecil.ResolutionException: Failed to resolve System.AttributeTargets") is about a type reference I haven't changed in this PR 😕

I added the nullable/nullable context attributes as you suggested. Dunno if it will fix it tho 😄

jnm2 · 2020-10-07T00:57:36Z

Waiting for @sharwell seems like a good plan. The only idea I have is to do a binary search of deleting half your changes repeatedly until we find the line that seems to perturb the behavior into failing. Bit slow of a process.

kevinchalet · 2020-10-07T00:59:56Z

@jnm2 I'll wait for @sharwell's feedback.

Anyway, thanks a lot for your assistance and your review. Much appreciated 😃

sharwell · 2020-10-07T15:24:41Z

TunnelVisionLabs.ReferenceAssemblyAnnotator/NullableAttributes.cs

+
+    /// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary>
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+    internal sealed class MemberNotNullAttribute : Attribute


📝 These need to be conditionally included for .NET Standard 2.1 and .NET Core 3.x builds when GenerateNullableAttributes is set. See here for an example:

https://github.com/dotnet/roslyn/blob/bc85a40ac042f9cf799f5e43deb0fee2d595247b/src/Compilers/Core/Portable/InternalUtilities/NullableAttributes.cs#L90-L94

This will require conditional directives in the source file, and modifications to the handling of GenerateNullableAttributes in the build targets.

Hum yeah, great point. I'll give it a try when I have a moment.

Updated. Let me know if I got it right.

kevinchalet · 2020-10-14T15:46:29Z

Alright, I'm now out of ideas and this is way a more painful process than I had anticipated.

@jnm2 @sharwell this is now failing in the WPF tests. If you guys have an idea, let me know. Otherwise, I'll close this PR as I don't think I'll be able to fix that.

kevinchalet · 2020-10-14T16:09:21Z

Latest commits improved things, but we're back to the Failed to resolve System.AttributeTargets error that makes no sense to me...

sharwell · 2020-10-15T16:31:29Z

@kevinchalet Been busy but hoping to get to this 😄

kevinchalet · 2020-10-15T16:33:18Z

Haha, no worries. I closed this PR as I wasn't able to fix it on my own, but I'd be happy to revive it if we can find the root cause of the issue 😄

kevinchalet commented Oct 6, 2020

View reviewed changes

jnm2 previously approved these changes Oct 6, 2020

View reviewed changes

kevinchalet added 2 commits October 7, 2020 01:41

Add [MemberNotNull] and [MemberNotNullWhen] support

7998d44

Seal the MemberNotNullAttribute class and update the NullableAttribut…

2bd2f90

…es.cs URL

kevinchalet dismissed jnm2’s stale review via 2bd2f90 October 6, 2020 23:42

kevinchalet force-pushed the member_not_null branch from 3a5c6b7 to 2bd2f90 Compare October 6, 2020 23:42

Decorate params array attributes with [ParamArray]

88aa51b

Add [Nullable(0)] and [NullableContext(1)] to match MemberNotNullAttr…

f3dddde

…ibute/MemberNotNullWhenAttribute's definition

sharwell reviewed Oct 7, 2020

View reviewed changes

kevinchalet added 4 commits October 14, 2020 17:10

Update the .targets file to include the nullable attributes on more TFMs

c7338d9

Remove extra parenthesis

d1c6a8f

Try a different approach

a7ce003

Remove extra quote

9b3aedf

kevinchalet added 2 commits October 14, 2020 17:55

Try again

5623152

Remove extra parenthesis

8e967b9

kevinchalet closed this Oct 15, 2020

jnm2 mentioned this pull request Nov 28, 2020

MemberNotNull and MemberNotNullWhen #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add [MemberNotNull] and [MemberNotNullWhen] support #85

Add [MemberNotNull] and [MemberNotNullWhen] support #85

kevinchalet commented Oct 6, 2020

kevinchalet Oct 6, 2020 •

edited

Loading

jnm2 Oct 6, 2020

kevinchalet Oct 6, 2020

kevinchalet Oct 6, 2020

kevinchalet commented Oct 6, 2020

jnm2 left a comment

jnm2 Oct 6, 2020

kevinchalet Oct 6, 2020

kevinchalet commented Oct 6, 2020

kevinchalet commented Oct 7, 2020

jnm2 commented Oct 7, 2020

kevinchalet commented Oct 7, 2020 •

edited

Loading

jnm2 commented Oct 7, 2020

kevinchalet commented Oct 7, 2020

sharwell Oct 7, 2020

kevinchalet Oct 8, 2020

kevinchalet Oct 14, 2020

kevinchalet commented Oct 14, 2020

kevinchalet commented Oct 14, 2020

sharwell commented Oct 15, 2020 •

edited

Loading

kevinchalet commented Oct 15, 2020

	protected override void ImplementAttribute(ModuleDefinition module, TypeDefinition attribute, WellKnownTypes wellKnownTypes, CustomAttributeFactory attributeFactory)
	{
	attribute.CustomAttributes.Add(attributeFactory.NullableContext(1));
	attribute.CustomAttributes.Add(attributeFactory.Nullable(0));
	attribute.CustomAttributes.Add(attributeFactory.AttributeUsage(AttributeTargets.Property \| AttributeTargets.Parameter \| AttributeTargets.ReturnValue, allowMultiple: true, inherited: false));

	var constructor = MethodFactory.Constructor(wellKnownTypes.TypeSystem);
	constructor.Parameters.Add(new ParameterDefinition("parameterName", ParameterAttributes.None, wellKnownTypes.TypeSystem.String));
	attribute.Methods.Add(constructor);
	}

Add [MemberNotNull] and [MemberNotNullWhen] support #85

Add [MemberNotNull] and [MemberNotNullWhen] support #85

Conversation

kevinchalet commented Oct 6, 2020

kevinchalet Oct 6, 2020 • edited Loading

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

kevinchalet commented Oct 6, 2020

jnm2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Oct 6, 2020

kevinchalet commented Oct 7, 2020

jnm2 commented Oct 7, 2020

kevinchalet commented Oct 7, 2020 • edited Loading

jnm2 commented Oct 7, 2020

kevinchalet commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Oct 14, 2020

kevinchalet commented Oct 14, 2020

sharwell commented Oct 15, 2020 • edited Loading

kevinchalet commented Oct 15, 2020

kevinchalet Oct 6, 2020 •

edited

Loading

kevinchalet commented Oct 7, 2020 •

edited

Loading

sharwell commented Oct 15, 2020 •

edited

Loading