Respect [MemberNotNull] on local functions #75448

jcouv · 2024-10-09T00:28:20Z

The MemberNotNull attribute takes effect if either:

the context is non-static (ie. the containing method and the nested local functions are not static). The target member can be static or not
the context is static (ie. either the containing method or the local function is static) and the target member is static

When the MemberNotNull takes effect, three things happen:

after the local function is called, the state of the target member is changed to not-null
when entering the local function, the state of the target member is changed to maybe-null
when existing the local function, we warn if the target member isn't not-null

Note: I didn't gate this change behind LangVer as it seems small and only affects nullability warnings, but I'm open to it.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

This reverts commit 3237e0e.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

jcouv · 2024-10-15T18:41:03Z

@dotnet/roslyn-compiler for second review. Thanks

cston · 2024-10-16T22:10:32Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        field4.ToString(); // 1
+
+        [MemberNotNull(nameof(field1), nameof(field2))]
+        bool init() => throw null!;


bool init() => throw null!;

Consider testing within nested local function.

void init() { init2(); [MemberNotNull(nameof(field1), nameof(field2))] void init2() { } } ``` #Closed

cston · 2024-10-16T22:12:34Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        field4.ToString(); // 1
+
+        [MemberNotNull(nameof(field1), nameof(field2))]
+        bool init() => throw null!;


bool init() => throw null!;

Consider testing the equivalent case with a static local function, with the same instance members.

[MemberNotNull(nameof(field1), nameof(field2))] static bool init() => throw null!; ``` #Closed

