Modify rule S2445: Add C# description and samples

[antonioaversa]

Implementation #6750

Kept as draft as it is to be merged after the implementation.

Narrowed scope for C#

The definition of the rule for C# has been narrowed during the review.
Outcome of the offline discussion:

• it may make sense to have a lock on a field of a class different from the one where the lock is acquired, for example in a situation where locking objects are externalized and shared among multiple classes, for inter-class synchronization (e.g. a serial port lock);
• so, requiring the readonly field to also be private appears in the end too constraining;
• it also makes the rule too complex and non-atomic, and doesn't allow the user to opt in one behavior (lock on a readonly object), opting out the other (lock on a non-private object);
• you may also want to have it protected, instead of private, to allow methods of a derived type to synchronize with methods of the base type, and you don't want to be forced to use properties to do that.

For these reasons, we are changing the rule and narrowing its scope for C#: when locking on a field, that field should be readonly. Whether it's private or not, it won't matter for the implementation of this rule.

[sonarsource-next]

SonarQube Quality Gate for 'rspec-tools'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarsource-next]

SonarQube Quality Gate for 'rspec-frontend'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[zsolt-kolbay-sonarsource]

LGTM
One note: shouldn't this be applied to VB.NET as well? Locking works there exactly the same way as it does in C# (with the SyncLock keyword), and it seems like a simple rule.

[antonioaversa]

@zsolt-kolbay-sonarsource

LGTM One note: shouldn't this be applied to VB.NET as well? Locking works there exactly the same way as it does in C# (with the SyncLock keyword), and it seems like a simple rule.

While only C# is mentioned in the task, it surely could. If you and @mary-georgiou-sonarsource both agree, I would invest some time extending the implementation to VB.NET as well.

[mary-georgiou-sonarsource]

@zsolt-kolbay-sonarsource

LGTM One note: shouldn't this be applied to VB.NET as well? Locking works there exactly the same way as it does in C# (with the SyncLock keyword), and it seems like a simple rule.

While only C# is mentioned in the task, it surely could. If you and @mary-georgiou-sonarsource both agree, I would invest some time extending the implementation to VB.NET as well.

In case this rule is valid also for VB.NET (I haven't checked) - IMO, it's better to create another issue regarding the VB.NET version of the rule and implement the C# part in way that the implementation can be re-used for the VB.NET (by putting the common code in a base class).

[antonioaversa]

@zsolt-kolbay-sonarsource

LGTM One note: shouldn't this be applied to VB.NET as well? Locking works there exactly the same way as it does in C# (with the SyncLock keyword), and it seems like a simple rule.

While only C# is mentioned in the task, it surely could. If you and @mary-georgiou-sonarsource both agree, I would invest some time extending the implementation to VB.NET as well.

In case this rule is valid also for VB.NET (I haven't checked) - IMO, it's better to create another issue regarding the VB.NET version of the rule and implement the C# part in way that the implementation can be re-used for the VB.NET (by putting the common code in a base class).

Reporting the outcome of the discussion offline on the possibility of extending the rule to VB.NET.

We are going to have a dedicated GitHub issue to implement this rule for VB.NET, rather than extending this GitHub issue.

We are going to stick with the C#-specific implementation for the time being, since the effort to refactor the code into a base class is possibly 90% or more, of the effort to implement the VB.NET rule.

[mary-georgiou-sonarsource]

Round 1:

• I'd update the description to match better the C# case. Seeing Pavel's comment regarding the title - I agree - leave as is.
• Add in the example the case of the interned string
• Please add educational links in the RSPEC. For example: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock and https://learn.microsoft.com/en-us/dotnet/api/system.string.intern?view=net-7.0

You'll need to rebase on master for the pipeline to succeed.

[pavel-mikula-sonarsource]

Cleanup

[antonioaversa]

@pavel-mikula-sonarsource @mary-georgiou-sonarsource I addressed your comments and I am reassigning this PR back to you, although GitHub Checks are currently failing.

The issue with the GitHub Checks seems to come from https://next.sonarqube.com/sonarqube/batch/index returning 503, rather than by actual issues with the adoc content itself.

[mary-georgiou-sonarsource]

• Refining a bit more the description
• One file probably needs reverting
• Please add the educational links

[mary-georgiou-sonarsource]

Just a bit more decription refinement - looks good otherwise.

[pavel-mikula-sonarsource]

@pavel-mikula-sonarsource We have the already existing SonarSource/sonar-dotnet#6750.
Can't we use that one?

Yes, sure. I wasn't aware in what state the actual implementation PR is.

[pavel-mikula-sonarsource]

Final polishing

[pavel-mikula-sonarsource]

LGTM

[pavel-mikula-sonarsource]

Is there a ticket created for the part that was left out? It should be in sonar-dotnet,labeled with the new rule idea

[mary-georgiou-sonarsource]

LGTM

[antonioaversa]

Is there a ticket created for the part that was left out? It should be in sonar-dotnet,labeled with the new rule idea

Created SonarSource/sonar-dotnet#6804

@pavel-mikula-sonarsource

[sonarsource-next]

SonarQube Quality Gate for 'rspec-tools'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarsource-next]

SonarQube Quality Gate for 'rspec-frontend'

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information