Enforce nullability contract in lambda body by jcouv · Pull Request #56590 · dotnet/roslyn

jcouv · 2021-09-21T22:44:06Z

Fixes #52827 (Enforce nullability attributes in lambda body)
Fixes #47896 (Enforce nullability attributes in local function body)
Fixes #45814 (Enforce DoesNotReturn in local function body)
Fixes #52598 (Enforce DoesNotReturn in local function body)
Fixes #55649 (Roslyn fails to verify nullable attributes on lambda expressions in delegate conversions)

Per discussion with Jared and Chuck, I'm pulling out the part of the change that relates to conversions. Opened #56668 to track this change in 17.1.

Relates to test plan #52192

jcouv · 2021-09-22T02:16:30Z

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

                }
            }

-            void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturnStatement returnStatement)


📝 The local functions were extracted mostly without modification (except for naming changes and tweak to location reported for WRN_ParameterDisallowsNull)

jcouv · 2021-09-23T04:09:13Z

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

            Debug.Assert(!useDelegateInvokeReturnType || delegateInvokeMethod is object);

-            var oldSymbol = this.CurrentSymbol;
+            var oldSymbol = this._symbol;


📝 _symbol drives various APIs, such as MethodParameters #Resolved

Did you consider modifying the relevant APIs to instead check CurrentSymbol?

Took another look, but the comment on _symbol says its primary purpose is to provide method parameters and return type. So I think this is correct.

cston · 2021-09-23T20:33:17Z

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

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


FunctionTypeConversion

Are these function type conversions or method group conversions? #Closed

cston · 2021-09-23T20:47:14Z

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

        /// 'MethodParameters', 'MethodThisParameter' and 'AnalyzeOutParameters(...)' should be used
        /// instead. _symbol is null during speculative binding.
        /// </summary>
-        protected readonly Symbol _symbol;


readonly Symbol _symbol

We already have a CurrentSymbol field that is mutable. Do we need both fields to be mutable? #Resolved

Yes, I think so, as it drives separate APIs, like MethodParameters which are used by existing helpers for nullability checks.

cston · 2021-09-23T21:06:11Z

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

+        }

-            void reportParameterIfBadConditionalState(SyntaxNode syntax, ParameterSymbol parameter, bool sense, LocalState stateWhen)
+        void EnforceParameterNotNullOnExit(SyntaxNode? syntaxOpt, LocalState state)


void

private for each of these. #Closed

cston · 2021-09-23T21:10:16Z

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

            }
+        }
+
+        void EnforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturnStatement returnStatement)


EnforceNotNullWhenForPendingReturn

Consider moving this before EnforceParameterNotNullOnExit() since it looks like that might reduce the differences in this section. #Closed

cston · 2021-09-23T21:33:57Z

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

        {
-            if (!unboundLambda.HasExplicitlyTypedParameterList)
+            MethodSymbol targetInvokeMethod = delegateType.DelegateInvokeMethod;
+            MethodSymbol sourceInvokeMethod = (MethodSymbol)lambda.ExpressionSymbol;


(MethodSymbol)lambda.ExpressionSymbol

The cast can be avoided with lambda.Symbol. #Resolved

cston · 2021-09-23T22:01:23Z

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

+                compilation,
+                sourceInvokeMethod,
+                targetInvokeMethod,
+                new BindingDiagnosticBag(Diagnostics),


new BindingDiagnosticBag(Diagnostics)

Can we re-use the instance from the previous call? #Resolved

cston · 2021-09-23T22:19:48Z

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

+        Func<object?> f9 = [return: NotNull] () => null; // 5
+        D1 f10 = [return: NotNull] () => null;
+        D2 f11 = [return: NotNull] () => null; // 6
+        D3 f12 = [return: NotNull] () => null;


Are these cases distinct from f6, f7, f8? #Closed

cston · 2021-09-23T22:20:29Z

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

+        _ = new Func<object?>([return: NotNull] () => null); // 5
+        _ = new D1([return: NotNull] () => null);
+        _ = new D2([return: NotNull] () => null); // 6
+        _ = new D3([return: NotNull] () => null);


Are these cases distinct from the three above? #Closed

cston · 2021-09-23T23:17:59Z

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

+                Diagnostic(ErrorCode.WRN_NullabilityMismatchInParameterTypeOfTargetDelegate, "([NotNullWhen(true)] out object? obj) =>").WithArguments("obj", "lambda expression", "D").WithLocation(10, 15),
+                // (13,17): warning CS8762: Parameter 'obj' must have a non-null value when exiting with 'true'.
+                //                 return true;
+                Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return true;").WithArguments("obj", "true").WithLocation(13, 17)


ErrorCode.WRN_ParameterConditionallyDisallowsNull

Should this be reported for the previous lambda as well? #Closed

One lambda returns true with obj=null and the other returns false. So only one of them reports this warning.

jcouv · 2021-09-27T16:10:30Z

@RikkiGibson Gentle ping to take a look. Thanks

RikkiGibson · 2021-09-27T18:49:02Z

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

+    }
+}";
+            // Wrong lambda conversion warnings tracked by https://github.com/dotnet/roslyn/issues/56668
+            // We're expecting WRN_NullabilityMismatchInReturnTypeOfTargetDelegate for each commented line


