Add support for nullable analysis in interpolated string handler constructors#57780
Add support for nullable analysis in interpolated string handler constructors#57780333fred merged 18 commits intodotnet:mainfrom
Conversation
…tructors Fixes dotnet#54583. Arguments with slots are treated like we do receivers: as though the previous argument value has already been loaded, and therefore postcondition attributes do not flow back to the original argument position (the effect of this can be seen in StringInterpolation_14 and _20), but future references to those locations will see the effects of the attribute. I additionally looked at updating region analysis as part of this PR to see if I could share data between these passes, but I don't believe we can do much sharing here. There's too much nullable-specific with finding the current state of the expression being used for the mapping to work.
src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
Outdated
Show resolved
Hide resolved
Is this something that should be approved by LDM? Also, do we plan to update nullable specification? In reply to: 974418424 |
|
Done with review pass (commit 2) |
|
Converting this to a draft PR while I'm out on vacation. In reply to: 974547865 |
src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 15) |
| // handler semantics), and then evaluate to the handler flow capture temp. | ||
|
|
||
| SpillEvalStack(); | ||
| SpillEvalStack(spillDeclarationExpressions: true); |
There was a problem hiding this comment.
This doesn't feel sufficient. This parameter will make the difference only for entries that haven't been spilled already. And there is no guarantee that that will be the case. For example, if an argument following a DeclarationExpression has a control flow, it will case spilling for the DeclarationExpression and after that this call won't do anything about it. It should be easy to come up with a test case that exposes this issue.
Some options to address this that come to mind:
- Unconditionally unwrap
DeclarationExpressions during spilling. If we can run into tuple-likeDeclarationExpressions, perhaps we can skip those. - Preemptively unwrap
DeclarationExpressions when we push arguments to the stack. - Handle spilling of
DeclarationExpressionout of order on demand once we realize that we need to use it multiple times due to the interpolation.
We can discuss our options in more details offline. #Closed
|
@AlekseyTs I implemented the change we talked about offline. Please take another look when you get the chance. |
| } | ||
|
|
||
| private void SpillEvalStack() | ||
| private void SpillEvalStack(bool spillDeclarationExpressions = false) |
| { | ||
| public static void M1(object o1, object o2) | ||
| /*<bind>*/{ | ||
| C.M(out int i, o1 ?? o2, $""literal{1}""); |
Is this delegate used? In reply to: 991351580 #Closed Refers to: src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs:2092 in 733946f. [](commit_id = 733946f, deletion_comment = False) |
| // this check is more complexity than this scenario needs. | ||
| if (i < lastIndexForSpilling) | ||
| { | ||
| SpillEvalStack(spillDeclarationExpressions: true); |
There was a problem hiding this comment.
Leaving the logic in SpillEvalStack feels fragile because it will continue to be a trap for user that might think that passing true actually provides guarantees on its own. Another shortcoming is that this call is going to unwrap declaration expressions that do not belong to this argument list. For example, arguments of an enclosing call. Please add a test case for this scenario.
|
Done with review pass (commit 16). |
|
@AlekseyTs addressed feedback. In reply to: 991353417 |
| // have unexpected affects on consumers. So we limit the spilling to those indexes before the last interpolated string | ||
| // handler. We _could_ limit this further by only spilling declaration expressions if the handler in question actually | ||
| // referenced a specific declaration expression in the argument list, but we think that the complexity in implementing | ||
| // this check is more complexity than this scenario needs. |
AlekseyTs
left a comment
There was a problem hiding this comment.
LGTM (commit 17), assuming CI is passing.
|
@dotnet/roslyn-compiler @RikkiGibson for a second review. |
…rations * upstream/main: (87 commits) Add support for nullable analysis in interpolated string handler constructors (dotnet#57780) Record list-patterns and newlines in interpolations as done (dotnet#58250) Swithc to acquiring the component model on a BG thread. Fix failure to propagate cancellation token [main] Update dependencies from dotnet/arcade (dotnet#58327) Change PROTOTYPE to issue reference (dotnet#58336) Resolve PROTOTYPE comments in param null checking (dotnet#58324) Address various minor param-nullchecking issues (dotnet#58321) Nullable enable GetTypeByMetadataName (dotnet#58317) Support CodeClass2.Parts returning parts in source generated files Allow the FileCodeModel.Parent to be null Ensure the CodeModel tests are using VisualStudioWorkspaceImpl Fix the bad words in TestDeepAlternation Fix regression in Equals/GetHashCode of LambdaSymbol. (dotnet#58247) Support "Enable nullable reference types" from disable keyword Support "Enable nullable reference types" from restore keyword Support "Enable nullable reference types" on the entire directive Add comment Remove descriptor Update src/Workspaces/Remote/ServiceHub/Services/SemanticClassification/RemoteSemanticClassificationService.cs ...
Implements most of #54583. Arguments with slots are treated like we do receivers: as though the previous argument value has already been loaded, and therefore postcondition attributes do not flow back to the original argument position (the effect of this can be seen in StringInterpolation_14 and _20), but future references to those locations will see the effects of the attribute.
I additionally looked at updating region analysis as part of this PR to see if I could share data between these passes, but I don't believe we can do much sharing here. There's too much nullable-specific with finding the current state of the expression being used for the mapping to work.