Analyzer proposal: Do not use ThreadStatic on non-static fields

[Evangelink]

Using the ThreadStatic attribute on a non-static field is a no-op which can result in unexpected result as the code won't behave as you would imagine.

Could relates to #594

[AArnott]

Is this a bug report, or an analyzer proposal? Please clarify by editing your issue description.

[sharwell]

I think @stephentoub was interested in having the ThreadStatic analyzers in dotnet/roslyn-analyzers.

[Evangelink]

I am quite confused which repo should handle things. IMO it seems weird to reference an analyzer with VisualStudio in its name when you are not doing something related to VS. My understanding is that the split is done because of people skills.

[weltkante]

Might have been a misunderstanding in dotnet/roslyn-analyzers#3408 to move it off roslyn-analyzers in the first place. This repo only has VisualStudio in its name historically and because VS is the primary user of the threading services provided, and ThreadStatic having "thread" in its name may have lead to suggesting to move it to the threading library, even though it doesn't have much to do with this library in itself.

[AArnott]

@weltkante is correct in that "VisualStudio" in the name of this repo only comes from its sponsorship. This library and its analyzers absolutely apply to any application that has threading concerns.

As far as adding new analyzers related to ThreadStatic, I don't object to it. And I don't know why a general threading related rule would go into a more focused library like roslyn-analyzers (if indeed it's focused on the roslyn compiler). So if an analyzer for this is to be introduced, this repo seems more appropriate of the two.
That said, some folks wish that the analyzers here were actually in the FxCop-successor analyzer packages, so if the ThreadStatic analyzer were added to that package, that might be even better. And if roslyn-analyzers is where the fxcop-replacement analyzers go, then that sounds like a better home.

FYI eventually the plan is to add a package ref from the FxCop packages to this one, so that makes all our analyzers here de-facto members of that package.

[stephentoub]

And I don't know why a general threading related rule would go into a more focused library like roslyn-analyzers (if indeed it's focused on the roslyn compiler).

It isn't. In addition to containing all of the analyzers for roslyn, it's also where all of the ported FxCop rules live, and where new rules are being added for .NET 5.

[AArnott]

Great. Do you have a work item that we can link from here for the roslyn-analyzers work?

[stephentoub]

You mean for new rules being added for .NET 5?
https://github.com/dotnet/runtime/projects/46

[AArnott]

That's a great link, @stephentoub. But I don't see anything about ThreadStatic there. Should there be a card added there, or do you want to define this analyzer in this repo so it can target < .NET5?

[stephentoub]

They had been filed as issues in dotnet/roslyn-analyzers, as they were opened a while ago, and then those issues were apparently closed and replaced by issues in VS threading ;)

My preference is for them to be tracked in dotnet/runtime or dotnet/roslyn-analyzers, and I believe @terrajobst wants them in dotnet/runtime, so, that.

As for .NET 5, these analyzers aren't limited to targeting .NET 5. They're generally just looking for the presence of certain APIs, and if they exist, they work. Many of the rules will be enabled by default for .NET 5 as part of the SDK, but anyone can grab the analyzer package from nuget, as many folks do today.

[AArnott]

On the @terrajobst intent, I guess we have a fresh conversation going on over at dotnet/runtime#33774, so we can carry that on there.
Suffice it to say though, if the winds blow toward roslyn-analyzers, let's get those issues reactivated then and I'll close this one.