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

Better System.Text.JSON experience in Source Generators #41174

Open
kkukshtel opened this issue May 26, 2024 · 9 comments
Open

Better System.Text.JSON experience in Source Generators #41174

kkukshtel opened this issue May 26, 2024 · 9 comments

Comments

@kkukshtel
Copy link

kkukshtel commented May 26, 2024

The current story for packing transient nuget packages into delivered source generators is generally not great, and makes working with them feel like a second class participant in the dotnet ecosystem. This is incredibly sad, because as an idea they are brilliant and incredibly useful.

This issue that goes into working with transient nuget packages for generators was filed three years ago and seems like no work has really been done to make this any better.

I'm making this issue in lieu of waiting for the transient experience to change, and instead suggest that I assume one of the biggest transient nuget packages people include in generators (even as shown in the linked issue) is System.Text.JSON (or Newtonsoft).

Including System.Text.JSON in a delivered nuget source generator, which feels like an absolutely perfect use case for System.Text.JSON, is pull-out-you-hair difficult. Given SGs being pinned to netstandard2.0, you have to use a dedicated package
to get that capability. But for above reasons, it doesn't work super well and introduces a lot of other annoying things to do to manage the inclusion of that singe package.

So my question is — can we just get some way of System.Text.JSON inside of a source generator by default? Or have some path for generators to work for netcore apps instead of being pinned to netstandard2.0 (as an opt-in?).

Maybe I'm dreaming here, but this issue feels like it is singlehandlely halting development progress for me on some generators and I'd love to have just a simple answer for "how to do JSON deserialization in a source generator".

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels May 26, 2024
@nagilson nagilson added needs design needs team triage Requires a full team discussion and removed untriaged Request triage from a team member labels May 28, 2024
@marcpopMSFT
Copy link
Member

Triage: @ericstj is the owner of STJ and an expert on generators. That other thread has a lot of history but I'm curious if anything has changed recently.

@ericstj
Copy link
Member

ericstj commented May 28, 2024

I'm actually not the owner for System.Text.Json @dotnet/area-system-text-json is, but this question is more about roslyn extensibility (which I also don't own, but have some opinions).

So my question is — can we just get some way of System.Text.JSON inside of a source generator by default? Or have some path for generators to work for netcore apps instead of being pinned to netstandard2.0 (as an opt-in?).

The problem here is not really with source generators, but more generally with the Roslyn plugin app model. Same applies to generators and analyzers. It's actually quite similar to MSBuild tasks - where they have similar difficulty distributing dependencies.

One thing @ViktorHofer, @baronfel, and I have been discussing recently that it might make sense to have host applications (MSbuild, or Compiler in this case) expose an SDK for plugins to use. Such an SDK could make all the assemblies provided by the host "available" for reference by a plugin without the plugin needing to reference those or carry them with it. That still wouldn't help for JSON since the compiler doesn't carry that today - but maybe it could since VS definitely carries it.

@chsienki @jaredpar - any plans to make it easier for roslyn plugin assemblies to deal with dependencies like this?

@jaredpar
Copy link
Member

One part that I don't think is well understood is that csc is just one of the many applications that hosts the compiler. There are many others across many different platforms: VBCSCompiler, VS, VS Code, CodeQL, Workspaces, etc ... Changes to the hosting API often mean that we need to change all of the hosts to reflect that change. That can be a very tall order.

Consider that in 17.7 we added a single new dependency into Roslyn: Microsoft.CodeAnalysis.RazorCompiler.ExternalAccess.dll. This is a DLL that we produced and controlled. Nine months later we still haven't fixed all of the problems that introduced. This is quite possibly the simplest of changes we could make given our hosting ecosystem.

any plans to make it easier for roslyn plugin assemblies to deal with dependencies like this?

Right now there are no plans to significantly alter our dependency model.

That still wouldn't help for JSON since the compiler doesn't carry that today - but maybe it could since VS definitely carries it.

VS only solves one case. To be ubiquitous for generators we'd need to update every single host which is not realistic.

@kasperk81
Copy link
Contributor

This issue that goes into working with transient nuget packages for generators was filed three years ago and seems like no work has really been done to make this any better.

and NuGet/Home#3891, the highest upvoted issue in nuget was opened eight years ago, still assigned priority 2.

@kkukshtel
Copy link
Author

kkukshtel commented May 29, 2024

To be ubiquitous for generators we'd need to update every single host which is not realistic.

On one hand I hear you but on the other - is this not the case for improving the generator experience at all? I understand that it's hard to make them better but not that that is a reason in and of itself for not trying to improve them in the first place. SGs were announced and released and as far as I can tell have seen little improvement since that initial release and this makes me think why.

I understand that politically it may have been what was required to do them in the first place, which I think is commendable, but if you've used generators you know that their experience in basically anything beside bleeding edge VS is subpar to the point of consistently just feeling "broken".

VS Code rarely picks up the code or its cache doesn't update so has references to old generated code (even if source isn't directly emitting files). Rider (not sure what it uses underneath) doesn't work without a full dotnet shutdown and restart for the code to be recognized, etc. I think if instead improvement could be targeted to a subset of all possible compilers that are high-impact (Code, VS, csc), with necessary caveats, that could be a tenable way to make forward progress on improving the experience without needing to improve everything uniformly.

I don't want this thread to devolve into me moaning about SGs, and I do think getting a better STJ experience somehow is still what I would love, but the previous comments really lead me to believe that whatever model for improvement is setup for SGs just seems like it's inadequate to actually support the needs of the project.

@jaredpar
Copy link
Member

