Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Enable CA2002:DoNotLockOnObjectsWithWeakIdentity #14289
Conversation
This change enables CA2002 and adds baseline files for current failures.
|
Do we care about this rule given we don't have Appdomains? The doc says "An object is said to have a weak identity when it can be directly accessed across application domain boundaries." |
|
This doesn't just have impact when app domains are involved. This also triggers when you lock on strings or objects that are public. Locking on strings in particular can be a very problematic pattern due to string interning. You should ideally only ever lock on |
|
The description of the rule at https://msdn.microsoft.com/en-us/library/ms182290.aspx does not match what the rule actually does. The description says that it kicks it for a very small set of types. Looking at this PR, it seems to be hitting for many other types. Historically, we assumed that it is a good hygiene to never lock on the public types. It is what this rule seems to be actually enforcing. It does not have much to do with AppDomains. I think we should keep it. |
|
Alex you might wish to encourage the doc owner (?Roslyn) to fix their doc then. |
| @@ -0,0 +1,3 @@ | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.DelimitedListTraceListener.#set_Delimiter(System.String)")] |
stephentoub
Dec 8, 2016
Member
What is the purpose of the lock in this case? It seems to be pointless in the current implementation... the right fix may be to just remove the lock rather than suppressing the warning.
What is the purpose of the lock in this case? It seems to be pointless in the current implementation... the right fix may be to just remove the lock rather than suppressing the warning.
| @@ -56,6 +56,8 @@ protected override void Dispose(bool disposing) | ||
| } | ||
| } | ||
|
|
||
| //TODO: Add this to FxCopBaseline.cs once https://github.com/dotnet/roslyn/issues/15728 is fixed |
stephentoub
Dec 8, 2016
Member
This is an internal type, so we could just change it from an explicit to an implicit interface implementation without impacting public surface area.
This is an internal type, so we could just change it from an explicit to an implicit interface implementation without impacting public surface area.
alexperovich
Dec 8, 2016
Author
Member
I wasn't looking closely at all of the usages. This change is just about getting all of the baselines in. Then we can fix the individual issues and remove the suppression.
I wasn't looking closely at all of the usages. This change is just about getting all of the baselines in. Then we can fix the individual issues and remove the suppression.
| @@ -0,0 +1,3 @@ | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Uri.#CreateUriInfo(System.Uri+Flags)")] |
alexperovich
Dec 8, 2016
Author
Member
added
added
| @@ -0,0 +1,49 @@ | ||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.IO.SyncTextReader.#Dispose(System.Boolean)")] |
weshaggard
Dec 8, 2016
Member
Any reason these cannot be applied directly to the member?
Any reason these cannot be applied directly to the member?
alexperovich
Dec 8, 2016
Author
Member
They can be, but I only want to do that if they aren't things we want to fix.
They can be, but I only want to do that if they aren't things we want to fix.
stephentoub
Dec 8, 2016
Member
They can be, but I only want to do that if they aren't things we want to fix.
The vast majority of these are things we're not going to change.
They can be, but I only want to do that if they aren't things we want to fix.
The vast majority of these are things we're not going to change.
alexperovich
Dec 8, 2016
Author
Member
Then we should review the cases and add justifications to the suppressions.
Then we should review the cases and add justifications to the suppressions.
weshaggard
Dec 9, 2016
Member
Did you file an issue to review these and actually apply them to the members with justification?
Did you file an issue to review these and actually apply them to the members with justification?
alexperovich
Dec 9, 2016
Author
Member
#14382
#14382
|
@dotnet-bot test Innerloop Ubuntu14.04 Release Build and Test please (S.C.Concurrent test failure) |
* Enable CA2002:DoNotLockOnObjectsWithWeakIdentity This change enables CA2002 and adds baseline files for current failures. * Add FxCop baseline for System.Console * Add reference comment for System.Uri issue
This change enables CA2002 and adds baseline files for current failures.