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

Use cancellation token throw if cancellation requested #4864

Merged
Show file tree
Hide file tree
Changes from 14 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
5 changes: 5 additions & 0 deletions src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ CA1841 | Performance | Info | PreferDictionaryContainsMethods, [Documentation](h
CA1842 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1842)
CA1843 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1843)

### New Rules
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
CA2250 | Usage | Info | UseCancellationTokenThrowIfCancellationRequested, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250)
NewellClark marked this conversation as resolved.
Show resolved Hide resolved

### Removed Rules

Rule ID | Category | Severity | Notes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1534,6 +1534,18 @@
<value>and all other platforms</value>
<comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment>
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedDescription" xml:space="preserve">
<value>'ThrowIfCancellationRequested' automatically checks whether the token has been canceled, and throws an 'OperationCanceledException' if it has.</value>
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedMessage" xml:space="preserve">
<value>Use 'ThrowIfCancellationRequested' instead of checking 'IsCancellationRequested' and throwing 'OperationCanceledException'</value>
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedTitle" xml:space="preserve">
<value>Use 'ThrowIfCancellationRequested'</value>
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedCodeFixTitle" xml:space="preserve">
<value>Replace with 'CancellationToken.ThrowIfCancellationRequested'</value>
</data>
<data name="PreferDictionaryContainsKeyCodeFixTitle" xml:space="preserve">
<value>Use 'ContainsKey'</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// 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;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Operations;
using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.UseCancellationTokenThrowIfCancellationRequested.RequiredSymbols;
using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;

namespace Microsoft.NetCore.Analyzers.Runtime
{
/// <summary>
/// Use <see cref="CancellationToken.ThrowIfCancellationRequested"/> instead of checking <see cref="CancellationToken.IsCancellationRequested"/> and
/// throwing <see cref="OperationCanceledException"/>.
/// </summary>
[ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared]
public sealed class UseCancellationTokenThrowIfCancellationRequestedFixer : CodeFixProvider
{
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(UseCancellationTokenThrowIfCancellationRequested.RuleId);

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
SemanticModel model = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
if (!RequiredSymbols.TryGetSymbols(model.Compilation, out RequiredSymbols symbols))
return;
SyntaxNode root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
SyntaxNode node = root.FindNode(context.Span);
if (model.GetOperation(node, context.CancellationToken) is not IConditionalOperation conditional)
return;

Func<CancellationToken, Task<Document>> createChangedDocument;
if (symbols.IsSimpleAffirmativeCheck(conditional, out IPropertyReferenceOperation? propertyReference))
{
// For simple checks of the form:
// if (token.IsCancellationRequested)
// throw new OperationCanceledException();
// Replace with:
// token.ThrowIfCancellationRequested();
//
// For simple checks of the form:
// if (token.IsCancellationRequested)
// throw new OperationCanceledException();
// else
// Frob();
// Replace with:
// token.ThrowIfCancellationRequested();
// Frob();
createChangedDocument = async token =>
{
var editor = await DocumentEditor.CreateAsync(context.Document, token).ConfigureAwait(false);
SyntaxNode expressionStatement = CreateThrowIfCancellationRequestedExpressionStatement(editor, conditional, propertyReference);
editor.ReplaceNode(conditional.Syntax, expressionStatement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to preserve the trivia, say:

if (token.IsCancellationRequested)
{
    // Comment to be preserved
    throw new OperationCanceledException();
}

Copy link
Contributor Author

@NewellClark NewellClark May 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Sorry I didn't see this earlier; for some reason I never received an email notification about this comment. I'll add some tests to ensure trivia is preserved.


if (conditional.WhenFalse is IBlockOperation block)
{
editor.InsertAfter(expressionStatement, block.Operations.Select(x => x.Syntax.WithAdditionalAnnotations(Formatter.Annotation)));
}
else if (conditional.WhenFalse is not null)
{
editor.InsertAfter(expressionStatement, conditional.WhenFalse.Syntax);
}

return editor.GetChangedDocument();
};
}
else if (symbols.IsNegatedCheckWithThrowingElseClause(conditional, out propertyReference))
{
// For negated checks of the form:
// if (!token.IsCancellationRequested) { DoStatements(); }
// else { throw new OperationCanceledException(); }
// Replace with:
// token.ThrowIfCancellationRequested();
// DoStatements();
createChangedDocument = async token =>
{
var editor = await DocumentEditor.CreateAsync(context.Document, token).ConfigureAwait(false);
SyntaxNode expressionStatement = CreateThrowIfCancellationRequestedExpressionStatement(editor, conditional, propertyReference)
.WithAdditionalAnnotations(Formatter.Annotation);
editor.ReplaceNode(conditional.Syntax, expressionStatement);

if (conditional.WhenTrue is IBlockOperation block)
{
editor.InsertAfter(expressionStatement, block.Operations.Select(x => x.Syntax.WithAdditionalAnnotations(Formatter.Annotation)));
}
else
{
editor.InsertAfter(expressionStatement, conditional.WhenTrue.Syntax);
}

// Ensure if-blocks with multiple statements maintain correct indentation.
return await Formatter.FormatAsync(editor.GetChangedDocument(), Formatter.Annotation, cancellationToken: token).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharwell Is this the recommend way to achieve this formatting? Normally, I see fixers just attaching the formatter annotation and not explicitly invoking the formatter API. @NewellClark Any reason that approach did not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mavasani I did not realize that editor.GetChangedDocument() would automatically apply formatting. That's good to know. I've gone ahead and simplified it.

};
}
else
{
return;
}

var codeAction = CodeAction.Create(
Resx.UseCancellationTokenThrowIfCancellationRequestedCodeFixTitle,
createChangedDocument,
Resx.UseCancellationTokenThrowIfCancellationRequestedCodeFixTitle);
context.RegisterCodeFix(codeAction, context.Diagnostics);
}

public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

private static SyntaxNode CreateThrowIfCancellationRequestedExpressionStatement(
DocumentEditor editor,
IConditionalOperation conditional,
IPropertyReferenceOperation isCancellationRequestedPropertyReference)
{
SyntaxNode memberAccess = editor.Generator.MemberAccessExpression(
isCancellationRequestedPropertyReference.Instance.Syntax,
nameof(CancellationToken.ThrowIfCancellationRequested));
SyntaxNode invocation = editor.Generator.InvocationExpression(memberAccess, Array.Empty<SyntaxNode>());
var firstWhenTrueStatement = conditional.WhenTrue is IBlockOperation block ? block.Operations.FirstOrDefault() : conditional.WhenTrue;

var result = editor.Generator.ExpressionStatement(invocation);
result = firstWhenTrueStatement is not null ? result.WithTriviaFrom(firstWhenTrueStatement.Syntax) : result;
return result.WithAdditionalAnnotations(Formatter.Annotation);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
// 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;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources;

namespace Microsoft.NetCore.Analyzers.Runtime
{
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public sealed class UseCancellationTokenThrowIfCancellationRequested : DiagnosticAnalyzer
{
internal const string RuleId = "CA2250";

private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedTitle), Resx.ResourceManager, typeof(Resx));
private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedMessage), Resx.ResourceManager, typeof(Resx));
private static readonly LocalizableString s_localizableDescription = new LocalizableResourceString(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedDescription), Resx.ResourceManager, typeof(Resx));

internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(
RuleId,
s_localizableTitle,
s_localizableMessage,
DiagnosticCategory.Usage,
RuleLevel.IdeSuggestion,
s_localizableDescription,
isPortedFxCopRule: false,
isDataflowRule: false);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();
context.RegisterCompilationStartAction(OnCompilationStart);
}

private static void OnCompilationStart(CompilationStartAnalysisContext context)
{
if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols))
return;

context.RegisterOperationAction(AnalyzeOperation, OperationKind.Conditional);
NewellClark marked this conversation as resolved.
Show resolved Hide resolved

void AnalyzeOperation(OperationAnalysisContext context)
{
var conditional = (IConditionalOperation)context.Operation;

if (symbols.IsSimpleAffirmativeCheck(conditional, out _) || symbols.IsNegatedCheckWithThrowingElseClause(conditional, out _))
{
context.ReportDiagnostic(conditional.CreateDiagnostic(Rule));
}
}
}

/// <summary>
/// If <paramref name="singleOrBlock"/> is a block operation with one child, returns that child.
/// If <paramref name="singleOrBlock"/> is a block operation with more than one child, returns <see langword="null"/>.
/// If <paramref name="singleOrBlock"/> is not a block operation, returns <paramref name="singleOrBlock"/>.
/// </summary>
/// <param name="singleOrBlock">The operation to unwrap.</param>
internal static IOperation? GetSingleStatementOrDefault(IOperation? singleOrBlock)
{
if (singleOrBlock is IBlockOperation blockOperation)
{
return blockOperation.Operations.Length is 1 ? blockOperation.Operations[0] : default;
}

return singleOrBlock;
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
}

// Use readonly struct to avoid allocations.
#pragma warning disable CA1815 // Override equals and operator equals on value types
internal readonly struct RequiredSymbols
#pragma warning restore CA1815 // Override equals and operator equals on value types
{
public static bool TryGetSymbols(Compilation compilation, out RequiredSymbols symbols)
{
symbols = default;
INamedTypeSymbol boolType = compilation.GetSpecialType(SpecialType.System_Boolean);
if (boolType is null)
return false;

if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemThreadingCancellationToken, out INamedTypeSymbol? cancellationTokenType))
return false;

IMethodSymbol? throwIfCancellationRequestedMethod = cancellationTokenType.GetMembers(nameof(CancellationToken.ThrowIfCancellationRequested))
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
.OfType<IMethodSymbol>()
.GetFirstOrDefaultMemberWithParameterInfos();
IPropertySymbol? isCancellationRequestedProperty = cancellationTokenType.GetMembers(nameof(CancellationToken.IsCancellationRequested))
.OfType<IPropertySymbol>()
.FirstOrDefault();

if (throwIfCancellationRequestedMethod is null || isCancellationRequestedProperty is null)
return false;

if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemOperationCanceledException, out INamedTypeSymbol? operationCanceledExceptionType))
return false;

IMethodSymbol? operationCanceledExceptionDefaultCtor = operationCanceledExceptionType.InstanceConstructors
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
.GetFirstOrDefaultMemberWithParameterInfos();
IMethodSymbol? operationCanceledExceptionTokenCtor = operationCanceledExceptionType.InstanceConstructors
.GetFirstOrDefaultMemberWithParameterInfos(ParameterInfo.GetParameterInfo(cancellationTokenType));

if (operationCanceledExceptionDefaultCtor is null || operationCanceledExceptionTokenCtor is null)
return false;

symbols = new RequiredSymbols
{
IsCancellationRequestedProperty = isCancellationRequestedProperty,
OperationCanceledExceptionDefaultCtor = operationCanceledExceptionDefaultCtor,
OperationCanceledExceptionTokenCtor = operationCanceledExceptionTokenCtor
};

return true;
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
}

public IPropertySymbol IsCancellationRequestedProperty { get; init; }
public IMethodSymbol OperationCanceledExceptionDefaultCtor { get; init; }
public IMethodSymbol OperationCanceledExceptionTokenCtor { get; init; }

/// <summary>
/// Indicates whether the specified operation is a conditional statement of the form
/// <code>
/// if (token.IsCancellationRequested)
/// throw new OperationCanceledException();
/// </code>
/// </summary>
public bool IsSimpleAffirmativeCheck(IConditionalOperation conditional, [NotNullWhen(true)] out IPropertyReferenceOperation? isCancellationRequestedPropertyReference)
{
IOperation? whenTrueUnwrapped = GetSingleStatementOrDefault(conditional.WhenTrue);

if (conditional.Condition is IPropertyReferenceOperation propertyReference &&
SymbolEqualityComparer.Default.Equals(propertyReference.Property, IsCancellationRequestedProperty) &&
whenTrueUnwrapped is IThrowOperation @throw &&
@throw.Exception is IObjectCreationOperation objectCreation &&
IsDefaultOrTokenOperationCanceledExceptionCtor(objectCreation.Constructor))
{
isCancellationRequestedPropertyReference = propertyReference;
return true;
}

isCancellationRequestedPropertyReference = default;
return false;
}

/// <summary>
/// Indicates whether the specified operation is a conditional statement of the form
/// <code>
/// if (!token.IsCancellationRequested)
/// {
/// // statements
/// }
/// else
/// {
/// throw new OperationCanceledException();
/// }
/// </code>
/// </summary>
public bool IsNegatedCheckWithThrowingElseClause(IConditionalOperation conditional, [NotNullWhen(true)] out IPropertyReferenceOperation? isCancellationRequestedPropertyReference)
{
IOperation? whenFalseUnwrapped = GetSingleStatementOrDefault(conditional.WhenFalse);

if (conditional.Condition is IUnaryOperation { OperatorKind: UnaryOperatorKind.Not } unary &&
unary.Operand is IPropertyReferenceOperation propertyReference &&
SymbolEqualityComparer.Default.Equals(propertyReference.Property, IsCancellationRequestedProperty) &&
whenFalseUnwrapped is IThrowOperation @throw &&
@throw.Exception is IObjectCreationOperation objectCreation &&
IsDefaultOrTokenOperationCanceledExceptionCtor(objectCreation.Constructor))
{
isCancellationRequestedPropertyReference = propertyReference;
return true;
}

isCancellationRequestedPropertyReference = default;
return false;
}

private bool IsDefaultOrTokenOperationCanceledExceptionCtor(IMethodSymbol method)
{
return SymbolEqualityComparer.Default.Equals(method, OperationCanceledExceptionDefaultCtor) ||
SymbolEqualityComparer.Default.Equals(method, OperationCanceledExceptionTokenCtor);
}
}
}
}
Loading