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

Allow suppressing nullability warnings in more ref scenarios #74498

Merged
merged 9 commits into from
Sep 11, 2024
Merged

Allow suppressing nullability warnings in more ref scenarios #74498

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5ca85fb
Allow suppressing ref ternary operands
jjonescz Jul 23, 2024
76cf2af
Allow suppressing the whole ref ternary
jjonescz Jul 24, 2024
f79dad9
Allow suppressing ref foreach
jjonescz Jul 24, 2024
fecd701
Use type inference for ref ternary nullability
jjonescz Aug 29, 2024
f1f76ff
Meet annotation and state
jjonescz Sep 4, 2024
8f4e1d2
Move local function to the end
jjonescz Sep 4, 2024
2b35639
Keep the states joined
jjonescz Sep 5, 2024
cbe141c
Test ref assignment
jjonescz Sep 9, 2024
7fd64a4
Skip failing test
jjonescz Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5772,6 +5772,11 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)

if (isRef)
{
Debug.Assert(node is not BoundConditionalOperator { WasTargetTyped: true }, """
Unexpected ref target typed conditional operator.
Should not do type inference below in this case.
""");

TypeWithAnnotations consequenceLValue;
TypeWithAnnotations alternativeLValue;

Expand All @@ -5780,20 +5785,40 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver)
(alternativeLValue, alternativeRValue) = visitConditionalRefOperand(alternativeState, originalAlternative);
Join(ref this.State, ref consequenceState);

var lValueAnnotation = consequenceLValue.NullableAnnotation.EnsureCompatible(alternativeLValue.NullableAnnotation);
var rValueState = consequenceRValue.State.Join(alternativeRValue.State);

TypeSymbol? refResultType = node.Type?.SetUnknownNullabilityForReferenceTypes();
if (IsNullabilityMismatch(consequenceLValue, alternativeLValue))
{
// l-value types must match
ReportNullabilityMismatchInAssignment(node.Syntax, consequenceLValue, alternativeLValue);
// If there is a mismatch between the operands, use type inference to determine the target type.
BoundExpression consequencePlaceholder = CreatePlaceholderIfNecessary(originalConsequence, consequenceLValue);
BoundExpression alternativePlaceholder = CreatePlaceholderIfNecessary(originalAlternative, alternativeLValue);
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
refResultType = BestTypeInferrer.InferBestTypeForConditionalOperator(consequencePlaceholder, alternativePlaceholder, _conversions, out _, ref discardedUseSiteInfo);

// Report warning for each operand that is not convertible to the target type.
var refResultTypeWithAnnotations = TypeWithAnnotations.Create(refResultType, lValueAnnotation);
reportMismatchIfNecessary(originalConsequence, consequenceLValue, refResultTypeWithAnnotations);
reportMismatchIfNecessary(originalAlternative, alternativeLValue, refResultTypeWithAnnotations);

// Set "not null" top-level nullability. Consistent with equivalent method type inference or non-ref ternary scenarios.
lValueAnnotation = NullableAnnotation.NotAnnotated;
Copy link
Member

@jcouv jcouv Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This feels strange. In a scenario where we have List<object?>? vs. List<object>?, why would we have a not-null annotation or state? #Closed

rValueState = NullableFlowState.NotNull;

void reportMismatchIfNecessary(BoundExpression node, TypeWithAnnotations source, TypeWithAnnotations destination)
Copy link
Member

@jcouv jcouv Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Consider moving the local function towards the end for method. We don't follow that guideline 100% but generally do. It helps keep the main flow of logic uninterrupted. #Closed

{
if (!node.IsSuppressed && IsNullabilityMismatch(source, destination))
{
ReportNullabilityMismatchInAssignment(node.Syntax, source, destination);
}
}
}
else if (!node.HasErrors)
{
refResultType = consequenceRValue.Type!.MergeEquivalentTypes(alternativeRValue.Type, VarianceKind.None);
}

var lValueAnnotation = consequenceLValue.NullableAnnotation.EnsureCompatible(alternativeLValue.NullableAnnotation);
var rValueState = consequenceRValue.State.Join(alternativeRValue.State);

SetResult(node, TypeWithState.Create(refResultType, rValueState), TypeWithAnnotations.Create(refResultType, lValueAnnotation));
return null;
}
Expand Down Expand Up @@ -10890,7 +10915,8 @@ public override void VisitForEachIterationVariables(BoundForEachStatement node)
if (iterationVariable.IsRef)
{
// foreach (ref DestinationType variable in collection)
if (IsNullabilityMismatch(sourceType, destinationType))
if (node.Expression is not BoundConversion { Operand.IsSuppressed: true } &&
IsNullabilityMismatch(sourceType, destinationType))
{
var foreachSyntax = (ForEachStatementSyntax)node.Syntax;
ReportNullabilityMismatchInAssignment(foreachSyntax.Type, sourceType, destinationType);
Expand Down
Loading