CA1812 false-positive when type is used as a generic type

[meziantou]

Describe the bug

CA1812 false-positive

Steps To Reproduce

    internal static class Repro
    {
        public static void Generate()
        {
            string str = "";
            var emojis = System.Text.Json.JsonSerializer.Deserialize<Dictionary<string, Emoji>>(str);
        }

        private sealed class Emoji // CA1812 whereas the type is used as a generic type
        {
            public string Name { get; set; }
            public string Moji { get; set; }
            public string Shortname { get; set; }
        }
    }

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="3.3.0">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

Expected behavior

No diagnostic

Actual behavior

It reports a diagnostic that is not appliable in the context

Analyzer

Package: Microsoft.CodeAnalysis.FxCopAnalyzers

Version: v3.3.0 (Latest)

Diagnostic ID: CA1812

[mavasani]

This is same concern mentioned at https://github.com/dotnet/roslyn/issues/44330#issuecomment-630395417. As mentioned in https://github.com/dotnet/roslyn/issues/44330#issuecomment-631487553 the analyzer should not handle this but user should use a source suppression to document this.

[mavasani]

Oh actually I think @Evangelink might have already fixed this?

[Evangelink]

@mavasani I can reproduce the issue but I am not entirely sure how to fix it. The first thing that comes to my mind is to say that if a type is used as a generic type then we don't report on it but that's killing too many valid cases. The other option would be to have a list of methods (maybe configurable) that are considered as creating instance of types.

[mavasani]

3 options here:

1. By design: User should add source suppression for each type used as generic argument to such special methods
2. Conservative: Do not report for any type used as a generic type argument anywhere. This seems to have too many false negatives, so lets not do this.
3. Configuration: Add an editorconfig option to specify special methods or types whose generic type arguments are instantiated, and should be excluded from analysis.

I think 1. or 3. would be fine.

[mavasani]

@genlu suggested a potential 4th option: Use a special attribute that can be applied to type/method to mark this semantics.

[meziantou]

Would it make sense to use the DynamicallyAccessedMembers attribute? Most methods from the framework are already annotated. For instance, Deserialize is annotated with PublicProperties | PublicFields | PublicConstructors (source). This means the method looks for the constructors of T. This doesn't guarantee to call it, but this is a good indication it will try to create an instance of the object.

That's being said, I'm not sure this will help for the reported code as the T parameter is actually a Dictionary<string, CustomType>, so the attribute doesn't explain what to do with CustomType. But it will help for Deserialize<CustomType>(...).

[geeknoid]

@mavasani I can reproduce the issue but I am not entirely sure how to fix it. The first thing that comes to my mind is to say that if a type is used as a generic type then we don't report on it but that's killing too many valid cases. The other option would be to have a list of methods (maybe configurable) that are considered as creating instance of types.

Hi there. I'm curious why not systematically suppressing any cases where the given type is used as a generic is not the right answer? The rule is really trying to say "hey, you've got a type here that nobody is using". When it's used a generic, the type is in fact being used, so no diagnostic should be produced.

[Youssef1313]

The other option would be to have a list of methods (maybe configurable) that are considered as creating instance of types.

If we go with this approach, we should consider AddHostedService as one of the methods in the list. See https://github.com/dotnet/roslyn-analyzers/issues/6086

Alternatively, https://github.com/dotnet/roslyn-analyzers/issues/3545 suggests that libraries that have methods which instantiate generic types should write their own DiagnosticSuppressor

[znakeeye]

What if you put a new() constraint on JsonSerializer.Deserialize<T>()? Or would that break?

Otherwise, a workaround is to have a helper method with the constraint:

public static class MySerializer
{
    public static TValue? Deserialize<TValue>(string json, JsonSerializerOptions? options = null)
        where TValue : new()
    {
        return JsonSerializer.Deserialize<TValue>(json, options);
    }
}

Obviously, this will only work for types with public parameterless constructors.

[mavasani]

I am going to move this issue to dotnet/runtime for them to help us with the best option. Let me list down all the options we have identified in this issue:

Issue: CA1812 false positives from non-explicit type instantiations

Potential solutions:

1. By design: User should add source suppression for each type used as generic argument to such special methods
2. Conservative: Do not report CA1812 for any type used as a generic type argument anywhere. This can have many false negatives.
3. Options based Configuration: Add an editorconfig option to specify special methods or types whose generic type arguments are instantiated.
4. Attribute based Annotation:
   1. Special attribute that can be added to generic methods to indicate it may instantiate types of its type arguments
   2. Use DynamicallyAccessedMembers attribute as explained in https://github.com/dotnet/roslyn-analyzers/issues/6561
5. DiagnosticSuppressors based suppression: https://github.com/dotnet/roslyn-analyzers/issues/3545 suggests that libraries that have methods which instantiate generic types should write their own DiagnosticSuppressor to suppress CA1812 warnings. This may be tricky as the suppressor would have to re-analyze the whole project to look for calls to such special methods in the compilation. Also expecting each library to write such a suppressor seems unscalable.

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[mavasani]

@stephentoub @bartonjs @buyaa-n @carlossanlop to help triage the options available in https://github.com/dotnet/roslyn-analyzers/issues/6561

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Describe the bug CA1812 false-positive ### Steps To Reproduce ````c# internal static class Repro { public static void Generate() { string str = ""; var emojis = System.Text.Json.JsonSerializer.Deserialize>(str); } private sealed class Emoji // CA1812 whereas the type is used as a generic type { public string Name { get; set; } public string Moji { get; set; } public string Shortname { get; set; } } } ```` ````xml all runtime; build; native; contentfiles; analyzers; buildtransitive ```` ### Expected behavior No diagnostic ### Actual behavior It reports a diagnostic that is not appliable in the context ### Analyzer **Package**: [Microsoft.CodeAnalysis.FxCopAnalyzers](https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers) **Version**: v3.3.0 (Latest) **Diagnostic ID**: [CA1812](https://docs.microsoft.com/visualstudio/code-quality/ca1812)

