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 5 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
@@ -1,5 +1,10 @@
; Please do not edit this file manually, it should only be updated through code fix application.

### 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 @@ -1510,4 +1510,16 @@
<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'</value>
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedTitle" xml:space="preserve">
<value>Use 'ThrowIfCancellationRequested'</value>
</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedCodeFixTitle" xml:space="preserve">
<value>Use 'ThrowIfCancellationRequested'</value>
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// 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.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)]
Copy link
Member

Choose a reason for hiding this comment

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

@sharwell @mavasani Does this needs SharedAttribute? Isn't there an analyzer for missing shared attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Youssef1313 I've gone ahead and added the Shared attribute.

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;
IOperation? whenTrue = UseCancellationTokenThrowIfCancellationRequested.GetSingleStatementOrDefault(conditional.WhenTrue);
IOperation? whenFalse = UseCancellationTokenThrowIfCancellationRequested.GetSingleStatementOrDefault(conditional.WhenFalse);
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved

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();
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.

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);
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>());
return editor.Generator.ExpressionStatement(invocation).WithTriviaFrom(conditional.Syntax);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
// 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;
IOperation? whenTrue = GetSingleStatementOrDefault(conditional.WhenTrue);
IOperation? whenFalse = GetSingleStatementOrDefault(conditional.WhenFalse);
NewellClark marked this conversation as resolved.
Show resolved Hide resolved

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
{
BoolType = boolType,
CancellationTokenType = cancellationTokenType,
OperationCanceledExceptionType = operationCanceledExceptionType,
ThrowIfCancellationRequestedMethod = throwIfCancellationRequestedMethod,
IsCancellationRequestedProperty = isCancellationRequestedProperty,
OperationCanceledExceptionDefaultCtor = operationCanceledExceptionDefaultCtor,
OperationCanceledExceptionTokenCtor = operationCanceledExceptionTokenCtor
};

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

public INamedTypeSymbol BoolType { get; init; }
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
public INamedTypeSymbol CancellationTokenType { get; init; }
public INamedTypeSymbol OperationCanceledExceptionType { get; init; }
public IMethodSymbol ThrowIfCancellationRequestedMethod { get; init; }
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) &&
conditional.WhenFalse is null)
Copy link
Contributor

@mavasani mavasani Apr 14, 2021

Choose a reason for hiding this comment

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

Why this restriction? Even if there is an else block, I believe the code can still be written as cancellationToken.ThrowIfCancellationRequested(); followed by the body of the else/WhenFalse block.

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.

I was implementing the analyzer as specified here, which specified two cases that should be detected and fixed:

  1. A simple affirmative check with no else clause, or
  2. A negated check with an else clause.

I'll remove the restriction. Edit: restriction has been removed.

{
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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2097,6 +2097,26 @@
<target state="translated">Metoda {0} zpracovává požadavek {1} bez ověřování tokenu proti padělkům. Je potřeba také zajistit, aby váš formulář HTML odesílal tokeny proti padělkům.</target>
<note />
</trans-unit>
<trans-unit id="UseCancellationTokenThrowIfCancellationRequestedCodeFixTitle">
<source>Use 'ThrowIfCancellationRequested'</source>
<target state="new">Use 'ThrowIfCancellationRequested'</target>
<note />
</trans-unit>
<trans-unit id="UseCancellationTokenThrowIfCancellationRequestedDescription">
<source>'ThrowIfCancellationRequested' automatically checks whether the token has been canceled, and throws an 'OperationCanceledException' if it has.</source>
<target state="new">'ThrowIfCancellationRequested' automatically checks whether the token has been canceled, and throws an 'OperationCanceledException' if it has.</target>
<note />
</trans-unit>
<trans-unit id="UseCancellationTokenThrowIfCancellationRequestedMessage">
<source>Use 'ThrowIfCancellationRequested' instead of checking 'IsCancellationRequested'</source>
<target state="new">Use 'ThrowIfCancellationRequested' instead of checking 'IsCancellationRequested'</target>
<note />
</trans-unit>
<trans-unit id="UseCancellationTokenThrowIfCancellationRequestedTitle">
<source>Use 'ThrowIfCancellationRequested'</source>
<target state="new">Use 'ThrowIfCancellationRequested'</target>
<note />
</trans-unit>
<trans-unit id="UseContainerLevelAccessPolicy">
<source>Use Container Level Access Policy</source>
<target state="translated">Použít zásady přístupu na úrovni kontejneru</target>
Expand Down
Loading