Roslyn: Conditional Local Functions are considered unused

[333fred]

using System.Diagnostics;

public class C {
    public void M() {
        local();

        [Conditional("DEBUG")]
        static void local() {} // Warning: unused local function
    }
}

This reproes when compiling for release mode.

[Youssef1313]

@333fred, Should the release build never reports this warning for local methods attributed with Conditional("DEBUG") even if they really unused, and leave this warning for the debug build instead? Or both debug and release warnings should be consistent?

[RikkiGibson]

The presence or absence of this warning should be unaffected by configuration.

One similar scenario where I think the warning should depend on configuration (please check my understanding @333fred):

using System.Diagnostics;

public class C {
    public void M() {
#if DEBUG
        local();
#endif
        [Conditional("DEBUG")]
        static void local() {} // Warning: unused local function
    }
}

I expect this to warn in release mode.

[Youssef1313]

@RikkiGibson I was thinking of just checking the attribute, but it seems this isn't the desired behavior :smile:

I gave the code for this a very quick read, and I think the following line isn't hit for the local function:

https://github.com/dotnet/roslyn/blob/851ecc5737e8ac3d8b337113e8a36a4db3ef9666/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs#L563

But I'm unaware of how flow analysis work.

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEAMACFBGAdAEQEsIBzAOwHtZCBjKAbgFgAoFlAZiwCYMBhDAN4sMIrJxQAWDAFkAFAEpBw0SoA2FGhFUKmzFSuX6MAbV4UyAE0IxC5rbIBE+AKIAhAKoBxB/IC6h/RwANiwpdU1tRQEAXwwAejiMAHUIBDJCMhIQDABXMhyoOAsMcK0MADM8mhtzAOiWaKA==

The compiler generated code for the corresponding code is:

using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Security;
using System.Security.Permissions;

[assembly: CompilationRelaxations(8)]
[assembly: RuntimeCompatibility(WrapNonExceptionThrows = true)]
[assembly: Debuggable(DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints)]
[assembly: SecurityPermission(SecurityAction.RequestMinimum, SkipVerification = true)]
[assembly: AssemblyVersion("0.0.0.0")]
[module: UnverifiableCode]
public class C
{
    public void M()
    {
    }

    [CompilerGenerated]
    [Conditional("DEBUG")]
    internal static void <M>g__local|0_0()
    {
    }
}

Does the flow analysis work on this version of the code? It might look a silly question, but I just don't know 😄

(Hope I'm not causing any inconvenience while trying to make mini-contributions 😄 )

[Youssef1313]

Note: this is a breaking change if fixed, you may want to include this in a warning wave.

[RikkiGibson]

It's not clear to me what's breaking about this change, since it removes warnings.

[RikkiGibson]

I think the fix to this bug is to ensure that the appropriate analysis pass adds the local function to the set of used local functions, regardless of the presence of any ConditionalAttribute, etc.

[Youssef1313]

It's not clear to me what's breaking about this change, since it removes warnings.

Ah very sorry. I mixed this with other thing I was thinking of. (was thinking of suggesting a similar warning for normal methods).

[RikkiGibson]

(Hope I'm not causing any inconvenience while trying to make mini-contributions 😄 )

Absolutely not, we're glad you're here.

[Youssef1313]

The problem seems to be here:

https://github.com/dotnet/roslyn/blob/768f4761b3948d9033579a394ae1156a5592eaf7/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs#L1171

The documentation for CallsAreOmitted explicitly says that it must return true if the condition isn't true in the source file.

So, the behavior for CallsAreOmitted is correct. But it's not the correct method we need to use here.