Add support for CA1305 suppression. #5374

MartyIX · 2021-08-15T15:20:26Z

codecov · 2021-08-15T15:41:51Z

Codecov Report

Merging #5374 (1f825aa) into main (664f3d5) will decrease coverage by 0.10%.
The diff coverage is 99.75%.

@@            Coverage Diff             @@
##             main    #5374      +/-   ##
==========================================
- Coverage   95.60%   95.49%   -0.11%     
==========================================
  Files        1200     1213      +13     
  Lines      276353   276401      +48     
  Branches    16633    16709      +76     
==========================================
- Hits       264196   263946     -250     
- Misses       9980    10263     +283     
- Partials     2177     2192      +15

MartyIX · 2021-08-17T11:01:48Z

@Youssef1313 Would you mind having a look?

Youssef1313

LGTM

Youssef1313

Please open an issue for dotnet/docs to update the documentation when this PR is merged and a new version is released.

MartyIX · 2021-08-17T12:03:50Z

Thank you!

MartyIX · 2021-08-23T12:59:07Z

@sharwell Gentle ping. Can a buddy be assigned here? :-)

Youssef1313 · 2021-08-28T12:06:26Z

@buyaa-n @jeffhandley Would you be the correct one to review this?

jeffhandley

The logic looks good to me. I added a couple of syntax nit comments. Beyond that, I'd like to see more test cases added to cover conditions of:

methodsWithSameNameAsTargetMethod.HasMoreThan(1) being true and false
With overloads that do and don't match the desired parameters
And overloads where the last parameter is and is not IFormatProvider, ensuring we hit each of the correctPOverloads.FirstOrDefault() possibilities
Where targetMethod.ReturnType is and is not a string
Where the symbol does and does not match one of the ...UICultureProperty properties

After that, I recommend getting a review from @sharwell or @jmarolf before we accept it.

jeffhandley · 2021-09-04T00:05:42Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

-
-                IEnumerable<IMethodSymbol> methodsWithSameNameAsTargetMethod = targetMethod.ContainingType.GetMembers(targetMethod.Name).OfType<IMethodSymbol>().WhereMethodDoesNotContainAttribute(obsoleteAttributeType).ToList();
-                if (methodsWithSameNameAsTargetMethod.HasMoreThan(1))
                {


It looks like this set of braces can be removed

Interestingly, it looks like Github moved your comments to different line numbers.

jeffhandley · 2021-09-04T00:06:54Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

                #region "UICultureStringRule & UICultureRule"
-                IEnumerable<int> IformatProviderParameterIndices = GetIndexesOfParameterType(targetMethod, iformatProviderType);
-                foreach (var index in IformatProviderParameterIndices)
                {


It looks like these braces can be removed.

Evangelink · 2021-09-05T15:39:38Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

-                    // Sample message for IFormatProviderAlternateRule: Because the behavior of Convert.ToInt64(string) could vary based on the current user's locale settings,
-                    // replace this call in IFormatProviderStringTest.TestMethod() with a call to Convert.ToInt64(string, IFormatProvider).
-                    if (correctOverload != null)
+                    if (!oaContext.Options.IsConfiguredToSkipAnalysis(rule, targetMethod, oaContext.ContainingSymbol, oaContext.Compilation))


[cosmetic] Could you revert the condition to reduce nesting?

Evangelink · 2021-09-05T15:39:43Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

-                                oaContext.ContainingSymbol.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat),
-                                correctOverload.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat)));
+                        IEnumerable<IMethodSymbol> methodsWithSameNameAsTargetMethod = targetMethod.ContainingType.GetMembers(targetMethod.Name).OfType<IMethodSymbol>().WhereMethodDoesNotContainAttribute(obsoleteAttributeType).ToList();
+                        if (methodsWithSameNameAsTargetMethod.HasMoreThan(1))


[cosmetic] Could you revert the condition to reduce nesting?

Evangelink · 2021-09-05T15:40:25Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs


-                    if (argument != null && currentUICultureProperty != null &&
-                        installedUICultureProperty != null && currentThreadCurrentUICultureProperty != null)
+                    if (!oaContext.Options.IsConfiguredToSkipAnalysis(rule, targetMethod, oaContext.ContainingSymbol, oaContext.Compilation))


[cosmetic] Could you revert the condition to reduce nesting?

Would you please elaborate on what you mean? It's not obvious to me. I understand what you want but not how to do it actually.

@Evangelink This probably can't be done without goto? We don't bail out if it's configured to skip analysis since there are more rules to check.

However, I actually think that all IsConfiguredToSkipAnalysis calls will return the same result since all the 4 rules use the same ID?

In this case, I'll consider updating IsConfiguredToSkipAnalysis to take a rule id instead. But that probably should be in a different PR. Is there anything I'm missing?

