AArnott / CodeGeneration.Roslyn

Assists in performing Roslyn-based code generation during a build.
Microsoft Public License
408 stars 59 forks source link

Consider Uno.SourceGeneration #100

Closed AArnott closed 4 years ago

AArnott commented 5 years ago

I just became aware of https://github.com/nventive/Uno.SourceGeneration

We should evaluate the use cases covered by this other project. If there is significant overlap, and if they do it better, we can retire this project. Otherwise, we might evangelize this project to those folks.

jarrodldavis commented 5 years ago

If I may offer my input, I prefer this project because it supports .NET Core and .NET Standard projects better. Uno.SourceGeneration requires code generators to be created in .NET Framework projects, which is unfortunate.

AArnott commented 5 years ago

Yes, I'm certainly looking for input. Thanks for yours, @jarrodldavis. How is it besides that? If it's just lacking .NET Standard support, that's something we may be able to fix if it is better or adequate in other ways.

jarrodldavis commented 5 years ago

I'm not sure; I didn't bother looking into Uno.SourceGeneration much further when I saw that it required .NET Framework, so I can't really compare against my experience with this project. I was able to go from your example in the README to what I had in mind fairly quickly (especially considering I had no experience with Roslyn APIs in general beforehand), so I'd say my experience is pretty positive so far.

The only positive I can see from the other project right now is built-in support for logging during code generation.

My main gripe is not being able to debug code generators out-of-the-box, but I worked around that by "printing" to the generated code files or running slices of code in a vanilla console project when I needed to explore the APIs.

mwpowellhtx commented 5 years ago

Uno.SourceGeneration requires code generators to be created in .NET Framework projects, which is unfortunate.

Indeed, unfortunate.

atifaziz commented 5 years ago

How is it besides that? If it's just lacking .NET Standard support, that's something we may be able to fix if it is better or adequate in other ways.

@AArnott Why not page @jeromelaban to comment here or open a question type of issue in Uno.SourceGeneration and asking there? If roadmaps largely overlap then the more forces that can join behind a project, the better.

mwpowellhtx commented 5 years ago

I ended up recasting CGR with a few key differences.

  1. We do not make any assumptions concerning the thing being annotated with CG attributes. CGR steps out of the way as early as possible and leaves it to the generator author to redress issues such as name spaces, etc. We do this by expecting a CompilationUnitSyntax.

  2. We allow for any number of such units to be generated per annotated request.

  3. We also allow for Assembly CGR annotation.

  4. We also purge the generated code based on a few leading indicators, an incomplete set of generated code, things of this nature, but, mostly, whether the source files were updated.

We have also figured out a way to perform unit testing in a more comprehensive and meaningful way.

amis92 commented 4 years ago

As I was called to drive this to conclusion, here's my attempt.


Disclosure: I'd love to create a very meaningful comparison of features, but I'm not really sure it'd be actually decisive/helpful. I'll make an attempt at the end.

1. External API

Mostly similar, both projects surface the Compilation from Roslyn. Uno is more language agnostic, at least at the actual API surface.

2. Infrastructure

Uno is superior in almost every way - the weak point being non-.NET Framework generator support (or rather the lack of it). It can use OOP "compilation server" which reduces run time because the assemblies of the engine and Roslyn are not loaded for every compilation.

3. Support

Uno is seemingly well supported and active, also it seems to be quite stable, and marked as such. Those things don't apply to CGR.

Summary

