Do not pass Utf8JsonReader by value

[terrajobst]

The rule should flag cases where a reader is passed around by value.

Category: Reliability

[jeffhandley]

Estimates:

• Analyzer: Small
• Fixer: Large

[carlossanlop]

@terrajobst is this what you intend the analyzer/fixer to do?:
Update: The answer below was "yes" :)

Suggested severity: Warning

Before

static void Main()
{
    Utf8JsonReader reader = new(/**/);
    MyMethod(reader);
}

static void MyMethod(Utf8JsonReader reader)
{
}

After

static void Main()
{
    Utf8JsonReader reader = new(/**/);
    MyMethod(ref reader);
}

static void MyMethod(ref Utf8JsonReader reader)
{
}

[carlossanlop]

Ping @terrajobst , @GrabYourPitchforks and @bartonjs to help clarify our question above.

[bartonjs]

Yep, the before/after is correct.

[GrabYourPitchforks]

Is the fixer's behavior of "whenever you see a byval argument, change it to byref" viable? That likely won't catch local copies being made within a method.

[bartonjs]

The fixer doesn't have to fix everything.

Method-call copies are probably almost always wrong. Local copies might be intentional, but if they're rare they can always suppress. (e.g. JsonDocument does a local copy to reset the ref if it hits an "I need more data" state)

[GrabYourPitchforks]

Sounds reasonable. Carry on! :)

[carlossanlop]

Thank you, both! I'll mark this as ready for review.

[bartonjs]

Video

Severity: Warning
Category: Reliability

• The analyzer, and the messaging, should be more generalized, like "Do not pass mutable value type '{0}' by value."
• We should have a list of well-known problematic types, like Utf8JsonReader, but also support loading other types from config.
  • Some types may be able to be identified heuristically, like "ends in Enumerator, is a value type, and is a nested type"
  • The list should also include SpinLock
• The analyzer should look for method parameter declarations where the parameter is of one of these types and the parameter mode is not ref or out (either by-value or in/readonly-ref).
  • It should also look for these types in "output positions", like property declared types or method returns.
• The fixer should change the parameter to be by ref, and then (as a stretch goal) update calls appropriately (if able).

[NewellClark]

I would like to be assigned to this, please.

[NewellClark]

I just ran my working prototype against dotnet/runtime and I found quite a few false positives in the System.Text.Json library. I found several false positives for the return by-value error in JsonElement.cs (EnumerateArray() and EnumerateObject()).
I also found several pass by-value false positives in ThrowHelper.Serialization.cs.

We may be able to improve the struct-enumerator heuristic to eliminate the return-by-value false positives. Currently, we recognize any nested value type that ends with "Enumerator" as a mutable value type, and we suppress diagnostics for methods named "GetEnumerator". We could also suppress methods containing "Enumerate", though I question whether that scenario is common enough to warrant special-casing. Might just be easier to suppress the warning in the runtime libraries.
Alternatively, we could simply not show a warning for method return values, since methods are likely to be factory methods. All of the return value false positives in dotnet/runtime were factory methods. I think properties should still be reported, though.

As for the pass by-value false positives, all of them are pass by-reference read-only parameters. I'm going to suggest that we not report diagnostics for "in" parameters. Since "in" is a language feature that beginners don't typically use, I think it's safe to assume that someone who marks a parameter as "in" likely knows what they're doing.

[buyaa-n]

Moving to future milestone as the PR blocked with an open question