Enable rule CA1869 - Cache and reuse 'JsonSerializerOptions' #90895
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsFollow up to dotnet/roslyn-analyzers#6850. Found 102 hits total on test projects, 1 in FunctionalTests\System.Text.RegularExpressions.Tests. The rest were in System.Text.Json.Tests, which is unsurprising.
|
FYI you have more flagged that failed the build |
| @@ -65,7 +67,7 @@ public static void Create(MetadataReader metadataReader, string clsidMapPath) | |||
|
|
|||
| using (StreamWriter writer = File.CreateText(clsidMapPath)) | |||
| { | |||
| writer.Write(JsonSerializer.Serialize(clsidMap, new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull })); | |||
| writer.Write(JsonSerializer.Serialize(clsidMap, s_jsonOptions)); | |||
There was a problem hiding this comment.
Wouldn't it be better to serialize directly into the FileStream instead of creating a string?
There was a problem hiding this comment.
It probably is but those optimizations are beyond the scope of this PR and this isn't product code so the performance gains probably are probably not that significant.
There was a problem hiding this comment.
It would be nice if there was an analyzer for this as well.
| @@ -34,7 +35,7 @@ public FileCache(string? cacheFilePath, TaskLoggingHelper log) | |||
| { | |||
| _oldCache = (CompilerCache?)JsonSerializer.Deserialize(File.ReadAllText(cacheFilePath), | |||
| typeof(CompilerCache), | |||
| new JsonSerializerOptions()); | |||
| s_jsonOptions); | |||
There was a problem hiding this comment.
Does this need to pass a JSO at all? Couldn't the argument just be removed?
There was a problem hiding this comment.
I think it's better for consistency to keep it here. In case another option is added to the options in the future apart from WriteIndented, someone might not realize that now they have to pass in the options here as well.
There was a problem hiding this comment.
The options just happen to not affect reading right now, but that could change.
There was a problem hiding this comment.
Also, this way, the same cache is reused for both reading & writing.
ghost
locked as resolved and limited conversation to collaborators
Follow up to dotnet/roslyn-analyzers#6850.
Found 102 hits on test projects, 1 in FunctionalTests\System.Text.RegularExpressions.Tests. The rest were in System.Text.Json.Tests, which is unsurprising.
Also found 5 occurrences in src/tasks projects which should be fixed.