How we're expected to resolve `CA1863` with language resources `resx`?

JustArchi commented 1 year ago

Analyzer

Diagnostic ID: CA1863: Cache a 'CompositeFormat' for repeated use in this formatting operation

Analyzer source

SDK: Built-in CA analyzers in .NET 8 SDK or later

Version: SDK 8.0.100

Version: 8.0.0 (Latest)

Describe the bug

When using string.Format() repeatedly with resources-generated code from resx files, CA1863 is emitted, suggesting us to use CompositeFormat instead. The warning makes sense, as in, it's correct that CompositeFormat is not used and could be used, the problem is that the code it's referencing is generated automatically with no means of being fixed easily. Please check out my project for better understanding.

Steps To Reproduce

You can reproduce with my OSS project: ASF.

git clone https://github.com/JustArchiNET/ArchiSteamFarm.git
git checkout 305d6a2d170dd85fb8e040952bed3445e2cb19a5
dotnet build ArchiSteamFarm

Then analyze the build output.

Expected behavior

My expectation would be one of the following:

Either improve resx generator to automatically use composite formats, which would make sense considering it's frequently used (maybe make it opt-in or opt-out?)
Or ignore CA1863 when dealing with code from resources generator

Actual behavior

We're warned about CA1863 with no possible fix, as it'd involve either writing our own generator, manually changing generated code, or not using resx files to begin with.

Additional context

Originally observed with my ASF project, where I use resx strings for localization purpose: https://github.com/JustArchiNET/ArchiSteamFarm/tree/305d6a2d170dd85fb8e040952bed3445e2cb19a5

sharwell commented 11 months ago

Here's a sample file produced by ResxSourceGenerator: https://github.com/dotnet/roslyn-analyzers/blob/5dc99c6249a4cf845fbdcc367641c1b32347b85a/src/Microsoft.CodeAnalysis.ResxSourceGenerator/Microsoft.CodeAnalysis.ResxSourceGenerator.UnitTests/Resources/SingleString_EmitFormatMethodsCSharpAsync_0_False/Resources.Designer.cs

Do you have a specific suggestion for how it could be updated to use CompositeFormat in .NET 8 scenarios?

JustArchi commented 11 months ago

Sadly no, I don't even know if CompositeFormat can be integrated with it, as I'm not much into .NET internals. Perhaps the correct thing to do is just to silence warnings when roslyn detects that we're using string.Format() with resx-generated files - at least until somebody more skilled than me implements composite formats into it.

sharwell commented 11 months ago

Can you link to a line of code in your project where the warning is produced?

JustArchi commented 11 months ago

Sure, for example here highlighting Strings.ErrorIsInvalid.

sharwell commented 11 months ago

Related to #6579 (not the same issue, but probably want to resolve them at the same time)

jeremy-visionaid commented 3 months ago

Oh, I just found Microsoft.CodeAnalysis.ResxSourceGenerator. Looks like the generated files now have // <auto-generated/> in the headers. Can this one just be closed?

JustArchi commented 3 months ago

Can this one just be closed?

Unless something changed in .NET 9 which I didn't test yet - no, because roslyn still complains about CA1863 even with auto-generated files.

Silencing CA1863 when auto-generated comment is present becomes another option solving this issue though.

jeremy-visionaid commented 3 months ago

Unless something changed in .NET 9 which I didn't test yet - no, because roslyn still complains about CA1863 even with auto-generated files.

Silencing CA1863 when auto-generated comment is present becomes another option solving this issue though.

Oh, I see, I misunderstood the problem you were reporting. I meant that CA1863 no longer triggers within the source generated files (with EmitFormatMethods="true" ), which I think was the case when you originally raised the issue. But yeah, if you reference the generated properties and write your own formatting methods, then you will still trigger CA1863. That seems reasonable/desirable to me, since you're the one referencing it, you could handle it yourself. So, could you avoid the warnings by enabling EmitFormatMethods (unless you're blocked by #6579)? Or would you only consider the issue resolved if ResxSourceGenerator also made use of CompositeFormat?

JustArchi commented 3 months ago

I took a quick look and indeed, when using emitted formatting methods, the issue does not happen.

I think the above updates this issue to the following:

The bare minimum to achieve parity would be making Format() methods public when Public="true" is defined. Otherwise, we can't avoid the warning from other assemblies as we can't use the internal format methods that were generated.
Once the above is done, we should decide whether this is acceptable enough. I believe it is considering that we can tell the users (including myself) to switch to Format() methods, which would avoid the warning and still give us room for eventual future enhancement.
Which brings us to the last point - perhaps the generator itself can be improved to use CompositeFormat, since a lot of those formats will likely be called more than once.

As of now, I think we should still wait for #6579 to happen in regards to at least point 1, before we decide abount 2/3. If you asked me, stopping at point 2 is sufficient, but perhaps CompositeFormat as under-the-hood improvement could still be beneficial - up to the team.

dotnet / roslyn-analyzers