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.

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

Short term field keyword nullable analysis #75246

Merged
merged 8 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7995,4 +7995,11 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_InlineArrayAttributeOnRecord" xml:space="preserve">
<value>Attribute 'System.Runtime.CompilerServices.InlineArray' cannot be applied to a record struct.</value>
</data>
<data name="WRN_UninitializedNonNullableBackingField" xml:space="preserve">
<value>Non-nullable {0} '{1}' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier, or declaring the {0} as nullable, or adding '[field: MaybeNull, AllowNull]' attributes.</value>
<comment>Similar diagnostic message as 'WRN_UninitializedNonNullableField'</comment>
</data>
<data name="WRN_UninitializedNonNullableBackingField_Title" xml:space="preserve">
<value>Non-nullable property must contain a non-null value when exiting constructor. Consider adding the 'required' modifier, or declaring the property as nullable, or adding '[field: MaybeNull, AllowNull]' attributes.</value>
</data>
</root>
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2345,6 +2345,8 @@ internal enum ErrorCode
ERR_CannotApplyOverloadResolutionPriorityToMember = 9262,
ERR_PartialPropertyDuplicateInitializer = 9263,

WRN_UninitializedNonNullableBackingField = 9264,

// Note: you will need to do the following after adding errors:
// 1) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ static ErrorFacts()
nullableWarnings.Add(GetId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnInterceptor));
nullableWarnings.Add(GetId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnInterceptor));

nullableWarnings.Add(GetId(ErrorCode.WRN_UninitializedNonNullableBackingField));
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved

NullableWarnings = nullableWarnings.ToImmutable();
}

Expand Down Expand Up @@ -557,6 +559,7 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_ConvertingLock:
case ErrorCode.WRN_PartialPropertySignatureDifference:
case ErrorCode.WRN_FieldIsAmbiguous:
case ErrorCode.WRN_UninitializedNonNullableBackingField:
return 1;
default:
return 0;
Expand Down Expand Up @@ -2456,6 +2459,7 @@ or ErrorCode.ERR_FeatureNotAvailableInVersion13
or ErrorCode.ERR_CannotApplyOverloadResolutionPriorityToOverride
or ErrorCode.ERR_CannotApplyOverloadResolutionPriorityToMember
or ErrorCode.ERR_PartialPropertyDuplicateInitializer
or ErrorCode.WRN_UninitializedNonNullableBackingField
=> false,
};
#pragma warning restore CS8524 // The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value.
Expand Down
48 changes: 32 additions & 16 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,10 @@ void checkMemberStateOnConstructorExit(MethodSymbol constructor, Symbol member,
return;
}

var annotations = symbol.GetFlowAnalysisAnnotations();
// If 'field' keyword is explicitly used by 'symbol', then use FlowAnalysisAnnotations from the backing field.
// Otherwise, use the FlowAnalysisAnnotations from the user-declared symbol (property or ordinary field).
var usesFieldKeyword = symbol is SourcePropertySymbolBase { UsesFieldKeyword: true };
var annotations = usesFieldKeyword ? field!.FlowAnalysisAnnotations : symbol.GetFlowAnalysisAnnotations();
if ((annotations & FlowAnalysisAnnotations.AllowNull) != 0)
{
// We assume that if a member has AllowNull then the user
Expand All @@ -782,7 +785,8 @@ void checkMemberStateOnConstructorExit(MethodSymbol constructor, Symbol member,
: NullableFlowState.MaybeNull;
if (memberState >= badState) // is 'memberState' as bad as or worse than 'badState'?
{
var info = new CSDiagnosticInfo(ErrorCode.WRN_UninitializedNonNullableField, new object[] { symbol.Kind.Localize(), symbol.Name }, ImmutableArray<Symbol>.Empty, additionalLocations: symbol.Locations);
var errorCode = usesFieldKeyword ? ErrorCode.WRN_UninitializedNonNullableBackingField : ErrorCode.WRN_UninitializedNonNullableField;
var info = new CSDiagnosticInfo(errorCode, new object[] { symbol.Kind.Localize(), symbol.Name }, ImmutableArray<Symbol>.Empty, additionalLocations: symbol.Locations);
Diagnostics.Add(info, exitLocation ?? symbol.GetFirstLocationOrNone());
}
}
Expand Down Expand Up @@ -920,7 +924,7 @@ void makeNotNullMembersMaybeNull()
continue;
case FieldSymbol { IsConst: true }:
continue;
case FieldSymbol { AssociatedSymbol: PropertySymbol prop }:
case FieldSymbol { AssociatedSymbol: SourcePropertySymbolBase { UsesFieldKeyword: false } prop }:
// this is a property where assigning 'default' causes us to simply update
// the state to the output state of the property
// thus we skip setting an initial state for it here
Expand Down Expand Up @@ -2130,6 +2134,30 @@ protected override int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, bo
}
}

