Skip to content

Commit ad5f6e8

Browse files
jpobstjpobst
authored
[build] Enable globalization analyzer errors and fix issues (#6368)
Context: #6271 Context: dotnet/java-interop@7068f4b Making culture-aware string comparisons when not intended can result in very hard to diagnose bugs (See/#6271). As we did in dotnet/java-interop@7068f4b2, update the appropriate Roslyn code analyzers so that errors are generated around "unsafe" string usage, and to ensure that we don't introduce any new issues around string & globalization support in the future. 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 `Configuration.props` to only enable them for non-.NET 6+ builds. Additionally, tweak and reuses the existing Roslyn Analyzers logic to exclude external code, as many libraries that are used for testing (like `Mono.Debugg[ing|er].Soft)` have issues surfaced by these analyzers. Note that some non-test comparisons on filenames/paths were switched from case-sensitive to case-insensitive compares.
1 parent 2f252d3 commit ad5f6e8

File tree

59 files changed

+120
-105
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

59 files changed

+120
-105
lines changed

‎Configuration.props‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,20 @@
226226

227227
<!-- Fix for IDEs -->
228228
<Target Name="CreateManifestResourceNames" />
229+
230+
<!-- Don't analyze code from external repos -->
231+
<PropertyGroup Condition=" !$(MSBuildProjectDirectory.Contains ('external')) and !$(MSBuildProjectDirectory.Contains ('NUnitLite')) ">
232+
<XAShouldAnalyzeAssembly>True</XAShouldAnalyzeAssembly>
233+
</PropertyGroup>
234+
235+
<!-- The net6.0 versions of these analyzers are stricter and require overloads not available in .NET Framework, so start with just .NET Framework -->
236+
<PropertyGroup Condition=" '$(TargetFramework)' != '' And (!$(TargetFramework.StartsWith('nets')) And !$(TargetFramework.StartsWith('net4')) And !$(TargetFramework.StartsWith('monoandroid'))) ">
237+
<XABuildingForNetCoreApp>True</XABuildingForNetCoreApp>
238+
</PropertyGroup>
239+
240+
<PropertyGroup Condition=" '$(XABuildingForNetCoreApp)' != 'True' And '$(XAShouldAnalyzeAssembly)' == 'True' ">
241+
<WarningsAsErrors>$(WarningsAsErrors);CA1309;CA1310</WarningsAsErrors>
242+
</PropertyGroup>
229243

230244
<Import Project="$(MSBuildThisFileDirectory)build-tools\scripts\Ndk.targets" />
231245
<Import Project="$(MSBuildThisFileDirectory)build-tools\scripts\RoslynAnalyzers.projitems" Condition=" '$(EnableRoslynAnalyzers)' == 'true' " />

‎build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public override bool Execute ()
7878
}
7979

8080
TargetImplementationPath = implementationPath.FullName;
81-
if (TargetImplementationPath.EndsWith ("\\") || TargetImplementationPath.EndsWith ("/")) {
81+
if (TargetImplementationPath.EndsWith ("\\", StringComparison.Ordinal) || TargetImplementationPath.EndsWith ("/", StringComparison.Ordinal)) {
8282
TargetImplementationPath = TargetImplementationPath.Substring (0, TargetImplementationPath.Length - 1);
8383
}
8484

@@ -189,7 +189,7 @@ void dataReceived (object sender, DataReceivedEventArgs args)
189189
if (!string.IsNullOrWhiteSpace (args.Data)) {
190190
lines.Add (args.Data.Trim ());
191191

192-
if (args.Data.IndexOf ("Native Crash Reporting") != -1) {
192+
if (args.Data.IndexOf ("Native Crash Reporting", StringComparison.Ordinal) != -1) {
193193
processHasCrashed = true;
194194
}
195195
}
@@ -245,13 +245,13 @@ void dataReceived (object sender, DataReceivedEventArgs args)
245245
Log.LogMessage (MessageImportance.High, $"{Environment.NewLine}*** CodeGen missing items***{Environment.NewLine}");
246246
var indent = 0;
247247
foreach (var item in missingItems) {
248-
if (item.StartsWith ("}")) {
248+
if (item.StartsWith ("}", StringComparison.Ordinal)) {
249249
indent--;
250250
}
251251

252-
Log.LogMessage (MessageImportance.High, $"{(item.StartsWith ("namespace ") ? Environment.NewLine : string.Empty)}{new string (' ', indent * 2)}{item}");
252+
Log.LogMessage (MessageImportance.High, $"{(item.StartsWith ("namespace ", StringComparison.Ordinal) ? Environment.NewLine : string.Empty)}{new string (' ', indent * 2)}{item}");
253253

254-
if (item.StartsWith ("{")) {
254+
if (item.StartsWith ("{", StringComparison.Ordinal)) {
255255
indent++;
256256
}
257257
}

‎build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CodeGenDiff.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ void dataReceived (object sender, DataReceivedEventArgs args)
114114
return;
115115
}
116116

117-
if (content.StartsWith ("namespace ") || content.IndexOf (" interface ") != -1 || content.IndexOf (" class ") != -1 || content.IndexOf (" partial struct ") != -1 || content.IndexOf (" enum ") != -1) {
117+
if (content.StartsWith ("namespace ", StringComparison.Ordinal) || content.IndexOf (" interface ", StringComparison.Ordinal) != -1 || content.IndexOf (" class ", StringComparison.Ordinal) != -1 || content.IndexOf (" partial struct ", StringComparison.Ordinal) != -1 || content.IndexOf (" enum ", StringComparison.Ordinal) != -1) {
118118
if (string.IsNullOrWhiteSpace (currentObject.Item)) {
119119
currentObject.Item = content;
120120
} else {

‎build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/GenerateWixFile.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System;
12
using System.IO;
23
using System.Linq;
34
using System.Security.Cryptography;
@@ -121,13 +122,13 @@ static void RecurseDirectory (string top_dir, XmlWriter packWriter, XmlWriter co
121122
packWriter.WriteAttributeString ("FileSource", directory);
122123
foreach (var child in Directory.EnumerateDirectories (directory)) {
123124
var directoryName = Path.GetFileName (child);
124-
if (directoryName.StartsWith (".") || directoryName.StartsWith ("_"))
125+
if (directoryName.StartsWith (".", StringComparison.Ordinal) || directoryName.StartsWith ("_", StringComparison.Ordinal))
125126
continue;
126127
RecurseDirectory (top_dir, packWriter, componentWriter, child);
127128
}
128129
foreach (var file in Directory.EnumerateFiles (directory)) {
129130
var fileName = Path.GetFileName (file);
130-
if (fileName.StartsWith (".") || fileName.StartsWith ("_"))
131+
if (fileName.StartsWith (".", StringComparison.Ordinal) || fileName.StartsWith ("_", StringComparison.Ordinal))
131132
continue;
132133
AddFile (top_dir, packWriter, componentWriter, file);
133134
}

‎build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RenameTestCases.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ static void CreateErrorResultsFile (string sourceFile, string destFile, string c
166166
static void GetTestCaseInfo (string sourceFile, string destinationFolder, string config, string flavor, Action<string> logDebugMessage, out string testSuiteName, out string testCaseName, out string logcatPath)
167167
{
168168
var name = Path.GetFileNameWithoutExtension (sourceFile);
169-
if (name.StartsWith ("TestResult-"))
169+
if (name.StartsWith ("TestResult-", StringComparison.Ordinal))
170170
name = name.Substring ("TestResult-".Length);
171171
testSuiteName = name;
172172
testCaseName = $"Possible Crash / {config}";

‎build-tools/api-merge/ApiDescription.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ static string GetParameterType (string type, Dictionary<string, string> mappings
397397
return "*";
398398

399399
// varargs should be normalized.
400-
if (type.EndsWith ("..."))
400+
if (type.EndsWith ("...", StringComparison.Ordinal))
401401
return GetParameterType (type.Substring (0, type.Length - 3) + "[]", mappings);
402402

403403
int first = type.IndexOfAny (type_sep);

‎build-tools/api-merge/Mono.Options-PCL.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1353,11 +1353,11 @@ private static string GetArgumentName (int index, int maxIndex, string descripti
13531353
for (int i = 0; i < nameStart.Length; ++i) {
13541354
int start, j = 0;
13551355
do {
1356-
start = description.IndexOf (nameStart [i], j);
1356+
start = description.IndexOf (nameStart [i], j, StringComparison.Ordinal);
13571357
} while (start >= 0 && j != 0 ? description [j++ - 1] == '{' : false);
13581358
if (start == -1)
13591359
continue;
1360-
int end = description.IndexOf ("}", start);
1360+
int end = description.IndexOf ("}", start, StringComparison.Ordinal);
13611361
if (end == -1)
13621362
continue;
13631363
return description.Substring (start + nameStart [i].Length, end - start - nameStart [i].Length);

‎build-tools/check-boot-times/Program.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ bool validation (string data, ManualResetEvent mre)
177177

178178
var activity = i % 2 == 0 ? "com.google.android.apps.photos/.home.HomeActivity" : "com.android.settings/.wifi.WifiStatusTest";
179179
await RunProcess (adbPath, $"-e shell am start -n '{activity}'", 5000, (data, mre) => {
180-
if (!string.IsNullOrWhiteSpace (data) && data.IndexOf ("com.android") != -1) {
180+
if (!string.IsNullOrWhiteSpace (data) && data.IndexOf ("com.android", StringComparison.Ordinal) != -1) {
181181
mre.Set ();
182182
}
183183
return true;
@@ -375,7 +375,7 @@ static async Task InstallSdk ()
375375
{
376376
bool validation (string data, ManualResetEvent mre)
377377
{
378-
if (!string.IsNullOrWhiteSpace (data) && data.IndexOf ("100%") != -1) {
378+
if (!string.IsNullOrWhiteSpace (data) && data.IndexOf ("100%", StringComparison.Ordinal) != -1) {
379379
mre.Set ();
380380
}
381381

‎build-tools/jnienv-gen/Generator.cs‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ static void GenerateJniEnv (TextWriter o)
209209

210210
switch (entry.ApiName) {
211211
case "NewArray":
212-
var copyArray = JNIEnvEntries.Single (e => e.Name.StartsWith ("Get") && e.Name.EndsWith ("ArrayRegion") &&
212+
var copyArray = JNIEnvEntries.Single (e => e.Name.StartsWith ("Get", StringComparison.Ordinal) && e.Name.EndsWith ("ArrayRegion", StringComparison.Ordinal) &&
213213
e.Parameters [0].Type.Type == entry.ReturnType.Type);
214214
o.Write ("\t\t{2} static {0} {1} (", entry.ReturnType.ManagedType, entry.ApiName, entry.Visibility);
215215
o.WriteLine ("{0} array)", copyArray.Parameters [3].Type.ManagedType);
@@ -233,7 +233,7 @@ static void GenerateJniEnv (TextWriter o)
233233
break;
234234
case "CopyArray":
235235
o.Write ("\t\t{2} static unsafe {0} {1} (", entry.ReturnType.ManagedType, entry.ApiName, entry.Visibility);
236-
if (entry.Name.StartsWith ("G")) {
236+
if (entry.Name.StartsWith ("G", StringComparison.Ordinal)) {
237237
o.WriteLine ("IntPtr src, {0} dest)", entry.Parameters [3].Type.ManagedType);
238238
o.WriteLine ("\t\t{");
239239
o.WriteLine ("\t\t\tif (src == IntPtr.Zero)");
@@ -319,15 +319,15 @@ entry.Parameters [entry.Parameters.Length-1].IsParamArray &&
319319
o.Write ("Env.{0} (Handle", entry.Name);
320320
for (int i = 0; i < entry.Parameters.Length; i++) {
321321
o.Write (", ");
322-
if (entry.Parameters [i].Type.ManagedType.StartsWith ("out "))
322+
if (entry.Parameters [i].Type.ManagedType.StartsWith ("out ", StringComparison.Ordinal))
323323
o.Write ("out ");
324324
o.Write (Escape (entry.Parameters [i].Name));
325325
}
326326
o.WriteLine (");");
327327
RaiseException (o, entry);
328328
if (is_void) {
329329
}
330-
else if (entry.ReturnType.ManagedType == "IntPtr" && entry.ReturnType.Type.StartsWith ("j") && !entry.ReturnType.Type.EndsWith ("ID")) {
330+
else if (entry.ReturnType.ManagedType == "IntPtr" && entry.ReturnType.Type.StartsWith ("j", StringComparison.Ordinal) && !entry.ReturnType.Type.EndsWith ("ID", StringComparison.Ordinal)) {
331331
o.WriteLine ("\t\t\treturn LogCreateLocalRef (tmp);");
332332
} else {
333333
o.WriteLine ("\t\t\treturn tmp;");
@@ -381,13 +381,13 @@ static bool GenerateDefaultJavaInteropForwarder (TextWriter o, JniFunction entry
381381
if (!is_void)
382382
o.Write ("return ");
383383
var n = entry.Name;
384-
if (n.StartsWith ("Call"))
384+
if (n.StartsWith ("Call", StringComparison.Ordinal))
385385
n = n.TrimEnd (new[]{'A'});
386386
o.Write ("JniEnvironment.{0}.{1} (", entry.DeclaringType, n);
387387
for (int i = 0; i < entry.Parameters.Length; i++) {
388388
if (i > 0)
389389
o.Write (", ");
390-
if (entry.Parameters [i].Type.ManagedType.StartsWith ("out "))
390+
if (entry.Parameters [i].Type.ManagedType.StartsWith ("out ", StringComparison.Ordinal))
391391
o.Write ("out ");
392392
if (entry.Parameters [i].Type.ManagedType == "JValue*")
393393
o.Write ("(JniArgumentValue*) " + Escape (entry.Parameters [i].Name));
@@ -445,7 +445,7 @@ static bool IsObjectReferenceType (TypeInfo type)
445445
case "jweak":
446446
return true;
447447
}
448-
if (type.Type.StartsWith ("j") && type.Type.EndsWith("Array"))
448+
if (type.Type.StartsWith ("j", StringComparison.Ordinal) && type.Type.EndsWith("Array", StringComparison.Ordinal))
449449
return true;
450450
return false;
451451
}
@@ -643,9 +643,9 @@ public string GetManagedType (string native_type)
643643
case "jobjectRefType":
644644
return "int";
645645
default:
646-
if (native_type.EndsWith ("Array"))
646+
if (native_type.EndsWith ("Array", StringComparison.Ordinal))
647647
return "IntPtr";
648-
if (native_type.EndsWith ("*"))
648+
if (native_type.EndsWith ("*", StringComparison.Ordinal))
649649
return "IntPtr";
650650
return native_type;
651651
}

‎build-tools/jnienv-gen/Mono.Options-PCL.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1353,11 +1353,11 @@ private static string GetArgumentName (int index, int maxIndex, string descripti
13531353
for (int i = 0; i < nameStart.Length; ++i) {
13541354
int start, j = 0;
13551355
do {
1356-
start = description.IndexOf (nameStart [i], j);
1356+
start = description.IndexOf (nameStart [i], j, StringComparison.Ordinal);
13571357
} while (start >= 0 && j != 0 ? description [j++ - 1] == '{' : false);
13581358
if (start == -1)
13591359
continue;
1360-
int end = description.IndexOf ("}", start);
1360+
int end = description.IndexOf ("}", start, StringComparison.Ordinal);
13611361
if (end == -1)
13621362
continue;
13631363
return description.Substring (start + nameStart [i].Length, end - start - nameStart [i].Length);

0 commit comments

Comments
 (0)