Definite assignment, initial nullable, IOperation, and some cleanup #54585
Conversation
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IInterpolatedStringOperation.cs
Show resolved
Hide resolved
Is this something you want to block on, or can it be added later? In reply to: 874428443 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:10243 in 1db9ee9. [](commit_id = 1db9ee9, deletion_comment = False) |
|
Done with review pass (iteration 5) In reply to: 699512610 |
|
@333fred There seems to be legitimate test failures |
| } | ||
| else | ||
| { | ||
| Join(ref State, ref beforePartsState); |
There was a problem hiding this comment.
I didn't understand this case. Why do we need to Join when none of the evaluations are conditional?
Feels like State is already correct when we get here. #Closed
There was a problem hiding this comment.
It is not. hasTrailingValidityParameter will be true in that case.
There was a problem hiding this comment.
Nit: this also confused me. Consider adding a comment.
I'm okay to do later. I think the nullability scenarios cover the parts of In reply to: 875184267 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:10243 in 1db9ee9. [](commit_id = 1db9ee9, deletion_comment = False) |
| { | ||
| // We assume non-bool returns if there was no parts to the string, and code below is predicated on that. | ||
| Debug.Assert(!node.Parts.IsEmpty); | ||
| // Start the sequence with appendProceedLocal, if appropriate | ||
| BoundExpression? currentExpression = appendShouldProceedLocal; | ||
|
|
||
| var boolType = _compilation.GetSpecialType(SpecialType.System_Boolean); |
There was a problem hiding this comment.
nit (ie. not blocking): we'll want a test where we make the type missing
There was a problem hiding this comment.
Of note, we required this type during initial binding.
This seems unexpected when In reply to: 875882504 In reply to: 875882504 In reply to: 875882504 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:10267 in 73be92a. [](commit_id = 73be92a, deletion_comment = False) |
You've covered the definite assignments that I could think of :-) In reply to: 875883536 In reply to: 875883536 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:10310 in 73be92a. [](commit_id = 73be92a, deletion_comment = False) |
nit: Not blocking, but I'd consider adding a In reply to: 875936424 #Closed Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs:10293 in 73be92a. [](commit_id = 73be92a, deletion_comment = False) |
| { | ||
| Debug.Assert(interpolatedString.InterpolationData != null); | ||
| var data = interpolatedString.InterpolationData.GetValueOrDefault(); | ||
| return GetValEscape(data.Construction, scopeOfTheContainingExpression); |
There was a problem hiding this comment.
Just to confirm we don't need to deal with GetRefEscape here in some cases?
The escapes rules are complicated enough that I'm having trouble reloading this on the fly...
There was a problem hiding this comment.
I don't believe so, but this is not my strongest area.
There was a problem hiding this comment.
Agree here that I think its ok, but I'm not 100% convinced. Can we add a work item to try and make a test that convinces us?
| { | ||
| var builder = ArrayBuilder<IInterpolatedStringContentOperation>.GetInstance(parts.Length); | ||
| foreach (var part in parts) | ||
| return data is { PositionInfo: var positionInfo } ? createHandlerInterpolatedStringContent(positionInfo) : createNonHandlerInterpolatedStringContent(); |
There was a problem hiding this comment.
Nit: personally I find these a or b methods with a pair of local functions harder to follow logically. Also the extra method call could be inefficient. (Purely personal, but just something to consider)
There was a problem hiding this comment.
Also the extra method call could be inefficient.
It's small enough that the JIT will likely just inline the containing call :).
| @@ -1105,7 +1105,10 @@ public override void VisitInterpolatedString(IInterpolatedStringOperation operat | |||
| public override void VisitInterpolatedStringText(IInterpolatedStringTextOperation operation) | |||
| { | |||
| Assert.Equal(OperationKind.InterpolatedStringText, operation.Kind); | |||
| Assert.Equal(OperationKind.Literal, operation.Text.Kind); | |||
| if (operation.Text.Kind != OperationKind.Literal) | |||
There was a problem hiding this comment.
Stupid question: why do we know its not a literal? because it could be dynamic?
There was a problem hiding this comment.
We don't know it's not a literal. If it is a literal, everything is fine. If it's not a literal, it must be a conversion wrapping a literal.
|
nit: It looks like this PR was merged instead of squashed :-/ |
Implements support for definite assignment, initial nullable support, initial IOperation support, and does a bit of prototype comment cleanup. Next week I'll add some additional commits cleaning up the rest of the prototype comments in this branch, they're mainly around adding tests for dynamic to be sure of behavior, ref escape behavior custom interpolated string handler arguments, and condensing message numbers for merge.
Test plan: #51499