CA1869: "Cache and reuse 'JsonSerializerOptions' instances" - false positives in top-level programs

[spydacarnage]

Description

CA1869 is caused by a local instance of JsonSerializerOptions being created and used once as the options argument of a Serialize or Deserialize call as this can cause performance degradation if the code is executed multiple times.

I initially found this bug in a top-level program where there was only a single Deserialize call in the whole file, so I thought it was a little strange, but upon further investigation found that this bug extends to other situations.

Reproduction Steps

There are 3 versions, all very similar, but each with potentially different solutions.

Version 1 - Top-level program

JsonSerializerOptions options = new()
{
    PropertyNameCaseInsensitive = true,
    ReadCommentHandling = JsonCommentHandling.Skip
};

var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);

Version 2 - In a class constructor:

    public Test()
    {
        JsonSerializerOptions options = new()
        {
            PropertyNameCaseInsensitive = true,
            ReadCommentHandling = JsonCommentHandling.Skip
        };

        var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);
    }

Version 3 - In a loop inside a standard method:

    public void M()
    {
        for (int i = 0; i < 10; i++)
        {
            JsonSerializerOptions options = new()
            {
                PropertyNameCaseInsensitive = true,
                ReadCommentHandling = JsonCommentHandling.Skip
            };

            var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);
        }
    }

Expected behavior

For version 1, I'm honestly not sure what should happen - maybe nothing, as the fix appears to make the code worse, not better.
For version 2, I could see the fixer creating a static variable to hold the options for use in each construction.
For version 3, I would hope the fixer would, at the very least, move the instance creation outside the loop, as duplicating it inside the loop doesn't improve the performance at all.

Actual behavior

After using the provided fixer, the following was produced:

Version 1 - Top-level program

JsonSerializerOptions jsonSerializerOptions = new()
{
    PropertyNameCaseInsensitive = true,
    ReadCommentHandling = JsonCommentHandling.Skip
};
JsonSerializerOptions options = jsonSerializerOptions;

var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);

Version 2 - In a class constructor:

    public Test()
    {
        JsonSerializerOptions jsonSerializerOptions = new()
        {
            PropertyNameCaseInsensitive = true,
            ReadCommentHandling = JsonCommentHandling.Skip
        };
        JsonSerializerOptions options = jsonSerializerOptions;

        var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);

Version 3 - In a loop inside a standard method:

    public void M()
    {
        for (int i = 0; i < 10; i++)
        {
            JsonSerializerOptions jsonSerializerOptions = new()
            {
                PropertyNameCaseInsensitive = true,
                ReadCommentHandling = JsonCommentHandling.Skip
            };
            JsonSerializerOptions options = jsonSerializerOptions;

            var output = JsonSerializer.Deserialize<int[]>("[1,2,3]", options);
        }
    }

Version 3 does also have an extra option, although after the one above, which alters the method to allow a JsonSerializerOptions instance to be passed in as a parameter, which is much better.

Regression?

The problem seems to also be in NET8, but not in NET7.

Known Workarounds

Ignore or suppress the rule.

Configuration

Using VS 17.13.0 Preview 4 in a NET9 console application, running on Windows 11 x64.

Other information

No response

[dotnet-policy-service]

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

[eiriktsarpalis]

After using the provided fixer

I'm pretty sure the analyzer in question doesn't come with a fixer. Can you clarify which suggestion you applied that resulted in these changes? The recommendations I'm seeing are unrelated to the analyzer per se:

[spydacarnage]

I used the "Introduce local for ..." option. I assumed that it was part of the analyser because of this:

[eiriktsarpalis]

It's unrelated. We intentionally didn't include a fixer because caching instances is highly context-specific and would be hard to automate meaningfully.

That being said, the analyzer firing in top-level programs is a known false positive and we should try to detect it. Leaving the issue open to track that.

[spydacarnage]

The false positives aren't just in top-level programs - it fires in methods in fully constructed classes, too.

[eiriktsarpalis]

The false positives aren't just in top-level programs - it fires in methods in fully constructed classes, too.

Can you clarify? The other examples that you cite are almost certainly inappropriate use of JsonSerializerOptions and the analyzer is correct to highlight them. In both cases they should have been lifted to statics.

[spydacarnage]

The only fixer offered is the same one as you've stated is nothing to do with this analyser.

And, as shown in the original issue, on both occasions, it just creates useless additional locals in the same scope. I would have expected it to be raised to a static, but that doesn't appear to be an option.

[eiriktsarpalis]

But, like I said, these suggestions are unrelated to the analyzer. Providing a fixer has never been a goal of this analyzer for the reasons stated above.

[spydacarnage]

So, if they're not related, why does the hover have the "Show potential fixes" shown under the analyser output?

But if CA1869 isn't triggered, there is no indication of a fixer being available, so there must be some connection between the scenarios.