Skip to content
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

Enable exception filters when exception patterns are simple #12737

Merged
merged 8 commits into from
Feb 18, 2022

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 16, 2022

Fixes #3297

F# exception handling often appears as "rethrow" in the exception debugging experience, e.g. see #3297. This is because F# doesn't routinely emit exception filters - and this is because the patterns used in exception matching might have side effects due to active patterns. On the other hand, exception filters give a much better debugging experience.

Historically we've had the hidden switch --generate-exception-filters however as noted in #66 we never turned them on by default because we were concerned about side effect order and side effects happening twice.

However basic pattern matches such as exception type tests are very common and in these cases we should generate a filter block.

In this PR we identify a subset of pattern matching that is side-effect free and use that to determine if we emit a filter block.

This particularly affects the experience for uncaught exceptions where a surrounding try/with fails to catch, e.g.

Before (showing the uncaught exception throwing at the "rethrow")

image

After (showing the uncaught exception at origin)

image

Codegen tests will fail and need updating

@dsyme dsyme changed the title [WIP] Enable exception filters when exception patterns are simple Enable exception filters when exception patterns are simple Feb 16, 2022
@dsyme
Copy link
Contributor Author

dsyme commented Feb 17, 2022

This is ready

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great change, I appreciate it greatly.
Subject to a response on the Compiler Service public api change.

| DebugPoints (Expr.Const _, _) -> true
| _ -> false

// Filters seem to generate invalid code for the ilreflect.fs backend
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a bug for this?

@@ -9,18 +9,22 @@ open System.Reflection
open System.Text
open System.Runtime.CompilerServices

exception IllegalFileNameChar of string * char
exception internal IllegalFileNameChar of string * char
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as the last pr it was in, is it possible that a compiler service developer does or could reasonably depend on this exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, no one will rely on this, it should never have been public

System.Console.WriteLine()
with
| _ -> System.Console.WriteLine()
let test1() =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol ... I'm trying to kill fsharpqa and you are adding more tests to it :-).
It's fine, when I get to the emitted il tests I will move them.
Kevin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup :) However note that the emitted IL tests can't be moved until we have an ILDASM that reports debug sequence points, or find some other way of taking useful baselines which include debug sequence points. fsharpqa is currently the only way we have of validating the way sequence points relate to generated IL

@dsyme
Copy link
Contributor Author

dsyme commented Feb 18, 2022

This is a great change, I appreciate it greatly. Subject to a response on the Compiler Service public api change.

Yup the FCS change is ok

@KevinRansom KevinRansom merged commit 055ebe1 into dotnet:main Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

F# emits catch/rethrow for exception type tests
2 participants