Handle tuple conversions and extension method this argument conversions in NullableWalker
#29679
Conversation
|
@dotnet/roslyn-compiler please review. |
| @@ -1457,6 +1457,7 @@ public Conversion ClassifyImplicitExtensionMethodThisArgConversion(BoundExpressi | |||
|
|
|||
| if (sourceExpressionOpt?.Kind == BoundKind.TupleLiteral) | |||
| { | |||
| Debug.Assert(!IncludeNullability); // Top-level nullability is ignored. | |||
There was a problem hiding this comment.
It is not clear why it top-level nullability should be ignored in this particular case. Could you please elaborate? Also, what about nested nullability? #Resolved
There was a problem hiding this comment.
This code path is not used when IncludeNullability is set. If it is in the future (when this assert fails), we'll need to update the delegate below.
In reply to: 215709176 [](ancestors = 215709176)
There was a problem hiding this comment.
This code path is not used when IncludeNullability is set. If it is in the future (when this assert fails), we'll need to update the delegate below.
Consider capturing this in the comment instead. Something like: "At the moment this code path is taken only under this condition, if that changes, the code below will need an adjustment". And for other similar places.
In reply to: 215710774 [](ancestors = 215710774,215709176)
| @@ -1858,6 +1864,7 @@ private Conversion ClassifyImplicitNullableConversion(TypeSymbol source, TypeSym | |||
|
|
|||
| private Conversion GetImplicitTupleLiteralConversion(BoundTupleLiteral source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics) | |||
| { | |||
| Debug.Assert(!IncludeNullability); // Top-level nullability is ignored. | |||
There was a problem hiding this comment.
Debug.Assert(!IncludeNullability); // Top-level nullability is ignored. [](start = 12, length = 71)
I am not sure about the purpose of these asserts. Is it not valid to call this function when condition isn't met, something else?
There was a problem hiding this comment.
This method is not used with IncludeNullability currently, and the delegate below does not handle top-level nullability.
In reply to: 215710436 [](ancestors = 215710436)
| @@ -32604,9 +32668,6 @@ static void F((B?, B) b) | |||
| }"; | |||
| var comp = CreateCompilation(new[] { source, NonNullTypesTrue, NonNullTypesAttributesDefinition }); | |||
| comp.VerifyDiagnostics( | |||
| // (12,21): warning CS8604: Possible null reference argument for parameter 'a' in 'A.implicit operator C(A a)'. | |||
There was a problem hiding this comment.
// (12,21): warning CS8604: Possible null reference argument for parameter 'a' in 'A.implicit operator C(A a)'. [](start = 16, length = 111)
I think this warning was more informative than the one below. The remaining warning might be somewhat misleading. For example, what would we get for:
static void F((B, B?) b)
{
(C, C?) c = b; // (ImplicitTuple)(ImplicitUserDefined)(ImplicitReference)
}
Would it say: "Nullability of reference types in value of type '(B, B?)' doesn't match target type '(C, C?)'." ?
Consider opening an issue to follow-up #Pending
No description provided.