Skip to content

Conversation

@Youssef1313
Copy link
Member

Fixes #4227

@Youssef1313 Youssef1313 requested a review from a team as a code owner September 26, 2020 18:19
@Evangelink
Copy link
Member

@Youssef1313 Have you been able to reproduce the issue? I couldn't...

@Youssef1313
Copy link
Member Author

@Evangelink I haven't tried, this PR was just a quick edit from GitHub UI 😄
But it's strange if the problem doesn't reproduce, I can't see anywhere in the existing code that it treats these attributes in any special way.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Sep 26, 2020

@Evangelink I read through the code again and I think I got it. The cancellation token should be optional to reproduce.
I'll open a draft PR to check test results from CI (opened #4229).

@Evangelink
Copy link
Member

@Youssef1313 I don't think so, some tests are raising an issue without having the parameter with a default value:

[Fact]
        public async Task DiagnosticOnExtensionMethodWhenCancellationTokenIsNotFirstParameter()
        {
            var test = @"
using System.Threading;
static class C1
{
    public static void M1(this object p1, CancellationToken p2, object p3)
    {
    }
}";

            var expected = VerifyCS.Diagnostic().WithLocation(5, 24).WithArguments("C1.M1(object, System.Threading.CancellationToken, object)");
            await VerifyCS.VerifyAnalyzerAsync(test, expected);
        }

@Youssef1313
Copy link
Member Author

Youssef1313 commented Sep 26, 2020

@Evangelink If optional parameter exists, the analyzer behavior is different based on whether the cancellation token is optional or not.

  • If cancellation token is optional: report diagnostic.
  • If cancellation token isn't optional: don't report because it cannot come last in this case.

So:

    public Task SomeAsync(CancellationToken cancellationToken,
        [CallerMemberName] string memberName = """",
        [CallerFilePath] string sourceFilePath = """",
        [CallerLineNumber] int sourceLineNumber = 0)
    {
        throw new NotImplementedException();
    } 

The above currently doesn't report diagnostic, which is correct.


    public Task SomeAsync(CancellationToken cancellationToken = default,
        [CallerMemberName] string memberName = """",
        [CallerFilePath] string sourceFilePath = """",
        [CallerLineNumber] int sourceLineNumber = 0)
    {
        throw new NotImplementedException();
    } 

But the above code will report diagnostic, this is the case reported in #4227


I haven't confirmed this, but both tests are added in this draft #4229, so waiting for CI to confirm.


Edit: @Evangelink The CI build in #4229 has completed and confirms that this analyzer reports a diagnostic when cancellation token is optional and is followed by caller information parameters. I'll close the draft PR once you give the build results a look.

@Youssef1313
Copy link
Member Author

@mavasani This is preferred to be squashed when merging.

@codecov
Copy link

codecov bot commented Sep 27, 2020

Codecov Report

Merging #4228 into master will increase coverage by 0.00%.
The diff coverage is 98.88%.

@@           Coverage Diff            @@
##           master    #4228    +/-   ##
========================================
  Coverage   95.80%   95.80%            
========================================
  Files        1170     1170            
  Lines      264435   264599   +164     
  Branches    15959    15966     +7     
========================================
+ Hits       253333   253504   +171     
+ Misses       9087     9081     -6     
+ Partials     2015     2014     -1     

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

LGTM

…nGuidelines/CancellationTokenParametersMustComeLast.cs
Copy link

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Change looks good, but needs moving well known type computation logic into CompilationStartAction.

@mavasani mavasani merged commit 193a3b6 into dotnet:master Sep 30, 2020
@Youssef1313 Youssef1313 deleted the patch-1 branch September 30, 2020 16:58
Evangelink added a commit to Evangelink/roslyn-analyzers that referenced this pull request Oct 12, 2020
…net#4228)

* CA1068: Allow caller attributes to come after cancellation token

* Add caller attributes tests for CA1068

* Ignore caller attribute only if optional

* Move Parameter.IsOptional check to HasCallerInformationAttribute

* Fix typo

* typo

* Use GetOrCreateTypeByMetadataName

Co-authored-by: Amaury Levé <[email protected]>

* Use AddIfNotNull

* Apply suggestions from code review

Co-authored-by: Amaury Levé <[email protected]>

* Update CancellationTokenParametersMustComeLastTests.cs

* Update CancellationTokenParametersMustComeLast.cs

* Fix

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Amaury Levé <[email protected]>

* Update src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/CancellationTokenParametersMustComeLast.cs

* Update WellKnownTypeNames.cs

* Use WellKnownTypeNames and move to compilation start

Co-authored-by: Amaury Levé <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CA1068 Should ignore reserved attributes from System.Runtime.CompilerServices

3 participants