Use cancellation token throw if cancellation requested #4864

NewellClark · 2021-02-17T19:36:42Z

Fix #33809

-Add no-diagnostic tests -Fix incorrect formatting by code fixer

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

codecov · 2021-02-17T20:10:44Z

Codecov Report

Merging #4864 (f09b43e) into release/6.0.1xx (4bda814) will increase coverage by 0.00%.
The diff coverage is 96.09%.

@@                Coverage Diff                @@
##           release/6.0.1xx    #4864    +/-   ##
=================================================
  Coverage            95.57%   95.58%            
=================================================
  Files                 1212     1215     +3     
  Lines               277225   277840   +615     
  Branches             16737    16759    +22     
=================================================
+ Hits                264971   265564   +593     
- Misses               10020    10033    +13     
- Partials              2234     2243     +9

Youssef1313 · 2021-02-17T20:12:29Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -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>


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>

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.

Youssef1313 · 2021-02-17T20:13:01Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

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


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>

Evangelink · 2021-02-21T08:58:50Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -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>


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.

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

Evangelink · 2021-02-21T09:01:55Z

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

+                    int position = conditional.Syntax.SpanStart;
+                    Diagnostic diagnostic = conditional.CreateDiagnostic(
+                        Rule,
+                        symbols.ThrowIfCancellationRequestedMethod.ToMinimalDisplayString(model, position, SymbolDisplayFormat.MinimallyQualifiedFormat),


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

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

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

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Evangelink · 2021-02-21T09:09:13Z

...icrosoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.Fixer.cs

+            }
+
+            var codeAction = CodeAction.Create(
+                Resx.UseCancellationTokenThrowIfCancellationRequestedTitle,


@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).

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

buyaa-n

Overall looks great, thanks @NewellClark, left few questions. Just curious did you tested it with runtime repo?

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

...icrosoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.Fixer.cs

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

Changes to code will be applied separately. Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>

…rowIfCancellationRequested

buyaa-n · 2021-03-09T07:24:27Z

Thanks @NewellClark for addressing comments, somehow the analyzer not working after adding it in VS SDK analyzers folder, did you tried that? I can see from VS the analyzer added within the SDK analyzers, I did changed the severity to warning but still no warning, I see similar unit tests passed but no warning in VS, do you have any idea? cc @mavasani

sharwell · 2021-04-08T15:36:18Z

somehow the analyzer not working after adding it in VS SDK analyzers folder

I would not expect this to work without other changes. Can you describe the complete set of steps you followed?

buyaa-n · 2021-04-08T17:23:15Z

I would not expect this to work without other changes.

That was what i was expecting

Can you describe the complete set of steps you followed?

Just normal steps i do whenever i need to test analyzer changes manually, build with a version my repo using and replace the build result with the nugget package/sdk analyzers with the new build, upgrade the rule level to warning, add a code that should warn, no warning. The steps worked for all i have ever tried, IDK what is wrong here

mavasani · 2021-04-14T03:14:30Z

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

+                    whenTrueUnwrapped is IThrowOperation @throw &&
+                    @throw.Exception is IObjectCreationOperation objectCreation &&
+                    IsDefaultOrTokenOperationCanceledExceptionCtor(objectCreation.Constructor) &&
+                    conditional.WhenFalse is null)


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.

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

A simple affirmative check with no else clause, or

A negated check with an else clause.

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

mavasani · 2021-04-14T03:17:33Z

...icrosoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.Fixer.cs

+                {
+                    var editor = await DocumentEditor.CreateAsync(context.Document, token).ConfigureAwait(false);
+                    SyntaxNode expressionStatement = CreateThrowIfCancellationRequestedExpressionStatement(editor, conditional, propertyReference);
+                    editor.ReplaceNode(conditional.Syntax, expressionStatement);


Do we need to preserve the trivia, say:

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

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.

mavasani · 2021-04-14T04:15:05Z

Overall looks good to me. @NewellClark Can you please retarget the PRs to preview4 branch?

- Add tests ensuring trivia is preserved - Ensure trivia is preserved

…rowIfCancellationRequested

Youssef1313 · 2021-05-13T19:20:51Z

...icrosoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.Fixer.cs

+    /// Use <see cref="CancellationToken.ThrowIfCancellationRequested"/> instead of checking <see cref="CancellationToken.IsCancellationRequested"/> and
+    /// throwing <see cref="OperationCanceledException"/>.
+    /// </summary>
+    [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic)]


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

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

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs

mavasani · 2021-05-17T11:11:43Z

...icrosoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.Fixer.cs

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


@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?

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

mavasani

Overall looks great to me. As always, thanks for the extensive unit tests and comments in source code with examples, very much appreciated!

Please retarget the PR to release\6.0.1xx branch and address the remaining comments so it can be merged.

Co-authored-by: Manish Vasani <mavasani@microsoft.com>

…ancellationTokenThrowIfCancellationRequested.cs Co-authored-by: Manish Vasani <mavasani@microsoft.com>

…ellationRequested

NewellClark added 4 commits February 16, 2021 12:34

Add files

5447dc5

Add affirmative tests

122ebad

More affirmative tests

214adef

Add no diagnostic tests

7b0522d

-Add no-diagnostic tests -Fix incorrect formatting by code fixer