I'm not 100% sure I understood. Does this mean that this baseline is missing a single diagnostic for each // n comment above? And the commented lines are not meant to indicate the entire set of expected diagnostics? #Resolved

That's correct. We should have one WRN_NullabilityMismatchInReturnTypeOfTargetDelegate for each commented line (and only those lines). Other lines have different warnings which are not the focus on this test.

Or is it more that the "check nullable attributes in lambda conversion" was implemented, the commented lines were added, then the conversion check was punted out, and the commented lines were intentionally not updated?

Your description is also correct (another way to say the same thing).

RikkiGibson · 2021-09-27T18:55:30Z

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

                // (9,28): warning CS8621: Nullability of reference types in return type of 'lambda expression' doesn't match the target delegate 'Func<string?>' (possibly because of nullability attributes).
                //         Func<string?> f3 = string () => default!;
-                Diagnostic(ErrorCode.WRN_NullabilityMismatchInReturnTypeOfTargetDelegate, "string () => default!").WithArguments("lambda expression", "System.Func<string?>").WithLocation(9, 28));
+                Diagnostic(ErrorCode.WRN_NullabilityMismatchInReturnTypeOfTargetDelegate, "string () =>").WithArguments("lambda expression", "System.Func<string?>").WithLocation(9, 28));


Removing the lambda body from the diagnostic span seems like a good change. #Resolved

RikkiGibson · 2021-09-27T18:57:30Z

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

+        assert(o2 is not null);
+        o2.ToString();
+
+        // Note: no enforcement on method body for DoesNotReturnIf


Was this limitation intentional? Or the functionality was punted out? #Resolved

I think we failed to find a design for that enforcement (if I recall correctly it's not possible for a reasonable cost).

RikkiGibson · 2021-09-27T19:07:17Z

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

+    }
+}
+";
+            var comp = CreateCompilation(new[] { source, AllowNullAttributeDefinition });


Should we also have a comment here that the missing warning will be handled in a follow-up? #Resolved

RikkiGibson · 2021-09-27T19:14:34Z

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