However, I actually think that all IsConfiguredToSkipAnalysis calls will return the same result since all the 4 rules use the same ID?

This is something I'm interested in too.

Evangelink · 2021-09-05T15:40:29Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

-                                targetMethod.ToDisplayString(SymbolDisplayFormats.ShortSymbolDisplayFormat)));
+                            var argument = invocationExpression.Arguments[index];
+
+                            if (argument != null && currentUICultureProperty != null &&


[cosmetic] Could you revert the condition to reduce nesting?

Evangelink · 2021-09-05T15:41:10Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyIFormatProvider.cs

+                                    // This property returns a culture that is inappropriate for formatting methods.
+
+                                    oaContext.ReportDiagnostic(
+                                    invocationExpression.Syntax.CreateDiagnostic(


[cosmetic] This line and the next are missing some indent.

MartyIX · 2021-09-06T20:05:24Z

@jeffhandley I did try to address your feedback. I'm not entirely sure that I did it correctly but I did try :-)

MartyIX · 2021-09-07T10:36:25Z

@sharwell @jmarolf Would you mind having a look, please?

jmarolf

jeffhandley

Thanks for addressing the feedback and for the additional tests.

MartyIX · 2021-09-13T10:33:26Z

@jeffhandley Would it be conceivable to port this PR even to .NET 6.0 branch? It seems to me that currently the PR is merged to .NET 7.0 branch.

Can I possibly help?

jeffhandley · 2021-09-17T00:45:58Z

I'm on the fence for this. We've closed down RC2 so the fix would be ported into the final 6.0 release if we did. Our bar for fixes in the GA milestone is very high (generally only what we would service 6.0 for), and I'm uncertain if this meets that bar.

@jmarolf What do you think?

MartyIX · 2021-09-17T05:59:54Z

I see. One thing to consider is that the change is rather small (most of the added code is tests).

The alternative for me is to wait a year and not use CA1305 at all, if I get it correctly. I mean the rule discovers useful things for us but without suppressing the rule is not that useful as we use NLog where we are not interested in the warnings - probably the case for many people.

Add support for CA1305 suppression.

4d5aa28

Youssef1313 approved these changes Aug 17, 2021

View reviewed changes

Youssef1313 suggested changes Aug 17, 2021

View reviewed changes

MartyIX marked this pull request as ready for review August 17, 2021 12:03

MartyIX requested a review from a team as a code owner August 17, 2021 12:03

jeffhandley reviewed Sep 4, 2021

View reviewed changes

MartyIX added 2 commits September 5, 2021 09:08

Remove braces (dotnet#5374 (comment) + dotnet#5374 (comment))

4a6163b

Remove ToList calls that seem superfluous.

276187a

Evangelink reviewed Sep 5, 2021

View reviewed changes

Update tests.

75d9f8f

MartyIX added 3 commits September 6, 2021 22:55

Fix formatting.

01efec1

Again.

997c5b4

Test for UICulture.

1f825aa

jmarolf approved these changes Sep 7, 2021

View reviewed changes

jmarolf merged commit 7f55d85 into dotnet:main Sep 7, 2021

jeffhandley reviewed Sep 7, 2021

View reviewed changes

MartyIX deleted the feature/ca1305-suppress branch September 7, 2021 18:56

MartyIX mentioned this pull request Sep 9, 2021

Add documentation for suppressions of CA1305 dotnet/docs#26028

Open

jmarolf pushed a commit to jmarolf/roslyn-analyzers that referenced this pull request Sep 23, 2021

Remove braces (dotnet#5374 (comment) + dotnet#5374 (comment))

04fa690

jmarolf mentioned this pull request Sep 23, 2021

Add support for CA1305 suppression. #5520

Merged

jmarolf mentioned this pull request Jan 31, 2022

infrastructure/report test hangs and crashes jmarolf/roslyn-analyzers#1

Closed

Youssef1313 mentioned this pull request Nov 5, 2022

Document excluded_symbol_names support for CA1305 dotnet/docs#32213

Closed

Add support for CA1305 suppression. #5374

Add support for CA1305 suppression. #5374

Uh oh!

Conversation

MartyIX commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MartyIX commented Aug 17, 2021

Uh oh!

Youssef1313 left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 left a comment

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Aug 17, 2021

Uh oh!

MartyIX commented Aug 23, 2021

Uh oh!

Youssef1313 commented Aug 28, 2021

Uh oh!

jeffhandley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartyIX commented Sep 7, 2021

Uh oh!

jmarolf left a comment

Choose a reason for hiding this comment

Uh oh!

jeffhandley left a comment

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Sep 13, 2021

Uh oh!

jeffhandley commented Sep 17, 2021

Uh oh!

MartyIX commented Sep 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MartyIX commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 15, 2021 •

edited

Loading

MartyIX commented Sep 6, 2021 •

edited

Loading