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

Non-nullable fields of default struct value should be treated as nullable #32304

Merged
merged 13 commits into from
Jan 11, 2019

Conversation

cston
Copy link
Member

@cston cston commented Jan 9, 2019

Fixes #30731

@cston cston requested a review from a team as a code owner January 9, 2019 22:29
@jcouv jcouv added this to the 16.0.P3 milestone Jan 9, 2019
@@ -17481,6 +17484,77 @@ static void F2(bool b, IOut<object, string> x2, IOut<object?, string?> y2)
comp.VerifyTypes();
}

#if false
Copy link
Member

@jcouv jcouv Jan 9, 2019

Choose a reason for hiding this comment

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

Is this intentional? If it is please add a comment and link to follow-up issue. #Resolved

@@ -20775,6 +20849,65 @@ static void G3(B<object>.IOut<string> x3, B<object?>.IOut<string?> y3)
comp.VerifyTypes();
}

#if false
Copy link
Member

@jcouv jcouv Jan 9, 2019

Choose a reason for hiding this comment

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

Same here #Resolved

@@ -49,7 +48,7 @@ public override bool Equals(object obj)
return !left.Equals(right);
}

internal string GetDebuggerDisplay()
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

GetDebuggerDisplay [](start = 28, length = 18)

I'm curious, why the change?
Just want to confirm it's not because DebuggerDisplay isn't working in dev16 (I've already filled a bug on that). #Closed

Copy link
Member Author

@cston cston Jan 10, 2019

Choose a reason for hiding this comment

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

Changed because ToString() is used in the EE for instances in a collection. #Closed

}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
// PROTOTYPE: Missing // 2
Copy link
Member Author

@cston cston Jan 10, 2019

Choose a reason for hiding this comment

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

PROTOTYPE [](start = 15, length = 9)

Need to log a bug if not resolved in this PR. #Resolved

}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
// PROTOTYPE: Missing // 1
Copy link
Member Author

@cston cston Jan 10, 2019

Choose a reason for hiding this comment

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

PROTOTYPE [](start = 15, length = 9)

Same issue as earlier. #Closed

```
If `T` is a value type, `default(T)` is `T` and any non-nullable fields in `T` are maybe null.
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

any non-nullable fields [](start = 48, length = 23)

Perhaps "all non-struct fields"?
int?, string? or T field would also get a nullable null-state, I assume. #Closed

```
If `T` is a value type, `default(T)` is `T` and any non-nullable fields in `T` are maybe null.
If `T` is a value type, `new T()` is equivalent to `default(T)`.
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

new T() is equivalent to default(T) [](start = 24, length = 39)

Does this depend on whether we're using a default constructor, or a user-defined one? #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.

This only applies to the default parameter-less constructor that cannot be overridden for structs.


In reply to: 246863817 [](ancestors = 246863817)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't know they couldn't be overridden. Thanks


In reply to: 246865325 [](ancestors = 246865325,246863817)

@@ -797,7 +831,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) ?
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

fieldOrPropertyType.IsReferenceType [](start = 62, length = 35)

Should this be ... && (!fieldOrPropertType.IsValueType || fieldOrProperty.IsNullableType())?

I think my proposal was wrong, but I still feel there is a problem here. Probably with unconstrained T. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

If the member type is a nullable value type, fieldOrPropertyType.NullableAnnotation should return Nullable already.


In reply to: 246866495 [](ancestors = 246866495)

Copy link
Member

Choose a reason for hiding this comment

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

From our discussion, it's not obvious how T works


In reply to: 246867516 [](ancestors = 246867516,246866495)

Copy link
Member Author

Choose a reason for hiding this comment

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

A member with a unconstrained type parameter type is already considered maybe null. The only cases we need to handle specifically here are members where the type is declared as not nullable.


In reply to: 246871048 [](ancestors = 246871048,246867516,246866495)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a related issue when copying not-nullable state for unconstrained types though. Logged #32338. Thanks.


In reply to: 246874741 [](ancestors = 246874741,246871048,246867516,246866495)

@@ -1290,7 +1329,7 @@ private void SetResult(BoundExpression node)
_resultType = TypeSymbolWithAnnotations.Create(node.Type);
}

private ObjectCreationPlaceholderLocal GetOrCreateObjectCreationPlaceholder(BoundExpression node)
private int GetOrCreateObjectCreationPlaceholder(BoundExpression node)
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

GetOrCreateObjectCreationPlaceholder [](start = 20, length = 36)

Perhaps GetOrCreateObjectCreationPlaceholderSlot ? #Resolved

@@ -928,13 +964,11 @@ 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: false, isByRefTarget: parameter.RefKind != RefKind.None, slotWatermark: GetSlotWatermark());
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

isDefaultValue: false, [](start = 105, length = 22)

What about void M(string s = default)?
I think we report a warning already, but in terms of null-state the optional value should probably still count. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks.


In reply to: 246871918 [](ancestors = 246871918)

