Make escape analysis over unexpected nodes to return conservative results#25819
Make escape analysis over unexpected nodes to return conservative results#25819VSadov merged 2 commits intodotnet:dev15.7.xfrom
Conversation
…ults There are expressions for which ref-struct escape analysis does not make sense. Yet we keep finding ourselves in situations where we have to do it. Generally in error conditions. So, instead of throwing "unreachable" we will return the most conservative results. There is still an assert here so that if new bound nodes are added, there will be a chance to think whether there is a need for nontrivial handling of the node. Fixes:dotnet#25398
|
@dotnet/roslyn-compiler - please review. Fix for a crashing bug in dev15.7.x |
| Diagnostic(ErrorCode.ERR_CallArgMixing, "M(await t, ref r)").WithArguments("C.M(S, ref S)", "t").WithLocation(15, 9), | ||
| // (17,9): error CS1997: Since 'C.M(Task<S>)' is an async method that returns 'Task', a return keyword must not be followed by an object expression. Did you intend to return 'Task<T>'? | ||
| // return default; | ||
| Diagnostic(ErrorCode.ERR_TaskRetNoObjectRequired, "return").WithArguments("C.M(System.Threading.Tasks.Task<S>)").WithLocation(17, 9) |
There was a problem hiding this comment.
How is this error relevant here? If it's not relevant, I think the return default; line should be removed.
|
As far as I can tell, this PR also fixes #25485, so I think that issue should be closed when this PR is merged. |
|
Yes. It fixes #25485 as well. |
| // only possible in error cases (if possible at all) | ||
| return false; | ||
|
|
||
| default: |
There was a problem hiding this comment.
default: [](start = 15, length = 9)
Why are we moving away from throw here? The existing code is finding a lot of bugs.
There was a problem hiding this comment.
There is a reasonable default behavior for the nodes that we do not want explicitly handle here. The alternative is that any time we have something unexpected here we have crash in VS.
The quality of the bugs that come from this lately is not very high - we are basically just adding cases to the switch section that does that same default behavior.
Basically - I think the resulting "improvements" no longer justify the risk of crashes and general customer grief for this case.
Considering that there is a safe default behavior - the assert seems more appropriate at this point.
|
Microsoft.CodeAnalysis.Editor.UnitTests.Peek.PeekTests.TestInvokeInEmptyFile failed. |
|
@dotnet-bot test windows_release_unit32_prtest please |
|
@dotnet-bot test windows_debug_vs-integration_prtest please |
|
Thanks!! |
There are expressions for which ref-struct escape analysis does not make sense. Yet we keep finding ourselves in situations where we have to do it. Generally in error conditions.
So, instead of throwing "unreachable" we will return the most conservative results.
There is still an assert here so that if new bound nodes are added, there will be a chance to think whether there is a need for nontrivial handling of the node.
Fixes:#25398
Fixes:#25485