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

Fix concurrency issue in CA1069 analyzer #5243

Merged
merged 5 commits into from
Jul 16, 2021

Conversation

RikkiGibson
Copy link
Contributor

Fixes #3871

I ended up revising the way this works pretty heavily.

Initially I tried to fix this by gathering up all the initializer operations into a ConcurrentDictionary<IFieldSymbol, IFieldInitializerOperation>, then enumerating the enum members at the end and looking up the initializers.

It occurred to me that most enum values are probably not duplicates, though, and it would probably take less memory and fewer modifications on concurrent collections by instead using:

  1. a membersByValue dictionary which is populated single-threaded and read concurrently
  2. a duplicates set which is read and written concurrently

@jmarolf @sharwell

@RikkiGibson RikkiGibson requested a review from a team as a code owner July 14, 2021 21:30
@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #5243 (7264d62) into main (f64768f) will increase coverage by 0.00%.
The diff coverage is 98.29%.

@@           Coverage Diff           @@
##             main    #5243   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files        1200     1200           
  Lines      276283   276350   +67     
  Branches    16623    16632    +9     
=======================================
+ Hits       264078   264147   +69     
+ Misses      10019    10017    -2     
  Partials     2186     2186           

@Youssef1313
Copy link
Member

There was another PR attempting to fix the same issue #5170.

{
Value1 = 1,
Value2 = Value1,
Value3 = Value2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a behavior change. The idea here is that if you are going to explicitly include a duplicate, its initializer should reference the same "original". To fix this code the user should change Value3 = Value2 to Value3 = Value1. I felt like this was in the spirit of the docs but I'm also fine with changing the behavior back so this test produces no diagnostics.

Copy link
Member

Choose a reason for hiding this comment

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

I think this makes sense here, but might not for bitwise enums. I'll try to find an example.

Copy link
Member

Choose a reason for hiding this comment

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

Consider MethodAttributes for example. The members PrivateScope and ReuseSlot have the same value; if another member were added to the end with the intended value Family | ReuseSlot, it would be annoying to have a warning appearing saying you need to put Family | PrivateScope instead (which is technically the same value, but clearly less meaningful).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added a test for this scenario in 37d8ae9. It appears the diagnostics given have not changed from before this PR.

@jmarolf jmarolf requested review from sharwell and jmarolf July 14, 2021 22:35
{
Value1 = null
}",
DiagnosticResult.CompilerError("CS0037").WithSpan(4, 14, 4, 18).WithArguments("int"));
Copy link
Member

Choose a reason for hiding this comment

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

💡 For this case I would normally just use markup syntax {|CS0037:...|} since it's an expected part of the input.

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.

CA1069: Indeterministic false positive
4 participants