@@ -1090,7 +1124,7 @@ private static bool IsTypeParameterDisallowingAnnotation(TypeSymbol typeOpt)
public override BoundNode VisitLocal(BoundLocal node)
{
var local = node.LocalSymbol;
int slot = GetOrCreateSlot(local);
int slot = GetOrCreateSlot(local, createIfMissing: false);
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

, createIfMissing: false [](start = 44, length = 24)

Does this matter? Consider removing for simplicity. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't matter (since a slot for the local should have been created already), but it seems reasonable to pass false explicitly so the intent is clear that a slot should not be created here.


In reply to: 246872573 [](ancestors = 246872573)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@cston
Copy link
Member Author

cston commented Jan 10, 2019

@dotnet/roslyn-compiler, please review.

parameterType.TypeSymbol,
slot,
valueSlot: -1,
isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true,
Copy link
Member

@jcouv jcouv Jan 10, 2019

Choose a reason for hiding this comment

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

parameter.ExplicitDefaultConstantValue?.IsNull == true, [](start = 40, length = 55)

Does that work for void M(T t = default)? #ByDesign

Copy link
Member Author

@cston cston Jan 10, 2019

Choose a reason for hiding this comment

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

We're only interested in cases where parameter.Type is a trackable struct. (See outer if.)


In reply to: 246931559 [](ancestors = 246931559)

@@ -91,11 +91,11 @@ protected int VariableSlot(Symbol symbol, int containingSlot = 0)
/// <summary>
/// Force a variable to have a slot. Returns -1 if the variable has an empty struct type.
/// </summary>
protected int GetOrCreateSlot(Symbol symbol, int containingSlot = 0)
protected int GetOrCreateSlot(Symbol symbol, int containingSlot = 0, bool createIfMissing = true)
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing [](start = 82, length = 15)

It feels like the "createIfMissing" parameter is specific to tuples, but the name doesn't reflect the fact. This might be confusing to consumers. For example, one might assume that, unless we already allocated a slot, this function won't allocate it when false is passed for createIfMissing parameter. However, it doesn't look like this is the case. #Closed

@@ -162,16 +162,10 @@ private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot,
return -1;
}

if (forceContainingSlotsToExist)
containingSlot = GetOrCreateSlot(restField, containingSlot, createIfMissing);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

containingSlot = GetOrCreateSlot(restField, containingSlot, createIfMissing); [](start = 20, length = 77)

Is propagating createIfMissing has any real effect here? It looks like restField is coming from an underlying named type. Therefore, it cannot be a TupleFieldSymbol. Therefore, the DescendThroughTupleRestFields call in the GetOrCreateSlot above will be a NoOp regardless of the value passed in createIfMissing.
If these observations are correct, then it means that now createIfMissing has no effect on the "result" of this function, but it looks like the forceContainingSlotsToExist parameter used to have an effect.
#Closed

@@ -82,7 +82,7 @@ protected override void Free()
/// </summary>
protected int VariableSlot(Symbol symbol, int containingSlot = 0)
{
containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, forceContainingSlotsToExist: false);
containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, createIfMissing: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing: false [](start = 87, length = 22)

Does passing false really makes any difference given the changes in DescendThroughTupleRestFields function? #Closed

{
if (symbol.Kind == SymbolKind.RangeVariable) return -1;

containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, forceContainingSlotsToExist: true);
containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, createIfMissing);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, createIfMissing); [](start = 12, length = 92)

I understand that this is an existing logic. It looks like DescendThroughTupleRestFields can return -1. Does it make sense to continue in this case? It looks like we will go ahead and allocate a new slot for the member even in this case. It also looks like that slot is going to be shared across distinct original containers, effectively treating members of different containers as the same storage location. #Closed

