NightOwl888 commented on issue #1211: URL: https://github.com/apache/lucenenet/issues/1211#issuecomment-3415900090
Thanks for submitting this issue. However, it is not clear what is meant by "string overloads" or "span overloads" in the description. The rule to use (LuceneDev6001 vs LuceneDev6002) has nothing to do with the type of parameter being passed to the method. The rule depends on the **type that the method is called upon**. - On `System.String`, the overloads without a `StringComparison` argument default to `StringComparison.CurrentCulture`. - On `System.Span<char>` the overloads without a `StringComparison` argument default to `StringComparison.Ordinal`. - On `System.ReadOnlySpan<char>` the overloads without a `StringComparison` argument default to `StringComparison.Ordinal`. ## "String Overloads" Therefore, the `LuceneDev6001` rule should only apply to all overloads of: - `System.String.IndexOf()` - `System.String.LastIndexOf()` - `System.String.StartsWith()` - `System.String.EndsWith()` - `J2N.StringBuilderExtensions.IndexOf()` - `J2N.StringBuilderExtensions.LastIndexOf()` See the upstream code at: https://github.com/dotnet/runtime/blob/v9.0.10/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs#L228 to ensure we cover every case (including those that accept a `startIndex` parameter). In all of these cases the `StringComparison` overload must be called and other cases should emit a compiler error with a code fix suggesting to call it with `StringComparison.Ordinal`. ## "Span Overloads" All of the other usage (including the `J2N.Text.OpenStringBuilder` and `Lucene.Net.Text.ValueStringBuilder` types) will be calling the Span/ReadOnlySpan extension methods under the hood, so they must use rule `LuceneDev6002`, instead. ## Additional Rules Further, we are skipping some of the nuance by limiting to only 3 rules. ### Suggest calling without a `StringComparison` argument on spans to improve performance On the `Span<char>` and `ReadOnlySpan<char>`, `J2N.Text.OpenStringBuilder`, and `Lucene.Net.Text.ValueStringBuilder` the overload that accepts a `StringComparison.Ordinal` value simply cascades the call to the overload that doesn't accept `StringComparison`. So, if the `StringComparson` value is `Ordinal`, we should recommend to remove the `StringComparison` argument. Do note that this rule should not apply if the `StringComparison` value is not `Ordinal` or if calling the method on a `System.String`. ``` ReadOnlySpan<char>.IndexOf("foo", StringComparison.Ordinal) -> ReadOnlySpan<char>.IndexOf("foo") // Warning/code fix ReadOnlySpan<char>.IndexOf("foo", StringComparison.OrdinalIgnoreCase) // Ignore ReadOnlySpan<char>.IndexOf("foo", StringComparison.CurrentCulture) // Error - we should only ever use Ordinal and OrdinalIgonreCase String.IndexOf("foo", StringComparison.Ordinal) // Ignore String.IndexOf("foo", StringComparison.OrdinalIgnoreCase) // Ignore String.IndexOf("foo") -> String.IndexOf("foo", StringComparson.Ordinal) // Error/code fix String.IndexOf("foo", StringComparison.InvariantCultureIgnoreCase) // Error - we should only ever use Ordinal and OrdinalIgonreCase ``` ### Suggest calling char overload on single character strings that are escaped There are already Microsoft diagnostic ids to catch when to optimize `StartsWith()`, `EndsWith()`, `IndexOf()` and `LastIndexOf()` to use the `char` overload. https://github.com/dotnet/roslyn-analyzers/pull/6799/files I think SonarCloud may have similar rules, as well. The problem is that these rules seem to exclude cases when there are escape characters within the string. For example, if the string is `"\""` or `"\x0020"`, there is technically only 1 character in it, but the analyzer doesn't suggest to use the `char` overload, so we need a custom analyzer and code fix to cover this case. It should work for any string escape sequence that resolves to a single `char`. ``` ReadOnlySpan<char>.IndexOf("\"", StringComparison.Ordinal) -> ReadOnlySpan<char>.IndexOf('\"') // Warning/code fix String.IndexOf("\"", StringComparison.Ordinal) -> String.IndexOf('\"') // Warning/code fix ``` Furthermore, I think we should use a warning in these cases. So, we should also elevate these to emit a warning in our `.editorconfig` file (in this repo). CA1865 | Performance | Info | UseStringMethodCharOverloadWithSingleCharacters, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1865) CA1866 | Performance | Info | UseStringMethodCharOverloadWithSingleCharacters, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1866) CA1867 | Performance | Disabled | UseStringMethodCharOverloadWithSingleCharacters, [Documentation](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1867) > NOTE: Use a plain text editor when working with `.editorconfig`, not the designer in Visual Studio. The latter may rewrite the whole `.editorconfig` file format. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
