Use default parameter values in flow analysis by RikkiGibson · Pull Request #38788 · dotnet/roslyn

RikkiGibson · 2019-09-20T23:12:59Z

/cc @jcouv. This flows data around in a somewhat disjunct way but I felt that adding lots of additional return values (i.e. ImmutableArray<BoundExpression> arguments, ImmutableArray<BoundExpression> argumentsNoConversions, ImmutableArray<Conversion> conversions) which "augment" the original arrays with the default parameter values was also kind of clumsy.

…riately

…uments

RikkiGibson · 2019-09-25T04:22:04Z

This change seems to consistently make the build hang. I don't understand why, though, because it seems to build fine locally. #Closed

AlekseyTs · 2019-09-25T04:53:21Z

This change seems to consistently make the build hang. I don't understand why, though, because it seems to build fine locally.

That is probably because CI builds everything with compiler that includes the change. Try build -restore -bootstrap locally #Closed

RikkiGibson · 2019-09-25T23:34:25Z

So, the on-the-fly creation of default parameter values is causing analysis to run an indefinite number of times on certain methods. It will probably be necessary to refactor to do something like create the effective arguments on demand on the BoundCall and reuse them. #Closed

jcouv · 2019-09-27T20:44:24Z

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

+                    if (parameter.IsOptional && !visitedParameters.Contains(parameter))
+                    {
+                        var annotations = GetParameterAnnotations(parameter);
+                        if (!_defaultValues.TryGetValue((syntax, parameter), out var argument))


syntax [](start = 57, length = 6)

Why do we need the syntax as part of the key? I would think that ParameterSymbol -> BoundExpression would be a sufficient map, no?
If we don't need it, then we don't need to pass syntax into VisitArgumentsEvaluate too. #Closed

The LocalRewriter.GetDefaultParameterValue uses the provided Syntax to get the actual values when CallerMemberName, CallerLineNumber, etc. attributes are in use. The produced BoundExpression uses the containing invocation such as BoundCall for its Syntax property.

It's possible we could make it work without actually providing syntax to the BoundExpression for the default argument but I'm not sure what would break/need to change as a result. #Closed

Makes sense. May be worth a comment

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

Also, could you add a test with CallerExpression or some other attribute, if we don't have one already?

In reply to: 329250289 [](ancestors = 329250289,329248471)

Adding additional comments to the declaration of _defaultValues

In reply to: 329250289 [](ancestors = 329250289,329248471)

jcouv · 2019-09-27T20:47:06Z

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

            _updatedSymbolMapOpt = updatedSymbolMapOpt;
            _returnTypesOpt = returnTypesOpt;
            _snapshotBuilderOpt = snapshotBuilderOpt;
+            _defaultValues = defaultValues;


defaultValues [](start = 29, length = 13)

from our discussion, is it possible to initialize the empty dictionary here? When do we need a dictionary passed in across analyses? #Closed

It seems that we don't, I somehow got the impression that we did when I was troubleshooting the bootstrap build

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

jcouv · 2019-09-27T20:51:25Z

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

                {
-                    method = InferMethodTypeArguments(node, method, GetArgumentsForMethodTypeInference(argumentsNoConversions, results));
-                    parameters = method.Parameters;
+                    var binder = (node as BoundCall)?.BinderOpt ?? (node as BoundCollectionElementInitializer)?.BinderOpt ?? throw ExceptionUtilities.UnexpectedValue(node);


BoundCall [](start = 42, length = 9)

Looking at the different kinds of bound nodes that have BinderOpt I noticed BoundIndexerAccess. and BoundObjectCreation. Should they be handled too? #Closed

Perhaps, I only did these because the overload of InferMethodTypeArguments that I deleted would switch on these two types and reject any other types. #Closed

The reason I only handled these two is that the InferMethodTypeArguments overload that I deleted only handled these two.

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

Could you add a test with indexer and default parameter?

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

I will add the test with the expectation that NotNullIfNotNull will not have an effect and that we won't get the UnexpectedValue exception.

In reply to: 329269700 [](ancestors = 329269700,329251239)

jcouv · 2019-09-27T20:55:11Z

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

+                        GetConversionIfApplicable(argument, argumentNoConversion),
                        argumentNoConversion,
-                        conversions.IsDefault ? Conversion.Identity : conversions[i],
+                        conversions.IsDefault || i >= conversions.Length ? Conversion.Identity : conversions[i],


i >= conversions.Length [](start = 49, length = 23)

I didn't understand this. Are we using the right index? #Closed

This conversions array was not resized as part of VisitArgumentsEvaluate. #Closed

I see. The important thing is that the extended arrays start the same as the original arrays, and only have some items appended at the end.
Thanks

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

jcouv · 2019-09-27T20:56:45Z

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

            }
-            var builder = ArrayBuilder<VisitArgumentResult>.GetInstance(n);
+
+            var visitedParameters = PooledHashSet<ParameterSymbol>.GetInstance();


() [](start = 78, length = 2)

can use n as size hint? #Closed

This seems reasonable, as long as it doesn't cause a bad behavior when we have a call with a large params array.

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

Scratch that, PooledHashSet.GetInstance does not actually accept a size hint.

In reply to: 329263552 [](ancestors = 329263552,329248589)

Ah, good point

In reply to: 329263995 [](ancestors = 329263995,329263552,329248589)

jcouv · 2019-09-27T21:02:44Z

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

+                }
+
+                var previousDisableNullabilityAnalysis = _disableNullabilityAnalysis;
+                _disableNullabilityAnalysis = true;


