New Rule S2445: Blocks should be synchronized on read-only fields

[antonioaversa]

Fixes #6701

Related RSPEC update: SonarSource/rspec#1570

Remarks:

Naming

Description and naming based on the fact that, while in Java blocks are synchronized (the verb) by having fields in synchronized (the keyword), in C#/VB.NET blocks are synchronized (they aren't locked themselves) by having fields in lock (the keyword). C# doesn't have support for synchronized methods.

Rule interpretation

The interpretation of the rule is that, when a block is synchronized on a field F then F should be both private and readonly.
Another possible interpretation is the following: when a block is synchronized on F then F should be a field, private and readonly.
The second interpretation would produce many false positives, as it is not uncommon in C# to use private properties wrapping the backing field.
Moreover, methods may be used to return a dynamically calculated object, which should be the target of the lock. That seems reasonable, especially when the locking behavior should be fine-grained (e.g. locking on a row object, a col object, or a cell object, depending on a complex condition).
For these reasons (plus the fact that the rule is of type BUG), the first interpretation was chosen.

False negatives

Of course, a method could hide the use of a local variable: lock ((x => localVar)()) { ... }.
Similarly, assignment is not supported and can also hide the use of a local variable: lock (x = localVar) { ... }.
Since the rule is a BUG defined in SonarWay, having all those scenarios as false negative was the selected behavior.

[antonioaversa]

By looking at this test case, I have some doubts about the validity of the rule on local method variables.

The code for this IT is the following:

TypeMap ResolveTypeMap(in TypePair typePair)
{
    if (_resolvedMaps.TryGetValue(typePair, out TypeMap typeMap))
    {
        return typeMap;
    }
    if (_sealed)
    {
        typeMap = _runtimeMaps.GetOrAdd(typePair);
        // if it's a dynamically created type map, we need to seal it outside GetTypeMap to handle recursion
        if (typeMap != null && typeMap.MapExpression == null)
        {
            lock (typeMap)
            {
                typeMap.Seal(this, null);
            }
        }
    }
    else
    {
        typeMap = GetTypeMap(typePair);
        _resolvedMaps.Add(typePair, typeMap);
    }
    return typeMap;
}

While typeMap is a local variable, which can mutate, it is read from _runtimeMaps.GetOrAdd(typePair), which is a private readonly LockingConcurrentDictionary<TypePair, TypeMap>.

So basically the variable itself is not immutable, but the collection from which is read it is, and such a collection is used to have fine-graned locking based on TypePair.

While this all seems perfectly legitimate to me, and even necessary when you need a variable number of locks, I don't see how to easily distinguish between this scenario and the other ones.

Do we accept it as a false positive, or do we reduce the scope of the rule, to just ignore local variables?

What do you think?

[zsolt-kolbay-sonarsource]

I think it can be refined by adding an exception to the rule: if the code block is locked on a local variable and that local variable is assigned the value of a method call then it should be ignored.

var localVar = MethodCall();
...
lock (localVar) // Compliant
{
  ...
}

But I'd do this in a separate PR, and mark it as FP for now.

@andrei-epure-sonarsource What do you think?

[andrei-epure-sonarsource]

sorry I didn't reply. If it's an edge case, it's acceptable to have an FP.

[antonioaversa]

While it appears in our ITs, it seems to me enough rare, to consider it as an FP.

[zsolt-kolbay-sonarsource]

Kudos for the level of detail in your test cases!
I only have some minor comments.
The additional test cases I mention in my comments are optional. I'll leave it to you to decide whether to add them or not.

[zsolt-kolbay-sonarsource]

I think it can be refined by adding an exception to the rule: if the code block is locked on a local variable and that local variable is assigned the value of a method call then it should be ignored.

var localVar = MethodCall();
...
lock (localVar) // Compliant
{
  ...
}

But I'd do this in a separate PR, and mark it as FP for now.

@andrei-epure-sonarsource What do you think?

[antonioaversa]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs 0 Vulnerabilities 0 Security Hotspots 1 Code Smell

100.0% Coverage 0.0% Duplication

Intentionally avoided ?., since in this case, it makes the code less legible.

[zsolt-kolbay-sonarsource]

LGTM
I only added suggestions for some minor fixes.

[mary-georgiou-sonarsource]

Suggestions to make things a bit clearer.

[mary-georgiou-sonarsource]

also this condition would be nice to be extracted to a local function with a name that describes what it checks.

[antonioaversa]

Done.

As in #6750 (comment), that has decreased the method complexity at the expense of the overall complexity of the class, due to an indentation more and the use of ?..

[antonioaversa]

@mary-georgiou-sonarsource Assigned back to you despite the failing checks on Sonar.Net and Sonar.Net (Tests: Java ITs Others), which don't seem specific to this PR, as they are happening on other PRs too (and master), as reported by Andrei.

[mary-georgiou-sonarsource]

