Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not pass Utf8JsonReader by value #33772

Open
Tracked by #57797
terrajobst opened this issue Mar 19, 2020 · 12 comments · May be fixed by dotnet/roslyn-analyzers#5454
Open
Tracked by #57797

Do not pass Utf8JsonReader by value #33772

terrajobst opened this issue Mar 19, 2020 · 12 comments · May be fixed by dotnet/roslyn-analyzers#5454
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@terrajobst
Copy link
Member

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

Category: Reliability

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Large

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@carlossanlop
Copy link
Member

carlossanlop commented Jan 15, 2021

@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
Copy link
Member

carlossanlop commented Feb 23, 2021

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

@bartonjs
Copy link
Member

Yep, the before/after is correct.

@GrabYourPitchforks
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Sounds reasonable. Carry on! :)

@carlossanlop
Copy link
Member

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

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs more info labels Feb 24, 2021
@bartonjs
Copy link
Member

bartonjs commented Jun 3, 2021

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).

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 3, 2021
@NewellClark
Copy link
Contributor

I would like to be assigned to this, please.

@NewellClark
Copy link
Contributor

NewellClark commented Sep 6, 2021

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
Copy link
Contributor

buyaa-n commented Aug 14, 2023

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

@buyaa-n buyaa-n modified the milestones: 8.0.0, Future Aug 14, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants