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

Suppress Lock conversion warnings when using object equality operators #72459

Merged
merged 3 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,38 @@ private BoundExpression BindSimpleBinaryOperator(BinaryExpressionSyntax node, Bi
Debug.Assert((object)signature.LeftType != null);
Debug.Assert((object)signature.RightType != null);

resultLeft = CreateConversion(left, best.LeftConversion, signature.LeftType, diagnostics);
resultRight = CreateConversion(right, best.RightConversion, signature.RightType, diagnostics);
// If this is an object equality operator, we will suppress Lock conversion warnings.
var needsFilterDiagnostics =
resultOperatorKind is BinaryOperatorKind.ObjectEqual or BinaryOperatorKind.ObjectNotEqual &&
diagnostics.AccumulatesDiagnostics;
var conversionDiagnostics = needsFilterDiagnostics ? BindingDiagnosticBag.GetInstance(template: diagnostics) : diagnostics;

resultLeft = CreateConversion(left, best.LeftConversion, signature.LeftType, conversionDiagnostics);
resultRight = CreateConversion(right, best.RightConversion, signature.RightType, conversionDiagnostics);

if (needsFilterDiagnostics)
{
Debug.Assert(conversionDiagnostics != diagnostics);
diagnostics.AddDependencies(conversionDiagnostics);

var sourceBag = conversionDiagnostics.DiagnosticBag;
Debug.Assert(sourceBag is not null);

if (!sourceBag.IsEmptyWithoutResolution)
{
foreach (var diagnostic in sourceBag.AsEnumerableWithoutResolution())
{
var code = diagnostic is DiagnosticWithInfo { HasLazyInfo: true, LazyInfo.Code: var lazyCode } ? lazyCode : diagnostic.Code;
if ((ErrorCode)code is not ErrorCode.WRN_ConvertingLock)
{
diagnostics.Add(diagnostic);
}
}
}

conversionDiagnostics.Free();
}

resultConstant = FoldBinaryOperator(node, resultOperatorKind, resultLeft, resultRight, resultType, diagnostics);
}
else
Expand Down
100 changes: 100 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1748,6 +1748,106 @@ .locals init (System.Threading.Lock.Scope V_0)
""");
}

[Fact]
public void ObjectEquality()
{
var source = """
#nullable enable
using System;
using System.Threading;

