From 4a94aece4fe0adaa0ba3e6739647b35e1578e19b Mon Sep 17 00:00:00 2001 From: Charles Stoner Date: Tue, 19 Feb 2019 21:23:52 -0800 Subject: [PATCH] Replace slot watermark (#33455) --- .../Portable/FlowAnalysis/NullableWalker.cs | 127 +++++++++++------- .../Semantics/NullableReferenceTypesTests.cs | 70 +++++++++- 2 files changed, 141 insertions(+), 56 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index e07ff9af35d..120bb3e51a1 100755 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -750,7 +750,7 @@ private void ReportNullabilityMismatchInAssignment(SyntaxNode syntaxNode, object /// /// Update tracked value on assignment. /// - private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWithAnnotations targetType, int targetSlot, TypeSymbolWithAnnotations valueType, int valueSlot = -1, int slotWatermark = -1) + private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWithAnnotations targetType, int targetSlot, TypeSymbolWithAnnotations valueType, int valueSlot = -1) { Debug.Assert(value != null); Debug.Assert(!IsConditionalState); @@ -780,11 +780,6 @@ private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWi InheritDefaultState(targetSlot); - if (slotWatermark < 0) - { - slotWatermark = GetSlotWatermark(); - } - // https://github.com/dotnet/roslyn/issues/33428: Can the areEquivalentTypes check be removed // if InheritNullableStateOfMember asserts the member is valid for target and value? if (areEquivalentTypes(targetType, valueType)) // https://github.com/dotnet/roslyn/issues/29968 Allow assignment to base type. @@ -796,12 +791,12 @@ private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWi // When that issue is fixed, Nullable should be handled there instead. if (valueSlot > 0) { - InheritNullableStateOfTrackableType(targetSlot, valueSlot, slotWatermark); + InheritNullableStateOfTrackableType(targetSlot, valueSlot, skipSlot: targetSlot); } } else if (EmptyStructTypeCache.IsTrackableStructType(targetType.TypeSymbol)) { - InheritNullableStateOfTrackableStruct(targetType.TypeSymbol, targetSlot, valueSlot, isDefaultValue: IsDefaultValue(value), slotWatermark); + InheritNullableStateOfTrackableStruct(targetType.TypeSymbol, targetSlot, valueSlot, isDefaultValue: IsDefaultValue(value), skipSlot: targetSlot); } } } @@ -810,8 +805,6 @@ private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWi t1.TypeSymbol.Equals(t2.TypeSymbol, TypeCompareKind.AllIgnoreOptions); } - private int GetSlotWatermark() => this.nextVariableSlot; - private void ReportNonSafetyDiagnostic(SyntaxNode syntax) { ReportNonSafetyDiagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, syntax); @@ -846,24 +839,29 @@ private void ReportDiagnostic(ErrorCode errorCode, SyntaxNode syntaxNode, params } } - private void InheritNullableStateOfTrackableStruct(TypeSymbol targetType, int targetSlot, int valueSlot, bool isDefaultValue, int slotWatermark) + private void InheritNullableStateOfTrackableStruct(TypeSymbol targetType, int targetSlot, int valueSlot, bool isDefaultValue, int skipSlot = -1) { Debug.Assert(targetSlot > 0); Debug.Assert(EmptyStructTypeCache.IsTrackableStructType(targetType)); + if (skipSlot < 0) + { + skipSlot = targetSlot; + } + // https://github.com/dotnet/roslyn/issues/29619 Handle properties not backed by fields. // See ModifyMembers_StructPropertyNoBackingField and PropertyCycle_Struct tests. foreach (var field in _emptyStructTypeCache.GetStructInstanceFields(targetType)) { - InheritNullableStateOfMember(targetSlot, valueSlot, field, isDefaultValue: isDefaultValue, slotWatermark); + InheritNullableStateOfMember(targetSlot, valueSlot, field, isDefaultValue: isDefaultValue, skipSlot); } } - // 'slotWatermark' is used to avoid inheriting members from inherited members. - private void InheritNullableStateOfMember(int targetContainerSlot, int valueContainerSlot, Symbol member, bool isDefaultValue, int slotWatermark) + // 'skipSlot' is the original target slot that should be skipped in case of cycles. + private void InheritNullableStateOfMember(int targetContainerSlot, int valueContainerSlot, Symbol member, bool isDefaultValue, int skipSlot) { Debug.Assert(targetContainerSlot > 0); - Debug.Assert(valueContainerSlot <= slotWatermark); + Debug.Assert(skipSlot > 0); // https://github.com/dotnet/roslyn/issues/33428: Ensure member is valid for target and value. TypeSymbolWithAnnotations fieldOrPropertyType = member.GetTypeOrReturnType(); @@ -874,12 +872,20 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont if (fieldOrPropertyType.IsReferenceType || fieldOrPropertyType.IsPossiblyNullableReferenceTypeTypeParameter() || fieldOrPropertyType.IsNullableType()) { int targetMemberSlot = GetOrCreateSlot(member, targetContainerSlot); + Debug.Assert(targetMemberSlot > 0); + NullableAnnotation value = (isDefaultValue && fieldOrPropertyType.IsReferenceType) ? NullableAnnotation.Nullable : fieldOrPropertyType.NullableAnnotation; + int valueMemberSlot = -1; + if (valueContainerSlot > 0) { - int valueMemberSlot = VariableSlot(member, valueContainerSlot); + valueMemberSlot = VariableSlot(member, valueContainerSlot); + if (valueMemberSlot == skipSlot) + { + return; + } value = valueMemberSlot > 0 && valueMemberSlot < this.State.Capacity ? this.State[valueMemberSlot] : NullableAnnotation.Unknown; @@ -887,13 +893,9 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont this.State[targetMemberSlot] = value; - if (valueContainerSlot > 0) + if (valueMemberSlot > 0) { - int valueMemberSlot = VariableSlot(member, valueContainerSlot); - if (valueMemberSlot > 0 && valueMemberSlot < slotWatermark) - { - InheritNullableStateOfTrackableType(targetMemberSlot, valueMemberSlot, slotWatermark); - } + InheritNullableStateOfTrackableType(targetMemberSlot, valueMemberSlot, skipSlot); } } else if (EmptyStructTypeCache.IsTrackableStructType(fieldOrPropertyType.TypeSymbol)) @@ -901,16 +903,12 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont int targetMemberSlot = GetOrCreateSlot(member, targetContainerSlot); if (targetMemberSlot > 0) { - int valueMemberSlot = -1; - if (valueContainerSlot > 0) + int valueMemberSlot = (valueContainerSlot > 0) ? GetOrCreateSlot(member, valueContainerSlot) : -1; + if (valueMemberSlot == skipSlot) { - int slot = GetOrCreateSlot(member, valueContainerSlot); - if (slot < slotWatermark) - { - valueMemberSlot = slot; - } + return; } - InheritNullableStateOfTrackableStruct(fieldOrPropertyType.TypeSymbol, targetMemberSlot, valueMemberSlot, isDefaultValue: isDefaultValue, slotWatermark); + InheritNullableStateOfTrackableStruct(fieldOrPropertyType.TypeSymbol, targetMemberSlot, valueMemberSlot, isDefaultValue: isDefaultValue, skipSlot); } } } @@ -932,13 +930,13 @@ private void InheritDefaultState(int targetSlot) } } - private void InheritNullableStateOfTrackableType(int targetSlot, int valueSlot, int slotWatermark) + private void InheritNullableStateOfTrackableType(int targetSlot, int valueSlot, int skipSlot) { Debug.Assert(targetSlot > 0); Debug.Assert(valueSlot > 0); // Clone the state for members that have been set on the value. - for (int slot = valueSlot + 1; slot < slotWatermark; slot++) + for (int slot = valueSlot + 1; slot < nextVariableSlot; slot++) { var variable = variableBySlot[slot]; if (variable.ContainingSlot != valueSlot) @@ -947,7 +945,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, isDefaultValue: false, slotWatermark); + InheritNullableStateOfMember(targetSlot, valueSlot, member, isDefaultValue: false, skipSlot); } } @@ -1004,8 +1002,7 @@ private void EnterParameter(ParameterSymbol parameter, TypeSymbolWithAnnotations parameterType.TypeSymbol, slot, valueSlot: -1, - isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true, - slotWatermark: GetSlotWatermark()); + isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true); } } } @@ -1293,8 +1290,7 @@ public override BoundNode VisitObjectCreationExpression(BoundObjectCreationExpre type, slot, valueSlot: -1, - isDefaultValue: isDefaultValueTypeConstructor, - slotWatermark: GetSlotWatermark()); + isDefaultValue: isDefaultValueTypeConstructor); } } } @@ -4227,11 +4223,7 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node) else { TypeSymbolWithAnnotations rightType = VisitOptionalImplicitConversion(right, leftType, UseLegacyWarnings(left), AssignmentKind.Assignment); - int rightSlot = MakeSlot(right); - int slotWatermark = GetSlotWatermark(); - // leftSlot is calculated last to avoid copying from that slot in cycles. - int leftSlot = MakeSlot(left); - TrackNullableStateForAssignment(right, leftType, leftSlot, rightType, rightSlot, slotWatermark); + TrackNullableStateForAssignment(right, leftType, MakeSlot(left), rightType, MakeSlot(right)); // https://github.com/dotnet/roslyn/issues/30066 Check node.Type.IsErrorType() instead? _resultType = node.HasErrors ? TypeSymbolWithAnnotations.Create(node.Type) : rightType; } @@ -4336,21 +4328,58 @@ private void VisitDeconstructionArguments(ArrayBuilder v for (int i = 0; i < n; i++) { var variable = variables[i]; + var underlyingConversion = conversion.UnderlyingConversions[i]; var rightPart = rightParts[i]; var nestedVariables = variable.NestedVariables; if (nestedVariables != null) { - VisitDeconstructionArguments(nestedVariables, conversion.UnderlyingConversions[i], rightPart); + VisitDeconstructionArguments(nestedVariables, underlyingConversion, rightPart); } else { var targetType = variable.Type; - var valueType = VisitOptionalImplicitConversion(rightPart, targetType, useLegacyWarnings: true, AssignmentKind.Assignment); - int valueSlot = MakeSlot(rightPart); - int slotWatermark = GetSlotWatermark(); - // targetSlot is calculated last to avoid copying from that slot in cycles. + TypeSymbolWithAnnotations operandType; + TypeSymbolWithAnnotations valueType; + int valueSlot; + if (underlyingConversion.IsIdentity) + { + operandType = default; + valueType = VisitOptionalImplicitConversion(rightPart, targetType, useLegacyWarnings: true, AssignmentKind.Assignment); + valueSlot = MakeSlot(rightPart); + } + else + { + operandType = VisitRvalueWithResult(rightPart); + valueType = ApplyConversion( + rightPart, + rightPart, + underlyingConversion, + targetType, + operandType, + checkConversion: true, + fromExplicitCast: false, + useLegacyWarnings: true, + AssignmentKind.Assignment, + reportTopLevelWarnings: true, + reportRemainingWarnings: true); + valueSlot = -1; + } + int targetSlot = MakeSlot(variable.Expression); - TrackNullableStateForAssignment(rightPart, targetType, targetSlot, valueType, valueSlot, slotWatermark); + TrackNullableStateForAssignment(rightPart, targetType, targetSlot, valueType, valueSlot); + + // Conversion of T to Nullable is equivalent to new Nullable(t). + // (Should this check be moved to VisitOptionalImplicitConversion or TrackNullableStateForAssignment?) + if (targetSlot > 0 && + underlyingConversion.Kind == ConversionKind.ImplicitNullable && + AreNullableAndUnderlyingTypes(targetType.TypeSymbol, operandType.TypeSymbol, out TypeSymbolWithAnnotations underlyingType)) + { + valueSlot = MakeSlot(rightPart); + if (valueSlot > 0) + { + TrackNullableStateOfNullableValue(targetSlot, targetType.TypeSymbol, rightPart, underlyingType, valueSlot); + } + } } } } @@ -5085,7 +5114,7 @@ public override BoundNode VisitDefaultExpression(BoundDefaultExpression node) if (slot > 0) { this.State[slot] = NullableAnnotation.NotNullable; - InheritNullableStateOfTrackableStruct(type, slot, valueSlot: -1, isDefaultValue: true, slotWatermark: GetSlotWatermark()); + InheritNullableStateOfTrackableStruct(type, slot, valueSlot: -1, isDefaultValue: true); } } _resultType = TypeSymbolWithAnnotations.Create(type, (type is null || type.IsNullableType() || !type.IsValueType) ? NullableAnnotation.Nullable : NullableAnnotation.Unknown); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 47abaf6d5d0..8bbeac37ae2 100755 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -49170,6 +49170,34 @@ static void M(C x) Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x.F.F").WithLocation(7, 9)); } + [Fact] + public void Members_FieldCycle_06() + { + var source = +@"#pragma warning disable 0649 +class A +{ + internal B B = default!; +} +class B +{ + internal A? A; +} +class Program +{ + static void M(A a) + { + a.B.A = a; + a.B.A.B.A.ToString(); + } +}"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (15,9): warning CS8602: Possible dereference of a null reference. + // a.B.A.B.A.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "a.B.A.B.A").WithLocation(15, 9)); + } + [Fact] public void Members_FieldCycle_Struct() { @@ -79477,14 +79505,42 @@ static void F(S s) } }"; var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); - // https://github.com/dotnet/roslyn/issues/33011: Should warn for `y.Value.F` only. comp.VerifyDiagnostics( - // (12,13): warning CS8629: Nullable value type may be null. - // _ = x.Value; - Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "x.Value").WithLocation(12, 13), - // (14,13): warning CS8629: Nullable value type may be null. - // _ = y.Value; - Diagnostic(ErrorCode.WRN_NullableValueTypeMayBeNull, "y.Value").WithLocation(14, 13)); + // (15,9): warning CS8602: Possible dereference of a null reference. + // y.Value.F.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "y.Value.F").WithLocation(15, 9)); + } + + [Fact] + [WorkItem(33011, "https://github.com/dotnet/roslyn/issues/33011")] + public void Deconstruction_ImplicitNullableConversion_03() + { + var source = +@"#pragma warning disable 0649 +struct S +{ + internal T F; +} +class Program +{ + static void F((S, S) t) where T : class, new() + { + (S? x, S? y) = t; // 1, 2 + x.Value.F.ToString(); // 3 + y.Value.F.ToString(); + } +}"; + var comp = CreateCompilation(source, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (10,31): warning CS8619: Nullability of reference types in value of type 'S' doesn't match target type 'S?'. + // (S? x, S? y) = t; // 1, 2 + Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "t").WithArguments("S", "S?").WithLocation(10, 31), + // (10,31): warning CS8619: Nullability of reference types in value of type 'S' doesn't match target type 'S?'. + // (S? x, S? y) = t; // 1, 2 + Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "t").WithArguments("S", "S?").WithLocation(10, 31), + // (11,9): warning CS8602: Possible dereference of a null reference. + // x.Value.F.ToString(); // 3 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x.Value.F").WithLocation(11, 9)); } [Fact] -- GitLab