@mary-georgiou-sonarsource Assigned back to you despite the failing checks on Sonar.Net and Sonar.Net (Tests: Java ITs Others), which don't seem specific to this PR, as they are happening on other PRs too (and master), as reported by Andrei.

@antonioaversa I think the issue was fixed on master so if you rebase it should be ok in this PR as well.

[mary-georgiou-sonarsource]

Nitpicks

[mary-georgiou-sonarsource]

Suggested change

	|| expression.IsKnownType(KnownType.System_String, model);
	|| expression.IsKnownType(KnownType.System_String, model);

[antonioaversa]

I have made the change.
I am a bit lost with the indentation rules here: they seem context-sensitive.

For example:

{
    return A
        && B
}

but

=>
    A
    && B

So, basically && indents to A, or to its level above, depending on the context.
Coding styles, as defined here, don't seem to define a rule precisely.

[mary-georgiou-sonarsource]

@antonioaversa Indeed - could you please drop a message for this in the team channel on slack?

[andrei-epure-sonarsource]

I left a comment , but don't wait for my approval

[mary-georgiou-sonarsource]

Offline discussion with @antonioaversa and @pavel-mikula-sonarsource
The scope of the rule will change a bit so that it covers scenarios that won't raise too many FPS.
We ask the lock field to be readonly + not a string + not a new instance of an object.

[mary-georgiou-sonarsource]

Nitpicks.

[mary-georgiou-sonarsource]

Suggested change

	private static IFieldSymbol FieldWritable(ExpressionSyntax expression, Lazy<ISymbol> lazySymbol) =>
	private static IFieldSymbol FieldIsWritable(ExpressionSyntax expression, Lazy<ISymbol> lazySymbol) =>

[antonioaversa]

The name FieldIsWritable seems to assume that the expression provided as input is necessarily a field.
In this case the method receives an ExpressionSyntax which may or may not be a field: for example it may be a SimpleMemberAccessExpressionSyntax accessing a property, for which Expression would be an IPropertySymbol.

IsFieldWritable would already be a better name, but imo violates the principle of least astonishment: one would expect a method starting with Is to return a bool. Instead, this method returns a IFieldSymbol which may or may not be null, depending on whether all the conditions of "being a member", "referring to a field" and "being a non-readonly field" are respected.

The closest semantics to this is the one of (a very elaborate) as operator: if you prefer, I may change the name into AsWritableField. WritableField may suggest a "cast" semantic, i.e. it may suggest throwing an exception rather than returning null.

However, I would avoid IsFieldWritable or FieldIsWritable... unless there is a specific code convention driving the choice. If there's not, I would assume any options of the ones above, except maybe IsFieldWritable due to POLA violation, should be considered valid for this PR.

[mary-georgiou-sonarsource]

🤦 my bad - you are right.
I think I'd just change it then to FindWritableField or ExtractFieldSymbol or something a bit more descriptive if you have other ideas.
IMO just FieldWritable is a bit confusing.

[mary-georgiou-sonarsource]

These seem to be unused fields - in case they should be deleted to reduce the noise.

[mary-georgiou-sonarsource]

Same for this - it can be deleted.

[pavel-mikula-sonarsource]

3 out of 4 should be removed

[antonioaversa]

staticReadWriteField seems the only one which is not used anymore:

• staticReadonlyField reference is returned on line 29
• readonlyField reference is returned on line 14
• readWriteField reference is returned on line 17

@pavel-mikula-sonarsource: was it rather 3 out of 4 to be kept?

[mary-georgiou-sonarsource]

Maybe change the name of this class to reflect better what it's used to test.
I had to do couple of ups and downs to really understand.

[pavel-mikula-sonarsource]

LGTM with some comments to be addressed before merging

[pavel-mikula-sonarsource]

None of the fields here is used => remove them all

[pavel-mikula-sonarsource]

3 out of 4 should be removed

[pavel-mikula-sonarsource]

All of them are locked, this branch is distinguished by being local

Suggested change

	else if (expression is IdentifierNameSyntax && lazySymbol.Value is ILocalSymbol lockedSymbol)
	else if (expression is IdentifierNameSyntax && lazySymbol.Value is ILocalSymbol localSymbol)

[andrei-epure-sonarsource]

@antonioaversa you have the approvals, can we get this closed asap today to do the validation?

if there are still some edge-case tests to be added or nitpicks to be addressed, they can be left out , we need to move forward, validate on Peach and close the sprint

[mary-georgiou-sonarsource]

LGTM - There are some small nitpicks only left - no need to re-approve.

[antonioaversa]

@pavel-mikula-sonarsource , @mary-georgiou-sonarsource : I have pushed a commit with the changes done after the last round of review. I am merging this, so that @zsolt-kolbay-sonarsource can deploy the master on peach and we can do validation of the last GitHub issues of the last sprint.
If you see something wrong or missing, please raise a flag here and I would address the new issues on a dedicated PR.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication