Enforce conditional attributes within method bodies by jcouv · Pull Request #41098 · dotnet/roslyn

jcouv · 2020-01-20T23:57:54Z

Fixes the second part of #36039 (enforcing nullability attributes in method bodies) by handling conditional attributes.

My previous PR (#40789) handled unconditional attributes and implemented a simple behavior for conditional attributes (ie. treat them unconditionally and leniently).

Note: the DoesNotReturnIf attribute won't have any enforcement. I don't think we can do that type of analysis.

jcouv · 2020-01-21T15:44:18Z

Tagging @cston @RikkiGibson since reviewed previous PR.
FYI @sharwell #Resolved

sharwell · 2020-01-21T17:02:59Z

        }

+        [Fact, WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")]
+        public void EnforcedInMethodBody_DoesNotReturn()


📝 Need tests for DoesNotReturn calling another DoesNotReturn, e.g.:

[DoesNotReturn] static void Fail() => Fail("message"); // should not warn [DoesNotReturn] static void Fail(string message) => throw null!; ``` #Resolved

➡️ added test (composes as expected) #Resolved

sharwell · 2020-01-21T17:03:57Z

 #endif

            Environment.FailFast(exception.ToString(), exception);
+            throw null!; // to satisfy [DoesNotReturn]


Suggested change

throw null!; // to satisfy [DoesNotReturn]

throw ExceptionUtilities.Unreachable;

This might be a good place for #pragma warning disable. In the future when we adopt the newer TFM we should get the [DoesNotReturn] attribute on Environment.FailFast and we can delete the pragma. @JoeRobich don't we have a mechanism to periodically search our sources for disabled warnings and turn them back on?

See https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/coreclr/src/System.Private.CoreLib/src/System/Environment.CoreCLR.cs#L52

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

@JoeRobich don't we have a mechanism to periodically search our sources for disabled warnings and turn them back on?

@RikkiGibson If we did, I would like to know about it. Find in Files maybe? #Resolved

sharwell · 2020-01-21T17:10:16Z

+    }
+}
+";
+            // Note: this is a situation where it is annoying that we're learning from pure null tests


I'm not sure I agree with this note. Each of the warnings is code that is at best misleading, and a likely source of annotation errors that could cascade once the attribute checks are fully implemented. #Closed

Chatted offline with @jcouv and it's feeling like these warnings are good.

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

➡️ removed comment #Resolved

RikkiGibson · 2020-01-22T01:10:34Z

                value.WasCompilerGenerated ||
-                !targetType.HasType ||
-                targetType.Type.IsValueType)
+                !ShouldReportNullableAssignment(targetType, valueType.State))


Are there any behavioral changes from this change? #Closed

No, this is just extracting the logic so that it can be called from the new checks. #Closed

RikkiGibson · 2020-01-22T01:12:21Z

-                if (node.RefKind == RefKind.None)
+                if (node.RefKind == RefKind.None &&
+                    returnType.Type.SpecialType == SpecialType.System_Boolean &&
+                    isUnconvertedBooleanExpression(expr))


Why do we need to look at the returned expression before conversions are applied? #Closed

If there is a conversion from some custom type to bool, then we can't do the new analysis. We don't know what logic that conversion might contain. We lose track of whenTrue/False branches in conversions, since it's custom logic.

Also imagine if that conversion produced nullability warnings, we'd want to run it through VisitImplicitConversion. #Closed

RikkiGibson · 2020-01-22T01:12:28Z

        }

+        /// <summary>
+        /// Can we assign this state into this type without a warning?


It feels like this doc comment is asking the opposite question of what the method is asking (should I assign without a warning, versus should I report) #Closed

➡️ fixed comment #Closed

