Discussion 104

[rdeago]

This PR aims to implement all bullet points listed in #104.

I started with a little housekeeping:

• reorganized projects so all source generator projects can share code;
• created a non-packable CommonCode project containing all the shared code;
• made shared source files invisible in source generator projects, so they don't clutter Visual Studio's Solution Explorer;
• updated dependencies and synced files with devlooped/oss.

Nest steps will be:

• remove the C# 9.0 restriction - instead, check for the Roslyn version in use to determine whether ThisAssembly can be used in a project;
• make one of the source generators incremental, meanwhile working on shared code that will facilitate work on remaining generators;
• make all the other generators incremental;
• write code to generate the "main" part of the ThisAssembly class - with XML docs, attributes, access modifiers, and an empty body - and run it in just one generator (the first whose .targets file is included in the project).

I will update this post as I add more commits. In the meanwhile, as of the first 13 commits (de4993c) the solution is buildable (at least locally, we'll see how it goes on CI) and passes all unit tests, so @kzu you may want to take a look and give some feedback on the direction I'm taking.

[kzu]

Hi @rdeago! Thanks a lot for taking the time to work on this. Looks very promising!

I merged a pending PR that did the dotnet-sync update for devlooped/oss, so you can skip that commit on your PR now so it remains clean-ish.

[rdeago]

I merged a pending PR that did the dotnet-sync update for devlooped/oss, so you can skip that commit on your PR now so it remains clean-ish.

I had already done the same thing, but no problem, I rebased the PR.

I have already incorporated code from all currently open PRs (update to Scriban 5.4.5 as per #106 coming soon) so you don't need to merge them all.

[kzu]

I just merged all the pending bumps. I'd rather keep this PR clean from those to avoid unrelated noise

[kzu]

Hey @rdeago this has become a monster of a PR. Very very hard to review and wrap my head around. I've been going through the code here and there, but I'm started to get the feeling that this should be several PRs entirely separate. Some obvious ones:

1. Move to central package references
2. Reorganize folders and move common stuff to new project
3. Make generators incremental
4. Masive refactoring of how code is actually generated

The last one in particular, makes the codegen much harder to follow than with simple scriban templates. Makes it very opaque to follow (for example, I was trying to figure out how the generated Constants look, and it's quite tricky to follow). I may need more time to digest the approach to determine if it's something that will be easy to maintain in the future, or if I'd rather stick to some duplication but a more straightforward approach with basic templating. I'm also not sure if the rework is a requirement of the incremental generator approach or not, I'd need to look into it some more.

Thanks again for taking the time to provide a path forward here, much appreciated!

[kzu]

Also, I'd use https://www.nuget.org/packages/isexternalinit for that particular polyfill.

[rdeago]

Hey @rdeago this has become a monster of a PR. Very very hard to review and wrap my head around.

Yeah, I'm sorry, my fault. I didn't anticipate it becoming such a behemoth.

I'm started to get the feeling that this should be several PRs entirely separate. Some obvious ones:

1. Move to central package references
2. Reorganize folders and move common stuff to new project
3. Make generators incremental
4. Masive refactoring of how code is actually generated

Agreed. This draft PR might remain for reference and be deleted when other PRs are merged or at least on the right track.

Central package management and folder reorganization are probably the first things to do, so Dependabot PRs and other PRs agree on which files to update. Rebasing after merging of Dependabot PRs has been a pain in the sacral region.

[Removing embedded Scriban] makes the codegen much harder to follow than with simple scriban templates.

I was trying to figure out how the generated Constants look, and it's quite tricky to follow

I admittedly followed a radically different approach. With templates, you look at a template and you can mostly figure out the outcome; with code generation classes, the first time you have to dig through code to find out how a Constant is translated to code, then a Class, then e.g. the Constants class. Of course when you then look at, say, AssemblyInfo, the previous cognitive burden pays up: you look at the AssemblyInfo class and you already know what the building blocks are.

I did this because it makes adding orthogonal features, such as the possibility of generating consts vs. readonly fields, a lot easier. The alternative would be to implement such features at the template level, essentially obtaining the same level of complexity, just duplicated four times, with more dollar signs, without syntax highlighting, and without Intellisense.

Other considerations that made me ditch Scriban include:

• Performance. You never know how many source generators a project uses, so the faster the better, within reasonable limits of course. Scriban is fast and memory-friendly when compared to other templating engines, but I think it can hardly beat not having templates at all.
• Since Scriban source files have no <auto-generated/> marker comments, they are subject to code analysis in every project that embeds them. This results in thousands (literally) of useless diagnostics, unless code analysis is turned off.

Of course, the relative weight of each of the points above is subjective, so your conclusions may wildly differ from mine.

I may need more time to digest the approach to determine if it's something that will be easy to maintain in the future, or if I'd rather stick to some duplication but a more straightforward approach with basic templating.

By all means, take your time to assess the implications of both approaches. If you then decide in favor of templates, no hard feelings on my part: it's been an instructing and inspiring experience anyway.

I'm also not sure if the rework is a requirement of the incremental generator approach or not

AFAIK nothing prevents you, in principle, from using templates in an incremental generator.

Thanks again for taking the time to provide a path forward here, much appreciated!

You're welcome! Any way you decide to go forward, I'm a @ mention away if you need me. 😉 Plus, of course, I'm still watching this PR and related discussions.

[rdeago]

Also, I'd use https://www.nuget.org/packages/isexternalinit for that particular polyfill.

Yeah, and https://www.nuget.org/packages/indexrange for System.Index and System.Range probably. To be honest, I got fed up of looking for polyfill packages some weeks ago. Also (shameless plug ahead) I'm in the course of writing my own - with a different approach (this shouldn't surprise you by now 😁) to prevent code duplication. Long story. If you want I'll tag you in the repo as soon as it goes public.

[kzu]

Also (shameless plug ahead) I'm in the course of writing my own - with a different approach (this shouldn't surprise you by now 😁) to prevent code duplication. Long story. If you want I'll tag you in the repo as soon as it goes public.

Please do!

[kzu]

Central package management and folder reorganization are probably the first things to do, so Dependabot PRs and other PRs agree on which files to update. Rebasing after merging of Dependabot PRs has been a pain in the sacral region.

Agreed. That's an easy yes for merging right-away for sure.

I did this because it makes adding orthogonal features, such as the possibility of generating consts vs. readonly fields, a lot easier. The alternative would be to implement such features at the template level, essentially obtaining the same level of complexity, just duplicated four times, with more dollar signs, without syntax highlighting, and without Intellisense.

There is another alternative: switch the template in use entirely at the generator level when you detect the setting. This will keep the template clean and simple, albeit somewhat duplicated here and there.

• Performance. You never know how many source generators a project uses, so the faster the better, within reasonable limits of course. Scriban is fast and memory-friendly when compared to other templating engines, but I think it can hardly beat not having templates at all.

In my experience doing codegen for many years, I appreciate maintainability over almost everything else. So, unless there is an actual performance issue that becomes severe, I'd still value readability and maintainability over everything else. Codegen is so "meta" that unless you can (more or less) easily mentally map from model > generated code, when time comes to update the generated code (and it always does), you will have a very hard time tweaking even minor things. It's a nightmare to maintain a few years down the road when you haven't look at it for a long while (or you're a new maintainer).

• Since Scriban source files have no <auto-generated/> marker comments, they are subject to code analysis in every project that embeds them. This results in thousands (literally) of useless diagnostics, unless code analysis is turned off.

I'm pretty sure this is trivial to fix. A simple .editorconfig setting can turn off code analysis for all of the templates. We could even place them all in the same folder and turn off analysis for the entire folder.

By all means, take your time to assess the implications of both approaches. If you then decide in favor of templates, no hard feelings on my part: it's been an instructing and inspiring experience anyway.

Cool, thanks!

[CLAassistant]

All committers have signed the CLA.

[kzu]

Closing this since most (all?) of it is now implemented on main.

Thanks @rdeago for the ideas and suggestions!