Design dataflow analysis violations in multi-threaded scenarios

[mavasani]

Newly implemented DFA based rules flag some violations which make sense if the code was single threaded, but are false reports if the function is multi-threaded.

void M()
{
   if (_field != null)
   {
      return;
   }

   // Computation via static method invocations where 'this' instance is not passed...
   // Note that we are already conservative if an instance method is invoked or 'this' reference is passed as an argument.

   // _field might have changed from another thread, but we flag [CA1508] (https://github.com/dotnet/roslyn-analyzers/pull/1637) that below condition is always false.
   if (_field != null)
   {
   }
}

I am not sure if we can do much here other than improve the diagnostic message to warn about being cautious for multi-threaded scenarios. Note that changing the code as per the violation can lead to subtle race-condition bugs.

[bartdesmet]

FWIW, I've just hit several instances of this for the double-checked locking pattern, e.g.:

private static ModuleBuilder s_mod;

private static ModuleBuilder Module
{
    get
    {
        if (s_mod == null)
        {
            lock (s_lock)
            {
                if (s_mod == null)
                {
                    s_mod = Assembly.DefineDynamicModule("Closures");
                }
            }
        }

        return s_mod;
    }
}

Maybe some typical popular patterns could be checked for even if not every single multi-threaded scenario can be addressed in a general manner?

[Treit]

I also just had this flagged in an existing codebase using the double checked locking pattern. It would be very helpful if this common pattern at least was not flagged by this analyzer.

[CyrusNajmabadi]

I'm actually ok with this flagging double-checked locking. Since it seems like a highly suspect pattern in the first place (for example, it would not work if any writes get reordered from teh perspective of the thread outside of the lock).

[CyrusNajmabadi]

One option (for the double-checked locking case) is to reset the data assumptions across certain operations (like locks).

[CyrusNajmabadi]

@mavasani

Newly implemented DFA based rules flag some violations which make sense if the code was single threaded, but are false reports if the function is multi-threaded.

Note: i think the analyzer is correct for that code, and you should not change it. What would be a reasonable reason to change things is if there are locks involved, or usages of things like Interlocked/Volatile, or if that field is volatile.