-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Change ??=
for nullable value types as specified in LDM
#36329
Conversation
…ying value type when targetting a nullable value type.
Ping @dotnet/roslyn-compiler @cston for a review please. #Closed |
Looking #Closed |
<source>'default' is converted to 'null', not 'default({0})'</source> | ||
<target state="translated">Hodnota default se převede na null, nikoli na default({0}).</target> | ||
<note /> | ||
</trans-unit> | ||
<trans-unit id="WRN_DefaultLiteralConvertedToNullIsNotIntended_Title"> |
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.
WRN_DefaultLiteralConvertedToNullIsNotIntended_Title [](start = 22, length = 52)
WRN_DefaultLiteralConvertedToNullIsNotIntended_Title
can also be removed #Closed
int? i2 = null; | ||
UseParam(i2 ??= default); // warn | ||
UseParamAsInt(i2 ??= default); |
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.
default [](start = 29, length = 7)
Please verify semantic model on default
. Also, let's verify the resulting value in execution (could be done in a standalone test).
If I understood correctly, this is now default(int)
(as opposed to default(int?)
/null
previously). #Closed
is this comment still correct? #Closed Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:3615 in 6977857. [](commit_id = 6977857, deletion_comment = False) |
{ | ||
Error(diagnostics, ErrorCode.WRN_DefaultLiteralConvertedToNullIsNotIntended, node.Right, leftType.GetNullableUnderlyingType()); | ||
// If B is implicitly convertible to A0, then the result type of this expression is A0, except if B is dynamic. |
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.
except if B is dynamic. [](start = 104, length = 23)
Why the exception for dynamic? Seems irregular #ByDesign
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.
This is explicitly called out in the spec, it's the same behavior that ??
has.
In reply to: 293040246 [](ancestors = 293040246)
diagnostics.Add(node, useSiteDiagnostics); | ||
var convertedRightOperand = CreateConversion(rightOperand, underlyingRightConversion, underlyingLeftType, diagnostics); | ||
// Create a nullable conversion for use by the local rewriter | ||
var nullableConversion = Conversions.ClassifyImplicitConversionFromType(underlyingLeftType, leftType, ref useSiteDiagnostics); |
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.
nullableConversion [](start = 24, length = 18)
This conversion doesn't seem to be used (it's not saved into the bound node we create) #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.
Whoops, forgot to remove. I was using this at one point and then removed it from the bound node, but forgot to remove the creation.
In reply to: 293042697 [](ancestors = 293042697)
I've updated a few comments to be more explicit about types. In reply to: 501382112 [](ancestors = 501382112) Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:3615 in 6977857. [](commit_id = 6977857, deletion_comment = False) |
TypeWithAnnotations targetType = VisitLvalueWithAnnotations(leftOperand); | ||
var leftState = this.State.Clone(); | ||
LearnFromNonNullTest(leftOperand, ref leftState); | ||
LearnFromNullTest(leftOperand, ref this.State); | ||
if (node.IsNullableValueTypeAssignment) | ||
{ | ||
targetType = TypeWithAnnotations.Create(node.Type, NullableAnnotation.NotAnnotated); |
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.
targetType = TypeWithAnnotations.Create(node.Type, NullableAnnotation.NotAnnotated); [](start = 16, length = 84)
Can we add a nullability test to cover this? ((nullableInt ??= default).ToString()
or something along those lines) #Closed
// if (!lhsRead.HasValue) { tmp = loweredRight; transformedAssignment = tmp; } | ||
// tmp | ||
// | ||
// Except that a is only evaluated once. |
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.
Except that a is only evaluated once. [](start = 19, length = 37)
I didn't understand this part. a
does not appear in the above lowered form. #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.
// Except that a is only evaluated once. | ||
|
||
var leftOperand = node.LeftOperand; | ||
if (!TryGetNullableMethod(leftOperand.Syntax, |
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.
if (!TryGetNullableMethod(leftOperand.Syntax, [](start = 16, length = 45)
Do we have tests for those missing methods? #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.
See CodeGenNullCoalescingAssignmentTests.MissingMembers
In reply to: 293090659 [](ancestors = 293090659)
|
||
// If lhsRead is a call, such as to a property accessor, save the result off to a temp. This doesn't affect | ||
// the standard ??= case because it only uses lhsRead once. | ||
if (lhsRead.Kind == BoundKind.Call) |
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.
if (lhsRead.Kind == BoundKind.Call) [](start = 16, length = 35)
I didn't understand this. I would have expected TransformCompoundAssignmentLHS
to already have done what's needed, and we can just use transformedLHS
that it returned.
FWIW, other scenarios that use TransformCompoundAssignmentLHS
do not special-case calls. #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.
Correct, because other compound assignments do not have to read the value twice like we have to. It leaves calls to property accessors as is, so if we don't store the value to a temp we'll end up calling the property accessor twice.
In reply to: 293093678 [](ancestors = 293093678)
// We lower the expression to this form: | ||
// | ||
// var tmp = lhsRead.GetValueOrDefault(); | ||
// if (!lhsRead.HasValue) { tmp = loweredRight; transformedAssignment = tmp; } |
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.
transformedAssignment [](start = 64, length = 21)
nit: should be transformedLhs
for consistency with comments below #Closed
// !lhsRead.HasValue | ||
var lhsReadHasValue = BoundCall.Synthesized(leftOperand.Syntax, lhsRead, hasValue); | ||
var lhsReadHasValueNegation = | ||
MakeUnaryOperator(UnaryOperatorKind.BoolLogicalNegation, leftOperand.Syntax, method: null, lhsReadHasValue, lhsReadHasValue.Type); |
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.
MakeUnaryOperator [](start = 20, length = 17)
Can this be _factory.Not(...)
? #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.
|
||
// { tmp = b; transformedLhs = tmp; } | ||
var consequenceBuilder = ArrayBuilder<BoundStatement>.GetInstance(2); | ||
consequenceBuilder.Add(new BoundExpressionStatement(node.Syntax, tmpAssignment) { WasCompilerGenerated = true }); |
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.
BoundExpressionStatement [](start = 43, length = 24)
Consider using _factory.ExpressionStatement(...)
. Also below #Closed
var consequenceBuilder = ArrayBuilder<BoundStatement>.GetInstance(2); | ||
consequenceBuilder.Add(new BoundExpressionStatement(node.Syntax, tmpAssignment) { WasCompilerGenerated = true }); | ||
consequenceBuilder.Add(new BoundExpressionStatement(node.Syntax, transformedLhsAssignment) { WasCompilerGenerated = true }); | ||
var consequence = new BoundBlock(node.Syntax, locals: ImmutableArray<LocalSymbol>.Empty, consequenceBuilder.ToImmutableAndFree()) |
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.
BoundBlock [](start = 38, length = 10)
Consider using _factor.Block(statement1, statement2)
#Closed
statementsBuilder.Add(ifHasValueStatement); | ||
|
||
_needsSpilling = true; | ||
return _factory.SpillSequence(temps.ToImmutableAndFree(), statementsBuilder.ToImmutableAndFree(), tmp); |
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.
SpillSequence [](start = 32, length = 13)
Technically, we could do without the spilling, just using a Sequence
. I don't know if it's worth it though.
// no statements, only expressions and side-effects in the following:
// var tmp = lhsRead.GetValueOrDefault();
// !lhsRead.HasValue ? { /*sequence*/ tmp = loweredRight; transformedAssignment = tmp; tmp } : tmp
``` #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 think this version is likely more efficient IL wise, since it doesn't have the multiple tmp
s at the end of a ternary. Also, it's the version that LDM approved, so I'd prefer to stick with the existing one.
In reply to: 293099162 [](ancestors = 293099162)
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 think LDM approved any specific IL-level lowered form. I recall we only discussed a C#-level lowered form and also guiding principles.
Don't forget that the lowered form here (described at the top of this local function) is not the final lowered form anyways, as the spilling will manipulate it further. Also, spilling makes this harder to reason about (exactly what is spilled? what is the final form?) and required some changes to TransformCompoundAssignmentLHS
which I'm a bit concerned about.
I'm curious what Neal, Aleksey or Chuck think about this.
In reply to: 293103887 [](ancestors = 293103887,293099162)
@@ -463,7 +463,7 @@ private BoundDynamicIndexerAccess TransformDynamicIndexerAccess(BoundDynamicInde | |||
/// A side-effect-free expression representing the LHS. | |||
/// The returned node needs to be lowered but its children are already lowered. | |||
/// </returns> | |||
private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalLHS, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps, bool isDynamicAssignment) | |||
private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalLHS, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps, bool isDynamicAssignment, bool needsSpilling = false) |
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.
bool needsSpilling = false) [](start = 189, length = 27)
As commented in lowering of null-coalescing operator, I'm not sure we need spilling. Changes in this file feel unnecessary. #Closed
{ | ||
Error(diagnostics, ErrorCode.WRN_DefaultLiteralConvertedToNullIsNotIntended, node.Right, leftType.GetNullableUnderlyingType()); | ||
// If B is implicitly convertible to A0, then the result type of this expression is A0, except if B is dynamic. |
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.
It looks like this comment conflicts with the comment above. Also, you've lost the definition of A0
#Resolved
diagnostics.Add(node, useSiteDiagnostics); | ||
var convertedRightOperand = CreateConversion(rightOperand, underlyingRightConversion, underlyingLeftType, diagnostics); | ||
// Create a nullable conversion for use by the local rewriter | ||
var nullableConversion = Conversions.ClassifyImplicitConversionFromType(underlyingLeftType, leftType, ref useSiteDiagnostics); |
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.
Not sure I understand what's going on here. Are we throwing away the conversion? Don't we need to attach it to something for the LocalRewriter? #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.
See the comment to Julien. I was using this at one point and just forgot to remove it.
In reply to: 293105993 [](ancestors = 293105993)
// the standard ??= case because it only uses lhsRead once. | ||
if (lhsRead.Kind == BoundKind.Call) | ||
{ | ||
var lhsTemp = _factory.StoreToTemp(lhsRead, out var store, kind: SynthesizedLocalKind.Spill); |
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.
Why do we need SynthesizedLocalKind.Spill
? #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.
When we get to the SpillSequenceSpiller, it asserts that all synthesized locals are long lived. The default (LoweringTemp
) is not.
In reply to: 293111279 [](ancestors = 293111279)
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 see no good reason why we need this temporary to be long lived, since the other ones used in compound assignments are not. #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.
The other ones are not used in a spill sequence, this is.
In reply to: 293135135 [](ancestors = 293135135)
@@ -491,6 +491,7 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL | |||
// we have a variable on the left. Transform it into: | |||
// ref temp = ref variable | |||
// temp = temp + value | |||
var localKind = needsSpilling ? SynthesizedLocalKind.Spill : SynthesizedLocalKind.LoweringTemp; |
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.
Seems worrying if we create a ref temp that needs spilling since that's not possible. #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.
nit: Let's xml doc the new parameter: Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs:461 in 6977857. [](commit_id = 6977857, deletion_comment = False) |
@@ -560,16 +561,16 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL | |||
var loweredArray = VisitExpression(arrayAccess.Expression); | |||
var loweredIndices = VisitList(arrayAccess.Indices); | |||
|
|||
return SpillArrayElementAccess(loweredArray, loweredIndices, stores, temps); | |||
return SpillArrayElementAccess(loweredArray, loweredIndices, stores, temps, localKind); |
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.
localKind); [](start = 104, length = 11)
I'm assuming that all the cases in this switch are covered by a nullable value type scenario. If not, let's add test coverage. #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.
Added array accesses. We had some for indexers, but not for regular arrays. Everything else should be covered already.
In reply to: 293121299 [](ancestors = 293121299)
@@ -813,6 +813,7 @@ PooledHashSet<CaptureId> adjustAndGetEntryState(ArrayBuilder<PooledHashSet<Captu | |||
((isFirstOperandOfDynamicOrUserDefinedLogicalOperator(reference) || | |||
isIncrementedNullableForToLoopControlVariable(reference) || | |||
isConditionalAccessReceiver(reference) || | |||
isCoalesceAssignment(reference) || |
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.
isCoalesceAssignment [](start = 37, length = 20)
Could we tighten this exception to only be for null-coalescing assignments where the LHS is a property? #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've updated the syntax used in the CFG to be a little more precise and removed this entirely.
In reply to: 293124296 [](ancestors = 293124296)
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.
To be clear, I actually didn't need it last iteration, I had left it in but had apparently already removed the need.
In reply to: 294962493 [](ancestors = 294962493,293124296)
.locals {R1} | ||
{ | ||
CaptureIds: [0] |
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.
[0] [](start = 16, length = 3)
Just curious: what previously caused only one capture to be reported here? (we already had IFlowCaptureOperation: 1
below) #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.
The capture was more locally scoped to just Region 2.
In reply to: 293125795 [](ancestors = 293125795)
// We lower the expression to this form: | ||
// | ||
// var tmp = lhsRead.GetValueOrDefault(); | ||
// if (!lhsRead.HasValue) { tmp = loweredRight; transformedAssignment = tmp; } |
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.
!lhsRead.HasValue [](start = 23, length = 17)
Should this rather be a null test on Nevermind. Got confused. #Closedtmp
?
if (!isStatement) | ||
{ | ||
intermediateFrame = PushStackFrame(); | ||
SpillEvalStack(); |
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.
SpillEvalStack [](start = 20, length = 14)
not related to this PR: I feel like I forgot what most of those utility methods do since last time I reviewed some CFG work. It may be worth investing in some documentation for CFG so that we can reload some context in future reviews. #Closed
|
||
return isStatement ? null : GetCaptureReference(resultCaptureId, operation); | ||
|
||
void nullableValueTypeReturn() |
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.
nullableValueTypeReturn [](start = 17, length = 23)
// var tmp = lhsRead.GetValueOrDefault();
// if (!lhsRead.HasValue) { tmp = loweredRight; transformedAssignment = tmp; }
// tmp
// OR for statement
// if (!lhsRead.HasValue) { transformedAssignment = loweredRight; }
Suggestion: If the comment referenced block names (afterCoalesce
and such), that'd be even better. #Closed
Consider adding a test for Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenNullCoalescingAssignmentTests.cs:29 in 6977857. [](commit_id = 6977857, deletion_comment = False) |
|
||
public static void Main() | ||
{ | ||
var c = new C(); | ||
Console.WriteLine(c.f1 ??= GetInt()); | ||
Console.WriteLine(c.f2 ??= GetObject()); |
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.
Console.WriteLine(c.f2 ??= GetObject()); [](start = 7, length = 41)
Consider extracting both WriteLine
statements to separate methods for clarity in VerifyIL
.
Also applies in other tests #Closed
public static void TestObject() | ||
{ | ||
var c = new C(); | ||
Console.WriteLine(c.P2 ??= GetInt(1)); |
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.
1 [](start = 42, length = 1)
Consider incrementing here (use 3
and 4
instead of 1
and 2
again) #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.
Done with review pass (iteration 2)
I'm still thinking about lowering with statement+spilling vs. lowering without statement.
I don't think this is necessary. I already have several tests that show that type of this is In reply to: 501481710 [](ancestors = 501481710) Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenNullCoalescingAssignmentTests.cs:29 in 6977857. [](commit_id = 6977857, deletion_comment = False) |
…ound a bug in TransformCompoundAssignmentLHS. Addressed PR 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)
…-types * dotnet/master: (63 commits) Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Fix parsing bug in invalid using statements (dotnet#36428) Do not suggest or diagnose use compound assignment when right hand of binary operator is a throw expression Add option to emit nullable metadata for public members only (dotnet#36398) Added null checks on F# external access services (dotnet#36469) Deal with discovering extra .editorconfig files Re-enable MSBuildWorkspaceTests.TestEditorConfigDiscovery Add support to VisualStudioMSBuildInstalled to support minimum versions Fix configuration of accessibilities in editorconfig Shorten a resource ID Revert "Extract the RDT implementation for Misc files and VS open file tracker" Add nullability support to use local function Add EditorFeatures.WPF dependency to F# ExternalAccess Ensure NullableWalker.AsMemberOfType locates the right new container for the member. (dotnet#36406) Replace `dynamic` with `object` when substituting constraints. (dotnet#36379) Add some string descriptions Adjust type of out var based on parameter state (dotnet#36284) ...
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
Implement the changes from dotnet/csharplang#2591.
a ??= b
, wherea
isint?
andb
isint
now has a type ofint
instead ofint?
, and a different lowered form. @cston @dotnet/roslyn-compiler for review.