RikkiGibson · 2020-01-22T01:20:16Z

                {
                    builder.Append(name);
-                    builder.Append(state[i] == NullableFlowState.MaybeNull ? "?" : "!");
+                    var annotation = (state[i]) switch


are the parens necessary? #Closed

➡️ removed #Closed

RikkiGibson · 2020-01-22T01:23:22Z

+                        if (!IsConstantFalse(expr))
+                        {
+                            // don't check WhenTrue state on a 'return false;'
+                            checkConditionalParameterState(node.Syntax, parameters, sense: true);


sense [](start = 84, length = 5)

It would be good to have a more descriptive name for this parameter. Maybe possibleReturnValue? (not in love with that name either, though..) #Closed

I don't love it either :-$
The reason I used "sense" is because I've seen it used in other places for such situations (applying some logic either to the true or the false branch). #Closed

RikkiGibson · 2020-01-22T01:25:15Z

                        && attr.CommonConstructorArguments.Length == 1
                        && attr.CommonConstructorArguments[0].Kind == TypedConstantKind.Type)
                    {
-                        builderArgument = attr.CommonConstructorArguments[0].ValueInternal;


Was this incidental to the change, or a result of the NullableWalker change? #Closed

Without this, we can't bootstrap. The compiler would complain that buildArgument is null upon exit.
But we know it's not null in this case, because we checked the .Kind above. #Closed

RikkiGibson · 2020-01-22T01:27:06Z

            var comp = CreateNullableCompilation(new[] { source, MaybeNullWhenAttributeDefinition });
-            comp.VerifyDiagnostics();
+            comp.VerifyDiagnostics(
+                // (11,13): error CS8762: Parameter 's' may not have a null value when exiting with 'false'.


error [](start = 28, length = 5)

I expected this to be a warning. Did I misinterpret something? #Closed

Not sure how I messed up the comment. I"ll fix #Closed

RikkiGibson · 2020-01-22T01:29:10Z

+using System.Diagnostics.CodeAnalysis;
+
+class C<T, TClass, TNotNull>
+    where TClass : class


TClass [](start = 10, length = 6)

Consider deleting the TClass from this test. #Closed

➡️ deleted extraneous type parameter #Closed

RikkiGibson · 2020-01-22T01:39:10Z

+    {
+        y = x;
+        z = x;
+        return y != null || z != null;


What does the "happy case" for this look like? Perhaps y == null || z == null? Is that test present here? #Closed

➡️ added test #Closed

RikkiGibson · 2020-01-22T02:01:18Z

+                );
+        }
+
+        [Theory]


Some more cases I'd like to see coverage for:

Suppression (am I expected to put ! on the value being assigned? or on the expression being returned?)

Composition

Should be able to use a decorator pattern on methods with equivalent sets of MaybeNullWhen/NotNullWhen attributes

Should be able to compose methods with "equivalent" combinations of attributes e.g. a [NotNullWhen(true)] out string? s1 composes with a [MaybeNullWhen(false)] out string s2 #Closed

In terms of suppression, you can't suppress this new warning directly (except maybe with #nullable disable warnings or explicit diagnostic suppression). You can also put non-null values into the parameters in question, with ! suppressions if needed.

I'll add tests for composition. Could you clarify the decorator scenario you had in mind? #Closed

➡️ added tests for composition #Resolved

I was thinking of:

using System.Collections.Generic; class DictionaryDecorator { readonly Dictionary<string, string> _dict; DictionaryDecorator(Dictionary<string, string> dict) { _dict = dict; } public bool TryGetValue(string key, [MaybeNullWhen(false)] out string value) { return _dict.TryGetValue(key, out value); } }

which is not all that different from simple method composition, I'll grant :)

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

RikkiGibson

Done with review pass (iteration 1)

cston · 2020-01-22T04:33:52Z

+    <value>Parameter may not have a null value when exiting in some condition.</value>
+  </data>
+  <data name="WRN_MethodShouldThrow" xml:space="preserve">
+    <value>A method marked with [DoesNotReturn] should throw on all code paths.</value>


should throw on all code paths [](start = 48, length = 30)

Is the following a valid use of [DoesNotReturn]?

[DoesNotReturn] static int F() { while (true) { } } ``` #Closed

➡️ added test (no warning on above method) #Closed

Sorry, I wasn't clear. I meant a method marked [DoesNotReturn] does not need to throw on all code paths. Perhaps the message should be "A method marked [DoesNotReturn] should not return."

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

cston · 2020-01-22T04:34:44Z

        ERR_ExternEventInitializer = 8760,
        ERR_AmbigBinaryOpsOnUnconstrainedDefault = 8761,
+        WRN_ParameterConditionallyDisallowsNull = 8762,
+        WRN_MethodShouldThrow = 8763,


WRN_MethodShouldThrow [](start = 8, length = 21)

Perhaps WRN_ShouldNotReturn. #Closed

➡️ done #Resolved

cston · 2020-01-22T04:42:15Z

            }

            ImmutableArray<PendingBranch> pendingReturns = base.Scan(ref badRegion);
+            EnforceDoesNotReturn(methodMainNode.Syntax.GetLastToken().GetLocation());


methodMainNode.Syntax.GetLastToken().GetLocation() [](start = 33, length = 50)

Consider passing in the method syntax instead rather than calling GetLocation() eagerly. #Closed

I don't think I could do that, because the check on end of method uses the last token of the syntax, instead of a SyntaxNode #Resolved

I don't follow. Couldn't the method be defined as:

void EnforceDoesNotReturn(SyntaxNode syntax) { ... ReportDiagnostic(ErrorCode.WRN_ShouldNotReturn, syntax.GetLastToken().GetLocation()); } --- In reply to: [369381283](https://github.com/dotnet/roslyn/pull/41098#discussion_r369381283) [](ancestors = 369381283)

cston · 2020-01-22T04:42:32Z

+        /// <summary>
+        /// Can we assign this state into this type without a warning?
+        /// </summary>
+        private bool ShouldReportNullableAssignment(TypeWithAnnotations type, NullableFlowState state)


private [](start = 8, length = 7)

static #Closed

➡️ done #Resolved

cston · 2020-01-22T04:47:26Z

+                if (parameters.IsEmpty)
+                {
+                    return;
+                }


The caller could check this. #Closed

➡️ makes sense. Done #Resolved

cston · 2020-01-22T04:53:34Z

+                    var annotation = (state[i]) switch
+                    {
+                        NullableFlowState.MaybeNull => "?",
+                        NullableFlowState.MaybeDefault => "??",


"??" [](start = 58, length = 4)

Will this be confused for two consecutive slots each MaybeNull? #Closed

It wasn't confusing because we print variable names and annotations. So it shows x?y??z! for example #Resolved

RikkiGibson · 2020-01-22T19:18:30Z

+    {
+        y = x;
+        z = x;
+        return y != null && z != null;


It would be handy to have // 1, // 2 comments in this test #Closed

RikkiGibson · 2020-01-22T19:20:51Z

+
+    static bool TryGetValue2([NotNullWhen(true)] out TYPE y)
+    {
+        return TryGetValue3(out y); // 1


Do we get an analogous warning behavior when a method equivalent to TryGetValue3 calls TryGetValue2? #Closed

RikkiGibson · 2020-01-22T19:23:06Z

+    static bool NeverReturns3()
+        => throw null!;
+
+    [DoesNotReturn]


Is DoesNotReturn supported on property/event accessors? Do we have analogous tests for those scenarios? #Closed

That is not supported. The attribute has [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)]
`

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

RikkiGibson

Done with review pass (iteration 2)

jcouv · 2020-01-22T21:39:33Z

@RikkiGibson @cston Please take another look when you get the chance. Thanks #Resolved

RikkiGibson · 2020-01-22T23:00:22Z

+        [InlineData("TStruct?")]
+        [InlineData("TClass?")]
+        [WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")]
+        public void EnforcedInMethodBody_NotNullWhen_Composition(string type)


I can't find tests which demonstrate the expected behavior around composition like the following:

bool TryGetValue1(string key, [MaybeNullWhen(false)] out string value) => TryGetValue2(key, out value); bool TryGetValue2(string key, [NotNullWhen(true)] out string? value) => TryGetValue1(key, out value); ``` #Closed

Added. Thanks

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

RikkiGibson

RikkiGibson · 2020-01-23T00:40:32Z

This is a really cool bit of functionality. Thanks for doing this work. #Resolved

jcouv · 2020-01-23T03:28:11Z

@RikkiGibson Haha, thanks. This PR reminds me of an earlier one that made ‘Debug.Assert(!string.IsNullOrEmpty(x))’ work. It’s nice when things fall nicely together ;) #Resolved

cston · 2020-01-23T18:08:02Z

+            bool isUnconvertedBooleanExpression(BoundExpression expr)
+            {
+                var (expression, _) = RemoveConversion(expr, includeExplicitConversions: true);
+                return expression.Type?.SpecialType == SpecialType.System_Boolean;


Can this be simplified to expr.Kind != BoundKind.Conversion? At that point, it can be inlined. #Closed

I worried that we might generate identity conversions.

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

Thanks. This check was unnecessary after all. Removed.

In reply to: 370294344 [](ancestors = 370294344,370273577)

cston · 2020-01-23T18:08:52Z

+                    {
+                        // Parameter '{name}' may not have a null value when exiting with '{sense}'.
+                        ReportDiagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, syntax.Location, parameter.Name, sense ? "true" : "false");
+                        continue;


continue; [](start = 24, length = 9)

Unnecessary. #Closed

cston · 2020-01-23T18:20:18Z

        return true;

Are we testing s = null; return true;? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:20739 in 59b3c0e. [](commit_id = 59b3c0e, deletion_comment = False)

cston · 2020-01-23T18:31:21Z

-                if (node.RefKind == RefKind.None)
+                if (node.RefKind == RefKind.None &&
+                    returnType.Type.SpecialType == SpecialType.System_Boolean &&
+                    isUnconvertedBooleanExpression(expr))


isUnconvertedBooleanExpression(expr) [](start = 20, length = 36)

Are we testing this case? (If this check is removed, would tests fail?) #Pending

cston · 2020-01-23T19:37:15Z

+                //             return false; // 1
+                Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return false;").WithArguments("s", "false").WithLocation(11, 13)
+                );
+        }


} [](start = 8, length = 1)

Please test:

static bool TryGetValue([MaybeNullWhen(false)]out string s) { s = null; return (bool)true; }

It doesn't necessarily need to warn but we should test the behavior.

Added the test
The cast ruins the conditional analysis (ie. no warning on return (bool)true; nor return (bool)false;.

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jan 20, 2020

jcouv added this to the 16.5.P3 milestone Jan 20, 2020

jcouv self-assigned this Jan 20, 2020

jcouv mentioned this pull request Jan 20, 2020

Should nullability attributes affect method bodies and OHI? #36039

Closed

3 tasks

jcouv force-pushed the cond-attributes branch 2 times, most recently from 2d83f8e to db14972 Compare January 21, 2020 05:25

Enforce conditional attributes within method bodies

7c15d31

jcouv force-pushed the cond-attributes branch from db14972 to 7c15d31 Compare January 21, 2020 06:20

jcouv marked this pull request as ready for review January 21, 2020 06:21

jcouv requested review from a team as code owners January 21, 2020 06:21

sharwell reviewed Jan 21, 2020

View reviewed changes

RikkiGibson self-requested a review January 21, 2020 18:40

RikkiGibson reviewed Jan 22, 2020

View reviewed changes

cston reviewed Jan 22, 2020

View reviewed changes

Address feedback

8c2f08e

RikkiGibson reviewed Jan 22, 2020

View reviewed changes

Address more feedback

3464d72

RikkiGibson reviewed Jan 22, 2020

View reviewed changes

One more test

59b3c0e

RikkiGibson approved these changes Jan 23, 2020

View reviewed changes

jcouv requested a review from cston January 23, 2020 16:54

cston reviewed Jan 23, 2020

View reviewed changes

More tests and remove unnecessary check

d56aa53

cston reviewed Jan 23, 2020

View reviewed changes

cston approved these changes Jan 23, 2020

View reviewed changes

Test one more scenario

9fcc6ec

jcouv merged commit 340e81a into dotnet:master Jan 24, 2020

jcouv deleted the cond-attributes branch January 24, 2020 02:59

jcouv mentioned this pull request Jan 29, 2020

CS8625 when assigning null to MaybeNullWhen(false) out parameter then returning false #36917

Closed

jcouv mentioned this pull request Mar 13, 2026

False negative on nullable analysis #82550

Open

	throw null!; // to satisfy [DoesNotReturn]
	throw ExceptionUtilities.Unreachable;

Conversation

jcouv commented Jan 20, 2020

Uh oh!

jcouv commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell Jan 21, 2020 • 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 Jan 22, 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!

sharwell Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

JoeRobich Jan 22, 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!

sharwell Jan 21, 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!

RikkiGibson Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 22, 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 22, 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!

jcouv Jan 22, 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!

RikkiGibson Jan 22, 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!

jcouv Jan 22, 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!

RikkiGibson Jan 22, 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!

jcouv Jan 22, 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!

RikkiGibson Jan 22, 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!

jcouv Jan 22, 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!

RikkiGibson Jan 22, 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!

jcouv Jan 22, 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!

RikkiGibson Jan 22, 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!

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

jcouv commented Jan 21, 2020 •

edited

Loading

sharwell Jan 21, 2020 •

edited by jcouv

Loading

jcouv Jan 22, 2020 •

edited

Loading

JoeRobich Jan 22, 2020 •

edited by RikkiGibson

Loading

sharwell Jan 21, 2020 •

edited by RikkiGibson

Loading

jcouv Jan 22, 2020 •

edited

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

RikkiGibson Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by RikkiGibson

Loading

jcouv Jan 22, 2020 •

edited

Loading

cston Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by cston

Loading

cston Jan 22, 2020 •

edited

Loading

jcouv Jan 22, 2020 •

edited by cston

Loading