If this doesn't touch bodies without a filter, how will this know not to run the outer filter when ArgumentException is throw here:

using System;

class Program
{
    static void Main()
    {
        foreach (var exception in new Exception[] { new ArgumentException(), new Exception(), new NullReferenceException() })
        {
            try
            {
                try
                {
                    try
                    {
                        throw exception;
                    }
                    catch (NullReferenceException) when (Print("inner"))
                    {
                        Console.WriteLine("In inner catch");
                    }
                }
                catch (ArgumentException)
                {
                    Console.WriteLine("In middle catch");
                }
            }
            catch (Exception) when (Print("outer"))
            {
                Console.WriteLine("In outer catch");
            }
        }

    }

    static bool Print(string s)
    {
        Console.WriteLine($"Running {s} filter");
        return true;
    }
}

That method has a filter so it will run and modify the outermost catch, if I understand your question correctly.

Okay, let's pretend each try/catch is in individual methods that call each other.

I just tried it locally, with the above snippet. Before rewriting :

In middle catch
Running outer filter
In outer catch
Running inner filter
In inner catch

After rewriting:

Running outer filter
In middle catch
Running outer filter
In outer catch
Running inner filter
In inner catch

I think this will need to pay a perf penalty for all try/catch blocks, not just the ones with a filter.

Given:

void A (Exception exc) {
    try {
        throw exc;
    } catch (NullReferenceException) when (Print("inner")) {
        Console.WriteLine("In inner catch");
    }
}

void B (Exception exc) {
    try {
        A(exc);
    } catch (ArgumentException) {
        Console.WriteLine("In middle catch");
    }
}

void C (Exception exc) {
    try {
        B(exc);
    } catch (Exception) when (Print("outer")) {
        Console.WriteLine("In outer catch");
    }
}

A and C will be rewritten and will evaluate filters when their catch receives an exception. My understanding of how exception filters are supposed to work says that this is correct and it matches what I've observed from desktop netframework.

Since the filter Print returns true, filter evaluation will stop at the first filter, which means that the C-level filter will not run unless the exception bubbles up out of both A and B.

The stack frame of B needs to put something on the filter chain that says: "I'm going to catch ArgumentException no matter what, so stop walking the filter chain if the exception derives from ArgumentException". From the output I pasted in the above comment, it seems like this part is missing.

Thanks for spotting this. It didn't come up in any of my test cases or the BCL scenarios I was looking at, but it does matter.

We're thinking this won't be something we fix at present and it will just be a documented limitation of the filtering tool (there is already one other limitation - generics don't work so it doesn't rewrite generic methods).

The baseline solution would just be clear documentation about this next to the switch to turn on filters. Do you think that's enough? Analyzing filters for observable side effects and generating a warning would be feasible, but the warning would be noisy since not all side effects are ones the user will actually observe, so I'd rather not burn time/energy doing that analysis. If the filters have no side effects then this bug is harmless.

The baseline solution would just be clear documentation about this next to the switch to turn on filters. Do you think that's enough?

Generics not working, or the closure not being able to capture byref-like types (like we discussed in dotnet/runtime#34921) feels less severe because it's easy to detect at the time of transforming and provide actionable error messages. Filters running when they shouldn't feels like something that will send people on a wild goose chase looking for mystery bugs until they find small print somewhere in release notes.

One thing that customers use filters for is to implement "crash dump" support - they run logging code in a filter to non-invasively log unhandled exceptions. These customers will see impossible exceptions leaking into the logger. Coincidentally, IIRC @filipnavara is one such customer - we discussed this in the past. Whether such behavior is acceptable really depends on whether these use cases are the customers this is targeting.

The rewriter as implemented has other visible behavioral differences:

class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Throw() => throw new NullReferenceException();

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void DoWork()
    {
        try
        {
            Throw();
        }
        catch (ArgumentException)
        {
            // Handle expected exception
        }
        finally
        {
            Console.WriteLine("This SHOULD NOT be the first line written to the console.");
        }
    }

    static void Main()
    {
        try
        {
            DoWork();
        }
        catch (Exception e) when (CaptureCrash(e)) { }
    }

    static bool CaptureCrash(Exception ex)
    {
        // e.g. People can log with EventSource that captures the stack at the time of loggig,
        // but for the purposes of a demo, let's just log Environment.StackTrace to console.
        if (Environment.StackTrace.Contains("DoWork"))
            Console.WriteLine("Exception happened while we were in DoWork.");
        else
            Console.WriteLine("Exception happended after DoWork was unwound.");
        return false;
    }
}

(The above will print the SHOULD NOT be the first line as the first line in the output and then take the else branch in CaptureCrash after transforming. The expected behavior is the opposite.)

These all look fixable with little overhead in your proposed model, except for the one place that requires bookkeeping whenever we enter an unfiltered try block.

I don't have a good sense for how much value the transform (as it is right now) is providing over simply doing the naive thing (move the code from the filter into an if block that runs as the first thing within the catch).

If we could achieve correctness (the transform either fails at build time, or works as expected), the value add would be very clear. I'm quite conflicted about the current semantics that try pretty hard to be correct with all the closures, but don't get there in the end.

(Don't get me wrong, I'm a fan of this transform and I can see it e.g. used in places like IL2CPP that have similar challenges mapping .NET EH model into C++ EH model).

One thing that customers use filters for is to implement "crash dump" support - they run logging code in a filter to non-invasively log unhandled exceptions. These customers will see impossible exceptions leaking into the logger. Coincidentally, IIRC @filipnavara is one such customer - we discussed this in the past. Whether such behavior is acceptable really depends on whether these use cases are the customers this is targeting.

Yep, we do that on Windows/CoreCLR. We already disabled it on macOS/Mono some time ago due to different issue so this would not really affect us too much. Now we essentially rewrite it as if inside catch manually and lose the ability to do full crash dumps. I don't think too many people do this but we are certainly not alone. Another thing that some people did is to use exception filters as a means to observe caught exceptions inside 3rd-party libraries. I do believe both of these use cases are rare but a documentation is bare minimum for the behavior change.