Roslyn-analyzers: Guarded lambda expressions still warning

[buyaa-n]

Analyzer

Diagnostic ID: CA1416: Validate platform compatibility

Analyzer source

SDK: Built-in CA analyzers in .NET 5 SDK or later

Version: SDK 5.0.0-RC2

Describe the bug

We did a good job supporting lambda expressions in flow analysis, which covered all possible scenarios of guard methods within a lambda expression but seems lambda expressions within a guarded context is not covered. cc @mavasani @jeffhandley

public delegate void VoidCallback();

        public void DelegateAsArgument(VoidCallback callback) { }

        [SupportedOSPlatform("windows")]
        private void WindowsOnly() { }

        public void GuardedCalls()
        {
            if (OperatingSystem.IsWindows())
            {
                WindowsOnly(); // No warn, correct

                Action a = () =>
                {
                    WindowsOnly(); // Warns, fail
                };

                Func<string> greetings = () =>
                {
                    WindowsOnly(); // Warns, fail
                    return  "Hi";
                };

                DelegateAsArgument(() => WindowsOnly()); // Warns, fail
            }
        }

        public void AssertedCalls()
        {
            Debug.Assert(OperatingSystem.IsWindows());

            WindowsOnly(); // No warn, correct

            Action a = () =>
            {
                WindowsOnly(); // Warns, fail
                };

            Func<string> greetings = () =>
            {
                WindowsOnly(); // Warns, fail
                    return "Hi";
            };

            DelegateAsArgument(() => WindowsOnly()); // Warns, fail
        }

[mavasani]

I would like to propose that these be considered as by-design. Passing a lambda as delegate means whenever it gets invoked by the callee, we do not know for sure if the state at the time of lambda creation still holds. For above example, consider the delegate is saved by the callee onto some field and then invoked from a non-Windows path. I think the correct resolution here is for the user to add a Debug.Assert at the start of the lambda for the current OS information.

[buyaa-n]

I think the correct resolution here is for the user to add a Debug.Assert at the start of the lambda for the current OS information

Yeah, i am doing that for now. It just needs more asserts, like some cases the lambda itself is platform-dependent and need to add the Assert before lambda and inside lambda, which not look good 😄.

I would like to propose that these be considered as by-design.

Hm, should i close this issue then? Could the design changed/improved to support this? Then i might keep it for future improvement

[Evangelink]

@mavasani Would it be possible to store extra information in the DFA to say for example that this lambda contains windows-only operation and so that if it is called in an unknown or non-windows context we raise a warning?

[mavasani]

This issue is not about adding more functionality, but due to limitations that state at delegate creation can not be guaranteed to be the same as state at delegate invocation - the repro case explicitly assumes that, but it is very easy to create a reverse case where a false positive will be reported with such an assumption. No amount of data flow analysis can help here.

[Evangelink]

I am not suggesting to report on the delegate creation but rather to have something like this:

public class C {
    [SupportedOSPlatform("windows")]
    private void WindowsOnly() { }

    public void M() {
        Action a = () =>
        {
            WindowsOnly(); // Learn that the delegate 'a' has implicitly 'SupportedOSPlatform("windows")'
        };

        // Some more code

        if (!OperatingSystem.IsWindows()) {
            a(); // warn here (with maybe another rule) because we know a is windows and this is non-windows context
        }
    }
}

[mavasani]

@Evangelink that scenario already works. The scenario that doesn’t work is when delegate is passed to another method that is not analyzed interprocedurally. We explicitly assume that state at start of such a context free flow analysis is unknown, not the state when lambda was created.

[buyaa-n]

Thank you @mavasani for the explanation, i have added tests for future reference https://github.com/dotnet/roslyn-analyzers/pull/4281/files#diff-87f9d456dd43ada6755d14e49e58265fR3570. I am gonna close this issue as we have a workaround: enable the interprocedural analysis https://github.com/dotnet/roslyn-analyzers/pull/4281/files#diff-87f9d456dd43ada6755d14e49e58265fR3633