[build] enable CA1305 for most projects #7993

jonathanpeppers · 2023-04-27T13:54:14Z

Context: https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305
Context: https://learn.microsoft.com/dotnet/csharp/tutorials/string-interpolation#how-to-create-a-culture-specific-result-string-with-string-interpolation

In b1079a0, we found a real bug where omitting CultureInfo.InvariantCulture caused a problem under certain locales.

Let's take this a step further by making CA1305 a build error. In every case I found, it seemed that invariant culture was fine -- and some might be bugs?

This also catches cases like this that use the current culture:

$"My integer: {x}"

Where we should do this instead:

FormattableString.Invariant($"My integer: {x}")

Only a single instance for displaying a localized string/error message from MSBuild seemed like it needed the current culture:

string.Format (CultureInfo.CurrentCulture, Resources.XA2000, assembly));

And this one might not even matter, really?

Some projects that are not shipped I didn't enable yet, to keep this change smaller:

Projects in build-tools
MSBuild test projects

jonathanpeppers · 2023-04-27T14:38:28Z

/azp run

azure-pipelines · 2023-04-27T14:38:49Z

Azure Pipelines successfully started running 1 pipeline(s).

src/Mono.Android/Android.Util/Log.cs

Context: https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305 Context: https://learn.microsoft.com/dotnet/csharp/tutorials/string-interpolation#how-to-create-a-culture-specific-result-string-with-string-interpolation In b1079a0, we found a real bug where omitting `CultureInfo.InvariantCulture` caused a problem under certain locales. Let's take this a step further by making `CA1305` a build error. In every case I found, it seemed that invariant culture was fine -- and some might be bugs? This also catches cases like this that use the current culture: $"My integer: {x}" Where we should do this instead: FormattableString.Invariant($"My integer: {x}") Only a single instance for displaying a localized string/error message from MSBuild seemed like it needed the current culture: string.Format (CultureInfo.CurrentCulture, Resources.XA2000, assembly)); And this one might not even matter, really? Some projects that are not shipped I didn't enable yet, to keep this change smaller: * Projects in `build-tools` * MSBuild test projects

jpobst · 2023-04-29T01:48:41Z

It might be simpler to add a new /src/.editorconfig that changes only this rule. It will override the root .editorconfig for just this setting and will only apply to /src, so we don't need to add the <NoWarn> in all the other project files.

https://learn.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2022#file-hierarchy-and-precedence

Context: https://learn.microsoft.com/visualstudio/ide/create-portable-custom-editor-options#file-hierarchy-and-precedence

jonathanpeppers · 2023-05-01T16:39:39Z

It might be simpler to add a new /src/.editorconfig that changes only this rule. It will override the root .editorconfig for just this setting and will only apply to /src, so we don't need to add the <NoWarn> in all the other project files.

https://learn.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2022#file-hierarchy-and-precedence

I made this change, and the diff is a lot smaller now. Only projects in src get the rule now.

src/Mono.Android/Android.Runtime/AndroidEnvironment.cs

jonpryor · 2023-05-02T19:16:27Z

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

@@ -530,7 +531,7 @@ public void RegisterNativeMembers (JniType nativeClass, Type type, ReadOnlySpan<
 								}

 							if (minfo == null)
-								throw new InvalidOperationException (String.Format ("Specified managed method '{0}' was not found. Signature: {1}", mname.ToString (), signature.ToString ()));
+								throw new InvalidOperationException (FormattableString.Invariant ($"Specified managed method '{mname.ToString ()}' was not found. Signature: {signature.ToString ()}"));