NewellClark requested a review from a team as a code owner February 17, 2021 19:36

NewellClark changed the base branch from master to release/6.0.1xx-preview2 February 17, 2021 19:37

Youssef1313 reviewed Feb 17, 2021

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Show resolved Hide resolved

Youssef1313 reviewed Feb 17, 2021

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 17, 2021

View reviewed changes

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 17, 2021

View reviewed changes

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 17, 2021

View reviewed changes

Evangelink reviewed Feb 21, 2021

View reviewed changes

Apply suggested changes

090bde5

buyaa-n approved these changes Feb 24, 2021

View reviewed changes

NewellClark and others added 3 commits February 27, 2021 10:29

Apply changes to resources from code review

4e6d467

Changes to code will be applied separately. Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>

Remove dead code found by buyaa-n

445c415

Merge branch 'release/6.0.1xx-preview2' into use-CancellationToken-Th…

d16627f

…rowIfCancellationRequested

mavasani reviewed Apr 14, 2021

View reviewed changes

NewellClark added 3 commits May 10, 2021 15:03

Allow affirmative checks with else-clause

f10db94

Add VB test

688388c

Preserve trivia

369b057

- Add tests ensuring trivia is preserved - Ensure trivia is preserved

NewellClark changed the base branch from release/6.0.1xx-preview2 to release/6.0.1xx-preview4 May 13, 2021 16:37

Merge branch 'release/6.0.1xx-preview4' into use-CancellationToken-Th…

bcb2ec9

…rowIfCancellationRequested

Youssef1313 reviewed May 13, 2021

View reviewed changes

NewellClark added 2 commits May 14, 2021 10:50

Fix CI

4d890bd

Add shared attribute to fixer

a551291

mavasani reviewed May 17, 2021

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Outdated Show resolved Hide resolved

mavasani reviewed May 17, 2021

View reviewed changes

...Core/Microsoft.NetCore.Analyzers/Runtime/UseCancellationTokenThrowIfCancellationRequested.cs Show resolved Hide resolved

mavasani reviewed May 17, 2021

View reviewed changes

NewellClark changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 17, 2021 17:01

NewellClark and others added 4 commits May 17, 2021 13:02

Update src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

24f0017

Co-authored-by: Manish Vasani <mavasani@microsoft.com>

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseC…

3a4b0c8

…ancellationTokenThrowIfCancellationRequested.cs Co-authored-by: Manish Vasani <mavasani@microsoft.com>

Simplify fixer

ea967fe

Merge branch 'release/6.0.1xx' into use-CancellationToken-ThrowIfCanc…

f09b43e

…ellationRequested

mavasani approved these changes May 20, 2021

View reviewed changes

mavasani merged commit 92a247f into dotnet:release/6.0.1xx May 20, 2021

Youssef1313 mentioned this pull request May 20, 2021

Tracking issue for missing documentation of .NET6 CAxxxx rules dotnet/docs#23900

Closed

22 tasks

NewellClark mentioned this pull request May 21, 2021

Ca2250 dotnet/docs#24324

Merged

NewellClark deleted the use-CancellationToken-ThrowIfCancellationRequested branch August 27, 2021 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cancellation token throw if cancellation requested #4864

Use cancellation token throw if cancellation requested #4864

NewellClark commented Feb 17, 2021

codecov bot commented Feb 17, 2021 •

edited

Loading

Youssef1313 Feb 17, 2021 •

edited

Loading

Evangelink Feb 21, 2021

NewellClark Feb 22, 2021

Youssef1313 Feb 17, 2021 •

edited

Loading

NewellClark Feb 22, 2021

Evangelink Feb 21, 2021

Evangelink Feb 21, 2021

NewellClark Feb 21, 2021

NewellClark Feb 22, 2021

Evangelink Feb 21, 2021

NewellClark Feb 22, 2021

buyaa-n left a comment

buyaa-n commented Mar 9, 2021

sharwell commented Apr 8, 2021

buyaa-n commented Apr 8, 2021

mavasani Apr 14, 2021 •

edited

Loading

NewellClark May 10, 2021 •

edited

Loading

mavasani Apr 14, 2021

NewellClark May 10, 2021 •

edited

Loading

mavasani commented Apr 14, 2021

Youssef1313 May 13, 2021

NewellClark May 14, 2021

mavasani May 17, 2021

NewellClark May 17, 2021

mavasani left a comment

	<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>

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

	<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>

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

Use cancellation token throw if cancellation requested #4864

Use cancellation token throw if cancellation requested #4864

Conversation

NewellClark commented Feb 17, 2021

codecov bot commented Feb 17, 2021 • edited Loading

Codecov Report

Youssef1313 Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n left a comment

Choose a reason for hiding this comment

buyaa-n commented Mar 9, 2021

sharwell commented Apr 8, 2021

buyaa-n commented Apr 8, 2021

mavasani Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

NewellClark May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark May 10, 2021 • edited Loading

Choose a reason for hiding this comment

mavasani commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2021 •

edited

Loading

Youssef1313 Feb 17, 2021 •

edited

Loading

Youssef1313 Feb 17, 2021 •

edited

Loading

mavasani Apr 14, 2021 •

edited

Loading

NewellClark May 10, 2021 •

edited

Loading

NewellClark May 10, 2021 •

edited

Loading