cston · 2024-10-16T22:17:49Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+""", MemberNotNullAttributeDefinition]);
+
+            c.VerifyDiagnostics(
+                // (6,23): error CS0103: The name 'field1' does not exist in the current context


'field1'

Consider testing with a defined field.

... partial class Program { object? field1; } ``` #Closed

jcouv · 2024-10-25T00:03:52Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

-                        }
-                    }
-                }
-                else if (returnStatement.ExpressionOpt is { ConstantValueOpt: { IsBoolean: true, BooleanValue: bool value } })


📝 this if block was removed (it was not reachable)

jcouv · 2024-10-25T00:23:31Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -791,113 +791,6 @@ void checkMemberStateOnConstructorExit(MethodSymbol constructor, Symbol member,
                }
            }

-            void enforceMemberNotNullOnMember(SyntaxNode? syntaxOpt, LocalState state, MethodSymbol method, string memberName)


📝 A number of local functions were switched to methods below

cston · 2024-10-25T19:01:49Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs


-                return GetOrCreateSlot(member, containingSlot);
+            if (member.IsStatic != (containingSlot == 0))


member.IsStatic != (containingSlot == 0)

Do we get here when member is static but the containing method is an instance method? #Resolved

Yes
This is illustrated by MemberNotNull_StaticMemberOnInstanceMethod
The opposite scenario is illustrated by MemberNotNull_InstanceMemberOnStaticMethod

Update: there's something fishy with that scenario. Investigating...

Making a fix to align the logic in ApplyMemberPostConditions and GetSlotForMemberPostCondition regarding this scenario

cston · 2024-10-25T19:50:43Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                }
+
+                var container = current.ContainingSymbol;
+                if (container.Kind == SymbolKind.NamedType)


if (container.Kind == SymbolKind.NamedType)

Perhaps check for SymbolKind.Method instead:

if (container.Kind != SymbolKind.Method) { ... } current = (MethodSymbol)container;

Then, the containing loop could be the following, and the final return -1; removed.

MethodSymbol current = method; while (true) ``` #Closed

cston · 2024-10-25T20:27:48Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+    {
+        init();
+        field1.ToString();
+        field2.ToString(); // 1


field2.ToString(); // 1

Is this warning expected? (It looks like we don't report a warning for the corresponding static method case - see sharplab.io.) #Closed

cston · 2024-10-25T20:32:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+    {
+        init();
+        field1.ToString();
+        field2.ToString(); // 1


field2.ToString(); // 1

Reporting a warning seems incorrect here. #Closed

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

cston · 2024-10-25T20:42:52Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                {
+                    MakeMembersMaybeNull(lambdaOrFunctionSymbol, lambdaOrFunctionSymbol.NotNullMembers);
+                    MakeMembersMaybeNull(lambdaOrFunctionSymbol, lambdaOrFunctionSymbol.NotNullWhenTrueMembers);
+                    MakeMembersMaybeNull(lambdaOrFunctionSymbol, lambdaOrFunctionSymbol.NotNullWhenFalseMembers);


Why set these members to maybe null rather than relying on the state from the calling method? #Resolved

Addressed in another comment

cston · 2024-10-25T20:44:12Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        [MemberNotNull(nameof(field1), nameof(field2))]
+        void init()
+        {
+            field1.ToString(); // 1


field1.ToString(); // 1

Are we reporting a warning for field1 because we're setting the members to maybe null when entering the local function? Is that necessary rather than relying on the state from the calling method as we would do if there were no [MemberNotNull] attributes? #Resolved

Yes, that's why.
For the enforcement of MemberNotNull attribute within a method body, we set the initial state of the field to maybe-null when entering the body and check that it's been changed to not-null by the time you exit the method. That way we know that the method did explicitly set a not-null state.
I used the same design for local functions. That analysis/enforcement can be performed by looking at the local function in isolation, without regard to its invocations.

cston · 2024-10-25T20:45:11Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            // Note: we add a synthesized BoundReturnStatement in BindLocalFunctionStatement
+            //       using the block body as syntax, but we report the diagnostic on the closing brace in that case
+            c.VerifyEmitDiagnostics(
+                // (16,9): warning CS8774: Member 'field1' must have a non-null value when exiting.


'field1'

Why report a warning for field1? #Resolved

That's the same behavior as with methods. If you put the MemberNotNull attribute, you must set the field to a not-null value by the time you exit the method. sharplab

jcouv · 2024-10-30T16:56:36Z

@cston @RikkiGibson This is ready for another look. Thanks

cston · 2024-10-30T17:35:19Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

-        [Fact]
-        public void MemberNotNull_LocalFunction()
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/56256")]
+        public void MemberNotNull_InstanceLocalFunction()


InstanceLocalFunction

Consider naming this _NonStaticLocalFunction, for consistency with other tests, and because there's no requirement that the local function captures this (and doesn't in this case). There are a couple of other uses of _InstanceLocalFunction below as well. #Resolved

Yup. Sorry, I missed those

cston · 2024-10-30T17:38:51Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            while (current.ContainingSymbol is MethodSymbol container)
+            {
+                current = container;
+                anyStatic |= container.IsStatic;


anyStatic |= container.IsStatic

Consider removing anyStatic and change this to if (container.IsStatic) return 0; #Resolved

RikkiGibson · 2024-10-31T20:27:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+            [MemberNotNull(nameof(field1), nameof(field2))]
+            bool init() => throw null!;


It would be interesting to have versions of a few of these tests (e.g. referencing instance members in 'static context'), where the end of the local function is reachable. #Pending

RikkiGibson · 2024-10-31T20:31:53Z

...Compilers/VisualBasic/Portable/Lowering/ExpressionLambdaRewriter/ExpressionLambdaRewriter.vb

@@ -447,7 +447,7 @@ lSelect:
            Debug.Assert(node.MethodGroupOpt Is Nothing)

            Dim delegateType As NamedTypeSymbol = DirectCast(node.Type, NamedTypeSymbol)
-            Debug.Assert(delegateType.TypeKind = TypeKind.Delegate)
+            Debug.Assert(delegateType.TypeKind = TYPEKIND.Delegate)


Was this change intended?

RikkiGibson · 2024-10-31T20:33:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

@@ -21002,7 +21002,7 @@ public void M()
        }

        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/56256")]
-        public void MemberNotNull_LocalFunction_BaseField()
+        public void MemberNotNull_NonStaticLocalFunction_BaseField()


Is there any test for 'MemberNotNull' on a local function within a constructor?

RikkiGibson

LGTM just minor questions/nits.

jcouv added the Area-Compilers label Oct 9, 2024

jcouv self-assigned this Oct 9, 2024

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 9, 2024

jcouv force-pushed the local-membernotnull branch from 74ff889 to f38640c Compare October 9, 2024 00:31

Respect [MemberNotNull] on local function

f49a448

jcouv force-pushed the local-membernotnull branch from ccce2b8 to f49a448 Compare October 10, 2024 16:53

jcouv marked this pull request as ready for review October 14, 2024 15:25

jcouv requested a review from a team as a code owner October 14, 2024 15:25

Add test

3237e0e

RikkiGibson self-assigned this Oct 14, 2024

RikkiGibson reviewed Oct 14, 2024

View reviewed changes

jcouv added 2 commits October 14, 2024 12:10

Revert "Add test"

814eb27

This reverts commit 3237e0e.

Address feedback

98388f6

jcouv requested a review from RikkiGibson October 14, 2024 19:15

RikkiGibson reviewed Oct 14, 2024

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

jcouv added 2 commits October 14, 2024 16:36

Adjust assertion

af0b613

Remove assertion and add repro test

5c6dd32

RikkiGibson previously approved these changes Oct 15, 2024

View reviewed changes

RikkiGibson requested a review from a team October 15, 2024 19:32

cston reviewed Oct 16, 2024

View reviewed changes

jcouv marked this pull request as draft October 17, 2024 00:12

jcouv added 2 commits October 16, 2024 18:28

Address feedback

9cd8e95

Enforce member post-conditions in local function bodies

5e27186

jcouv commented Oct 25, 2024

View reviewed changes

Cleanup

ecad9a6

jcouv commented Oct 25, 2024

View reviewed changes

Cleanup

e610a7d

cston reviewed Oct 25, 2024

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs Show resolved Hide resolved

cston reviewed Oct 25, 2024

View reviewed changes

Address feedback

3095a41

jcouv requested a review from cston October 30, 2024 16:56

cston reviewed Oct 30, 2024

View reviewed changes

cston approved these changes Oct 30, 2024

View reviewed changes

Address feedback

67b7130

cston approved these changes Oct 30, 2024

View reviewed changes

RikkiGibson reviewed Oct 31, 2024

View reviewed changes

RikkiGibson approved these changes Oct 31, 2024

View reviewed changes

Address feedback

b5ae55c

RikkiGibson approved these changes Oct 31, 2024

View reviewed changes

jcouv enabled auto-merge (squash) October 31, 2024 22:41

jcouv merged commit 6200694 into dotnet:main Oct 31, 2024
24 checks passed

dotnet-policy-service bot added this to the Next milestone Oct 31, 2024

This was referenced Nov 4, 2024

[Automated] PRs inserted in VS build main-35504.12 #75712

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35507.203 #75826

Closed

dotnet-bot mentioned this pull request Nov 19, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #75965

Closed

jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect [MemberNotNull] on local functions #75448

Respect [MemberNotNull] on local functions #75448

jcouv commented Oct 9, 2024 •

edited

Loading

jcouv commented Oct 15, 2024

cston Oct 16, 2024 •

edited

Loading

cston Oct 16, 2024 •

edited

Loading

cston Oct 16, 2024 •

edited

Loading

jcouv Oct 25, 2024

jcouv Oct 25, 2024

cston Oct 25, 2024 •

edited by jcouv

Loading

jcouv Oct 29, 2024 •

edited

Loading

jcouv Oct 29, 2024

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited by jcouv

Loading

jcouv Oct 29, 2024

cston Oct 25, 2024 •

edited by jcouv

Loading

jcouv Oct 29, 2024

cston Oct 25, 2024 •

edited by jcouv

Loading

jcouv Oct 29, 2024

jcouv commented Oct 30, 2024

cston Oct 30, 2024 •

edited by jcouv

Loading

jcouv Oct 30, 2024

cston Oct 30, 2024 •

edited by jcouv

Loading

RikkiGibson Oct 31, 2024 •

edited by jcouv

Loading

RikkiGibson Oct 31, 2024

RikkiGibson Oct 31, 2024 •

edited

Loading

RikkiGibson left a comment


		return GetOrCreateSlot(member, containingSlot);
		if (member.IsStatic != (containingSlot == 0))

		[MemberNotNull(nameof(field1), nameof(field2))]
		bool init() => throw null!;

Respect [MemberNotNull] on local functions #75448

Respect [MemberNotNull] on local functions #75448

Conversation

jcouv commented Oct 9, 2024 • edited Loading

jcouv commented Oct 15, 2024

cston Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

cston Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

cston Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 25, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Oct 30, 2024

cston Oct 30, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 30, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson Oct 31, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

jcouv commented Oct 9, 2024 •

edited

Loading

cston Oct 16, 2024 •

edited

Loading

cston Oct 16, 2024 •

edited

Loading

cston Oct 16, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited by jcouv

Loading

jcouv Oct 29, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited

Loading

cston Oct 25, 2024 •

edited by jcouv

Loading

cston Oct 25, 2024 •

edited by jcouv

Loading

cston Oct 25, 2024 •

edited by jcouv

Loading

cston Oct 30, 2024 •

edited by jcouv

Loading

cston Oct 30, 2024 •

edited by jcouv

Loading

RikkiGibson Oct 31, 2024 •

edited by jcouv

Loading

RikkiGibson Oct 31, 2024 •

edited

Loading