I assume this should either use {mname}…{signature} or use {mname.ToString(CultureInfo.InvariantCulture)}…{signature.ToString(CultureInfo.InvariantCulture). What is here will call .ToString() with whatever is the current culture, which is most likely not the Invariant culture, and then pass the output of .ToString() on to FormattableString.Invariant().

For these two you have to call ToString() because they are both ReadonlySpan<char>.

For some reason you can't string interpolate these like $"Foo: {foo}"

For example, "playing around" in csharp:

CultureInfo.CurrentCulture = new CultureInfo("de-DE"); double d = 42.5; d.ToString(); // "42,5" FormattableString.Invariant($"{d}"); // "42.5" FormattableString.Invariant($"{d.ToString()}"); // "42,5"

d.ToString() is "immediate", and thus uses the current culture ("de-DE", German), which is not Invariant output.

With this change, we'd get a CA1305 build error if these were wrong. ReadonlySpan<char> doesn't have an overload for ToString():

https://learn.microsoft.com/en-us/dotnet/api/system.readonlyspan-1.tostring?view=net-8.0

In such cases we should use code like this

src/Mono.Android/Android.Runtime/AndroidRuntime.cs

src/Mono.Android/Android.Util/Log.cs

jonpryor · 2023-05-10T14:54:41Z

src/.editorconfig

@@ -0,0 +1,2 @@
+[*.{cs,vb}]
+dotnet_diagnostic.CA1305.severity = error		# Specify IFormatProvider


Why enable this just in src and not repo-wide?

Because of the suggestion here: #7993 (comment)

There were a lot of tests, build-tools, Mono.Options, etc. that would also need to be addressed or use $(NoWarn).

jonpryor · 2023-05-16T01:33:28Z

src/Mono.Android/Android.Util/Log.cs

+		/// <summary>
+		/// IFormatProvider passed to any underlying string.Format() calls. Defaults to System.Globalization.CultureInfo.CurrentCulture.
+		/// </summary>
+		public static IFormatProvider FormatProvider { get; set; } = CultureInfo.CurrentCulture;


Doh, one last change: this should be wrapped in a #if ANDROID_34 block, as we shouldn't be updating API-33!

Should probably just change visibility?

#if ANDROID_34 public #endif static IFormatProvider FormatProvider { get; set; } = CultureInfo.CurrentCulture;

jonpryor · 2023-05-16T01:34:46Z

Commit message:

Context: b1079a0e02117b626dd98f8f8608be470056ce3f
Context: https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305
Context: https://learn.microsoft.com/dotnet/csharp/tutorials/string-interpolation#how-to-create-a-culture-specific-result-string-with-string-interpolation

In b1079a0e, we found a real bug where omitting
`CultureInfo.InvariantCulture` caused problems under certain locales.

Let's take this a step further by making `CA1305` a build error.
In every case I found, it seemed that invariant culture was fine, and
some might be bugs?

This also catches cases like this that use the current culture:

	$"My integer: {x}"

Where we should do this instead:

	FormattableString.Invariant($"My integer: {x}")

Only a single instance for displaying a localized string/error
message from MSBuild seemed like it needed the current culture:

	string.Format (CultureInfo.CurrentCulture, Resources.XA2000, assembly);

And this one might not even matter, as `assembly` is a string.

For on-device app execution, add a new
`Android.Util.Log.FormatProvider` property, which is used by all the
log methods.  This mirrors [`TextWriter.FormatProvider`][1]:

	namespace Android.Util {
	    partial class Log {
	        public static IFormatProvider FormatProvider { get; set; } = CultureInfo.CurrentCulture;
	        public static int Debug (string tag, string format, params object[] args) =>
	            Debug (tag, string.Format (FormatProvider, format, args));
	    }
	}

Some projects that are not shipped I didn't enable yet, to keep this
change smaller:

  * Projects in `build-tools`

  * MSBuild test projects

  * Use `src/.editorconfig`; see also
    [`.editorconfig` File hierarchy and precedence][0]

Finally, `build-tools/automation/guardian/source.gdnsuppress` needed
to be updated because the existing `Log.Wtf()` methods were changed
to use the new `Log.FormatProvider` property.

[0]: https://learn.microsoft.com/visualstudio/ide/create-portable-custom-editor-options#file-hierarchy-and-precedence
[1]: https://learn.microsoft.com/en-us/dotnet/api/system.io.textwriter.formatprovider?view=net-8.0

* main: [build] enable CA1305 for most projects (dotnet#7993) Bump to xamarin/monodroid@5aed7edd (dotnet#8043)

jonathanpeppers commented Apr 27, 2023

View reviewed changes

src/Mono.Android/Android.Util/Log.cs Outdated Show resolved Hide resolved

jonathanpeppers marked this pull request as ready for review April 27, 2023 21:59

jonathanpeppers requested review from grendello, jonpryor and dellis1972 as code owners April 27, 2023 21:59

grendello approved these changes Apr 27, 2023

View reviewed changes

jonathanpeppers force-pushed the CA1305 branch from 851a023 to a675045 Compare April 28, 2023 19:40

Use src/.editorconfig

15fd519

Context: https://learn.microsoft.com/visualstudio/ide/create-portable-custom-editor-options#file-hierarchy-and-precedence

More CultureInfo.InvariantCulture

5ae588c

jonpryor reviewed May 2, 2023

View reviewed changes

src/Mono.Android/Android.Runtime/AndroidEnvironment.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 2, 2023

View reviewed changes

src/Mono.Android/Android.Runtime/AndroidRuntime.cs Outdated Show resolved Hide resolved

jonpryor reviewed May 2, 2023

View reviewed changes

src/Mono.Android/Android.Util/Log.cs Outdated Show resolved Hide resolved

jonathanpeppers added 3 commits May 2, 2023 16:21

PR Feedback

5cbf0f5

Add static property Log.FormatProvider

cd8d1aa

Update Polycheck exclusions

0dd338f

jonathanpeppers requested a review from pjcollins as a code owner May 9, 2023 22:09

pjcollins approved these changes May 9, 2023

View reviewed changes

jonpryor reviewed May 10, 2023

View reviewed changes

jonpryor reviewed May 16, 2023

View reviewed changes

Log.FormatProvider is only public in API-34+

4fcbed9

jonpryor merged commit 04e4bb1 into dotnet:main May 17, 2023

jonathanpeppers deleted the CA1305 branch May 17, 2023 15:55

grendello added a commit to grendello/xamarin-android that referenced this pull request May 18, 2023

Merge branch 'main' into blazor-hang

3673a3a

* main: [build] enable CA1305 for most projects (dotnet#7993) Bump to xamarin/monodroid@5aed7edd (dotnet#8043)

pjcollins mentioned this pull request Jun 5, 2023

Audit EnableNETAnalyzers usage and fix CA1305 instances in Xamarin.Android.Build.Tasks #8109

Open

github-actions bot locked and limited conversation to collaborators Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[build] enable CA1305 for most projects #7993

[build] enable CA1305 for most projects #7993

jonathanpeppers commented Apr 27, 2023 •

edited

Loading

jonathanpeppers commented Apr 27, 2023

azure-pipelines bot commented Apr 27, 2023

jpobst commented Apr 29, 2023

jonathanpeppers commented May 1, 2023

jonpryor May 2, 2023

jonathanpeppers May 2, 2023

jonpryor May 2, 2023

jonathanpeppers May 3, 2023

grendello May 4, 2023

jonpryor May 10, 2023

jonathanpeppers May 10, 2023 •

edited

Loading

jonpryor May 16, 2023

jonpryor May 16, 2023

jonpryor commented May 16, 2023

		@@ -0,0 +1,2 @@
		[*.{cs,vb}]
		dotnet_diagnostic.CA1305.severity = error # Specify IFormatProvider

[build] enable CA1305 for most projects #7993

[build] enable CA1305 for most projects #7993

Conversation

jonathanpeppers commented Apr 27, 2023 • edited Loading

jonathanpeppers commented Apr 27, 2023

azure-pipelines bot commented Apr 27, 2023

jpobst commented Apr 29, 2023

jonathanpeppers commented May 1, 2023

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

jonathanpeppers May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpryor commented May 16, 2023

jonathanpeppers commented Apr 27, 2023 •

edited

Loading

jonathanpeppers May 10, 2023 •

edited

Loading