Update CA2241 to handle cases from IDE0043 #4226

Evangelink · 2020-09-25T17:21:02Z

Evangelink · 2020-09-25T17:21:42Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+                        if (missingIndexes.Any())
                        {
-                            // can't deal with this for now.
+                            context.ReportDiagnostic(invocation.CreateDiagnostic(Rule));


We may want to report with a different diagnostic (message + desc).

I think, since we are saying we want to break with legacy FxCop behavior that introducing a new, more specific warning message would be nice.

I would still keep the same rule ID except if you think that's valuable to let users disable some part of the rule (I don't see the need as I don't expect much FPs).

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

Evangelink · 2020-09-25T17:24:18Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

+        {|#1:" + invocation + @"(""{0} {1} {2}"", 1)|};
+        {|#2:" + invocation + @"(""{0} {1} {2}"", 1, 2)|};
+
+        // These cases are not handled as we cannot find a parameter named format


I was tempted to update the code to handle these cases but I was a bit worried about the perf impact of analyzing the extra overloads. Let me know what you think.

I would go ahead and handle them.

codecov · 2020-09-29T09:42:05Z

Codecov Report

❌ Patch coverage is 99.15254% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.83%. Comparing base (5f3a4ce) to head (9003d4f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4226      +/-   ##
==========================================
+ Coverage   95.81%   95.83%   +0.02%     
==========================================
  Files        1170     1170              
  Lines      266447   266951     +504     
  Branches    16050    16051       +1     
==========================================
+ Hits       255293   255836     +543     
+ Misses       9102     9087      -15     
+ Partials     2052     2028      -24

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mavasani · 2020-10-05T14:22:50Z

@jmarolf You have the most context on this analyzer. Can you please review?

Evangelink · 2020-10-05T14:29:21Z

@mavasani @jmarolf I think I need to rewrite the analyzer to use the regex as suggested in the ticket but I am fine with having a first review to start with, it shouldn't change much the main portion of the code.

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

jmarolf · 2020-10-05T15:01:40Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

        }

-        private static int GetFormattingArguments(string format)
+        private static HashSet<int>? GetStringFormatItemIndexes(string format)


I would extract this to a helper class and add unit tests explicitly for it. If we are going to maintain our own fork of the mscorlib parser code we should have tests to document its behavior.

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

jmarolf · 2020-10-05T15:10:01Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs


 namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests
 {
    public class ProvideCorrectArgumentsToFormattingMethodsTests


Can we add a test for __arglist?

Do you mean an empty arglist? We have some tests with __arglist(5)

I am not sure how to get it to work, I was expecting the following to be good but that's not the case:

public void M(__arglist) { System.Console.WriteLine("", __arglist); }

jmarolf

looks good! I would appreciate it if we could add a few more tests while we are here and I think having a separate, more descriptive diagnostic message would be nice

# Conflicts: # src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

Evangelink · 2020-10-06T13:57:28Z

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

-  <data name="ProvideCorrectArgumentsToFormattingMethodsMessage" xml:space="preserve">
-    <value>Provide correct arguments to formatting methods</value>
+  <data name="ProvideCorrectArgumentsToFormattingMethodsMessageArgs" xml:space="preserve">
+    <value>Incorrect number of arguments passed to this formatting method call</value>


Do we want to add how many params are expected and how many were passed?

This sounds like a great idea

Evangelink · 2020-10-06T13:58:32Z

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

    <value>Replace with 'string.Contains'</value>
  </data>
+  <data name="ProvideCorrectArgumentsToFormattingMethodsMessageFormatItem" xml:space="preserve">
+    <value>Some string format items are missing from this formatting method call</value>


I don't know if it brings much to add the missing indexes, WDYT?

Considering we will "squiggle" the entire thing I think some text explaining how to fix what is wrong would help. Saying

"Some string format items are missing from this formatting method call. Nothing was passed for index '0'"

Evangelink · 2020-10-06T13:59:21Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+            isDataflowRule: false);
+
+        internal static DiagnosticDescriptor MissingFormatItemRule = DiagnosticDescriptorHelper.Create(
+            RuleId,


I don't think there is a need for a different ID (i.e. being able to activate one behavior without the other).

I think that users may want to configure different severities for these rules which would necessitate a different ID.

having too few parameters, run time error, should be a warning

having too many parameters, not a runtime error, should probably be info by default

Evangelink · 2020-10-06T14:00:55Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+        /// <summary>
+        /// This regex is used to remove escaped brackets from the format string before looking for valid {} pairs.
+        /// </summary>
+        private static readonly Regex s_removeEscapedBracketsRegex = new("{{");


Not sure if it's better to use the compiled regex here. It's not present in IDE0043 so I am assuming this was a conscious call.

compiling the regex has binary size implications in visual studio which we are very paranoid about. Having the regex be precompiled here is fine.

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

Evangelink · 2020-10-06T14:05:24Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+                    // but we want to skip it only in C# since VB doesn't support __arglist
+                    if (info.ExpectedStringFormatArgumentCount >= 0 &&
+                        invocation.TargetMethod.IsVararg &&
+                        invocation.Language == LanguageNames.CSharp)


This filter was suggested but not present in the original implementation. Let me know if you prefer to revert this.

I am fine with this change

Evangelink · 2020-10-06T14:05:44Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+                        if (missingIndexes.Any())
                        {
-                            // can't deal with this for now.
+                            context.ReportDiagnostic(invocation.CreateDiagnostic(MissingFormatItemRule));


Do we want to report only on the string parameter?

Evangelink · 2020-10-06T14:08:30Z

There is an extra user option available in IDE0043, do we want to integrate it?

jmarolf · 2020-10-07T20:33:34Z

There is an extra user option available in IDE0043, do we want to integrate it?

If you have the time, I would love that. I think each of these options should be a different diagnostic Id that the user can configure independently.

jmarolf · 2020-10-07T20:35:23Z

@mavasani I would like providing too few parameters (which will cause a runtime error) to be enabled as a warning for analysis level 6. Do you have any objections?

mavasani · 2020-10-08T15:40:03Z

@mavasani I would like providing too few parameters (which will cause a runtime error) to be enabled as a warning for analysis level 6. Do you have any objections?

Sounds reasonable. However, lets follow a process for promoting any CA rule to a warning:

Create a separate design issue for the proposal
Discuss it via a design meeting with NetAnalyzers v-team OR get confirmation via email
Run the analyzer over dotnet/roslyn and dotnet/runtime to get confidence on no false positives and acceptable performance
Open a PR to promote it as a warning
File a doc issue on dotnet/docs to document breaking change in net6.0

jmarolf · 2020-10-08T18:41:02Z

@mavasani sounds like a plan. Looking over my notes from the last meeting I believe this has already been approved, but I agree let's not put foist those additional requirements on this PR. I'll create a follow up PR after this is merged.

mavasani · 2020-10-08T19:05:21Z

I'll create a follow up PR after this is merged.

As part of that we should also bump the analyzer to warning in dotnet/roslyn and dotnet/runtime and confirm no false positives and acceptable performance. I think that should be our miniumn quality gate before bumping up a rule to a warning.

Evangelink · 2020-10-12T12:45:49Z

I am working on the various open comment but I have a side question regarding the descriptors we want to create, basically we have the following use-cases:

Not enough args compared for well-known string format methods (overload with expected arguments)
a. Console.WriteLine("{0} {1}", 42); -> causes runtime failure = Descriptor1 (e.g. NotEnoughArgsRule)
b. Console.WriteLine("{1}", 42); -> causes runtime failure = Descriptor1 (e.g. NotEnoughArgsRule) but maybe also Descriptor2 (e.g. NotEnoughArgsMissingFormatItemRule). In this case we might want Descriptor2 to have the same ID as 1 but with a different message. I wouldn't report 2 diag on this line because we don't know what's the correct fix (i.e. it could be change 1 to 0 or add one more parameter).
Not enough args compared for well-known string format methods (overload without expected arguments). For example, Console.WriteLine("{0} {1}") or Console.WriteLine("{1}"), there is no runtime error so we probably want to use Descriptor3 (e.g. FormatItemInNonStringFormatMethodRule) and maybe Descriptor4 (e.g. FormatItemInNonStringFormatMethodMissingFormatItemRule) for the case with the missing {0}.
Correct number of args but missing format item (e.g. Console.WriteLine("{1}", 1, 2)) -> Descriptor5 (e.g. EnoughArgsMissingFormatItemRule).
Too many args for well-known string format methods. For example, Console.WriteLine("{0}", 1, 2) -> Descriptor6 (e.g. TooManyArgsRule) and for Console.WriteLine("{1}", 1, 2, 3) Descriptor7 (e.g. TooManyArgsMissingFormatItemRule).

In addition to the previous cases, there is a user option to provide additional string format methods, shall we reuse the previous descriptors or create a new set of them? Especially for case 1, where we are not sure if these additional methods will cause the runtime error so we might not want them to be raised as Warning by default.

Regarding the user-option available in roslyn - IDE0043 looking at the tests it seems that this is used to validate what I have called missing format item here. Do we want to keep this as a separate option?

Evangelink · 2020-10-20T16:20:03Z

I am wondering what can be done in term of diagnostic ID, I mean could we depreciate an old ID? I'd like to avoid having a too big gap between the 2 analyzers ID. The next available ID is CA2250. Except if we want to have 2 separate categories too.

I am also wondering if we want to simply bail-out for C# varargs or if we still want to report for missing format indexes? Related to this subject, I would need help for the syntax of the empty __arglist, I cannot make it compile.

I will push a WIP soon, here are the remaining steps:

Change location of the diagnostics
Update all title, message and descriptions
Add support for detection of format items in overload without the format string argument
Ensure I have ported all the editorconfig from CA2241 and IDE0043

mavasani · 2020-10-20T16:29:59Z

@Evangelink Let's not deprecate CA2241 ID, let's just repurpose it for one of the new buckets. Microsoft.CodeAnalysis.FxCopAnalyzers is soon going to be deprecated in favor of Microsoft.CodeAnalysis.NetAnalyzers, and we do not need to preserve any backcompat behavior from FxCop in the latter, so I wouldn't worry about breaking changes in CA2241.

Evangelink · 2020-10-20T16:38:22Z

It just feels a little weird as a user to have two really related rules so separated while I don't see much people who wouldn't both running.

mavasani · 2020-10-20T16:50:34Z

It just feels a little weird as a user to have two really related rules so separated while I don't see much people who wouldn't both running.

We already have multiple such cases, so it should be fine. Rule IDs should essentially be considered opaque.

Evangelink · 2020-10-20T17:10:10Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

-                csharpTest.ExpectedDiagnostics.Add(
-                    // Test0.cs(8,17): warning CA2241: Provide correct arguments to formatting methods
-                    GetCSharpResultAt(8, 17));
+                // TODO: Make sure to report only the right diagnostic not both


@jmarolf @mavasani Do we have some kind of built-in mechanism I can rely on to fix this? Otherwise I think I will need to return the list of diagnostic allowed and use this at the time where I report.

@sharwell as he is the expert on test framework helpers. I am not aware of one.

Evangelink · 2020-10-20T17:11:46Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

+        // Not enough args
+        var s3 = MyFormat(""{0} {1}"", 1);
+        // Not enough args and missing format index
+        var s4 = MyFormat(""{0} {2}"", 1);


I am tempted to have a separate set of descriptors for these cases (they will use CA2241 as the too many cases) because we are not sure these will lead to runtime failure as this is a custom implementation.

I would be fine with not issuing these diagnostics at all to start. Yes, historically we've done this but and I can see the utility but I would not turn these on for folks by default. This is always going to be something that you need to opt into. I like the idea of having a separate ID for custom methods so folks can still get these warnings on their custom loggers.

Evangelink · 2020-10-20T17:12:08Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

+        // Not enough args
+        var s3 = MyFormat(""{0} {1}"", 1);
+        // Not enough args and missing format index
+        var s4 = MyFormat(""{0} {2}"", 1);


Same comment regarding the separate descriptors.

yep, lets add a new descriptor for this case.

Evangelink · 2020-10-20T17:13:15Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

-        var g = String.Format(p, ""{0}"", 1, 2);
-        var h = String.Format(p, ""{0} {1}"", 1, 2, 3);
-        var i = String.Format(p, ""{0} {1} {2}"", 1, 2, 3, 4);
+        {|#0:" + invocation + @"("""", 1)|};


@jmarolf so on these cases I need to report on all extra args, is it right?

I think thats what we want. Once we get the preview out for .NET 6 well see if feedback directs us to change this.

Evangelink · 2020-10-20T17:14:22Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

-        Console.Write(""{0} {1}"", 1, 2, 3);
-        Console.Write(""{0} {1} {2}"", 1, 2, 3, 4);
-        Console.Write(""{0} {1} {2} {3}"", 1, 2, 3, 4, 5);
+        {|#0:" + invocation + @"(""{1}"", 1, 2, 3)|};


@jmarolf here I need to report on all extra args + somewhere for the missing format indexes (any suggestion?)

for this case I think the most useful location would be 1, and 3 indicating that these are unused.

Evangelink · 2020-10-20T17:15:04Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

-        var s = String.Format(""{0} {1} {2} {3}"", new object[] {1, 2});
-        Console.Write(""{0} {1} {2} {3}"", new object[] {1, 2, 3, 4, 5});
-        Console.WriteLine(""{0} {1} {2} {3}"", new object[] {1, 2, 3, 4, 5});
+        {|#0:" + invocation + @"(""{0} {1}"", 1)|};


Here I highlight all format indexes without associated args, right?

That would be my preference

Evangelink · 2020-10-20T17:15:30Z

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

    {
-        Console.Write(""{0} {1} {2} {3} {4}"", 1, 2, 3, 4, __arglist(5));
-        Console.WriteLine(""{0} {1} {2} {3} {4}"", 1, 2, 3, 4, __arglist(5));
+        {|#0:" + invocation + @"(""{1}"", 1)|};


How to highlight the missing indexes?

In this case I think we want to have a diagnostic on , 1 and say that is will not be used.

mavasani · 2020-11-23T16:10:34Z

@jmarolf Would be you able to review the latest commit and answer @Evangelink's questions?

jmarolf · 2020-11-23T19:07:23Z

@mavasani I'll review this after lunch, thanks for the ping

jmarolf · 2020-11-24T15:22:55Z

@Evangelink done with review pass, please ping me if I missed something.

Evangelink · 2020-11-24T15:29:39Z

@jmarolf Thank you for the various feedback! I will try to work on the pending changes by the end of the week (I might need a bit of time to get back to what's done and what's left to do, it's no longer really fresh in my mind).

mavasani

Please retarget this PR to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx-preview1

jmarolf · 2021-02-04T18:56:27Z

@Evangelink gentle ping, do you have time to retarget this?

Evangelink · 2021-02-07T10:37:07Z

@jmarolf I will try to finish this PR by the end of next week. Looking back at it, there is still a lot of work to do.

mavasani · 2022-01-31T08:42:02Z

@Evangelink Would you have time to proceed further on this PR? Thanks!

Youssef1313 · 2022-07-01T04:44:03Z

...yzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethods.cs

+            DiagnosticCategory.Usage,
+            RuleLevel.BuildWarningCandidate,
+            description: s_localizableDescription,
+            isPortedFxCopRule: true,


Were all rules supported by FxCop?

Removing stale PR block

Youssef1313 · 2022-07-22T09:15:40Z

Ping @Evangelink

Update CA2241 to handle cases from IDE0043

968b2e0

Evangelink requested a review from a team as a code owner September 25, 2020 17:21

Evangelink commented Sep 25, 2020

View reviewed changes

This was referenced Sep 25, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

Perform placeholder validation before checking for __arglist

ba01034

mavasani requested a review from jmarolf October 5, 2020 14:22

mavasani assigned jmarolf Oct 5, 2020

jmarolf reviewed Oct 5, 2020

View reviewed changes

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

jmarolf reviewed Oct 5, 2020

View reviewed changes

...Tests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs Show resolved Hide resolved

jmarolf reviewed Oct 5, 2020

View reviewed changes

jmarolf suggested changes Oct 5, 2020

View reviewed changes

Evangelink added 5 commits October 6, 2020 10:09

Merge branch 'master' into CA2241-IDE0043

b0021b5

# Conflicts: # src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideCorrectArgumentsToFormattingMethodsTests.cs

Update link to mscorlib code

0082564

Add more tests

0ff407c

Create separate rule for missing format item

45c5b59

Rewrite parsing algorithm using a regex

875b492

Evangelink commented Oct 6, 2020

View reviewed changes

Merge branch 'master' into CA2241-IDE0043

79bde80

Implement core logic of the new analyzers

fc9c370

Introduce CA2250 for not enough args cases

9003d4f

Evangelink commented Oct 20, 2020

View reviewed changes

mavasani previously requested changes Dec 3, 2020

View reviewed changes

mavasani mentioned this pull request Jan 5, 2021

Ban DiagnosticDescriptor ctor in CodeStyle layer dotnet/roslyn#50031

Merged

Base automatically changed from master to main March 5, 2021 02:20

mavasani assigned Evangelink Jan 31, 2022

jmarolf removed their assignment Jun 6, 2022

Youssef1313 reviewed Jul 1, 2022

View reviewed changes

Youssef1313 mentioned this pull request Jul 1, 2022

Deprecate IDE0043 in favor of CA2241 dotnet/roslyn#62303

Open

manfred-brands mentioned this pull request Aug 24, 2023

Added support for StringSyntaxAttribute.CompositeFormat in CA2241 (#6… #6272

Merged

ViktorHofer deleted the branch dotnet:main August 15, 2025 11:13

ViktorHofer closed this Aug 15, 2025

Update CA2241 to handle cases from IDE0043 #4226

Update CA2241 to handle cases from IDE0043 #4226

Uh oh!

Conversation

Evangelink commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarolf Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mavasani commented Oct 5, 2020

Uh oh!

Evangelink commented Oct 5, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarolf Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Evangelink commented Oct 6, 2020

Uh oh!

jmarolf commented Oct 7, 2020

Uh oh!

jmarolf commented Oct 7, 2020

Uh oh!

mavasani commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarolf commented Oct 8, 2020

Uh oh!

mavasani commented Oct 8, 2020

Uh oh!

jmarolf Oct 5, 2020 •

edited

Loading

codecov bot commented Sep 29, 2020 •

edited

Loading

jmarolf Oct 5, 2020 •

edited

Loading

mavasani commented Oct 8, 2020 •

edited

Loading

Evangelink commented Oct 20, 2020 •

edited

Loading

jmarolf Nov 24, 2020 •

edited

Loading

jmarolf Nov 24, 2020 •

edited

Loading

jmarolf Nov 24, 2020 •

edited

Loading

jmarolf Nov 24, 2020 •

edited

Loading

jmarolf Nov 24, 2020 •

edited

Loading