true [](start = 46, length = 4)

consider adding a comment too #Closed

jcouv · 2019-09-27T21:05:28Z

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

        /// </summary>
        private VisitResult _currentConditionalReceiverVisitResult;

+        /// <summary>


summary [](start = 13, length = 7)

May be worth adding a note as to why we save/cache these #Closed

jcouv · 2019-09-27T21:25:25Z

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

+                        var annotations = GetParameterAnnotations(parameter);
+                        if (!_defaultValues.TryGetValue((syntax, parameter), out var argument))
+                        {
+                            _defaultValues[(syntax, parameter)] = argument = LocalRewriter.GetDefaultParameterValue(syntax, parameter, ThreeState.Unknown, localRewriter: null, _binder, Diagnostics);


Unknown [](start = 146, length = 7)

Should this be ThreeState.True instead? #Closed

I think so, because we know we are in an invocation. It seems that passing Unknown makes it examine the syntax to decide if the default argument is contained in a kind of syntax where caller info makes sense.

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

jcouv · 2019-09-27T21:26:44Z

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

+            }
+
+            if (!parametersOpt.IsDefaultOrEmpty)
+            {


{ [](start = 12, length = 1)

It may also be good to leave a comment explaining that it's okay to analyze the optional arguments after the real arguments because they won't have any side-effects (constants only) #Closed

jcouv · 2019-09-27T21:31:50Z

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

            CompileAndVerify(comp2);
        }

+        [Fact]


Fact [](start = 9, length = 4)

Please add a test with attribute on a local function. It will fail for now, but will pass sometime soon ;-) #Closed

jcouv

Done with review pass (iteration 10)

…uments

RikkiGibson · 2019-10-14T18:35:15Z

@jcouv @dotnet/roslyn-compiler sorry that I neglected this PR for a bit, I have addressed review comments and have the bootstrap build working again.

jcouv

LGTM Thanks (iteration 16)

cston · 2019-10-15T17:37:07Z

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

            _analyzedNullabilityMapOpt = analyzedNullabilityMapOpt;
            _returnTypesOpt = returnTypesOpt;
            _snapshotBuilderOpt = snapshotBuilderOpt;
+            _defaultValues = new Dictionary<(SyntaxNode, ParameterSymbol), BoundExpression>();


new Dictionary<(SyntaxNode, ParameterSymbol), BoundExpression>() [](start = 29, length = 64)

Consider using PooledDictionary<,> and delaying the allocation until the first use, similar to other dictionary fields. #Closed

cston · 2019-10-15T18:02:43Z

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

+                visitedParameters.Add(parameter);
+            }
+
+            if (!parametersOpt.IsDefaultOrEmpty)


) [](start = 47, length = 1)

Can we skip this block (and avoid allocating new arrays for arguments, etc.) if visitedParameters.Count == parametersOpt.Length? #Closed

This is the scenario where we know every parameter was visited, right? Maybe we can change the condition to !parametersOpt.IsDefaultOrEmpty && parametersOpt.Length != visitedParameters.Count

cston · 2019-10-15T18:26:11Z

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

+        _ = M1(p1: null, p2: ""hello"").ToString(); // 3
+        _ = M1(p2: null, p1: ""hello"").ToString();
+    }
+}


Are we testing cases where the optional value is null?

