-
Notifications
You must be signed in to change notification settings - Fork 64
[build] Enable string operations globalization code analyzers. #879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a92882d to
d24f074
Compare
|
|
||
| public static IEnumerable<JniTypeName> FromSignature (string signature) | ||
| { | ||
| if (signature.StartsWith ("(")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this instead use signature.StartsWith('('), which "mirrors" changes elsewhere which use char instead of string, StringComparison overloads? For example the signature.IndexOf(')') change on the following line?
Likewise many of the other changes that only deal with a single character…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally yes, but those overloads are introduced in NET Core 2.0/NET Standard 2.1 so they are not available to us.
|
Is this a guess that a culture-aware comparison is failing, or is this known? If so, which culture? |
|
Looks like this is a culture-related issue; behold! using System;
using System.Globalization;
class App {
public static void Main ()
{
var infos = CultureInfo.GetCultures(CultureTypes.AllCultures);
foreach (var ci in infos) {
if (!"Ljava/lang/String;".StartsWith ("L", ignoreCase:false, culture:ci)) {
Console.WriteLine(ci.Name);
}
}
}
}The above app tries The result? There are 19 cultures, across 3 languages, for which our |
|
Good sleuthing! This was a guess on my part, so I'm glad you confirmed that it is the culprit! |
|
For completeness, the issue is specifically |
|
Commit message for review: Context: https://github.com/xamarin/xamarin-android/issues/6271
Context: https://developercommunity.visualstudio.com/t/XamarinAndroid-binding-compiling-proble/1477963
Context: https://www.learncroatian.eu/blog/the-croatian-letters
If you attempt to build an Android Binding project on Windows within
a Bosnian, Croatian, or Serbian locale, the build may break as the
delegate type names created by 56955d9a may not be valid C#, e.g.:
delegate IntPtr _JniMarshal_PP_Ljava/Lang/String; (IntPtr jnienv, IntPtr klass)
It *should* be declaringn a `_JniMarshal_PP_L` type, *not*
`_JniMarshal_PP_Ljava/Lang/String;`, the latter of which results in
numerous C# errors:
error CS1003: Syntax error, '(' expected
error CS1001: Identifier expected
error CS1001: Identifier expected
error CS1003: Syntax error, ',' expected
error CS1003: Syntax error, ',' expected
error CS1001: Identifier expected
error CS1026: ) expected
The problem is caused by the interplay of two factors:
1. Commit 56955d9a uses the culture-sensitive
[`string.StartsWith(string)`][0] method to determine if a JNI
type name starts with `L`:
if (jni_name.StartsWith ("L") || jni_name.StartsWith ("[")) return "L";
2. In the `bs`, `hr`, and `sr` locales, the strings `Lj` and `lj`
are treated as a single letter, *distinct from* `L` or `l`.
In those locales, this expression is *false*, not true:
"Ljava/lang/String;".StartsWith ("L") // false in bs, hr, sr; true everywhere else
Additionally, this issue only arises when Java package names
starting with `java` are used, e.g. `Ljava/lang/Object;`. Java types
from packages that *don't* start with `java` don't encounter this bug.
Fix this issue by enabling the [CA1307][1] and [CA1309][2],
previously disabled in commit ac914ce36. These code analysis rules
require the use of string methods which use the
[`StringComparison`][3] enumeration, for which we then specify
`StringComparison.Ordinal`, which is *not* culture-sensitive.
One complication with enabling these rules is that the .NET 6+
version of these rules are stricter and require overloads that do not
exist in .NET Framework or .NET Standard to fix the violations.
Enabling these rules in `.editorconfig` affects all
`$(TargetFrameworkMoniker)`s; we will instead use
`Directory.Build.props` to only enable them for non-.NET 6+ builds.
[0]: https://docs.microsoft.com/en-us/dotnet/api/system.string.startswith?view=net-5.0#System_String_StartsWith_System_String_
[1]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1307
[2]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1309
[3]: https://docs.microsoft.com/en-us/dotnet/api/system.stringcomparison?view=net-5.0 |
7b38e3b to
b07d2a8
Compare
b07d2a8 to
d9ded1d
Compare
Context: dotnet/android#6271 Context: https://developercommunity.visualstudio.com/t/XamarinAndroid-binding-compiling-proble/1477963 Context: https://www.learncroatian.eu/blog/the-croatian-letters If you attempt to build an Android Binding project on Windows within a Bosnian, Croatian, or Serbian locale, the build may break as the delegate type names created by 56955d9 may not be valid C#, e.g.: delegate IntPtr _JniMarshal_PP_Ljava/Lang/String; (IntPtr jnienv, IntPtr klass) It *should* be declaringn a `_JniMarshal_PP_L` type, *not* `_JniMarshal_PP_Ljava/Lang/String;`, the latter of which results in numerous C# errors: error CS1003: Syntax error, '(' expected error CS1001: Identifier expected error CS1001: Identifier expected error CS1003: Syntax error, ',' expected error CS1003: Syntax error, ',' expected error CS1001: Identifier expected error CS1026: ) expected The problem is caused by the interplay of two factors: 1. Commit 56955d9 uses the culture-sensitive [`string.StartsWith(string)`][0] method to determine if a JNI type name starts with `L`: if (jni_name.StartsWith ("L") || jni_name.StartsWith ("[")) return "L"; 2. In the `bs`, `hr`, and `sr` locales, the strings `Lj` and `lj` are treated as a single letter, *distinct from* `L` or `l`. In those locales, this expression is *false*, not true: "Ljava/lang/String;".StartsWith ("L") // false in bs, hr, sr; true everywhere else Additionally, this issue only arises when Java package names starting with `java` are used, e.g. `Ljava/lang/Object;`. Java types from packages that *don't* start with `java` don't encounter this bug. Fix this issue by enabling the [CA1307][1] and [CA1309][2] rules, previously disabled in commit ac914ce. These code analysis rules require the use of string methods which use the [`StringComparison`][3] enumeration, for which we then specify `StringComparison.Ordinal`, which is *not* culture-sensitive. One complication with enabling these rules is that the .NET 6+ version of these rules are stricter and require overloads that do not exist in .NET Framework or .NET Standard to fix the violations. Enabling these rules in `.editorconfig` affects all `$(TargetFrameworkMoniker)`s; we will instead use `Directory.Build.props` to only enable them for non-.NET 6+ builds. Finally, add a new `.yaml` step that shuts down the cached `dotnet` MSBuild processes between invocations, to fix the error that has been happening on Windows - NET Core: error MSB3027: Could not copy "obj\\Release\Java.Interop.BootstrapTasks.dll" to "D:\a\1\s\bin\BuildRelease\Java.Interop.BootstrapTasks.dll". Exceeded retry count of 10. Failed. [D:\a\1\s\build-tools\Java.Interop.BootstrapTasks\Java.Interop.BootstrapTasks.csproj] error MSB3021: Unable to copy file "obj\\Release\Java.Interop.BootstrapTasks.dll" to "D:\a\1\s\bin\BuildRelease\Java.Interop.BootstrapTasks.dll". The process cannot access the file 'D:\a\1\s\bin\BuildRelease\Java.Interop.BootstrapTasks.dll' because it is being used by another process. [D:\a\1\s\build-tools\Java.Interop.BootstrapTasks\Java.Interop.BootstrapTasks.csproj] [0]: https://docs.microsoft.com/en-us/dotnet/api/system.string.startswith?view=net-5.0#System_String_StartsWith_System_String_ [1]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1307 [2]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1309 [3]: https://docs.microsoft.com/en-us/dotnet/api/system.stringcomparison?view=net-5.0
Context: dotnet/android#6271
Context: https://developercommunity.visualstudio.com/t/XamarinAndroid-binding-compiling-proble/1477963
We are seeing some bizarre results from seemingly simple code where we generate JNI delegate names that should not be possible, like:
which should be:
The code in question seems like it should notice that
Ljava/Lang/String;starts withLand returnLinstead of the full string:Perhaps the user's current culture causes this to fail somehow? Regardless, we should follow best practices and follow .NET's string globalization suggestions. As such, this enables the relevant code analysis rules (CA1307;CA1309;CA1310) and fixes violations of them.
ex:
The catch is that the
net6.0versions of these rules are stricter and require overloads that do not exist in .NET Framework to fix the violations. Turning them on in.editorconfigaffects all TFMs, so instead we are going to useDirectory.Build.propsto only enable them for!net6.0builds.Additionally, add a new
.yamlstep that shuts down the cacheddotnetMSBuild processes between invocations, to fix the error that has been happening on Windows - NET Core: