Merge pull request #738 from gafter/fix663

Suppress the struct==null warning except in "strict" mode

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_Warnings.cs

@@ -744,13 +744,16 @@ private void CheckLiftedBinOp(BoundBinaryOperator node)
 
                     string always = node.OperatorKind.Operator() == BinaryOperatorKind.NotEqual ? "true" : "false";
 
-                    if (node.Right.NullableNeverHasValue() && node.Left.NullableAlwaysHasValue())
+                    if (this._compilation.FeatureStrictEnabled || !node.OperatorKind.IsUserDefined())
                     {
-                        Error(node.OperatorKind.IsUserDefined() ? ErrorCode.WRN_NubExprIsConstBool2 : ErrorCode.WRN_NubExprIsConstBool, node, always, node.Left.Type.GetNullableUnderlyingType(), GetTypeForLiftedComparisonWarning(node.Right));
-                    }
-                    else if (node.Left.NullableNeverHasValue() && node.Right.NullableAlwaysHasValue())
-                    {
-                        Error(node.OperatorKind.IsUserDefined() ? ErrorCode.WRN_NubExprIsConstBool2 : ErrorCode.WRN_NubExprIsConstBool, node, always, node.Right.Type.GetNullableUnderlyingType(), GetTypeForLiftedComparisonWarning(node.Left));
+                        if (node.Right.NullableNeverHasValue() && node.Left.NullableAlwaysHasValue())
+                        {
+                            Error(node.OperatorKind.IsUserDefined() ? ErrorCode.WRN_NubExprIsConstBool2 : ErrorCode.WRN_NubExprIsConstBool, node, always, node.Left.Type.GetNullableUnderlyingType(), GetTypeForLiftedComparisonWarning(node.Right));
+                        }
+                        else if (node.Left.NullableNeverHasValue() && node.Right.NullableAlwaysHasValue())
+                        {
+                            Error(node.OperatorKind.IsUserDefined() ? ErrorCode.WRN_NubExprIsConstBool2 : ErrorCode.WRN_NubExprIsConstBool, node, always, node.Right.Type.GetNullableUnderlyingType(), GetTypeForLiftedComparisonWarning(node.Left));
+                        }
                     }
                     break;
                 case BinaryOperatorKind.Or:

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOptimizedNullableOperators.cs

@@ -990,7 +990,7 @@ .maxstack  2
             comp.VerifyIL("Program.M6", expectedIL6);
         }
 
-        [Fact]
+        [Fact, WorkItem(663, "https://github.com/dotnet/roslyn/issues/663")]
         public void TestNullableComparisonOpsOneNullOneNonNull()
         {
             // An ==, !=, <, >, <= or >= operation where one operand is null and the
@@ -1000,9 +1000,10 @@ public void TestNullableComparisonOpsOneNullOneNonNull()
             // Note that Roslyn produces considerably more warnings here than the
             // native compiler; the native compiler only produces warnings for
             // "((int?)null) < new decimal?(N3())" and "((S?)null) < new S?(N4())".
-            // However, the native compiler does not produce the warning
-            // "comparing null with S? always produces false" -- it incorrectly warns
-            // that it produces a null of type bool? !  Roslyn does not reproduce this bug.
+            // For compatibility Roslyn reports the same diagnostics by default,
+            // but in "strict" mode (which will be part of the "warning waves" once
+            // we do that) Roslyn will report warnings for
+            // new S() == null and new S() != null.
 
             string source = @"
 struct S // User-defined relational ops
@@ -1092,7 +1093,21 @@ .maxstack  1
 }";
             string expectedIL6 = expectedIL4;
 
