Factor nullability logic for placeholders by jcouv · Pull Request #58036 · dotnet/roslyn

jcouv · 2021-11-30T19:10:01Z

Introduce NullableWalker.AddPlaceholderReplacement and RemovePlaceholderReplacement

AlekseyTs · 2021-12-01T13:44:54Z

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

            if (expression is BoundValuePlaceholderBase placeholder)
            {
-                if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
+                if (PlaceholderMustBeRegistered(placeholder))


PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

AlekseyTs · 2021-12-01T13:45:22Z

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


        private void VisitPlaceholderWithReplacement(BoundValuePlaceholderBase node)
        {
+            if (PlaceholderMustBeRegistered(node))


PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

AlekseyTs

LGTM (commit 1)

jcouv · 2021-12-01T16:13:50Z

@dotnet/roslyn-compiler for second review. Small PR. Thanks

AlekseyTs · 2021-12-01T17:50:06Z

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

            if (expression is BoundValuePlaceholderBase placeholder)
            {
-                if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
+                if (_resultForPlaceholdersOpt != null)


_resultForPlaceholdersOpt != null

Could you elaborate why the condition got split? Don't we want to assert if the dictionary is null? #Closed

Just me being stupid... :-P

AlekseyTs · 2021-12-01T17:52:02Z

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

-                SetResult(node, result.RValueType, result.LValueType);
-            }
-            else
+            if (_resultForPlaceholdersOpt != null)


_resultForPlaceholdersOpt != null

Same comment as for LearnFromNonNullTest #Closed

cston · 2021-12-01T23:08:20Z

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

+            Debug.Assert(removed);
+        }
+
+        private bool PlaceholderMustBeRegistered(BoundValuePlaceholderBase placeholder)


private

static

333fred · 2021-12-02T19:24:54Z

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

+            Debug.Assert(removed);
+        }
+
+        private static bool PlaceholderMustBeRegistered(BoundValuePlaceholderBase placeholder)


This appears to only be used as an assert. Consider making this a conditional method and doing the assert directly.

AlekseyTs · 2021-12-03T18:26:33Z

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

+                default:
+                    // Newer placeholders are expected to follow placeholder discipline, namely that
+                    // they must be added to the map before they are visited, then removed.
+                    throw ExceptionUtilities.UnexpectedValue(placeholder.Kind);


throw

Did you really want to throw? Calling ExceptionUtilities.UnexpectedValue(placeholder.Kind) should be sufficient, it will assert.

I don't see a problem with throwing as we do everywhere else. It's clear.

jcouv · 2021-12-04T02:09:39Z

/azp run

azure-pipelines · 2021-12-04T02:10:04Z

Azure Pipelines successfully started running 4 pipeline(s).

…rovements * upstream/main: (68 commits) Lazy load ISourceLinkService to reduce DLL loads (dotnet#58108) [main] Update dependencies from dotnet/source-build (dotnet#57707) [main] Update dependencies from dotnet/arcade (dotnet#57968) Factor nullability logic for placeholders (dotnet#58036) Standardize list pattern lowering on `Index` constructor. (dotnet#58055) Add scripts to verify if a branch is ready to review Merge pull request dotnet#58100 from dotnet/dev/jorobich/skip-test Fix some places we weren't correctly disposing of VisualStudioAnalyzers Fix analyzer references being removed and added in one batch Fix indenting Ensure we don't silently capture any exceptions Don't MEF import the implementation directly if the public type will do Change comment Add comment Use actual jump tables Remove unused function Revert Simplify code Compute kind on demand Reorder ...

jcouv added the Area-Compilers label Nov 30, 2021

jcouv self-assigned this Nov 30, 2021

jcouv force-pushed the nullable-placeholders branch 4 times, most recently from f841fb3 to 2b93066 Compare November 30, 2021 20:52

Factor nullability logic for placeholders

c4596a2

jcouv force-pushed the nullable-placeholders branch from 2b93066 to c4596a2 Compare November 30, 2021 20:52

jcouv marked this pull request as ready for review November 30, 2021 23:04

jcouv requested a review from a team as a code owner November 30, 2021 23:04

AlekseyTs reviewed Dec 1, 2021

View reviewed changes

AlekseyTs approved these changes Dec 1, 2021

View reviewed changes

Use assertion only

a7cf49f

AlekseyTs reviewed Dec 1, 2021

View reviewed changes

Also in null map case

8c2c74f

cston reviewed Dec 1, 2021

View reviewed changes

cston approved these changes Dec 1, 2021

View reviewed changes

static

aa96ae6

jcouv enabled auto-merge (squash) December 2, 2021 03:34

This was referenced Dec 2, 2021

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

Open

Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.CommandLineTests.ArgumentParsing is flaky #58077

Open

333fred reviewed Dec 2, 2021

View reviewed changes

Use assertion method

f50b6e3

runfoapp bot mentioned this pull request Dec 3, 2021

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

AlekseyTs reviewed Dec 3, 2021

View reviewed changes

jcouv disabled auto-merge December 4, 2021 01:13

jcouv enabled auto-merge (squash) December 4, 2021 01:15

jcouv merged commit 1741824 into dotnet:main Dec 4, 2021

ghost added this to the Next milestone Dec 4, 2021

This was referenced Dec 6, 2021

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

jcouv modified the milestones: Next, 17.1 Dec 7, 2021

jcouv deleted the nullable-placeholders branch December 7, 2021 05:19

Conversation

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

Uh oh!

Uh oh!

AlekseyTs Dec 1, 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!

AlekseyTs Dec 1, 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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 1, 2021

Uh oh!

AlekseyTs Dec 1, 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 Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 1, 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 Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

333fred Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 4, 2021

Uh oh!

azure-pipelines bot commented Dec 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Nov 30, 2021 •

edited

Loading

AlekseyTs Dec 1, 2021 •

edited

Loading

AlekseyTs Dec 1, 2021 •

edited

Loading

AlekseyTs Dec 1, 2021 •

edited

Loading

AlekseyTs Dec 1, 2021 •

edited

Loading