using System.Diagnostics.CodeAnalysis; public class C { [return: NotNullIfNotNull("p1")] string? M1(string? p1 = null, string? p2 = null) => p1; void M2() { _ = M1(p2: "a").ToString(); // 1 _ = M1(p1: "b").ToString(); _ = M1(p1: "a", p2: null).ToString(); _ = M1(p2: "a", p1: null).ToString(); // 2 _ = M1(p1: null, p2: "b").ToString(); // 3 _ = M1(p2: null, p1: "b").ToString(); } } ``` #Closed

Looks like no, will add the test

cston · 2019-10-15T21:15:00Z

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

Dictionary [](start = 47, length = 10)

PooledDictionary #Closed

Oops, pushed too soon.

cston · 2019-10-15T21:15:27Z

_defaultValues?.Free(); #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:347 in fc1ca4a. [](commit_id = fc1ca4a, deletion_comment = False)

RikkiGibson added 7 commits September 18, 2019 14:05

WIP

e1934e7

Tolerate null parameters arrays and pass reinferred parameters approp…

d8f88d9

…riately

Rebuild argsToParamsOpt in VisitArgumentsEvaluate

580a13e

Don't show warnings for implicit default arguments at call sites

63b6ee7

Test DoesNotReturnIf

f627af2

Cleanup

9d9e1af

Merge remote-tracking branch 'upstream/master' into get-effective-arg…

ee24ffd

…uments

RikkiGibson added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Sep 20, 2019

RikkiGibson requested a review from a team as a code owner September 20, 2019 23:13

jcouv self-assigned this Sep 20, 2019

Merge remote-tracking branch 'upstream/master' into get-effective-arg…

920bffd

…uments

RikkiGibson closed this Sep 25, 2019

RikkiGibson reopened this Sep 25, 2019

RikkiGibson added 2 commits September 26, 2019 12:57

Try caching default arguments in a dictionary

130695c

Fix more bootstrap build breaks

9d81148

jcouv reviewed Sep 27, 2019

View reviewed changes

RikkiGibson added 6 commits September 27, 2019 14:46

Remove ArgumentNoConversion from VisitArgumentResult

4659987

Add a few comments

9125e8f

Address feedback

0f680c0

Merge remote-tracking branch 'upstream/master' into get-effective-arg…

8007f6f

…uments

Fix typo.

5a5f973

Simplify usage of defaultValues dictionary

fc1ca4a

RikkiGibson mentioned this pull request Oct 14, 2019

Test bootstrap #39161

Closed

jcouv approved these changes Oct 14, 2019

View reviewed changes

RikkiGibson added this to the 16.4.P3 milestone Oct 14, 2019

RikkiGibson requested a review from a team October 14, 2019 22:05

cston reviewed Oct 15, 2019

View reviewed changes

Address feedback

48a713f

cston reviewed Oct 15, 2019

View reviewed changes

RikkiGibson added 2 commits October 15, 2019 14:19

Don't use a nullable annotation just yet

4793f34

Free pooled dictionary

35699ab

RikkiGibson force-pushed the get-effective-arguments branch from a320fcb to 35699ab Compare October 15, 2019 21:23

cston approved these changes Oct 15, 2019

View reviewed changes

RikkiGibson merged commit 3484227 into dotnet:master Oct 15, 2019

RikkiGibson deleted the get-effective-arguments branch October 15, 2019 23:09

Conversation

RikkiGibson commented Sep 20, 2019

Uh oh!

RikkiGibson commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Sep 25, 2019 • edited by RikkiGibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Sep 27, 2019 • 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 Sep 27, 2019 • 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!

jcouv Sep 27, 2019 • 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 Sep 27, 2019 • 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 Sep 27, 2019 • 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!

RikkiGibson Oct 9, 2019 • 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 27, 2019 • 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 Sep 27, 2019 • 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!

jcouv Sep 27, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Sep 27, 2019 • 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 27, 2019 • 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 27, 2019 • 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 commented Sep 25, 2019 •

edited

Loading

AlekseyTs commented Sep 25, 2019 •

edited by RikkiGibson

Loading

RikkiGibson commented Sep 25, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

RikkiGibson Sep 27, 2019 •

edited by jcouv

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

RikkiGibson Sep 27, 2019 •

edited by jcouv

Loading

RikkiGibson Oct 9, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

RikkiGibson Sep 27, 2019 •

edited by jcouv

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

jcouv Sep 27, 2019 •

edited

Loading

cston Oct 15, 2019 •

edited

Loading

cston Oct 15, 2019 •

edited

Loading

cston Oct 15, 2019 •

edited

Loading

cston Oct 15, 2019 •

edited

Loading

cston commented Oct 15, 2019 •

edited

Loading