Compiler warning for lock when the target type is string

[alfattah-id]

Compiler warning for lock when the target type is string

Summary

I recently found a behavior that is valid according to the current C# lock semantics, but is very easy to misunderstand and can silently reduce concurrency.

C# currently allows this without any compiler warning:

private readonly string _valueLock = "";

lock (_valueLock)
{
    // Critical section
}

Because string literals may be interned, two unrelated object instances can reference the same string object and therefore use the same monitor.

The code remains thread-safe, but work that should be independent can unexpectedly become serialized.

I propose that the C# compiler produce a warning whenever the compile-time type of a lock expression is string.

---

Reproduction

Consider this class:

public sealed class SafeValue<T>
{
    private readonly string _valueLock = "";
    private T _value;

    public SafeValue(T value)
    {
        _value = value;
    }

    public T LockGet(int delay)
    {
        lock (_valueLock)
        {
            if (delay > 0)
                Thread.Sleep(delay);

            return _value;
        }
    }

    public void LockUpdate(T value, int delay)
    {
        lock (_valueLock)
        {
            if (delay > 0)
                Thread.Sleep(delay);

            _value = value;
        }
    }
}

Test it using two separate instances:

var a = new SafeValue<string>("SAMPLE-A");
var b = new SafeValue<string>("SAMPLE-B");

var threadA = new Thread(() =>
{
    a.LockGet(2000);
    a.LockUpdate("SAMPLE-A-N", 1000);
});

var threadB = new Thread(() =>
{
    b.LockGet(1000);
    b.LockUpdate("SAMPLE-B-N", 3000);
});

threadA.Start();
threadB.Start();

threadA.Join();
threadB.Join();

A developer may reasonably expect operations on a and b to run concurrently because:

• a and b are different instances
• _valueLock is an instance field
• _valueLock is private
• _valueLock is readonly
• _valueLock is not static

However, both _valueLock fields may reference the same interned empty-string object.

Conceptually:

SafeValue instance A ──┐
                       ├── interned string object ""
SafeValue instance B ──┘

As a result, both instances lock the same monitor and otherwise independent operations become serialized.

Changing the field to this restores the expected per-instance concurrency:

private readonly object _valueLock = new object();

---

Why this is misleading

The syntax:

lock (_valueLock)

can easily be interpreted as protecting the field or variable named _valueLock.

The actual semantics are based on the identity of the object returned by the expression.

Conceptually, the code behaves like this:

object temporary = _valueLock;
bool lockTaken = false;

try
{
    Monitor.Enter(temporary, ref lockTaken);

    // Critical section
}
finally
{
    if (lockTaken)
        Monitor.Exit(temporary);
}

The monitor belongs to the referenced object.

It does not belong to:

• the variable
• the field
• the storage location
• the containing class instance

This distinction is especially easy to miss when the field is private, readonly, non-static, and declared separately in each object instance.

---

Scope

This behavior applies to both legacy and modern runtimes, including:

• .NET Framework
• .NET Core
• .NET 5 and later

The issue is based on C# monitor semantics and string interning, not on a specific runtime version.

For that reason, the diagnostic should be implemented at the compiler or language-analysis level.

---

Related work

The risks of locking on strings are already known.

Related mechanisms include:

• CA2002, which warns against locking on objects with weak identity
• System.Threading.Lock, available on modern .NET targets
• existing runtime discussions about string interning and string deduplication

However, these mechanisms do not fully solve the discoverability problem:

• CA2002 may not be enabled
• analyzer severity varies between projects
• older .NET Framework projects may not run modern analyzers
• System.Threading.Lock is not available on .NET Framework
• existing lock(string) code can still compile silently

This proposal is specifically about compiler-level visibility when the compile-time type of a lock expression is string.

---

Proposal

The compiler should issue a warning whenever the compile-time type of a lock target is string.

Example:

string sync = "";

lock (sync)
{
}

Suggested diagnostic:

Do not lock on a string instance. String objects may be interned
or shared by unrelated code. Use a dedicated private readonly
synchronization object instead.

Recommended replacement:

private readonly object _sync = new object();

lock (_sync)
{
}

For modern targets, another valid option is:

private readonly System.Threading.Lock _sync = new();

The warning should apply regardless of whether the string comes from:

• a literal
• a field
• a local variable
• a property
• a method result
• a private or public member
• a static or non-static member

The important condition is that the compile-time type of the lock target is string.

---

Why a compiler warning is appropriate

The compiler already knows the compile-time type of the lock expression.