var type = ((BoundDefaultExpression)node).Type;
if (EmptyStructTypeCache.IsTrackableStructType(type))
{
int slot = GetOrCreateObjectCreationPlaceholderSlot(node);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

int slot = GetOrCreateObjectCreationPlaceholderSlot(node); [](start = 28, length = 58)

It feels like there is an inconsistency between when we create placeholder locals for the default expression and for an object creation. I think it would be better to use the same approach for all such cases. For example, here we would simply do what is done for the case BoundKind.ObjectCreationExpression: and the code that is currently here could be moved to VisitDefault etc. #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.

Yes, that is necessary to handle split states as well.


In reply to: 246945160 [](ancestors = 246945160)

{
int slot = _lastConditionalAccessSlot;
_lastConditionalAccessSlot = -1;
return slot;
}
break;
default:
return base.MakeSlot(node);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

return base.MakeSlot(node); [](start = 20, length = 27)

This used to be called for all cases that don't return, now we always return -1 for scenarios like that. Is this the right thing to do? #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.

Yes, I believe so. There is no overlap between the cases here and in the base.


In reply to: 246946078 [](ancestors = 246946078)


bool isDefaultOfUnconstrainedTypeParameter(BoundExpression expr)
private static bool IsDefaultOfUnconstrainedTypeParameter(BoundExpression expr)
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

IsDefaultOfUnconstrainedTypeParameter [](start = 28, length = 37)

It looks like this function is called only from the original call site. Why make it a member vs. a local function? #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.

To allow grouping the two IsDefault... methods so the similarity is more obvious.


In reply to: 246948367 [](ancestors = 246948367)

parameterType.TypeSymbol,
slot,
valueSlot: -1,
isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true,
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

isDefaultValue: parameter.ExplicitDefaultConstantValue?.IsNull == true, [](start = 24, length = 71)

Is this going to properly handle structures? #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.

parameter.ExplicitDefaultConstantValue.IsNull is true for default(S) and new S().


In reply to: 246951009 [](ancestors = 246951009)

Copy link
Contributor

Choose a reason for hiding this comment

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

parameter.ExplicitDefaultConstantValue.IsNull is true for default(S) and new S().

Judging by the doc comments on ExplicitDefaultConstantValue property, this is not intended behavior of the API.


In reply to: 246955015 [](ancestors = 246955015,246951009)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a missing default value test, and checked the implementations of ExplicitDefaultConstantValue look reasonable.


In reply to: 246959155 [](ancestors = 246959155,246955015,246951009)

@@ -832,13 +868,13 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont
int valueMemberSlot = -1;
if (valueContainerSlot > 0)
{
int slot = GetOrCreateSlot(member, valueContainerSlot);
int slot = GetOrCreateSlot(member, valueContainerSlot, createIfMissing: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing: false [](start = 79, length = 22)

I am not sure if passing this value is going to have any useful effect. If we are not interested in allocating new slots, perhaps we should use VariableSlot API instead. #Closed

@@ -1090,7 +1130,7 @@ private static bool IsTypeParameterDisallowingAnnotation(TypeSymbol typeOpt)
public override BoundNode VisitLocal(BoundLocal node)
{
var local = node.LocalSymbol;
int slot = GetOrCreateSlot(local);
int slot = GetOrCreateSlot(local, createIfMissing: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing: false [](start = 46, length = 22)

I am not sure if passing this value is going to have any useful effect. If we are not interested in allocating new slots, perhaps we should use VariableSlot API instead. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 10, 2019

        LocalSymbol receiver = null;

It looks like receiver is never changed to anything else. #Closed


Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1225 in 50416ca. [](commit_id = 50416ca, deletion_comment = False)

type,
slot,
valueSlot: -1,
isDefaultValue: (node as BoundObjectCreationExpression)?.Constructor.IsImplicitConstructor == true,
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

(node as BoundObjectCreationExpression)?.Constructor.IsImplicitConstructor == true [](start = 44, length = 82)

This doesn't look like a test for a Default Value Type Constructor. Am I misinterpreting the intent? #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.

Changed.


In reply to: 246953718 [](ancestors = 246953718)

@@ -3940,7 +3984,7 @@ private void VisitThisOrBaseReference(BoundExpression node)
public override BoundNode VisitParameter(BoundParameter node)
{
var parameter = node.ParameterSymbol;
int slot = GetOrCreateSlot(parameter);
int slot = GetOrCreateSlot(parameter, createIfMissing: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing: false [](start = 50, length = 22)

I am not sure if passing this value is going to have any useful effect. If we are not interested in allocating new slots, perhaps we should use VariableSlot API instead. #Closed

@@ -4301,7 +4345,7 @@ private void VisitMemberAccess(BoundExpression node, BoundExpression receiverOpt
if (!resultType.IsValueType || resultType.IsNullableType())
{
int containingSlot = getReceiverSlot();
int slot = (containingSlot < 0) ? -1 : GetOrCreateSlot(member, containingSlot);
int slot = (containingSlot < 0) ? -1 : GetOrCreateSlot(member, containingSlot, createIfMissing: false);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

createIfMissing: false [](start = 103, length = 22)

I am not sure if passing this value is going to have any useful effect. If we are not interested in allocating new slots, perhaps we should use VariableSlot API instead. But do we really want to skip allocating the slot and fail to adjust resultType? #Closed

@@ -771,7 +804,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)
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

bool isDefaultValue [](start = 113, length = 19)

If we are propagating this flag separately anyway, why do we need slots for the "fake" state? #Closed

Copy link
Member Author

@cston cston Jan 11, 2019

Choose a reason for hiding this comment

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

As we discussed, slots are needed for direct member access (e.g.: default(S).F.ToString()), although that is not a particularly compelling scenario to handle.


In reply to: 246955178 [](ancestors = 246955178)

where U : class
where V : struct
{
new S<T>().F/*T:T*/.ToString(); // 1
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

new S() [](start = 8, length = 10)

Do we have a test covering non-default value type constructors and explicitly defined parameter-less constructors (legal in metadata I think)? #Closed

where V : struct
{
new S<T>().F/*T:T*/.ToString(); // 1
new S<U>().F/*T:U?*/.ToString(); // 2
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 10, 2019

Choose a reason for hiding this comment

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

new S() [](start = 8, length = 10)

Do we have a test covering combination of default value type constructors with object member initializers? #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 11, 2019

Done review pass (iteration 11) #Closed

@jcouv jcouv self-assigned this Jan 11, 2019
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 13)

@cston cston merged commit a2d838e into dotnet:master Jan 11, 2019
@cston cston deleted the 30731 branch January 11, 2019 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants