From e923e6ceaa059dceeea6f0970bb94dd27a64692b Mon Sep 17 00:00:00 2001 From: Jan Jones Date: Fri, 8 Mar 2024 14:07:24 +0100 Subject: [PATCH 1/3] Suppress Lock conversion warnings when using object equality operators --- .../Portable/Binder/Binder_Operators.cs | 33 +++++- .../CSharp/Test/Emit2/Semantics/LockTests.cs | 100 ++++++++++++++++++ .../Portable/Binding/Binder_Operators.vb | 26 ++++- .../Test/Semantic/Binding/SyncLockTests.vb | 85 +++++++++++++++ 4 files changed, 240 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs index 4e4347eba66c6..ab872e2ea4c74 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs @@ -637,8 +637,37 @@ 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.DiagnosticBag is not null; + 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()) + { + if ((ErrorCode)diagnostic.Code is not ErrorCode.WRN_ConvertingLock) + { + diagnostics.Add(diagnostic); + } + } + } + + conversionDiagnostics.Free(); + } + resultConstant = FoldBinaryOperator(node, resultOperatorKind, resultLeft, resultRight, resultType, diagnostics); } else diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs index 392bfa04527f5..61b08bdf399e5 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/LockTests.cs @@ -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() { diff --git a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb index e9988e68415b8..060e9e1ca10a6 100644 --- a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb +++ b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb @@ -38,8 +38,30 @@ 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. + Dim needsFilterDiagnostics = diagnostics.DiagnosticBag IsNot Nothing + 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() + If diagnostic.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) diff --git a/src/Compilers/VisualBasic/Test/Semantic/Binding/SyncLockTests.vb b/src/Compilers/VisualBasic/Test/Semantic/Binding/SyncLockTests.vb index 98f71f7808904..fff1e134bbdcb 100644 --- a/src/Compilers/VisualBasic/Test/Semantic/Binding/SyncLockTests.vb +++ b/src/Compilers/VisualBasic/Test/Semantic/Binding/SyncLockTests.vb @@ -984,5 +984,90 @@ BC42508: A value of type 'System.Threading.Lock' converted to a different type w IL_002b: ret }]]>) End Sub + + + Public Sub LockType_ObjectEquality() + Dim source = .Value + Dim comp = CreateCompilation(source, options:=TestOptions.ReleaseExe) + Dim verifier = CompileAndVerify(comp, expectedOutput:="12345") + verifier.Diagnostics.AssertTheseDiagnostics() + End Sub End Class End Namespace From 7e3c01e08ee4867b7fd494fed47c282cbc9f83a9 Mon Sep 17 00:00:00 2001 From: Jan Jones Date: Mon, 11 Mar 2024 10:50:02 +0100 Subject: [PATCH 2/3] Use `AccumulatesDiagnostics` helper --- src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs | 2 +- src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs index ab872e2ea4c74..cd816a7b3a8fb 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs @@ -640,7 +640,7 @@ private BoundExpression BindSimpleBinaryOperator(BinaryExpressionSyntax node, Bi // If this is an object equality operator, we will suppress Lock conversion warnings. var needsFilterDiagnostics = resultOperatorKind is BinaryOperatorKind.ObjectEqual or BinaryOperatorKind.ObjectNotEqual && - diagnostics.DiagnosticBag is not null; + diagnostics.AccumulatesDiagnostics; var conversionDiagnostics = needsFilterDiagnostics ? BindingDiagnosticBag.GetInstance(template: diagnostics) : diagnostics; resultLeft = CreateConversion(left, best.LeftConversion, signature.LeftType, conversionDiagnostics); diff --git a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb index 060e9e1ca10a6..b0eca7fa47ccc 100644 --- a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb +++ b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb @@ -39,7 +39,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic right = MakeRValue(right, diagnostics) ' Suppress Lock conversion warnings for Is operator. - Dim needsFilterDiagnostics = diagnostics.DiagnosticBag IsNot Nothing + Dim needsFilterDiagnostics = diagnostics.AccumulatesDiagnostics Dim conversionDiagnostics = If(needsFilterDiagnostics, BindingDiagnosticBag.GetInstance(diagnostics), diagnostics) left = ValidateAndConvertIsExpressionArgument(left, right, [isNot], conversionDiagnostics) From 5985b7148100002e31f351b769e15569b719d119 Mon Sep 17 00:00:00 2001 From: Jan Jones Date: Mon, 11 Mar 2024 11:08:45 +0100 Subject: [PATCH 3/3] Avoid resolving diagnostic code --- .../CSharp/Portable/Binder/Binder_Operators.cs | 3 ++- .../VisualBasic/Portable/Binding/Binder_Operators.vb | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs index cd816a7b3a8fb..40acd645365d9 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs @@ -658,7 +658,8 @@ resultOperatorKind is BinaryOperatorKind.ObjectEqual or BinaryOperatorKind.Objec { foreach (var diagnostic in sourceBag.AsEnumerableWithoutResolution()) { - if ((ErrorCode)diagnostic.Code is not ErrorCode.WRN_ConvertingLock) + 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); } diff --git a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb index b0eca7fa47ccc..99cb7ac6d1ad8 100644 --- a/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb +++ b/src/Compilers/VisualBasic/Portable/Binding/Binder_Operators.vb @@ -54,7 +54,15 @@ Namespace Microsoft.CodeAnalysis.VisualBasic If Not sourceBag.IsEmptyWithoutResolution Then For Each diagnostic In sourceBag.AsEnumerableWithoutResolution() - If diagnostic.Code <> ERRID.WRN_ConvertingLock Then + 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