-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Correctly handle conversions in null coalescing #52009
Conversation
We weren't correctly using the results of the left conversion for cases when the rhs of a null-coalescing assignment determines the output type and was a user-defined conversion. I've also added tests for dotnet#29955, which I don't believe needs to be addressed as I couldn't find any scenario that was broken due to it. Fixes dotnet#51975. Fixes dotnet#29955.
@dotnet/roslyn-compiler @RikkiGibson for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't yet completely understand the nullable changes. Maybe we can go over them.
@@ -634,6 +635,7 @@ public bool IsReference | |||
/// <remarks> | |||
/// Implicit and explicit user-defined conversions are described in section 6.4 of the C# language specification. | |||
/// </remarks> | |||
[MemberNotNullWhen(true, nameof(Method), nameof(MethodSymbol))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it feels like it could be good to change the implementation to something like:
var isUserDefined = Kind.IsUserDefinedConversion();
Debug.Assert(!isUserDefined || (Method is not null && MethodSymbol is not null))
return isUserDefined;
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is necessary. When construction occurs, these conditions are validated. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit thrown off by the use of MemberNotNullWhen
here too because this method doesn't initialize or check Method
or MethodSymbol
.
Adding the assertion would make sense to me, but I'm not sure it would hold true.
Seems like we can create user-defined conversions that return null
Method
:
internal Conversion(UserDefinedConversionResult conversionResult, bool isImplicit)
{
_kind = conversionResult.Kind == UserDefinedConversionResultKind.NoApplicableOperators
? ConversionKind.NoConversion
: isImplicit ? ConversionKind.ImplicitUserDefined : ConversionKind.ExplicitUserDefined;
_uncommonData = new UncommonData(
isExtensionMethod: false,
isArrayIndex: false,
conversionResult: conversionResult,
conversionMethod: null, // this is used for `Method`
nestedConversions: default);
}
In reply to: 599078854 [](ancestors = 599078854)
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
@dotnet/roslyn-compiler for a second review please. |
1 similar comment
@dotnet/roslyn-compiler for a second review please. |
@dotnet/roslyn-compiler for a second review please. |
1 similar comment
@dotnet/roslyn-compiler for a second review please. |
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 2)
@jcouv addressed feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 3)
We weren't correctly using the results of the left conversion for cases when the rhs of a null-coalescing assignment determines the output type and was a user-defined conversion. I've also added tests for #29955, which I don't believe needs to be addressed as I couldn't find any scenario that was broken due to it.
Fixes #51975. Fixes #29955.