jaredpar commented May 29, 2024

SGs were announced and released and as far as I can tell have seen little improvement since that initial release and this makes me think why.

Since releasing ISourceGenerator we've made many enhancements to generators:

  • Designed and implemented incremental generators
  • Released ForAttributeWithMetadataName to make incremental generation easier to author.
  • Substantially redesigned analyzer / generator loading to be more resilient of dependencies.
  • Added multi-targeted analyzer loading capabilities to the SDK to better support generators across a variety of SDK releases.
  • Worked with hosts like Visual Studio to better support dependency loading.
  • Designed and released interceptors to give more AOT capabilities to generators.
  • Design, but not yet implemented, support for generating non source file artifacts.
  • Moved generator handling completely out of proc in Visual Studio which opens the door for many improvements like live reloading of generators.
  • Refactored the generator implementation so it can be moved off the hot keystroke path in Visual Studio (massive amount of work by IDE team).

Just because we aren't doing the specific request you want doesn't mean we are idle on generators.

I understand that politically it may have been what was required to do them in the first place,

I have no idea what you think is political about generators.

but if you've used generators you know that their experience in basically anything beside bleeding edge VS is subpar to the point of consistently just feeling "broken".

As the roslyn compiler lead I use generators quite regularly and I assure you the experience feels anything but broken.

@kkukshtel
Copy link
Author

kkukshtel commented May 29, 2024

Okay I was being ungenerous and facetious, and I can directly say that I have appreciated the introduction of Incremental Generators, but the list you mention also feels a little bit like pointing to bikeshedding when the community consensus around generators, nearly four years on, is that they are still unstable and unreliable. Despite the compiler surface you mention as something necessary to support for generators, the only surefire way to get them to work is to use Visual Studio, and restart Visual Studio (or the build server) if you want them to work 100% of the time:

This general inability for it to "just work" prevents me (and I'm sure plenty of others) from engaging meaningfully with that feature list you posted. I would love to get excited about AOT improvements for generators, but if I have to restart my IDE 30 times a day just to get the generator to work at all, I'm not going to engage with them.

I appreciate and respect your position as the compiler lead to have a full scope and understanding of the problem space, but would also gently suggest that your deep knowledge of the compiler wants and needs makes it hard to put yourself in the shoes of the average user (like me) that wants to engage with a truly incredible C# feature but doesn't have the compiler internals internalized to know what is/isn't possible (especially when they were pitched as so effortless by Luca's blog posts).

Just because we aren't doing the specific request you want doesn't mean we are idle on generators.

I'll take the punch here because I was a bit of an ass above, but I am not trying to ramrod my own desired feature here. My point in opening this issue is to just investigate what the dotnet team's recommendation on JSON deserialization is for a source generator. I'd like to see better integration of STJ, but if that's not feasible, what's the way to do it? Are there other options?

If the party line of the team is that you shouldn't use external deps for generators, I think that's one thing. But the docs that sketch the solution are a bit disingenuous because they make it seem like a simple flag, but don't actually address non-trivial packages (aka packages with larger dep trees) like the required version of STJ.

@jaredpar
Copy link
Member

... and restart Visual Studio (or the build server) if you want them to work 100% of the time:

I understand and definitely emphasize with this being a long standing pain point in generators. At the same time, it is one that requires a massive amount of work to fix. At the time we introduced generators the entirety of C# analysis was inside the main VS process which is .NET Framework based. That means there are pretty hard limitations on our ability to reload plugins on changes. The only realistic way to approach fixing this is having generators run out of process and in .NET core where we have a lot more options for reloading plugins.

The good news is that the IDE team has treated this problem seriously and been chipping away at it steadily over the last few years. In 17.10 they took a huge step by getting our generation code path to work entirely out of process on .NET Core. Among other things that opened the door for us to reload generators in the running process. That work hasn't been implemented yet but is very much in our future plans.

If the party line of the team is that you shouldn't use external deps for generators, I think that's one thing. But the docs that sketch the solution are a bit disingenuous because they make it seem like a simple flag, but don't actually address non-trivial packages (aka packages with larger dep trees) like the required version of STJ.

Our stance is that dependencies are supported for generators but it's very much an advanced scenario. The compiler is hosted in a number of different environments and generator authors need to be resilient to the loading demands of all them. Every host can, and often does, put different demands on assembly loading.

Consider as a concrete example Visual Studio. That host requires that for platform assemblies we load their copy vs. whatever a generator author specifies. So even if you reference a very specific version of STJ in your generator, in the context of VS you will get the VS version. That is the type of complications that generator authors need to consider when taking dependencies.

I understand the desire for simple dependency scenarios but reality is not so simple. Even if you limit yourself to the core scenarios (VS, VS Code and dotnet build) there are still a large variety of constraints to consider:

  • VS and VS Code have policy decisions about platform assemblies that differ from build.
  • There are two major variations in loading strategy: AssemblyLoadContext vs. AppDomain.
  • There are three minor variations: load by stream, shadow load and direct load.

Any of these can impact how dependencies are loaded.

The compiler is a flexible tool that is used in a huge variety of scenarios. It is very difficult to hide that complexity from DLLs that plug into it. Particularly once dependencies get pushed into the mix.

@marcpopMSFT marcpopMSFT removed the needs team triage Requires a full team discussion label Jun 4, 2024
@marcpopMSFT marcpopMSFT added this to the Backlog milestone Jun 4, 2024
@marcpopMSFT
Copy link
Member

Triage: There is nothing the SDK can do at the moment. Moving to backlog as the discussion on options continues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants