From 56683b30404ea36b836cb16c26d4483019338912 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 14 Sep 2023 18:56:17 +1000 Subject: [PATCH 1/2] Include type name in Assumes.Null/NotNull failures Recently I've seen a few errors from customers with error text "value is null" (the caller's expression), which isn't particularly helpful. This change attempts to include the name of the type in the message, which will help with diagnosis in many cases. With this change the message would read something like "Unexpected null value of type 'String'.". I've attempted to preserve the inlineability and tiny code size of the original methods by moving the formatting into a separate method. That method is generic (to avoid the caller having code to produce a type name) which could increase generated code size, however I wouldn't expect that to be a huge concern. It's worth calling out for review though. --- .../Assumes.cs | 35 ++++++++++++++++--- .../Strings.resx | 6 ++++ .../AssumesTests.cs | 20 ++++++++--- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.VisualStudio.Validation/Assumes.cs b/src/Microsoft.VisualStudio.Validation/Assumes.cs index 10f389e4..d14754a1 100644 --- a/src/Microsoft.VisualStudio.Validation/Assumes.cs +++ b/src/Microsoft.VisualStudio.Validation/Assumes.cs @@ -4,7 +4,6 @@ using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; -using System.Globalization; using System.Runtime; using System.Runtime.CompilerServices; @@ -24,7 +23,10 @@ public static partial class Assumes public static void NotNull([ValidatedNotNull, NotNull]T? value) where T : class { - True(value is object); + if (value is null) + { + FailNotNull(); + } } /// @@ -36,7 +38,10 @@ public static void NotNull([ValidatedNotNull, NotNull]T? value) public static void NotNull([ValidatedNotNull, NotNull]T? value) where T : struct { - True(value.HasValue); + if (!value.HasValue) + { + FailNotNull(); + } } /// @@ -97,7 +102,10 @@ public static void NotNullOrEmpty([ValidatedNotNull, NotNull]IEnumerable? public static void Null(T? value) where T : class { - True(value is null); + if (value is not null) + { + FailNull(); + } } /// @@ -109,7 +117,10 @@ public static void Null(T? value) public static void Null(T? value) where T : struct { - False(value.HasValue); + if (value.HasValue) + { + FailNull(); + } } /// @@ -356,6 +367,20 @@ public static Exception Fail([Localizable(false)] string? message, Exception? in } } + [DoesNotReturn] + [MethodImpl(MethodImplOptions.NoInlining)] + private static void FailNotNull() + { + Fail(Strings.FormatUnexpectedNull(typeof(T).Name)); + } + + [DoesNotReturn] + [MethodImpl(MethodImplOptions.NoInlining)] + private static void FailNull() + { + Fail(Strings.FormatUnexpectedNonNull(typeof(T).Name)); + } + /// /// Helper method that formats string arguments. /// diff --git a/src/Microsoft.VisualStudio.Validation/Strings.resx b/src/Microsoft.VisualStudio.Validation/Strings.resx index 6e893c33..ff610335 100644 --- a/src/Microsoft.VisualStudio.Validation/Strings.resx +++ b/src/Microsoft.VisualStudio.Validation/Strings.resx @@ -144,4 +144,10 @@ Cannot find an instance of the {0} service. + + Unexpected null value of type '{0}'. + + + Unexpected non-null value of type '{0}'. + diff --git a/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs b/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs index 72a76ccd..6d6e091d 100644 --- a/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs +++ b/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs @@ -79,28 +79,40 @@ public void Fail() [Fact] public void NotNull() { - Assert.ThrowsAny(() => Assumes.NotNull((object?)null)); + Exception ex = Assert.ThrowsAny(() => Assumes.NotNull((string?)null)); + + Assert.Equal("Unexpected null value of type 'String'.", ex.Message); + Assumes.NotNull("success"); } [Fact] public void NotNull_NullableStruct() { - Assert.ThrowsAny(() => Assumes.NotNull((int?)null)); + Exception ex = Assert.ThrowsAny(() => Assumes.NotNull((int?)null)); + + Assert.Equal("Unexpected null value of type 'Int32'.", ex.Message); + Assumes.NotNull((int?)5); } [Fact] public void Null() { - Assert.ThrowsAny(() => Assumes.Null("not null")); + Exception ex = Assert.ThrowsAny(() => Assumes.Null("not null")); + + Assert.Equal("Unexpected non-null value of type 'String'.", ex.Message); + Assumes.Null((object?)null); } [Fact] public void Null_NullableStruct() { - Assert.ThrowsAny(() => Assumes.Null((int?)5)); + Exception ex = Assert.ThrowsAny(() => Assumes.Null((int?)5)); + + Assert.Equal("Unexpected non-null value of type 'Int32'.", ex.Message); + Assumes.Null((int?)null); } From 6ef72471b861e504c4fbf8996c2d351435fb5569 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Tue, 10 Oct 2023 08:12:18 +1100 Subject: [PATCH 2/2] Use invariant culture in tests This ensures tests run correctly, regardless of the environment's culture. --- .../AssumesTests.cs | 5 +++- .../OverrideCulture.cs | 28 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 test/Microsoft.VisualStudio.Validation.Tests/OverrideCulture.cs diff --git a/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs b/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs index 6d6e091d..e7403efa 100644 --- a/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs +++ b/test/Microsoft.VisualStudio.Validation.Tests/AssumesTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Globalization; using System.Runtime.Serialization.Formatters.Binary; using Microsoft; using Moq; @@ -9,11 +10,13 @@ public partial class AssumesTests : IDisposable { private const string TestMessage = "Some test message."; - private AssertDialogSuppression suppressAssertUi = new AssertDialogSuppression(); + private readonly AssertDialogSuppression suppressAssertUi = new(); + private readonly OverrideCulture overrideCulture = new(CultureInfo.InvariantCulture); public void Dispose() { this.suppressAssertUi.Dispose(); + this.overrideCulture.Dispose(); } [Fact] diff --git a/test/Microsoft.VisualStudio.Validation.Tests/OverrideCulture.cs b/test/Microsoft.VisualStudio.Validation.Tests/OverrideCulture.cs new file mode 100644 index 00000000..d0ed7220 --- /dev/null +++ b/test/Microsoft.VisualStudio.Validation.Tests/OverrideCulture.cs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Globalization; + +/// +/// Sets a specific culture for a duration, restoring the prior culture upon disposal. +/// +internal sealed class OverrideCulture : IDisposable +{ + private readonly CultureInfo priorCulture; + private readonly CultureInfo priorUICulture; + + public OverrideCulture(CultureInfo culture) + { + this.priorCulture = CultureInfo.CurrentCulture; + this.priorUICulture = CultureInfo.CurrentUICulture; + + CultureInfo.CurrentCulture = culture; + CultureInfo.CurrentUICulture = culture; + } + + public void Dispose() + { + CultureInfo.CurrentCulture = this.priorCulture; + CultureInfo.CurrentUICulture = this.priorUICulture; + } +}