Skip to content

Commit

Permalink
Non-nullable fields of default struct value should be treated as null…
Browse files Browse the repository at this point in the history 
…able (#32304)
  • Loading branch information
@cston
cston authored Jan 11, 2019
1 parent 00cbaf0 commit a2d838e
Show file tree
Hide file tree
Showing 5 changed files with 1,145 additions and 112 deletions.
19 changes: 15 additions & 4 deletions docs/features/nullable-reference-types.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,25 @@ Invocation of methods annotated with the following attributes will also affect f
- `[AssertsTrue]` (e.g. `Debug.Assert`) and `[AssertsFalse]`

## `default`
`default(T)` is `T?` if `T` is a reference type.
_Is `default(T)` also `T?` if `T` is an unconstrained type parameter?_
_Is `default(T?)` an error?_
If `T` is a reference type, `default(T)` is `T?`.
```c#
string? s = default(string); // assigns ?, no warning
string t = default; // assigns ?, warning
T t = default; // assigns ?, warning
```
If `T` is a value type, `default(T)` is `T` and any non-value-type fields in `T` are maybe null.
If `T` is a value type, `new T()` is equivalent to `default(T)`.
```c#
struct Pair<T, U> { public T First; public U Second; }
var p = default(Pair<object?, string>); // ok: Pair<object?, string!> p
p.Second.ToString(); // warning
(object?, string) t = default; // ok
t.Item2.ToString(); // warning
```
If `T` is an unconstrained type parameter, `default(T)` is a `T` that is maybe null.
```c#
T t = default; // warning
```
_Is `default(T?)` an error?_

### Conversions
Conversions can be calculated with ~ considered distinct from ? and !, or with ~ implicitly convertible to ? and !.
Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.VariableIdentifier.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ namespace Microsoft.CodeAnalysis.CSharp
{
internal partial class LocalDataFlowPass<TLocalState>
{
[DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
internal struct VariableIdentifier : IEquatable<VariableIdentifier>
{
public readonly Symbol Symbol;
Expand Down Expand Up @@ -49,7 +48,7 @@ public override bool Equals(object obj)
return !left.Equals(right);
}

internal string GetDebuggerDisplay()
public override string ToString()
{
return $"ContainingSlot={ContainingSlot}, Symbol={Symbol.GetDebuggerDisplay()}";
}
Expand Down
125 changes: 82 additions & 43 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ protected override int MakeSlot(BoundExpression node)
}
}
break;
case BoundKind.DefaultExpression:
case BoundKind.ObjectCreationExpression:
case BoundKind.DynamicObjectCreationExpression:
case BoundKind.AnonymousObjectCreationExpression:
Expand All @@ -445,15 +446,16 @@ protected override int MakeSlot(BoundExpression node)
}
break;
case BoundKind.ConditionalReceiver:
if (_lastConditionalAccessSlot != -1)
{
int slot = _lastConditionalAccessSlot;
_lastConditionalAccessSlot = -1;
return slot;
}
break;
default:
return base.MakeSlot(node);
}
return base.MakeSlot(node);

return -1;

MethodSymbol getTopLevelMethod(MethodSymbol method)
{
Expand Down Expand Up @@ -587,7 +589,7 @@ private bool ReportNullableAssignmentIfNecessary(BoundExpression value, TypeSymb
// target (e.g.: `object x = null;` or calling `void F(object y)` with `F(null)`).
bool reportNullLiteralAssignmentIfNecessary(BoundExpression expr)
{
if (expr.ConstantValue?.IsNull != true && !isDefaultOfUnconstrainedTypeParameter(expr))
if (expr.ConstantValue?.IsNull != true && !IsDefaultOfUnconstrainedTypeParameter(expr))
{
return false;
}
Expand All @@ -602,22 +604,39 @@ bool reportNullLiteralAssignmentIfNecessary(BoundExpression expr)
}
return true;
}
}

