Roslyn-analyzers: CA1307 Incorrectly reports string.Replace(string,string) on platforms without string.Replace(string, string, StringComparison)

[conniey]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers, version 2.6.0

Analyzer

CA1307: Specify StringComparison

Repro steps

1. Create test.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp1.0</TargetFramework>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Net.Compilers" Version="2.6.1">
      <PrivateAssets>All</PrivateAssets>
    </PackageReference>

    <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.6.0">
      <PrivateAssets>All</PrivateAssets>
    </PackageReference>
  </ItemGroup>
</Project>

1. Create Program.cs

using System;

namespace test
{
    class Program
    {
        static void Main()
        {
            var contents = "something".Replace("s", "o");
            Console.WriteLine(contents);
        }
    }
}

1. Execute dotnet build

Expected behavior

It builds. There is no overload for string.Replace() that takes System.StringComparison.

Actual behavior

Build fails and reports:

D:\git\test> dotnet build
Microsoft (R) Build Engine version 15.6.54.9755 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 144.47 ms for D:\git\test\test.csproj.
Program.cs(9,28): error CA1307: The behavior of 'string.Replace(string, string)' could vary based on the current user's locale settings. Replace this call in 'test.Program.Main()' with a call to 'string.Replace(string, string, System.StringComparison)'. [D:\git\test\test.csproj]

Build FAILED.

Program.cs(9,28): error CA1307: The behavior of 'string.Replace(string, string)' could vary based on the current user's locale settings. Replace this call in 'test.Program.Main()' with a call to 'string.Replace(string, string, System.StringComparison)'. [D:\git\test\test.csproj]
    0 Warning(s)
    1 Error(s)

[333fred]

@conniey, this warning is not incorrect. The Replace method it's telling you to use is here.

[conniey]

@333fred Hey,

I'm a little confused because Intellisense does not have this and the MSDN Documentation doesn't mention it.

The first one is string.Replace(char, char). Is there a package I am missing?

[conniey]

Ahh, I see. My application is targeting netcoreapp1.0 which doesn't have this overload. Thanks for the clarification!

[conniey]

@333fred Is the documentation incorrect?

The Applies To section implies that those overloads should work for .NET Core 1.0, .NET 4.5+. However, I tried to target .NET 4.7 with my project from .NET 4.6.1 but the overload does not show up. Nor, does it show up in .NET Core 1.0.

[333fred]

The applies to section applies to the string.Replace method as a whole, all overloads of it, and there is a version of Replace for 1.0 (the char/string only overloads). That being said, we shouldn't be flagging code for using this API when it doesn't exist. I'll reopen the issue and update the title.

[mladedav]

Could someone clarify the warning, since the documentation for Replace(String, String) states that: "This method performs an ordinal (case-sensitive and culture-insensitive) search to find oldValue."

So, in what situation would the output be dependant on culture info?

[Evangelink]

@mavasani, @sharwell I might be missing something but it seems that the rule CA1307 only checks for string.Compare and string.CompareTo overloads. Besides, I made a test following what is described and making sure the various culture rules are turned on and none of them is reporting for string.Replace. I think this issue can be closed.

[Evangelink]

I made a test with the latest release (i.e. 2.9.7).

[mavasani]

I believe the original report was coming from https://github.com/dotnet/roslyn-analyzers/blob/7ad9f725f22c7f5a9e8f70a5ebe6e5fd4c14d8d6/src/Microsoft.NetCore.Analyzers/Core/Runtime/SpecifyStringComparison.cs#L117-L133

[Evangelink]

@mavasani I would gladly provide a new test for the .net core 1.0 case but I cannot really figure out how to force the test to be compiled as a netcore1.0 app, could you help? FYI I have tried to look at both CSharpCompilationOptions and CSharpParseOptions classes but couldn't find something useful.

Also, as said in my previous comment, I cannot repro the issue with the analyzer 2.9.7.

[msmolka]

This error exists in multiple target environment even on latest .net core and .net framework,
if you have:

  <TargetFrameworks>net461;netstandard2.0;netcoreapp3.0</TargetFrameworks>

then
c# string.Replace("Basic ", string.Empty);
will report as CA1307

[Evangelink]

@msmolka thanks for the indication, I do understand what's the issue now.

The problem raised is that the various target frameworks don't all have the same available APIs. Basically if you have <TargetFrameworks>netstandard2.0;netcoreapp3.0</TargetFrameworks> you will have an issue raised correctly only for netcoreapp3.0, not for netstandard2.0 but you think that no error shall be raised as one of the 2 target framework doesn't have the extra API. I, personally, tend to think the issue is valid and that you could have 2 different string calls embedded in pragmas or you could ignore the issue for the current case. I don't really think that the issue shall be muted (or that the analyzer shall be changed) because it is valid as it is. When using netcoreapp2.0+ or netstandard2.1+ you do have the possibility to specify the culture or comparison and so it shall be done.

I think some of the maintainers shall be taking the decision here.
cc @mavasani

[msmolka]

The problem is error is not saying what the issue is. If somebody is not familiar with some api differences it is confusing. I had to google to find out why my code is wrong. In multi target environment there is no intellisense saying that other option is available. even now i don't see additional options for Replace. In pragma outside current condition it is also no intellisense. In that case error should specify for which target framework the issue is. Not global.

[Evangelink]

I guess the message could be improved to say that this is a new available API. Regarding the fact that the issue is global, I tend to disagree because after the project in the error list, you can see the target framework leading to the error.

For example with the target frameworks you were mentioning I have:

And if you update netstandard2.0 into netstandard2.1 now you have:

which clearly shows the target framework.

Regarding the Intellisense when using pragmas that might be a bug on your machine cause I do have the other overloads:

[msmolka]

In your setting the second one (else) is highlighted. In mine net461 is main, so it is highlighted and has intellisense. But you are right it is visible in errors.

[Evangelink]

@msmolka You can change the currently activated target framework on the top of the file, that helps with the intellisense:

[msmolka]

So many years... still don't know how to use VS. :thinking:

[Evangelink]

I think that this issue can be closed @mavasani

[tillig]

I'm running into this right now building in VS Code on Mac. I'm building a library that targets netstandard2.0 only.

From what I gather, while the three-parameter-overload of string.Replace is available in netcoreapp2.0, it is _not_ available in netstandard2.0.

https://github.com/dotnet/standard/issues/539#issuecomment-336922821

I'm not sure if the Roslyn analyzers are potentially running in the netcoreappX.Y corresponding to the netstandardX.Y runtime or something, so the analyzers may think the overload is available, but if I try adding it... I get the compiler error that there's no such overload.

Given that, I don't know that this issue is really resolved. There's no workaround and the only way to stop the warning is to suppress the analyzer or disable that rule.