This project is inferior in almost every single way, at least from my PoV. The one big question mark I have no answer to is how open would they be for outside collaborators - it seems all contributors are from nventive-associated users (https://github.com/unoplatform/Uno.SourceGeneration/graphs/contributors). It's heavily used in their company's framework. It could mean their focus is quite limited, because it's specifically created to fit their needs.

This poses the question whether Uno.SourceGeneration is a good project to migrate to. That question I can't answer on my own. I'll talk about my feelings then: I don't feel so.

I'll also happily read other PoVs.


Feature comparison

Feature CodeGeneration.Roslyn Uno.SourceGeneration
P2P generators
X-plat generator execution
Compilation provided to generators
OOP execution
MSBuild Project provided to generators
MSBuild properties available to generators (selected) (customizable)
OOP execution optimization
Compilation generations (Uno's before/after)
Generator can create any number of source files
Generator can have dependencies
Stable
Active
Company-owned
netcoreapp generators
jeromelaban commented 4 years ago

Thanks @amis92 for creating this comparison!

The last issue is something we have on our radar, the reason we did not start is because of one missing feature that is introduced in .NET Core 3.0 that will help significantly.

There's very basic support for .NET Core (the CI is testing it somehow) but there are issues with assemblies that are referenced by generators.

jeromelaban commented 4 years ago

I forgot to mention, yes, we're accepting external contributors (/cc @ghuntley). The main reason it's only nventive's contributors is because source generation has not picked up the public steam we expected, yet... and I'm happy you noticed it!

The Uno Platform is the main consumer for the generators, but there are also those generators that we are heavily using for nventive's projects: https://github.com/nventive/Uno.CodeGen

AArnott commented 4 years ago

MSBuild Project provided to generators

This should not be available to generators. As a part of a build, an MSBuild Task should never evaluate the project itself (and doing so can cause very real problems). Tasks should only have the insight passed to it explicitly from the .targets file. @jeromelaban what do you think of this? I'm guessing you evaluate the msbuild project yourself in order to give the generators access to it. Why do they need it? Can we find another way?

@amis92 In light of this, how do you feel about #162 ? That would be a huge work item and while it would likely dramatically improve this project, it sounds like focusing efforts on Uno might make more sense based on your research.

jeromelaban commented 4 years ago

@AArnott thanks for answering :)

Yes, the Uno.SourceGenerationTasks do evaluate the build project, and it's run in a separate process's doing the same job as the main build pipeline (aside from having to propagate the global properties).

I get the argument, and it's something I've been debating with the Roslyn team for years, and as far as Roslyn (and only Roslyn without considering the larger picture) is concerned, I see why msbuild should not be present at that level (if the feature was to be implemented as part of Roslyn).

Ultimately, what's generally needed by the generators is an IDictionary<string, List<string, List<string>>> where all the MSBuild Properties and Items as evaluated values with their metadata are available to generators. The API could be changed to expose only that to the generators and make it more isolated.

Yet, the problem with all that is still that getting a proper out-of-process version of the msbuild workspace is needed to create the same Compilation the main build pipeline creates. It also needs to be out-of-proc for stability, memory efficiency (w.r.t. Visual Studio), cross-platform and compatibility reasons.

So at current time, MSBuild will need to stay in the loop somehow, even if it ends up being not being exposed directly to generators.

Also, for @amis92, I've added support for .NET Core 3.0 in the 2.0 experimental builds of Uno.SourceGenerationTasks, if that can help you.

AArnott commented 4 years ago

Ultimately, what's generally needed by the generators is an IDictionary<string, List<string, List>> where all the MSBuild Properties and Items as evaluated values with their metadata are available to generators. Yet, the problem with all that is still that getting a proper out-of-process version of the msbuild workspace is needed to create the same Compilation the main build pipeline creates.

The csc task itself gets the properties and items it needs because MSBuild itself invokes the task and passes those things in. I don't understand why you can't do the same with your targets/tasks, or why that's easier when you host msbuild yourself and re-execute many msbuild targets in an attempt to get what you can already get within the original build.

mwpowellhtx commented 4 years ago

The problem for me in my take on the CG area is not MSBuild. I consider that a foregone conclusion for many of the same reasons. The build is kicked off by something, MSBuild, apart from my CGR. No, the problem for me is assembly and reference resolution, particularly when those generators are non-trivial, even moderately complex, with some secondary and tertiary package dependencies.

AArnott commented 4 years ago

Yup: complex generators with dependencies are likely impossible to load properly in every case, at least so long as the generation runs within the msbuild.exe process. That's why we now run as a dotnet codegen CLI tool, and the msbuild task simply spawns it. At least now we control our own destiny a bit more. But I'm still pretty fuzzy when it comes to how to write an extensible .NET Core process so that I can load diverse dependency trees from... who knows where.

mwpowellhtx commented 4 years ago

I've struggled with this, and I am not entirely positive the issue is with the dependency resolution, and my take on the CGR does employ the dotnet cgr CLI approach. I haven't quite sorted it out, but I think it is because of the assembly/reference resolution, and my CLI is targeting netcore2.1. I'm not sitting right in front of the project at the moment, but I think it is due to something in the System.Runtime.Loader not loading package dependencies properly. At one point, we side stepped for a fit for purpose CLI tool with fat package references, i.e. packaging the dependencies themselves. However, long range, I would like for package references to work correctly in this context.

Corniel commented 4 years ago

Might this be worth starting up a new Open Source project combining CGR and Uno? I'm more than willing to volunteer, I guess @amis92 would like to volunteer too, so if both @AArnott and @jeromelaban agree, you have a core of at least 4 developers.

mwpowellhtx commented 4 years ago

@Corniel Feel free to check out my CGR. I started from the same basis as here, but made some key strategic decisions a bit differently than here. Open to suggestions on that, or more contemporary, even forward thinking, paths.

Corniel commented 4 years ago

@mwpowellhtx I belief the 'world' is better of with one shared effort. It should be possible to come up with a view on how to build/develop/maintain a solution we all can be proud of. :)

mwpowellhtx commented 4 years ago

@Corniel Not sure what that means, but if you would like to join forces on the issue and come to some agreement what "code generation" means moving forward, like I said, I am open to suggestions. It is something that I have a need and a use for and would like to see something come of it.

Corniel commented 4 years ago

@mwpowellhtx I'm under the impression that your spin-off (with a potentially confusing namespace) only offers more competing solutions, and by that is making all options to choose from less attempting.

I think the solution we are searching for is tooling that helps us to overcome 'limitations' in C#, that make that applying some software patterns require too much plumbing. This requires both concrete solutions (Like @andrewlock's StronglyTypedId, or Uno's implementation of the read-only type), but also generic tooling (like @AArnott provides in this repro) to create custom build-time custom solutions to custom problems.

(The reason I entered up here, is that I want to overcome the fact that you can not use inheritage when defining a struct as I do in my library Qowaiv to model (Single) Value Objects)

amis92 commented 4 years ago

I'd suggest moving this discussion over to #162 - there I've laid out a proposal for a redesign that may very well be a new project altogether, @mwpowellhtx @Corniel - there's also @hypervtechnics that was interested in that.

I'm very open to working out an initial design that we all agree on, and teaming up to do it together.

Also, appropriate pic:

XKCD Standards

amis92 commented 4 years ago

With the source generators being seriously developed in Roslyn, I think there's no reason to attempt building anything new. Also, because of the XKCD above.

So, I'm closing this with the following conclusions:

Thank you all for good work and all input given.