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 6 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,32 @@ 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);

// Prefer "not null" (correct if the operand is suppressed, otherwise we have warned above).
lValueAnnotation = consequenceLValue.NullableAnnotation.Meet(alternativeLValue.NullableAnnotation);
rValueState = consequenceRValue.State.Meet(alternativeRValue.State);
Copy link
Member

@jcouv jcouv Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The usage of Meet for result state is surprising, I would have expected a Join which is used when two branches join.
For the annotation, I think EnsureCompatible is correct (that's what we use when merging type arguments when there is no variance).
So I think we could remove those two lines and just rely on the logic above (initialization of lValueAnnotation and rValueState at lines 5788 and 5789). Anything wrong happens if we do that? #Closed

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 makes sense, thanks. I must have had a mistake in the code previously when I tried that (I was seeing some weird nullability warnings).

}
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 @@ -5957,6 +5974,14 @@ void addConvertArmsAsCompletion(
TypeWithAnnotations lValueType = VisitLvalueWithAnnotations(operand);
return (lValueType, ResultType);
}

void reportMismatchIfNecessary(BoundExpression node, TypeWithAnnotations source, TypeWithAnnotations destination)
{
if (!node.IsSuppressed && IsNullabilityMismatch(source, destination))
{
ReportNullabilityMismatchInAssignment(node.Syntax, source, destination);
}
}
}

private TypeWithState ConvertConditionalOperandOrSwitchExpressionArmResult(
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