+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics(
+                // (11,14): warning CS8622: Nullability of reference types in type of parameter 'o' of 'lambda expression' doesn't match the target delegate 'Action<object>' (possibly because of nullability attributes).
+                //         a1 = (object? o) => { }; // warning


nit: it looks like // warning comments are present in this test baseline but not in the source code. #Resolved

RikkiGibson

LGTM. Would be nice if we could address CurrentSymbol vs _symbol in this PR but can be punted if necessary.

jcouv · 2021-09-28T00:04:44Z

Re-targeted to release/dev17.0 branch

jcouv added the Feature - Lambda Improvements label Sep 21, 2021

jcouv added this to the 17.0 milestone Sep 21, 2021

jcouv self-assigned this Sep 21, 2021

ghost added the Area-Compilers label Sep 21, 2021

jcouv force-pushed the lambda-nullability branch 2 times, most recently from 40a2b89 to 2c450c7 Compare September 22, 2021 02:14

jcouv commented Sep 22, 2021

View reviewed changes

runfoapp bot mentioned this pull request Sep 22, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

jcouv force-pushed the lambda-nullability branch 2 times, most recently from 7c3908a to 7bec213 Compare September 23, 2021 04:06

Enforce nullability attributes in lambda and local function bodies

ae40c07

jcouv force-pushed the lambda-nullability branch from 7bec213 to c082359 Compare September 23, 2021 04:08

jcouv commented Sep 23, 2021

View reviewed changes

Improve diagnostic location

4220574

jcouv force-pushed the lambda-nullability branch from c082359 to 4220574 Compare September 23, 2021 04:12

tweak

d7e40ec

jcouv force-pushed the lambda-nullability branch from 56a06e4 to d5b9c77 Compare September 23, 2021 06:00

more tests

0f4fdb7

jcouv force-pushed the lambda-nullability branch from d5b9c77 to 0f4fdb7 Compare September 23, 2021 07:14

runfoapp bot mentioned this pull request Sep 23, 2021

SmartBreakLineWithTabTabCompletion2 is flaky #56394

Closed

jcouv requested review from a team, RikkiGibson and cston September 23, 2021 15:31

jcouv marked this pull request as ready for review September 23, 2021 16:53

RikkiGibson self-assigned this Sep 23, 2021

cston reviewed Sep 23, 2021

View reviewed changes

Revert analysis of attributes in conversions of lambdas

3bb400f

jcouv mentioned this pull request Sep 23, 2021

Check nullability attributes in conversions of lambdas #56668

Closed

Address feedback and update test baselines

94b6026

jcouv requested a review from cston September 24, 2021 00:38

cston approved these changes Sep 24, 2021

View reviewed changes

RikkiGibson reviewed Sep 27, 2021

View reviewed changes

Add tests with lambdas with implicit types

3f253eb

RikkiGibson approved these changes Sep 27, 2021

View reviewed changes

tweak

28dcdf4

jcouv enabled auto-merge (squash) September 27, 2021 19:22

jcouv added 3 commits September 27, 2021 13:38

tweak

f4e7a2f

Merge remote-tracking branch 'dotnet/main' into lambda-nullability

e7b2b70

resolve conflict

131a2e6

jcouv disabled auto-merge September 28, 2021 00:04

jcouv changed the base branch from main to release/dev17.0 September 28, 2021 00:04

jcouv enabled auto-merge (squash) September 28, 2021 00:07

jcouv merged commit 3a4b369 into dotnet:release/dev17.0 Sep 28, 2021

jcouv deleted the lambda-nullability branch September 28, 2021 02:16

cston mentioned this pull request Jan 11, 2022

Test plan for lambda improvements #52192

Closed

Conversation

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

Uh oh!

Uh oh!

jcouv Sep 22, 2021 • 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 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Sep 23, 2021 • 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 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

cston Sep 23, 2021 • 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 23, 2021 • 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 23, 2021 • 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!

cston Sep 23, 2021 • 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!

cston Sep 23, 2021 • 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 23, 2021 • 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 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Sep 27, 2021

Uh oh!

RikkiGibson Sep 27, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 27, 2021 • 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!

RikkiGibson Sep 27, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Sep 27, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jcouv commented Sep 21, 2021 •

edited

Loading

jcouv Sep 22, 2021 •

edited

Loading

jcouv Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited by jcouv

Loading

cston Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited by jcouv

Loading

cston Sep 23, 2021 •

edited by jcouv

Loading

cston Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited

Loading

cston Sep 23, 2021 •

edited

Loading

RikkiGibson Sep 27, 2021 •

edited by jcouv

Loading

RikkiGibson Sep 27, 2021 •

edited by jcouv

Loading

RikkiGibson Sep 27, 2021 •

edited by jcouv

Loading

RikkiGibson Sep 27, 2021 •

edited by jcouv

Loading

RikkiGibson Sep 27, 2021 •

edited by jcouv

Loading