// Share a slot between backing field and associated property/event in the context of a constructor which owns initialization of that backing field.
if (this._symbol is MethodSymbol constructor
&& constructor.IsConstructor()
&& constructor.IsStatic == symbol.IsStatic)
Copy link
Member

Choose a reason for hiding this comment

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

constructor.IsStatic == symbol.IsStatic

Are we testing this case? That is, are we testing use of static property in an instance constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is only really going to be relevant in a scenario like the following:

  1. static property is used in instance constructor, and
  2. somehow, backing field of the same static property is also used in instance constructor. I think this is not possible.

I am adding some tests to poke at that area, but, even if the tests don't show the necessity of the check, I think I will feel more comfortable having it there.

{
if ((constructor.IsStatic && containingSlot == 0 && constructor.ContainingType.Equals(symbol.ContainingType))
|| (!constructor.IsStatic && containingSlot > 0 && _variables[containingSlot].Symbol is ThisParameterSymbol))
{
// If symbol is a backing field, but property does not use the field keyword,
// then use the property to determine initial state and to own the slot.
// Example scenarios:
// - property initializer on normal auto-property
// - property assignment on getter-only auto-property.
// Example test: NullableReferenceTypesTests.ConstructorUsesStateFromInitializers will fail without this.
if (symbol is SynthesizedBackingFieldSymbol { AssociatedSymbol: SourcePropertySymbolBase { UsesFieldKeyword: false } property })
symbol = property;
// If symbol is a property that uses field keyword, then use field to determine initial state and to own the slot.
else if (symbol is SourcePropertySymbolBase { UsesFieldKeyword: true, BackingField: { } backingField })
symbol = backingField;
else if (symbol is SourceEventFieldSymbol eventField)
symbol = eventField.AssociatedSymbol;
}
}

return base.GetOrCreateSlot(symbol, containingSlot, forceSlotEvenIfEmpty, createIfMissing);
}

Expand Down Expand Up @@ -9767,18 +9795,6 @@ private void VisitThisOrBaseReference(BoundExpression node)
Debug.Assert(!IsConditionalState);

var left = node.Left;
switch (left)
{
// when binding initializers, we treat assignments to auto-properties or field-like events as direct assignments to the underlying field.
// in order to track member state based on these initializers, we need to see the assignment in terms of the associated member
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: PropertySymbol autoProperty }, Syntax: not FieldExpressionSyntax } fieldAccess:
left = new BoundPropertyAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, autoProperty, autoPropertyAccessorKind: AccessorKind.Unknown, LookupResultKind.Viable, autoProperty.Type, fieldAccess.HasErrors);
break;
case BoundFieldAccess { ExpressionSymbol: FieldSymbol { AssociatedSymbol: EventSymbol @event } } fieldAccess:
left = new BoundEventAccess(fieldAccess.Syntax, fieldAccess.ReceiverOpt, @event, isUsableAsField: true, LookupResultKind.Viable, @event.Type, fieldAccess.HasErrors);
break;
}

var right = node.Right;
VisitLValue(left);
// we may enter a conditional state for error scenarios on the LHS.
Expand Down Expand Up @@ -9891,7 +9907,7 @@ private FlowAnalysisAnnotations GetLValueAnnotations(BoundExpression expr)

private static FlowAnalysisAnnotations GetFieldAnnotations(FieldSymbol field)
{
return field.AssociatedSymbol is PropertySymbol property ?
return field.AssociatedSymbol is SourcePropertySymbolBase { UsesFieldKeyword: false } property ?
property.GetFlowAnalysisAnnotations() :
field.FlowAnalysisAnnotations;
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading