search

dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.67k stars 1.06k forks source link

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

Open kkukshtel opened 4 months ago

kkukshtel commented 4 months ago

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

marcpopMSFT commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/NuGet/Home/issues/3891, the highest upvoted issue in nuget was opened eight years ago, still assigned priority 2.

kkukshtel commented 4 months ago

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 commented 4 months ago

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:

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 commented 4 months ago

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 commented 4 months ago

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

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 commented 4 months ago

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