-
Notifications
You must be signed in to change notification settings - Fork 802
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
Address StackOverflowExceptions in typechecking #17654
Conversation
❗ Release notes required
|
…ting it in IL .filter is used
…/dotnet/fsharp into patch-netframework-stackoverflow
|
||
| SynModuleDecl.Expr _ -> failwith "unreachable: SynModuleDecl.Expr - ElimSynModuleDeclExpr" | ||
|
||
| SynModuleDecl.NamespaceFragment _ as d -> error(Error(FSComp.SR.tcUnsupportedMutRecDecl(), d.Range))) |
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.
Is it possible to use errorR here too ?
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.
TBH this was just a code block moved, I did not touch it nor investigate it.
…er bug big being fixed in the meantime.
Would it be possible to port this fix also to VS 2022 v17.11 ? |
@Thorium : 17.11 is not LTS and will go out of support in about a month. So even if we started the process right now, it would take a while to get inserted. Is using the 17.12 preview version an option for you? |
It's working for me, but I work with other F# devs, so I don't know how many struggles with VS crash without being aware of this issue. |
There have been two detected scenarios of System.StackoverFlowException crashing the IDE.
First was happening during typecheck, at TcModuleOrNamespaceElementNonMutRec (non recursive module/ns).
This is addressed by adding specific StackGuard calls integrated with the cancellable CE.
The second only happened in series of edits, which was eventually drilled down to be a consequence of System.OperationCanceledException triggered by the ID (new keystrokes invalidated previous requests for diagnostics).
It turned out that recursive calls into TcExpr were fitting into the stack (and they are wrapped in StackGuard as well), but when unrolling an exception, CLR has an overhead of roughly 10K for every catch+handler+rethrow.
The way the codegen for
try .. with | SomeActivePatternOnException -> ..
works lead to always hitting the catch handler, and rethrowing in the scenario of the active pattern not matching.Which was happening every single frame with a catch handler, repeating this across 50-100 calls into TcExpr.
The change is to replace the IL generated
.try ... catch
with an.try ... filter ... catch
in scenarios with either an active pattern or a when guard. With the filter IL block, the catch handler is invoked only if the filter passes.The reproducible StackOverflow was added as a test, and the same test is recompiled with different compiler switch to demonstrate it no longer stack overflows.