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 4 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,14 @@
<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>'{0}' automatically checks whether the token has been canceled, and throws an '{2}' if it has.</value>
Copy link
Member

@Youssef1313 Youssef1313 Feb 17, 2021

Choose a reason for hiding this comment

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

Title and description doesn't use the format arguments as far as I know. They are for "Message" only.

@Evangelink Do you think it's worth extending DiagnosticDescriptorCreation analyzer for this?

Suggested change
<value>'{0}' automatically checks whether the token has been canceled, and throws an '{2}' if it has.</value>
<value>'ThrowIfCancellationRequested ' automatically checks whether the token has been canceled, and throws an 'OperationCanceledException' if it has.</value>

Copy link
Member

Choose a reason for hiding this comment

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

Hum that's a good question, I haven't really made the test. It's probably better to hardcode that call ToMinimalDisplayString when possible.

Regarding the improvement of the analyzer, I would personally rather go for a separate analyzer as it is something that would be useful for other situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedMessage" xml:space="preserve">
<value>Use '{0}' instead of checking '{1}'</value>
<comment>0=ThrowIfCancellationRequested method, 1=IsCancellationRequested property, 2=OperationCanceledException type</comment>
Copy link
Member

@Youssef1313 Youssef1313 Feb 17, 2021

Choose a reason for hiding this comment

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

Because these are already known, I don't think there is a value in passing them as format arguments.

Suggested change
<value>Use '{0}' instead of checking '{1}'</value>
<comment>0=ThrowIfCancellationRequested method, 1=IsCancellationRequested property, 2=OperationCanceledException type</comment>
<value>Use 'ThrowIfCancellationRequested' instead of checking 'IsCancellationRequested'</value>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

</data>
<data name="UseCancellationTokenThrowIfCancellationRequestedTitle" xml:space="preserve">
<value>Use '{0}'</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.UseCancellationTokenThrowIfCancellationRequestedTitle,
Copy link
Member

Choose a reason for hiding this comment

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

@mavasani The title makes sense as code-fix title in this case, would you prefer to have a separate entry for it anyway (with the same content).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a separate resource for the code fix title to be consistent.

createChangedDocument,
Resx.UseCancellationTokenThrowIfCancellationRequestedTitle);
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,209 @@
// 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 = CreateResource(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedTitle));
private static readonly LocalizableString s_localizableMessage = CreateResource(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedMessage));
private static readonly LocalizableString s_localizableDescription = CreateResource(nameof(Resx.UseCancellationTokenThrowIfCancellationRequestedDescription));
NewellClark marked this conversation as resolved.
Show resolved Hide resolved

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.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
NewellClark marked this conversation as resolved.
Show resolved Hide resolved
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 _))
{
var model = conditional.SemanticModel;
int position = conditional.Syntax.SpanStart;
Diagnostic diagnostic = conditional.CreateDiagnostic(
Rule,
symbols.ThrowIfCancellationRequestedMethod.ToMinimalDisplayString(model, position, SymbolDisplayFormat.MinimallyQualifiedFormat),
Copy link
Member

Choose a reason for hiding this comment

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

@mavasani I don't remember if you said we should avoid these calls or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a commit with the suggested changes tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've inlined the calls. Also removed calls to ToMinimalDisplayString as arguments are no longer used in the messages.

symbols.IsCancellationRequestedProperty.ToMinimalDisplayString(model, position, SymbolDisplayFormat.MinimallyQualifiedFormat),
symbols.OperationCanceledExceptionType.ToMinimalDisplayString(model, position, SymbolDisplayFormat.MinimallyQualifiedFormat));
context.ReportDiagnostic(diagnostic);
}
}
}

/// <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);
}
}

private static LocalizableString CreateResource(string resourceName)
{
return new LocalizableResourceString(resourceName, Resx.ResourceManager, typeof(Resx));
}
}
}
Loading