No complex flow analysis is required to detect this case.

For example:

string sync = GetSynchronizationValue();

lock (sync)
{
}

Even if the compiler cannot determine whether a particular string is currently interned, dynamically created, or shared elsewhere, string is still an unsafe synchronization target because its identity may be shared or explicitly interned.

A compiler warning would make the risk visible without depending entirely on optional analyzer configuration.

---

Compatibility

This proposal does not require changing:

• CLR monitor behavior
• Monitor.Enter
• Monitor.Exit
• string interning
• existing runtime semantics

The diagnostic could initially be introduced as a warning to preserve source compatibility.

Projects that intentionally lock on a string could suppress the warning explicitly.

The warning should also support normal .editorconfig severity configuration, for example:

dotnet_diagnostic.CSXXXX.severity = warning

or:

dotnet_diagnostic.CSXXXX.severity = error

---

Optional strict lock analysis

If enabling the warning by default is considered too disruptive, an optional strict lock-analysis mode could be introduced, similar to nullable reference analysis.

For example:

<PropertyGroup>
  <StrictLockAnalysis>enable</StrictLockAnalysis>
</PropertyGroup>

A strict mode could later detect additional unsafe lock targets such as:

lock (this)
{
}

lock (typeof(MyType))
{
}

However, the primary proposal is intentionally limited to string, because that case is direct, unambiguous, and easy for the compiler to identify.

---

Documentation improvement

The documentation for lock should prominently explain that:

lock (expression)

locks the monitor associated with the object reference produced by expression.

It does not lock the variable or field represented by that expression.

Conceptually, the current behavior is closer to:

lockReference(expression)

than:

lockVariable(expression)

No syntax change is required, but this distinction should be clearer in both documentation and diagnostics.

---

Expected outcome

The following code should not compile silently when modern safety diagnostics are enabled:

private readonly string _sync = "";

lock (_sync)
{
}

The compiler should guide the developer toward:

private readonly object _sync = new object();

lock (_sync)
{
}

This would help prevent:

• accidental monitor sharing between unrelated instances
• unexpected serialization
• hidden lock contention
• reduced throughput
• difficult-to-diagnose latency
• possible deadlocks involving otherwise unrelated components

The change would improve synchronization safety for both .NET Framework and modern .NET without changing runtime behavior.

[CyrusNajmabadi]

This is not something that is suitable at the compiler level. Locking on a string is fine, if you know the ownership of your string, and it is safe. For things like literals, that is likely not what you want (however, that is not something the compiler knows/tracks).

You could write an analyzer for this. Regardless, this isn't something the language has a say in. (it could say this is not allowed. but we would not do that as that would definitely be a breaking change for people doing this intentionally).

[alfattah-id]

Thanks for the clarification.

I agree that not every string reference is necessarily unsafe as a monitor target, especially when ownership and identity are fully controlled.

My main concern is the common and misleading case where a private, readonly, non-static field initialized from a string literal still causes unrelated instances to share the same monitor because of string interning.

I also agree that prohibiting lock(string) at the language level would be a breaking change.

Would it make sense to improve or enable analyzer diagnostics for this case, especially when the lock target is:

• a string literal;
• a field initialized from a string literal;
• or another compile-time-known interned string?

CA2002 appears related, but it is not consistently visible or enabled in many projects, especially older .NET Framework projects.

My goal is not necessarily to ban lock(string), but to make accidental shared-monitor scenarios much harder to miss.

[CyrusNajmabadi]

My main concern is the common and misleading case where a private, readonly, non-static field initialized from a string literal still causes unrelated instances to share the same monitor because of string interning.

Yup, this would be fine for an analyzer to do.

My goal is not necessarily to ban lock(string), but to make accidental shared-monitor scenarios much harder to miss.

I'd write an analyzer if i were you :)

[alfattah-id]

I tested CA2002 in a project and confirmed that it correctly detects both lock statements in my example.

So I agree that this can be handled by an analyzer.

My remaining concern is that CA2002 is not enabled by default. The code still compiles silently unless the developer already knows about the rule, installs or enables the analyzers, and configures its severity.

For a synchronization issue that can silently serialize unrelated instances, I think CA2002 should be enabled by default, at least for new projects or as part of the recommended analyzer ruleset.

The important outcome for me is that developers see a warning without having to know in advance that this specific rule exists.

[CyrusNajmabadi]

This is not a concern for the language (this is dotnet/csharplang here :)). You can request the owners of that analyzer consider enabling it by default. They can consider if it would be too noisy for the ecosystem.