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

[jnm2]

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.

Why do we not know for sure if the state at the time of lambda creation still holds? What we know is:

• The lambda did not get created unless the platform is Windows
• Therefore, if the lambda is executing, the platform is Windows.

How can the lambda be invoked from a non-Windows path, when it is only created at all in a Windows path?

Here's another example.

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

if (!OperatingSystem.IsWindows()) return 1;

await Host.CreateDefaultBuilder(args)
    .ConfigureServices((hostContext, services) =>
    {
        services.AddLogging(builder =>
        {
            // ⚠️ CA1416: This call site is reachable on all platforms. 'EventLoggerFactoryExtensions.AddEventLog(
            // ILoggingBuilder, Action<EventLogSettings>)' is only supported on: 'windows'.
            builder.AddEventLog(eventLogSettings =>
            {
                // ⚠️ CA1416: This call site is reachable on all platforms. 'EventLogSettings.SourceName' is only
                // supported on: 'windows'.
                eventLogSettings.SourceName = "Foo";
            });
        });
    }).Build().RunAsync();
return 0;

[jnm2]

What I would propose that we could do about it is to treat the rest of the method after the if (!OperatingSystem.IsWindows()) return 1; as though it was inside a method decorated with [SupportedOSPlatform("windows")].

More generally, any code paths that are only reachable by passing the assertion/guard would be treated as though they were inside a method with [SupportedOSPlatform] matching the assertion/guard.

This already works as you'd expect for methods decorated with [SupportedOSPlatform]. See the workaround in #7323.

[jasonmalinowski]

I ran into this as well and agree with @jnm2's analysis here: the lambda doesn't even exist due to the OS check, so unless we have a magical technology to run the lambda on another machine, it's safe. My pattern in this case was:

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
    app.Services.GetRequiredService<IHostApplicationLifetime>().ApplicationStarted.Register(() => File.SetUnixFileMode(...));
}

The lambda doesn't exist on Windows, so there's no concern about calling the Unix API. And in this case there's no fancier analysis needed here since I'm not writing the lambda to a local or anything like that.

(As a second test I also tried sticking an UnsupportedOSPlatform attribute on the lambda, but that seems like that didn't work.)

[jasonmalinowski]

@buyaa-n should we reopen this bug, or reactivate @jnm2's other bug?