From d6e823d3370fc8973d3c409588dcb518afbae457 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 2 Jan 2023 22:59:55 -0500 Subject: [PATCH 1/2] Fix a couple of issues in UseExceptionThrowHelpers Based on additional false positives discovered while enabling the rules in dotnet/runtime: - We don't want to warn to use ObjectDisposedException.ThrowIf when inside of a struct, as doing so is likely to lead to additional costs (e.g. boxing). - We don't want to warn to use ArgumentXxException.ThrowIfXx when the code to compute the argument name is more complicated than just a literal / nameof, e.g. a method call that determines what parameter name should be employed. --- .../Runtime/UseExceptionThrowHelpers.cs | 29 +++++++++++-- .../Runtime/UseExceptionThrowHelpersTests.cs | 42 ++++++++++++++++++- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs index a7d001cea3..cdf161269b 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs @@ -181,7 +181,8 @@ aooreThrowIfGreaterThan is not null || aooreThrowIfGreaterThanOrEqual is not nul { if (aneThrowIfNull is not null && IsParameterNullCheck(condition.Condition, out IParameterReferenceOperation? nullCheckParameter) && - nullCheckParameter.Type.IsReferenceType) + nullCheckParameter.Type.IsReferenceType && + HasReplaceableArgumentName(objectCreationOperation, 0)) { context.ReportDiagnostic(condition.CreateDiagnostic( UseArgumentNullExceptionThrowIfNullRule, @@ -197,7 +198,8 @@ aooreThrowIfGreaterThan is not null || aooreThrowIfGreaterThanOrEqual is not nul if (SymbolEqualityComparer.Default.Equals(objectCreationOperation.Type, ae)) { if (aeThrowIfNullOrEmpty is not null && - IsNullOrEmptyCheck(stringIsNullOrEmpty, stringLength, stringEmpty, condition.Condition, out IParameterReferenceOperation? nullOrEmptyCheckParameter)) + IsNullOrEmptyCheck(stringIsNullOrEmpty, stringLength, stringEmpty, condition.Condition, out IParameterReferenceOperation? nullOrEmptyCheckParameter) && + HasReplaceableArgumentName(objectCreationOperation, 1)) { context.ReportDiagnostic(condition.CreateDiagnostic( UseArgumentExceptionThrowIfNullOrEmptyRule, @@ -218,7 +220,8 @@ aooreThrowIfGreaterThan is not null || aooreThrowIfGreaterThanOrEqual is not nul // Handle ArgumentOutOfRangeException.ThrowIfLessThanOrEqual if (SymbolEqualityComparer.Default.Equals(objectCreationOperation.Type, aoore)) { - if (hasAnyAooreThrow) + if (hasAnyAooreThrow && + HasReplaceableArgumentName(objectCreationOperation, 0)) { ImmutableArray additionalLocations = ImmutableArray.Empty; @@ -251,7 +254,12 @@ static bool AvoidComparing(IParameterReferenceOperation p) => // Handle ObjectDisposedException.ThrowIf. if (SymbolEqualityComparer.Default.Equals(objectCreationOperation.Type, ode)) { - if (odeThrowIf is not null) + // If we have ObjectDisposedException.ThrowIf and if this operation is in a reference type, issue a diagnostic. + // We check whether the containing type is a reference type because we want to avoid passing `this` at the call + // site to ThrowIf for a struct as that will box, and we want to avoid using `GetType()` at the call site as + // that adds additional cost prior to the guard check. + if (odeThrowIf is not null && + context.ContainingSymbol.ContainingType.IsReferenceType) { // We always report a diagnostic. However, the fixer is only currently provided in the case // of the argument to the ObjectDisposedException constructor containing a call to {something.}GetType().{Full}Name, @@ -282,6 +290,19 @@ nameReference.Instance is IInvocationOperation getTypeCall && } }, OperationKind.Throw); + // As a heuristic, we only want to replace throws with ThrowIfNull if either there isn't currently + // a specified parameter name, e.g. the parameterless constructor was used, or if it's specified in a + // simple fashion, e.g. a nameof or a literal string. This is primarily to avoid false positives + // with complicated expressions for computing the parameter name to use, which with ThrowIfNull would + // need to be done prior to the guard check, and thus something we want to avoid. + bool HasReplaceableArgumentName(IObjectCreationOperation creationOperation, int argumentIndex) + { + ImmutableArray args = creationOperation.Arguments; + return + argumentIndex >= args.Length || + args[argumentIndex].Value.WalkDownConversion() is ILiteralOperation or INameOfOperation; + } + // As a heuristic, we avoid issuing diagnostics if there are additional arguments (e.g. message) // to the exception that could be useful. bool HasPossiblyMeaningfulAdditionalArguments(IObjectCreationOperation objectCreationOperation) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs index 35b4a557ac..3f893814a1 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs @@ -100,12 +100,12 @@ void M(string arg) {|CA1510:if (arg is null) throw new ArgumentNullException(nameof(arg));|} {|CA1510:if (arg == null) - throw new ArgumentNullException(nameof(arg), (string)null);|} + throw new ArgumentNullException(""arg"", (string)null);|} {|CA1510:if (null == arg) throw new ArgumentNullException(nameof(arg));|} {|CA1510:if (arg is null) { - throw new ArgumentNullException(nameof(arg)); + throw new ArgumentNullException(""arg""); }|} {|CA1510:if (arg == null) { @@ -165,9 +165,17 @@ void M(string arg) Console.WriteLine(arg); } + if (arg is null) + throw new ArgumentNullException(ComputeName(nameof(arg))); + + if (arg is null) + throw new ArgumentNullException(IntPtr.Size == 8 ? ""arg"" : ""arg""); + throw new ArgumentNullException(nameof(arg)); // no guard } + static string ComputeName(string arg) => arg; + string this[string name] { get @@ -282,9 +290,17 @@ void M(string arg) Console.WriteLine(arg); } + if (arg is null) + throw new ArgumentNullException(ComputeName(nameof(arg))); + + if (arg is null) + throw new ArgumentNullException(IntPtr.Size == 8 ? ""arg"" : ""arg""); + throw new ArgumentNullException(nameof(arg)); // no guard } + static string ComputeName(string arg) => arg; + string this[string name] { get @@ -980,6 +996,17 @@ string Prop } } } + +struct S +{ + private bool IsDisposed { get; set; } + + void M() + { + if (IsDisposed) throw new ObjectDisposedException(null); + if (IsDisposed) throw new ObjectDisposedException(this.GetType().FullName); + } +} ", FixedCode = @" @@ -1033,6 +1060,17 @@ string Prop } } } + +struct S +{ + private bool IsDisposed { get; set; } + + void M() + { + if (IsDisposed) throw new ObjectDisposedException(null); + if (IsDisposed) throw new ObjectDisposedException(this.GetType().FullName); + } +} " }; test.FixedState.MarkupHandling = CodeAnalysis.Testing.MarkupMode.Allow; From 14bc4de591296e001a4a27b4b2c4538bdce8a55f Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 3 Jan 2023 08:45:18 -0500 Subject: [PATCH 2/2] Address PR feedback --- .../Runtime/UseExceptionThrowHelpers.cs | 6 +++--- .../Runtime/UseExceptionThrowHelpersTests.cs | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs index cdf161269b..4a62fe7ca0 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpers.cs @@ -291,8 +291,8 @@ nameReference.Instance is IInvocationOperation getTypeCall && }, OperationKind.Throw); // As a heuristic, we only want to replace throws with ThrowIfNull if either there isn't currently - // a specified parameter name, e.g. the parameterless constructor was used, or if it's specified in a - // simple fashion, e.g. a nameof or a literal string. This is primarily to avoid false positives + // a specified parameter name, e.g. the parameterless constructor was used, or if it's specified as a + // constant, e.g. a nameof or a literal string. This is primarily to avoid false positives // with complicated expressions for computing the parameter name to use, which with ThrowIfNull would // need to be done prior to the guard check, and thus something we want to avoid. bool HasReplaceableArgumentName(IObjectCreationOperation creationOperation, int argumentIndex) @@ -300,7 +300,7 @@ bool HasReplaceableArgumentName(IObjectCreationOperation creationOperation, int ImmutableArray args = creationOperation.Arguments; return argumentIndex >= args.Length || - args[argumentIndex].Value.WalkDownConversion() is ILiteralOperation or INameOfOperation; + args.GetArgumentForParameterAtIndex(argumentIndex).Value.ConstantValue.HasValue; } // As a heuristic, we avoid issuing diagnostics if there are additional arguments (e.g. message) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs index 3f893814a1..c0e4f434ce 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseExceptionThrowHelpersTests.cs @@ -168,6 +168,9 @@ void M(string arg) if (arg is null) throw new ArgumentNullException(ComputeName(nameof(arg))); + if (arg is null) + throw new ArgumentNullException(innerException: null, paramName: ComputeName(nameof(arg))); + if (arg is null) throw new ArgumentNullException(IntPtr.Size == 8 ? ""arg"" : ""arg""); @@ -293,6 +296,9 @@ void M(string arg) if (arg is null) throw new ArgumentNullException(ComputeName(nameof(arg))); + if (arg is null) + throw new ArgumentNullException(innerException: null, paramName: ComputeName(nameof(arg))); + if (arg is null) throw new ArgumentNullException(IntPtr.Size == 8 ? ""arg"" : ""arg"");