static class C
{
static void Main()
{
Lock? l = new();
if (l != null)
{
lock (l) { Console.Write("1"); }
}
if (l == null)
{
throw null!;
}
if (l is { })
{
lock (l) { Console.Write("2"); }
}
if (l is { } l2)
{
lock (l2) { Console.Write("3"); }
}
if (l is not { })
{
throw null!;
}
if (l is null)
{
throw null!;
}
if (l is not null)
{
lock (l) { Console.Write("4"); }
}
if (l is not null and var l3)
{
lock (l3) { Console.Write("5"); }
}
if (null != l)
{
lock (l) { Console.Write("6"); }
}
if (null == l)
{
throw null!;
}
if (!(l == null))
{
lock (l) { Console.Write("7"); }
}
if (!(l != null))
{
throw null!;
}

Lock? l4 = new();
if (l == l4)
{
throw null!;
}
if (l != l4)
{
lock (l4) { Console.Write("8"); }
}
if (ReferenceEquals(l, l4))
{
throw null!;
}
if (((object)l) == l4)
{
throw null!;
}
if (l == new Lock())
{
throw null!;
}
}
}
""";
var verifier = CompileAndVerify([source, LockTypeDefinition], verify: Verification.FailsILVerify,
expectedOutput: "E1DE2DE3DE4DE5DE6DE7DE8D");
verifier.VerifyDiagnostics(
// (68,29): warning CS9216: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement.
// if (ReferenceEquals(l, l4))
Diagnostic(ErrorCode.WRN_ConvertingLock, "l").WithLocation(68, 29),
// (68,32): warning CS9216: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement.
// if (ReferenceEquals(l, l4))
Diagnostic(ErrorCode.WRN_ConvertingLock, "l4").WithLocation(68, 32),
// (72,22): warning CS9216: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in 'lock' statement.
// if (((object)l) == l4)
Diagnostic(ErrorCode.WRN_ConvertingLock, "l").WithLocation(72, 22));
}

[Fact]
public void Await()
{
Expand Down
34 changes: 32 additions & 2 deletions src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,38 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
left = MakeRValue(left, diagnostics)
right = MakeRValue(right, diagnostics)

left = ValidateAndConvertIsExpressionArgument(left, right, [isNot], diagnostics)
right = ValidateAndConvertIsExpressionArgument(right, left, [isNot], diagnostics)
' Suppress Lock conversion warnings for Is operator.
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
Dim needsFilterDiagnostics = diagnostics.AccumulatesDiagnostics
Dim conversionDiagnostics = If(needsFilterDiagnostics, BindingDiagnosticBag.GetInstance(diagnostics), diagnostics)

left = ValidateAndConvertIsExpressionArgument(left, right, [isNot], conversionDiagnostics)
right = ValidateAndConvertIsExpressionArgument(right, left, [isNot], conversionDiagnostics)

If needsFilterDiagnostics Then
Debug.Assert(conversionDiagnostics IsNot diagnostics)
diagnostics.AddDependencies(conversionDiagnostics)

Dim sourceBag = conversionDiagnostics.DiagnosticBag
Debug.Assert(sourceBag IsNot Nothing)

If Not sourceBag.IsEmptyWithoutResolution Then
For Each diagnostic In sourceBag.AsEnumerableWithoutResolution()
Dim code As Integer
Dim diagnosticWithInfo = TryCast(diagnostic, DiagnosticWithInfo)
If diagnosticWithInfo IsNot Nothing AndAlso diagnosticWithInfo.HasLazyInfo Then
code = diagnosticWithInfo.LazyInfo.Code
Else
code = diagnostic.Code
End If

If code <> ERRID.WRN_ConvertingLock Then
diagnostics.Add(diagnostic)
End If
Next
End If

conversionDiagnostics.Free()
End If

Dim result As BoundExpression
Dim booleanType = GetSpecialType(SpecialType.System_Boolean, node, diagnostics)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -984,5 +984,90 @@ BC42508: A value of type 'System.Threading.Lock' converted to a different type w
IL_002b: ret
}]]>)
End Sub

<Fact>
Public Sub LockType_ObjectEquality()
Dim source = <![CDATA[
Imports System
Imports System.Threading

Module Program
Sub Main()
Dim l As Lock = New Lock()

If l IsNot Nothing Then
Console.Write("1")
End If

If l Is Nothing Then
Throw New Exception
End If

If l IsNot Nothing Then
Console.Write("2")
End If

If l Is Nothing Then
Throw New Exception
End If

If Not (l Is Nothing) Then
Console.Write("3")
End If

If Not (l IsNot Nothing) Then
Throw New Exception
End If

Dim l2 As Lock = New Lock()

If l Is l2 Then
Throw New Exception
End If

If l IsNot l2 Then
Console.Write("4")
End If

If ReferenceEquals(l, l2) Then
Throw New Exception
End If

If (CObj(l)) Is l2 Then
Throw New Exception
End If

If (CObj(l)) IsNot l2 Then
Console.Write("5")
End If

If l Is New Lock() Then
Throw New Exception
End If
End Sub
End Module

Namespace System.Threading
Public Class Lock
End Class
End Namespace
]]>.Value
Dim comp = CreateCompilation(source, options:=TestOptions.ReleaseExe)
Dim verifier = CompileAndVerify(comp, expectedOutput:="12345")
verifier.Diagnostics.AssertTheseDiagnostics(<![CDATA[
BC42508: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in SyncLock statement.
If ReferenceEquals(l, l2) Then
~
BC42508: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in SyncLock statement.
If ReferenceEquals(l, l2) Then
~~
BC42508: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in SyncLock statement.
If (CObj(l)) Is l2 Then
~
BC42508: A value of type 'System.Threading.Lock' converted to a different type will use likely unintended monitor-based locking in SyncLock statement.
If (CObj(l)) IsNot l2 Then
~
]]>)
End Sub
End Class
End Namespace