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

Consider sharing diagnostics issued by JSON source generator with reflection-based serializer #61262

Open
layomia opened this issue Nov 5, 2021 · 5 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Nov 5, 2021

Many of the diagnostics issued by the JSON source generator are also applicable to the reflection-based serializer. For example, a type cannot contain multiple [JsonConstructor] applications. When the source generator is used, it logs a compile error when a type violates this expectation. A runtime exception is thrown in the reflection-based serializer instead.

Rather than having different behavior, we could package these diagnostic examinations into an analyzer that could be shared between the source generator and the reflection serializer, potentially giving the reflection serializer less validation work to do at runtime. We'd have to consider where this analyzer would live. There are a couple options

The source generator would be dependent on the analyzer, so we'd need to ship the analyzer in a manner that guarantees its visibility to the source generator.

cc @dotnet/area-system-text-json @jeffhandley

@layomia layomia added the enhancement Product code improvement that does NOT require public API changes/additions label Nov 5, 2021
@layomia layomia added this to the 7.0.0 milestone Nov 5, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Nov 5, 2021
@ghost
Copy link

ghost commented Nov 5, 2021

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

Issue Details

Many of the diagnostics issued by the JSON source generator are also applicable to the reflection-based serializer. For example, a type cannot contain multiple [JsonConstructor] applications. When the source generator is used, it logs a compile error when a type violates this expectation. A runtime exception is thrown in the reflection-based serializer instead.

Rather than having different behavior, we could package these diagnostic examinations into an analyzer that could be shared between the source generator and the reflection serializer, potentially giving the reflection serializer less validation work to do at runtime. We'd have to consider where this analyzer would live. There are a couple options

The source generator would be dependent on the analyzer, so we'd need to ship the analyzer in a manner that guarantees its visibility to the source generator.

cc @dotnet/area-system-text-json @jeffhandley

Author: layomia
Assignees: -
Labels:

enhancement, area-System.Text.Json, untriaged

Milestone: 7.0.0

@jkotas
Copy link
Member

jkotas commented Nov 5, 2021

it could ship in the same assembly as the source generator (becoming the first analyzer to be shipped in the dotnet/runtime repo)

The new DllImportImportGenerator is same analyzer + source generator bundle. @jkoritzinsky Is that right?

@jkoritzinsky
Copy link
Member

Yes that is correct. We have both analyzers and generators in the DllImportGenerator assembly in dotnet/runtime.

@layomia
Copy link
Contributor Author

layomia commented Nov 5, 2021

Great, will look into how this is wired up.

@layomia layomia added source-generator Indicates an issue with a source generator feature and removed untriaged New issue has not been triaged by the area owner labels Nov 5, 2021
@eiriktsarpalis eiriktsarpalis modified the milestones: 7.0.0, Future Apr 18, 2022
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Apr 18, 2022

Moving to Future, as we won't have time to work on this in the .NET 7 timeframe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

4 participants