bool isDefaultOfUnconstrainedTypeParameter(BoundExpression expr)
private static bool IsDefaultOfUnconstrainedTypeParameter(BoundExpression expr)
{
switch (expr.Kind)
{
switch (expr.Kind)
{
case BoundKind.Conversion:
{
var conversion = (BoundConversion)expr;
return conversion.Conversion.Kind == ConversionKind.DefaultOrNullLiteral &&
isDefaultOfUnconstrainedTypeParameter(conversion.Operand);
}
case BoundKind.DefaultExpression:
return IsTypeParameterDisallowingAnnotation(expr.Type);
default:
return false;
}
case BoundKind.Conversion:
{
var conversion = (BoundConversion)expr;
return conversion.Conversion.Kind == ConversionKind.DefaultOrNullLiteral &&
IsDefaultOfUnconstrainedTypeParameter(conversion.Operand);
}
case BoundKind.DefaultExpression:
return IsTypeParameterDisallowingAnnotation(expr.Type);
default:
return false;
}
}

private static bool IsDefaultValue(BoundExpression expr)
{
switch (expr.Kind)
{
case BoundKind.Conversion:
{
var conversion = (BoundConversion)expr;
return conversion.Conversion.Kind == ConversionKind.DefaultOrNullLiteral &&
IsDefaultValue(conversion.Operand);
}
case BoundKind.DefaultExpression:
return true;
default:
return false;
}
}

Expand Down Expand Up @@ -709,9 +728,9 @@ private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWi
}
}
else if (EmptyStructTypeCache.IsTrackableStructType(targetType.TypeSymbol) &&
targetType.TypeSymbol.Equals(valueType.TypeSymbol, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
targetType.TypeSymbol.Equals(valueType.TypeSymbol, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))
{
InheritNullableStateOfTrackableStruct(targetType.TypeSymbol, targetSlot, valueSlot, IsByRefTarget(targetSlot), slotWatermark: GetSlotWatermark());
InheritNullableStateOfTrackableStruct(targetType.TypeSymbol, targetSlot, valueSlot, isDefaultValue: IsDefaultValue(value), isByRefTarget: IsByRefTarget(targetSlot), slotWatermark: GetSlotWatermark());
}
}
}
Expand Down Expand Up @@ -771,7 +790,7 @@ private void ReportDiagnostic(ErrorCode errorCode, SyntaxNode syntaxNode, params
}
}

private void InheritNullableStateOfTrackableStruct(TypeSymbol targetType, int targetSlot, int valueSlot, bool isByRefTarget, int slotWatermark)
private void InheritNullableStateOfTrackableStruct(TypeSymbol targetType, int targetSlot, int valueSlot, bool isDefaultValue, bool isByRefTarget, int slotWatermark)
{
Debug.Assert(targetSlot > 0);
Debug.Assert(EmptyStructTypeCache.IsTrackableStructType(targetType));
Expand All @@ -780,13 +799,14 @@ private void InheritNullableStateOfTrackableStruct(TypeSymbol targetType, int ta
// See ModifyMembers_StructPropertyNoBackingField and PropertyCycle_Struct tests.
foreach (var field in _emptyStructTypeCache.GetStructInstanceFields(targetType))
{
InheritNullableStateOfMember(targetSlot, valueSlot, field, isByRefTarget, slotWatermark);
InheritNullableStateOfMember(targetSlot, valueSlot, field, isDefaultValue: isDefaultValue, isByRefTarget: isByRefTarget, slotWatermark);
}
}

// 'slotWatermark' is used to avoid inheriting members from inherited members.
private void InheritNullableStateOfMember(int targetContainerSlot, int valueContainerSlot, Symbol member, bool isByRefTarget, int slotWatermark)
private void InheritNullableStateOfMember(int targetContainerSlot, int valueContainerSlot, Symbol member, bool isDefaultValue, bool isByRefTarget, int slotWatermark)
{
Debug.Assert(targetContainerSlot > 0);
Debug.Assert(valueContainerSlot <= slotWatermark);

TypeSymbolWithAnnotations fieldOrPropertyType = member.GetTypeOrReturnType();
Expand All @@ -797,7 +817,9 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont
if (fieldOrPropertyType.IsReferenceType || fieldOrPropertyType.IsNullableType())
{
int targetMemberSlot = GetOrCreateSlot(member, targetContainerSlot);
NullableAnnotation value = fieldOrPropertyType.NullableAnnotation;
NullableAnnotation value = (isDefaultValue && fieldOrPropertyType.IsReferenceType) ?
NullableAnnotation.Nullable :
fieldOrPropertyType.NullableAnnotation;
// https://github.com/dotnet/roslyn/issues/29968 Remove isByRefTarget check?
if (isByRefTarget)
{
Expand Down Expand Up @@ -838,7 +860,7 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont
valueMemberSlot = slot;
}
}
InheritNullableStateOfTrackableStruct(fieldOrPropertyType.TypeSymbol, targetMemberSlot, valueMemberSlot, isByRefTarget, slotWatermark);
InheritNullableStateOfTrackableStruct(fieldOrPropertyType.TypeSymbol, targetMemberSlot, valueMemberSlot, isDefaultValue: isDefaultValue, isByRefTarget: isByRefTarget, slotWatermark);
}
}
}
Expand Down Expand Up @@ -875,7 +897,7 @@ private void InheritNullableStateOfTrackableType(int targetSlot, int valueSlot,
}
var member = variable.Symbol;
Debug.Assert(member.Kind == SymbolKind.Field || member.Kind == SymbolKind.Property || member.Kind == SymbolKind.Event);
InheritNullableStateOfMember(targetSlot, valueSlot, member, isByRefTarget, slotWatermark);
InheritNullableStateOfMember(targetSlot, valueSlot, member, isDefaultValue: false, isByRefTarget, slotWatermark);
}
}

