WTG3007 code fix tries to delete overridden method with #if directive within it

[andrewhongnsw]

In the unusual case of overridden methods with #if directive code within it, the WTG3007 code fix attempts to delete the whole method:

protected override void Dispose(bool disposing)
{
    base.Dispose(disposing);
#if !Winzor
    // some code here
#endif
}

Recommend checking for #if directives within a method and allowing in such cases.

(edited for readability)

[brian-reichle]

I'm going to assume that the example you provided is just an "artists impression" of the actual code and the real code looks more like the following. If it is the actual code then whoever wrote it needs a serious talking to (the #if is removing the close brace of the method).

protected override void Dispose(bool disposing)
{
    base.Dispose(disposing);
#if !Winzor
    DoNonWinzorCleanUp();
#endif
}

On the one hand, this method doesn't really qualify as "pointless" per-se and so probably shouldn't raise the diagnostic; but on the other hand, I think in this case you would be better off writing the method as:

#if !Winzor
protected override void Dispose(bool disposing)
{
    base.Dispose(disposing);
    DoNonWinzorCleanUp();
}
#endif

[andrewhongnsw]

Yes good point, will be refactoring the code as your second example.

[yaakov-h]

On the one hand, this method doesn't really qualify as "pointless" per-se and so probably shouldn't raise the diagnostic

seems like it would have pointless vtable jumps, so the second option should be slightly faster assuming the CLR doesn't inline it all anyway

[brian-reichle]

assuming the CLR doesn't inline it all anyway

.NET Framework doesn't seem to inline any of this.

.NET Core on the other hand incline's it rather aggressively, I think the virtual call from Dispose() to Dispose(bool) was the only thing it didn't inline.

[yaakov-h]

I seem to remember something about the .NET Framework >= 4.7.2 doing some very basic devirtualization, but I can't find any reference to it now except one random Stack Overflow comment.

Ironically, with the #if !Winzor in place specifically, the net6.0 winzor build would probably get devirtualized and not become "pointless" overhead at runtime, whilst the regular net48 build has additional code so it's not pointless there either.