Author:	meziantou
Assignees:	-
Labels:	`area-System.Runtime`, `untriaged`, `code-analyzer`
Milestone:	-

[abatishchev]

Here's my case:

internal sealed class ServiceFabricHealthLoggerProvider : ILoggerProvider { }

builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<ILoggerProvider, ServiceFabricHealthLoggerProvider>());
LoggerProviderOptions.RegisterProviderOptions<ServiceFabricHealthLoggerOptions, ServiceFabricHealthLoggerProvider>(builder.Services);

Neither option to suppress seems to be working to me:

dotnet_diagnostic.CA1812.severity = none
dotnet_code_quality.CA1812.excluded_symbol_names = ServiceFabricHealthLoggerProvider

Context:

<PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="7.0.0">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>

[mavasani]

@abatishchev I was able to confirm that dotnet_diagnostic.CA1812.severity = none does indeed work fine and suppresses the warning. Please find a sample project showing the same: ClassLibrary7.zip. If you remove the editorconfig entry, CA1812 does fire as expected.

Regarding dotnet_code_quality.CA1812.excluded_symbol_names, that option is not yet supported. It is one of the proposed suggestions in this issue to allow such custom suppressions.

[abatishchev]

hi @mavasani, I've got it reproducing when replaced

<AnalysisLevel>7-all</AnalysisLevel>

with

<EnableNETAnalyzers>true</EnableNETAnalyzers>
<EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
<CodeAnalysisTreatWarningsAsErrors>true</CodeAnalysisTreatWarningsAsErrors>
<AnalysisLevel>latest</AnalysisLevel>

See ClassLibrary7.zip

Even without updating the package version to:

<PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="8.0.0-preview1.22631.1">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

Which in my case is 7.0.0.

[ghost]

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Describe the bug CA1812 false-positive ### Steps To Reproduce ````c# internal static class Repro { public static void Generate() { string str = ""; var emojis = System.Text.Json.JsonSerializer.Deserialize>(str); } private sealed class Emoji // CA1812 whereas the type is used as a generic type { public string Name { get; set; } public string Moji { get; set; } public string Shortname { get; set; } } } ```` ````xml all runtime; build; native; contentfiles; analyzers; buildtransitive ```` ### Expected behavior No diagnostic ### Actual behavior It reports a diagnostic that is not appliable in the context ### Analyzer **Package**: [Microsoft.CodeAnalysis.FxCopAnalyzers](https://www.nuget.org/packages/Microsoft.CodeAnalysis.FxCopAnalyzers) **Version**: v3.3.0 (Latest) **Diagnostic ID**: [CA1812](https://docs.microsoft.com/visualstudio/code-quality/ca1812)

Author:	meziantou
Assignees:	-
Labels:	`area-Meta`, `untriaged`, `code-analyzer`
Milestone:	-

[buyaa-n]

3. Options based Configuration: Add an editorconfig option to specify special methods or types whose generic type arguments are instantiated.

Options based Configuration sounds best to me, probably should be at type level, specifying at method level might be too granular

Regarding dotnet_code_quality.CA1812.excluded_symbol_names, that option is not yet supported. It is one of the proposed suggestions in this issue to allow such custom suppressions.

👍 I would vote for this config

[ericstj]

I raised this with @stephentoub who recommended option 2, which is where I would lean as well. This would make the rule stay useful in most cases and the small sacrifice for a potential false negative. This would make things work without having the need for folks (or API authors) to configure the rule.

You could reduce the false negatives with a slightly better heuristic. Instead of making it "used in any generic parameter" you could make it "used in a generic parameter which is also used as a return value".

This would make it flag a case where someone was calling Serialize<T>(null) but not flag a case where someone was calling Deserialize<T>(string) (since it would return T).

[buyaa-n]

Transferred back to analyzers repo as question answered

[alexrp]

You could reduce the false negatives with a slightly better heuristic. Instead of making it "used in any generic parameter" you could make it "used in a generic parameter which is also used as a return value".

Unless I'm missing something, it seems like that would still leave us with a sea of CA1812 false positives for common M.E.DI usage, e.g. services.AddSingleton<T>() and then taking T as a constructor parameter somewhere.

[Rhialto74]

I had CA1812 come up when using CommandLine in a command line project.

internal static class Program
{
    public class Options //CA1812
    {
        [Option('p', "verbose", Required = false, Default = 5, HelpText = "")]
        public int Verbose { get; set; }
    }
    private static void Main(string[] args)
    {
        _ = Parser.Default.ParseArguments<Options>(args)
        .WithParsed(opts =>
        {

The only reason I'm adding a comment here is is that I noticed the error no longer came up when I made internals visible to my test project by adding this to the command line project. I'm not pointing this out as a fix, just making note of the scenario.

<ItemGroup>
  <InternalsVisibleTo Include="MyCommandLineProject.Tests" />
</ItemGroup>

These are the command project settings.

<PropertyGroup>
  <OutputType>Exe</OutputType>
  <TargetFramework>net8.0</TargetFramework>
  <ImplicitUsings>disable</ImplicitUsings>
  <Nullable>enable</Nullable>
  <AssemblyName>MyCommandLineProject</AssemblyName>
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
  <AnalysisLevel>latest-all</AnalysisLevel>
</PropertyGroup>