Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add [ThreadStatic] correctness analyzers #5920

Merged
merged 2 commits into from
Mar 17, 2022

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Mar 15, 2022

Fixes dotnet/runtime#46626

Note that we'd discussed adding a fixer for the case of [ThreadStatic] applied to an instance field, but on further thought I don't think that's a good idea. If someone is using [ThreadStatic] on an instance field, it's possible it's a mistake and they intended for it to be static, but it's also quite likely they assumed it would create a per-thread instance value (as you might get with ThreadLocal<T>) so providing a fix to just mark it static would potentially lead someone down the wrong path, and just adding static is trivial for someone to do. There should also be very few violations of this, so a bulk fix isn't going to be meaningful.

@stephentoub stephentoub requested a review from a team as a code owner March 15, 2022 21:35
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #5920 (9e05a37) into main (2b89a41) will decrease coverage by 0.00%.
The diff coverage is 96.44%.

@@            Coverage Diff             @@
##             main    #5920      +/-   ##
==========================================
- Coverage   96.03%   96.03%   -0.01%     
==========================================
  Files        1310     1312       +2     
  Lines      302825   302990     +165     
  Branches     9572     9579       +7     
==========================================
+ Hits       290808   290964     +156     
- Misses       9733     9739       +6     
- Partials     2284     2287       +3     

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge cases to consider whether they should be supported:

using System;

public class C
{
    [field: ThreadStatic]
    public string S { get; }

    [field: ThreadStatic]
    event EventHandler E;
}

public record R([field: ThreadStatic] string S);

@stephentoub stephentoub force-pushed the threadstaticanalyzers branch from 2585e3c to c67a612 Compare March 16, 2022 18:24
@stephentoub
Copy link
Member Author

Edge cases to consider whether they should be supported:

I added support for properties (which implicitly adds support for records). I don't believe it's possible today to find the backing field for an IEventSymbol.

@stephentoub
Copy link
Member Author

Thanks for reviewing, @Youssef1313.

@stephentoub stephentoub force-pushed the threadstaticanalyzers branch from c67a612 to 9e05a37 Compare March 17, 2022 19:15
@stephentoub stephentoub merged commit 0384031 into dotnet:main Mar 17, 2022
@stephentoub stephentoub deleted the threadstaticanalyzers branch March 17, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analyzer proposal: ThreadStatic on a non-static field
3 participants