Expand Down Expand Up @@ -928,13 +950,17 @@ private void EnterParameter(ParameterSymbol parameter, TypeSymbolWithAnnotations
{
if (EmptyStructTypeCache.IsTrackableStructType(parameterType.TypeSymbol))
{
InheritNullableStateOfTrackableStruct(parameterType.TypeSymbol, slot, valueSlot: -1, isByRefTarget: parameter.RefKind != RefKind.None, slotWatermark: GetSlotWatermark());
InheritNullableStateOfTrackableStruct(
parameterType.TypeSymbol,
slot,
valueSlot: -1,
isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true,
isByRefTarget: parameter.RefKind != RefKind.None,
slotWatermark: GetSlotWatermark());
}
}
}

#region Visitors

public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node)
{
VisitRvalue(node.Expression);
Expand Down Expand Up @@ -1182,27 +1208,31 @@ private void VisitObjectOrDynamicObjectCreation(BoundExpression node, BoundExpre
{
Debug.Assert(node.Kind == BoundKind.ObjectCreationExpression || node.Kind == BoundKind.DynamicObjectCreationExpression);

LocalSymbol receiver = null;
int slot = -1;
TypeSymbol type = node.Type;
if ((object)type != null)
{
bool isTrackableStructType = EmptyStructTypeCache.IsTrackableStructType(type);
if (!type.IsValueType || isTrackableStructType)
{
receiver = GetOrCreateObjectCreationPlaceholder(node);
slot = GetOrCreateSlot(receiver);
slot = GetOrCreateObjectCreationPlaceholderSlot(node);
if (slot > 0 && isTrackableStructType)
{
this.State[slot] = NullableAnnotation.NotNullable;
InheritNullableStateOfTrackableStruct(type, slot, valueSlot: -1, isByRefTarget: false, slotWatermark: GetSlotWatermark());
InheritNullableStateOfTrackableStruct(
type,
slot,
valueSlot: -1,
isDefaultValue: (node as BoundObjectCreationExpression)?.Constructor.IsDefaultValueTypeConstructor() == true,
isByRefTarget: false,
slotWatermark: GetSlotWatermark());
}
}
}

if (initializerOpt != null)
{
VisitObjectCreationInitializer(receiver, slot, initializerOpt);
VisitObjectCreationInitializer(null, slot, initializerOpt);
}

_resultType = TypeSymbolWithAnnotations.Create(type, NullableAnnotation.NotNullable);
Expand All @@ -1218,7 +1248,7 @@ private void VisitObjectCreationInitializer(Symbol containingSymbol, int contain
switch (initializer.Kind)
{
case BoundKind.AssignmentOperator:
VisitObjectElementInitializer(containingSymbol, containingSlot, (BoundAssignmentOperator)initializer);
VisitObjectElementInitializer(containingSlot, (BoundAssignmentOperator)initializer);
break;
default:
VisitRvalue(initializer);
Expand All @@ -1242,6 +1272,7 @@ private void VisitObjectCreationInitializer(Symbol containingSymbol, int contain
break;
default:
TypeSymbolWithAnnotations resultType = VisitRvalueWithResult(node);
Debug.Assert((object)containingSymbol != null);
if ((object)containingSymbol != null)
{
var type = containingSymbol.GetTypeOrReturnType();
Expand All @@ -1252,7 +1283,7 @@ private void VisitObjectCreationInitializer(Symbol containingSymbol, int contain
}
}

private void VisitObjectElementInitializer(Symbol containingSymbol, int containingSlot, BoundAssignmentOperator node)
private void VisitObjectElementInitializer(int containingSlot, BoundAssignmentOperator node)
{
var left = node.Left;
switch (left.Kind)
Expand Down Expand Up @@ -1290,7 +1321,7 @@ private void SetResult(BoundExpression node)
_resultType = TypeSymbolWithAnnotations.Create(node.Type);
}

private ObjectCreationPlaceholderLocal GetOrCreateObjectCreationPlaceholder(BoundExpression node)
private int GetOrCreateObjectCreationPlaceholderSlot(BoundExpression node)
{
ObjectCreationPlaceholderLocal placeholder;
if (_placeholderLocalsOpt == null)
Expand All @@ -1309,7 +1340,7 @@ private ObjectCreationPlaceholderLocal GetOrCreateObjectCreationPlaceholder(Boun
_placeholderLocalsOpt.Add(node, placeholder);
}

return placeholder;
return GetOrCreateSlot(placeholder);
}

public override BoundNode VisitAnonymousObjectCreationExpression(BoundAnonymousObjectCreationExpression node)
Expand Down Expand Up @@ -1337,8 +1368,7 @@ public override BoundNode VisitAnonymousObjectCreationExpression(BoundAnonymousO
PropertySymbol property = node.Declarations[i].Property;
if (receiverSlot <= 0)
{
ObjectCreationPlaceholderLocal implicitReceiver = GetOrCreateObjectCreationPlaceholder(node);
receiverSlot = GetOrCreateSlot(implicitReceiver);
receiverSlot = GetOrCreateObjectCreationPlaceholderSlot(node);
}

TypeSymbolWithAnnotations propertyType = property.Type;
Expand Down Expand Up @@ -3558,7 +3588,7 @@ private TypeSymbolWithAnnotations ApplyConversion(

// conversion "from" type -> method parameter type
NullableAnnotation operandAnnotation = operandType.NullableAnnotation;
operandType = ClassifyAndApplyConversion(operandOpt ?? node, parameterType, isLiftedConversion ? underlyingOperandType : operandType,
_ = ClassifyAndApplyConversion(operandOpt ?? node, parameterType, isLiftedConversion ? underlyingOperandType : operandType,
useLegacyWarnings, AssignmentKind.Argument, target: parameter, reportWarnings: reportRemainingWarnings);

// method parameter type -> method return type
Expand Down Expand Up @@ -4638,8 +4668,19 @@ public override BoundNode VisitFieldInfo(BoundFieldInfo node)

public override BoundNode VisitDefaultExpression(BoundDefaultExpression node)
{
Debug.Assert(!this.IsConditionalState);

var result = base.VisitDefaultExpression(node);
TypeSymbol type = node.Type;
if (EmptyStructTypeCache.IsTrackableStructType(type))
{
int slot = GetOrCreateObjectCreationPlaceholderSlot(node);
if (slot > 0)
{
this.State[slot] = NullableAnnotation.NotNullable;
InheritNullableStateOfTrackableStruct(type, slot, valueSlot: -1, isDefaultValue: true, isByRefTarget: false, slotWatermark: GetSlotWatermark());
}
}
_resultType = TypeSymbolWithAnnotations.Create(type, (type is null || type.IsNullableType() || !type.IsValueType) ? NullableAnnotation.Nullable : NullableAnnotation.Unknown);
return result;
}
Expand Down Expand Up @@ -5150,8 +5191,6 @@ public override BoundNode VisitYieldReturnStatement(BoundYieldReturnStatement no
return null;
}

#endregion Visitors

protected override string Dump(LocalState state)
{
return string.Empty;
Expand Down
Loading

0 comments on commit a2d838e

Please sign in to comment.