-            var comp = CompileAndVerify(source, expectedOutput: expectedOutput);
+            CompileAndVerify(source, expectedOutput: expectedOutput).VerifyDiagnostics(
+                // (21,16): warning CS0472: The result of the expression is always 'false' since a value of type 'int' is never equal to 'null' of type 'short?'
+                //         return new int?(N1()) == new short?();
+                Diagnostic(ErrorCode.WRN_NubExprIsConstBool, "new int?(N1()) == new short?()").WithArguments("false", "int", "short?").WithLocation(21, 16),
+                // (25,16): warning CS0472: The result of the expression is always 'true' since a value of type 'double' is never equal to 'null' of type 'double?'
+                //         return default(double?) != new short?(N2());
+                Diagnostic(ErrorCode.WRN_NubExprIsConstBool, "default(double?) != new short?(N2())").WithArguments("true", "double", "double?").WithLocation(25, 16),
+                // (29,16): warning CS0464: Comparing with null of type 'int?' always produces 'false'
+                //         return ((int?)null) < new decimal?(N3());
+                Diagnostic(ErrorCode.WRN_CmpAlwaysFalse, "((int?)null) < new decimal?(N3())").WithArguments("int?").WithLocation(29, 16),
+                // (41,16): warning CS0464: Comparing with null of type 'S?' always produces 'false'
+                //         return ((S?)null) < new S?(N4());
+                Diagnostic(ErrorCode.WRN_CmpAlwaysFalse, "((S?)null) < new S?(N4())").WithArguments("S?").WithLocation(41, 16)
+                );
+            var comp = CompileAndVerify(source, expectedOutput: expectedOutput, options: TestOptions.ReleaseExe.WithStrictMode());
             comp.VerifyDiagnostics(
                 // (21,16): warning CS0472: The result of the expression is always 'false' since a value of type 'int' is never equal to 'null' of type 'short?'
                 //         return new int?(N1()) == new short?();

src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs

@@ -19034,12 +19034,13 @@ static void Main()
                 new ErrorDescription[] { new ErrorDescription { Code = (int)ErrorCode.WRN_GotoCaseShouldConvert, Line = 13, Column = 13, IsWarning = true } });
         }
 
-        [Fact]
+        [Fact, WorkItem(663, "https://github.com/dotnet/roslyn/issues/663")]
         public void CS0472WRN_NubExprIsConstBool()
         {
-            // Due to a long-standing bug, the native compiler does not produce warnings for "guid == null", but does 
-            // for "int == null". Roslyn corrects this lapse and produces warnings for both built-in and
-            // user-defined lifted equality operators.
+            // Due to a long-standing bug, the native compiler does not produce warnings for "guid == null",
+            // but does for "int == null". Roslyn corrects this lapse and produces warnings for both built-in
+            // and user-defined lifted equality operators, but the new warnings for user-defined types are
+            // only given in "strict" more.
 
             var text = @"
 using System;
@@ -19145,8 +19146,7 @@ public static void Main()
 ftftftftftftftftftftftft
 tf
 ftftftft";
-            var comp = this.CompileAndVerify(source: text, expectedOutput: expected);
-            comp.VerifyDiagnostics(
+            var fullExpected = new DiagnosticDescription[] {
                 // (19,11): warning CS0472: The result of the expression is always 'false' since a value of type 'int' is never equal to 'null' of type 'int?'
                 //         W(i == null);            // CS0472
                 Diagnostic(ErrorCode.WRN_NubExprIsConstBool, "i == null").WithArguments("false", "int", "int?").WithLocation(19, 11),
@@ -19243,7 +19243,10 @@ public static void Main()
                 // (96,11): warning CS0472: The result of the expression is always 'true' since a value of type 'MyClass.E' is never equal to 'null' of type 'MyClass.E?'
                 //         W((E?)null != 0);
                 Diagnostic(ErrorCode.WRN_NubExprIsConstBool, "(E?)null != 0").WithArguments("true", "MyClass.E", "MyClass.E?").WithLocation(96, 11)
-                    );
+            };
+            var compatibleExpected = fullExpected.Where(d => !d.Code.Equals((int)ErrorCode.WRN_NubExprIsConstBool2)).ToArray();
+            this.CompileAndVerify(source: text, expectedOutput: expected).VerifyDiagnostics(compatibleExpected);
+            this.CompileAndVerify(source: text, expectedOutput: expected, options: TestOptions.ReleaseExe.WithStrictMode()).VerifyDiagnostics(fullExpected);
         }
 
         [Fact]

src/Compilers/Test/Utilities/CSharp/TestOptions.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System.Collections.Immutable;
+
 namespace Microsoft.CodeAnalysis.CSharp.Test.Utilities
 {
     public static class TestOptions
@@ -32,5 +34,10 @@ public static class TestOptions
 
         public static readonly CSharpCompilationOptions UnsafeDebugDll = DebugDll.WithAllowUnsafe(true);
         public static readonly CSharpCompilationOptions UnsafeDebugExe = DebugExe.WithAllowUnsafe(true);
+
+        public static CSharpCompilationOptions WithStrictMode(this CSharpCompilationOptions options)
+        {
+            return options.WithFeatures(options.Features.Add("strict"));
+        }
     }
 }

src/Test/Utilities/DiagnosticDescription.cs

@@ -192,6 +192,8 @@ public DiagnosticDescription WhereSyntax(Func<SyntaxNode, bool> syntaxPredicate)
             return new DiagnosticDescription(_code, _isWarningAsError, _squiggledText, _arguments, _startPosition, syntaxPredicate, _argumentOrderDoesNotMatter, _errorCodeType);
         }
 
+        public object Code => _code;
+
         public override bool Equals(object obj)
         {
             var d = obj as DiagnosticDescription;