Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add nullable annotations to NullableWalker #45640

Merged
merged 16 commits into from
Jul 31, 2020
Merged

Add nullable annotations to NullableWalker #45640

merged 16 commits into from
Jul 31, 2020

Conversation

cston
Copy link
Member

@cston cston commented Jul 3, 2020

All methods are annotated except VisitConversion().

@cston cston requested a review from a team as a code owner July 3, 2020 07:01
@RikkiGibson
Copy link
Contributor

RikkiGibson commented Jul 3, 2020 •
edited by cston
Loading

Test failures likely due to #44348 #Resolved

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

                         compilation.GetBinderFactory(node.SyntaxTree!).GetBinder(node.Syntax);

Why is this safe? I think we should prefer an assert over suppression if we're certain. #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

            var previousSlot = snapshotBuilderOpt?.EnterNewWalker(symbol!) ?? -1;

Seems like we have a mismatch here. Why is symbol annotated? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

                                    if (AreNullableAndUnderlyingTypes(operandType, convertedType, out _))

Annotate the method with [NotNullWhen(true)]? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

    protected override void VisitRvalue(BoundExpression? node, bool isKnownToBeAnLvalue = false)

Got to here. Will pick back up in the morning. #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        var expr = _binder!.BindObjectCreationForErrorRecovery(node, discardedDiagnostics);

Why are we certain this is safe? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

            if (containingSlot >= 0 && (node as BoundObjectInitializerExpressionBase)?.Initializers.Length != 0)

We could break this out into something like node is BoundObjectInitializerExpressionBase { Type: {} nodeType, Initializers: {} initializers } && initializers.Length != 0 if we want to avoid the ! below? Can't wait until we can actually use not patterns though :) #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        SetUpdatedSymbol(node, node.AddMethod, reinferredMethod!);

Perhaps we should assert that this is never null instead. #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        SetResultType(node, TypeWithState.Create(arrayType, NullableFlowState.NotNull));

Is there a test where this change is observed? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 

        SetResultType(node, TypeWithState.Create(arrayType, NullableFlowState.NotNull));

Ah, nevermind, this is from the inlining of the helper.

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

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        var leftResultType = leftResult.Type!;

Perhaps assertions instead of suppressions? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

            if (nextConditionalAccessSlot > 0 && receiver.Type!.IsNullableType())

Perhaps one assertion instead of 2 suppressions. #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited by cston
Loading 

        if (!receiverType.HasNullType)

Prime candidate for MemberNotNull when we get it #Pending

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        var visitedParameters = PooledHashSet<ParameterSymbol?>.GetInstance();

When would a parameter symbol actually be null? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

                return result!;

Yet another C# 9 feature waiting to be used :) #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

                        method = CheckMethodGroupReceiverNullability(group, parameters, method!, conversion.IsExtensionMethod);

What's the suppressions in this method, I thought it was being skipped? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

                    if ((variable.Expression as BoundLocal)?.DeclarationKind == BoundLocalDeclarationKind.WithInferredType)

Could make this a pattern and get rid of the suppression: variable.Expression is BoundLocal { DeclarationKind: BoundLocalDeclarationKind.WithInferredType, ExpressionSymbol: {} expressionSymbol } #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

        if (!argumentType.HasNullType && IsNullabilityMismatch(paramType.Type, argumentType.Type!))

Perhaps an assert instead? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

            var inferredCollectionType = ResultType.Type!;

Assert instead? #Closed

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

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading 

            ReportArgumentWarnings(left!, leftType, logicalOperator.Parameters[0]);

I don't think the above assert actually guarantees that left is not null here. trueFalseOperator could have been null. #Closed

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

333fred
333fred reviewed Jul 10, 2020
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs
@@ -271,9 +271,9 @@ protected virtual int MakeSlot(BoundExpression node)
case BoundKind.FieldAccess:
case BoundKind.EventAccess:
case BoundKind.PropertyAccess:
if (TryGetReceiverAndMember(node, out BoundExpression receiver, out Symbol member))
if (TryGetReceiverAndMember(node, out BoundExpression? receiver, out Symbol? member))
Copy link
Member

@333fred 333fred Jul 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

member [](start = 97, length = 6)

Can we add an attribute? #Closed

@333fred
Copy link
Member

333fred commented Jul 10, 2020 •
edited
Loading

Done review pass (commit 2) #Closed

@cston
Copy link
Member Author

cston commented Jul 29, 2020 

            var previousSlot = snapshotBuilderOpt?.EnterNewWalker(symbol!) ?? -1;

All callers that pass non-null for snapshotBuilderOpt also pass non-null for symbol. Added an assert.

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

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

jcouv
jcouv reviewed Jul 30, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with review pass (iteration 8)

@cston
Copy link
Member Author

cston commented Jul 31, 2020

@333fred, @jcouv, thanks for the feedback. All comments have been addressed.

jcouv
jcouv approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 15)

@jcouv jcouv added the Concept-Null Annotations The issue involves annotating an API for nullable reference types label Jul 31, 2020
@333fred
Copy link
Member

333fred commented Jul 31, 2020 •
edited by cston
Loading 

        bool tryAsMemberOfSingleType(NamedTypeSymbol singleType, out Symbol? result)

Can we [NotNullWhen(true)] this? #Resolved

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

333fred
333fred approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 15). One more small comment on a possible use of NotNullWhen to remove some suppressions, but otherwise looks good.

jaredpar
jaredpar reviewed Jul 31, 2020
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Comment on lines +559 to +560
var method = _symbol as MethodSymbol;
if (method is object)
Copy link
Member

@jaredpar jaredpar Jul 31, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var method = _symbol as MethodSymbol;
if (method is object)
if (_symbol is MethodSymbol method)
``` #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the syntax before this change. Unfortunately, that does not allow an annotation on method. (if (_symbol is MethodSymbol? method) is not valid syntax.)

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

@cston cston merged commit 552de9a into dotnet:master Jul 31, 2020
@ghost ghost added this to the Next milestone Jul 31, 2020
@cston cston deleted the nullable-enable branch July 31, 2020 21:41
@RikkiGibson RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types
Projects
None yet
Milestone
16.8.P2
Development

Successfully merging this pull request may close these issues.
5 participants
@cston @RikkiGibson @333fred @jaredpar @jcouv