amis92 commented 5 years ago

Riding on the back of #137 through some discussions with @hypervtechnics this is what we came up with.

Terms

First let's define some terms we'll use across this document:

Term	Definition
CGR	CodeGeneration.Roslyn - this project, this framework
CLI tool	the `dotnet-codegen` tool invoked from MSBuild targets of this framework
Plugin	An assembly that publishes non-framework components used for code generation, produced by Providers
Plugin Manifest	A component that registers all components published by this Plugin
Provider	An owner of the Plugin
Consumer	An owner of the project using CGR and some Plugins to add generated code to it

UX

Let's start with UX because that's a really important part, and we've had many issues with users struggling to get value from this project, because of many factors - unclear usage, shady error messages, complicated installation, etc. etc.

Our goals:

Consumer can very easily use CGR framework and Plugins, with a very simple installation scenario - this includes using Plugins both in P2P scenario as well from NuGet.
Provider can very easily create Plugins:
- There's a very clean and simple API to use when building attribute-based triggers (the only scenario supported today)
- There's a powerful lower-level API to tap into for building custom code-generation scenarios
- There's a well-defined scenario for publishing Plugins as Nugets

For achieving goal 1 we create an MSBuild Sdk CodeGeneration.Roslyn.Sdk. This Sdk will:

Set the CGR framework version used
Set a property that says the Sdk is used
Add the DotNetCliToolReference of dotnet-codegen
Add the CGR.BuildTime package reference
Possibly provide some help messages when issues are detected (e.g. p2p plugin reference detected but not correct)

For achieving goal 2:

We create an MSBuild Sdk CodeGeneration.Roslyn.Plugin.Sdk which:
- validates TargetFramework used is compatible with dotnet-codegen
- adds .props and .targets to the nuget packaging (if needed)
- adds a reference to the CodeGeneration.Roslyn.Abstractions package
We rewrite the framework. The proposed new design follows.

New framework design

Let's start with the MSBuild target that invokes the CLI tool. No changes there are required. The CLI tool's interface will stay the same or very similar.

Then the CLI tool will pass the arguments into a new component, codenamed "PipelineManager". This component takes responsibility of the rest of the process.

This pipeline process consists of three stages:

Plugin discovery
Pipeline building (includes registration of plugin components)
Pipeline execution

Plugin discovery

Plugins are discovered from the CodeGenerationRoslynPlugin MSBuild ItemGroup passed into CLI tool. This list contains assembly paths. These assemblies are loaded, and a special assembly attribute is looked-up: [assembly: CodeGenerationRoslynPluginManifest(typeof(FooManifest))]

This new attribute provided in CodeGeneration.Roslyn.Attributes package would look like that:

[AttributeUsage(AttributeTargets.Assembly, Inherited = false, AllowMultiple = false)]
public sealed class CodeGenerationRoslynPluginManifestAttribute : Attribute
{
    public CodeGenerationRoslynPluginManifestAttribute(Type manifestType)
    {
        ManifestType = manifestType;
    }
    public Type ManifestType { get; }
}

Pipeline building

The CodeGenerationRoslynPluginManifestAttribute.ManifestType is a type that can implement some contracts. For now we propose a new interface IConfigurePipeline, with more/new added in future (like IConfigureServices).

The manifest is used to configure the pipeline builder, which then gets built and invoked with the context.

namespace CodeGeneration.Roslyn.Abstractions
{
    using System;
    using System.Collections.Generic;
    using System.Threading.Tasks;

    public interface IConfigurePipeline
    {
        void Configure(IPipelineBuilder builder);
    }

    // the following imitates ASP.NET Core's IApplicationBuilder and friends

    public abstract class PipelineContext
    {
        public abstract IDictionary<object, object> Items { get; }
    }

    public delegate Task PipelineDelegate(PipelineContext context);

    public interface IPipelineBuilder
    {
        IPipelineBuilder Use(Func<PipelineDelegate, PipelineDelegate> middleware);

        PipelineDelegate Build();
    }

    public static class PipelineBuilderExtensions
    {
        public static IPipelineBuilder Use(
            this IPipelineBuilder builder,
            Func<PipelineContext, Func<Task>, Task> middleware)
        {
            return builder.Use(next => ctx => middleware(ctx, () => next(ctx)));
        }
    }

    // usage
    class FooManifest : IPluginManifest
    {
        public void Configure(IPipelineBuilder builder)
        {
            builder.Use(async (ctx, next) =>
            {
                var compilation = ctx.Items["Compilation"];
                // generate some files
                await next();
            });
        }
    }
}

The pipeline is pre-filled with some built-in middleware that:

saves the generated file list to pass back to MSBuild target
translates exceptions to exit code
generates Compilation from the inputs
maybe other stuff if needed, like logging diagnostics

The last part looks a bit like a service, which we could also implement as ASP.NET Core does, via ServiceProvider and IConfigureServices extension point on PluginManifest. To be considered in future.

Pipeline execution

The pipeline context is instantiated and filled with inputs. The pipeline is built using builder.Build() and the delegate returned is invoked with the pipeline context.

The delegate is awaited and the process is finished.

Work plan

We'll track the work at https://github.com/hypervtechnics/CodeGeneration.Roslyn/projects/1 and in issues here. The prioritization follows:

Create Consumer Sdk. This would provide us with a tool to direct the migration effort later.
Create the pipeline such that the existing Engine still works as expected (and consider rename to AttributeBasedEngine)
Create the Provider UX:
- Plugin.Sdk
- dotnet new template for plugin
- documentation for the Pipeline, Middlewares, how it works, etc.

Please share your thoughts, concerns, ask questions - we'd like to discuss this before getting dirty in implementation.

AArnott commented 5 years ago

Wow. The goals look great. Thanks for writing this up, @amis92!

The pipeline plugin mechanism you describe looks... complicated. If this is all based on real requirements folks have, I may be ok with it once I see a sample plugin that fills it and docs to help convince me it won't scare everyone away.

Can we still support incremental build and minimize code generation costs?

amis92 commented 4 years ago

Let's fall back a little and try to list out all the requirements that we want from code generation framework.

My thoughts:

Roslyn's Compilation is available for the generator to use.
Generator creates C# source files.
Generator can be included in the same solution as the using project (P2P support).

Corniel commented 4 years ago

About the redesign, simplicity by default would be a prerequisite. Furthermore, I would try to put as much effort in to join forces with Uno (@jeromelaban and his colleguaes). It might be worth (I think I would prefer that), to start a separate Github organisation for this and some satellite repositories.

Corniel commented 4 years ago

Anyone?

amis92 commented 4 years ago

@Corniel let's first try to define the shape of this abstract new project. If we have something nice to show, it'll have more chance to gain interest of others.

So let me ask: how would you expand on this simplicity you mentioned? As in, first thing that comes to your mind, what currently complicated thing would you like to see simplified.

Corniel commented 4 years ago

public enum CompilationStep
{
    PreCompile,
    PostComile,
}

[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)]
public abstract class CodeGenerationAttributeAttribute<TGenerator> : Attribute
    where TGenerator : ICodeGenerator
{
    protected CodeGenerationAttributeAttribute(CompilationStep compilationStep);
}

public interface ICodeGenerator
{
    Task<GeneratedCode> GenerateAsync(CodeGenerationContext context);
}

The context should have direct access to the attribute (values) that triggered the code generation, and the GeneratedCode class should be flexible on where to write the generated code to. The current contract of SyntaxList<MemberDeclarationSyntax> is too limiting.

It might be possible to privide hook on points on the build pipeline too, but I think that @AArnott hinted already that in most (90%+ of the) cases, you do not need that.

amis92 commented 4 years ago

I like the idea of GeneratedCode result class, that could open up possibilities.

As per the attribute, we can't implement it like you've shown, because assembly where attribute is defined doesn't need to reference the generator assembly. This'd introduce dependency on the generator DLL, and thus limit consuming code's TargetFramework to be compatible with generator's. That's why current approach is needed (typeof or string).

The attribute is available to generators currently via constructor argument. It could be part of the context, but it's on a cosmetic level, I'd say.

I'm also not sure what CompilationStep would impact.

Corniel commented 4 years ago

@amis92 It is (or could be) cosmetic, but we're talking about simplicity, and this would help getting stuff right. The depedency on the TargetFramework is a big downside.

Uno has GenerateBefore, and GenerateAfter attributes, I assume with a good reason, if not is should be dropped. But as I mentioned before, if they are not on board, we missing a big opportunity.

amis92 commented 4 years ago

Yes I think it would be a nice simplification. (attribute in context instead of in ctor)

Uno's GenerateBefore/After is actually a bit more advanced, actually. It requires multiple compilation generations/tiers. Example: GeneratorA is annotated as GenerateBefore(GeneratorB). Thus, first compilation is done with the non-generated files, GeneratorA is run, and then second compilation with both original and first-generation generated files is created, and GeneratorB is run on that one, effectively allowing generators to consume outputs of other generators. I don't know if you know MSBuild well, but it's a bit like Before/AfterTargets on Target elements, albeit a bit less complicated.

Corniel commented 4 years ago

@amis92 Claiming that i'm an expert on MSBuild would be too far fetched, but have some (actually more than that) experience. I gave it a second taught, and I think that by default generators should be independent of other ones (the default should be simple). If you want that, it should be possible though.

amis92 commented 4 years ago

I mean, I've never needed it, but it can be a future enhancement anyway. I'd not say this should be a part of the MVP we'd start with.

dominikjeske commented 4 years ago

Hello, @amis92 , @Corniel I'm monitoring this project for a while and looking forward for improvements. I think this kind of project could be breaking change in many scenarios. I don't have time to contribute now (maybe in couple of months) but I would like only to point you to this approach https://cezarypiatek.github.io/post/generate-mappings-on-build/. Cezary spotted some drawbacks is project and maybe it would be some interesting for you in on your planning process. For me the most important this to make project more popular apart from architecture is live documentation with all functionality listed and explained – now it is searching in issues discussion and PR’s. Please not give up on this project!

amis92 commented 4 years ago

@bigdnf Thanks for the link. I was aware of Cezary's blog but didn't read that post before.

For brevity, I'll comment on listed changes:

Use Microsoft.CodeAnalysis.MSBuild to load the C# project instead of building compilation unit manually

That's a no-go. We're (contrary to Uno) definitely not following that path. It's a conscious design decision to reuse existing MSBuild pipeline, instead of re-evaluating it all. For @AArnott's explanation, see https://github.com/AArnott/CodeGeneration.Roslyn/issues/100#issuecomment-545737959

Use .NET Core 3.0 features for loading generator plugins (link)

Well, we're using McMaster's package for the same result, I'd consider it done by PR #156 .

Remove caching mechanism which in my opinion was built based on the wrong assumptions (CodeGeneration.Roslyn is only using files that trigger generator as cache dependency, rather than tracking source of all symbols used in generated code)

I'd agree, caching/incremental support is not the best it could be.

Simplify the plugins API

I really don't think it's complicated?

Add parallelism for documents processing

That might be doable but it's not a high priority issue.

Create SDK that simplify the process of plugins development

I'd consider this done as well by PR #156 .

amis92 commented 4 years ago

@bigdnf re: documentation, have you seen wiki? I've "recently" put considerable effort there. Also new Readme should be more helpful.

dominikjeske commented 4 years ago

Thanks @amis92 for brief response. I will not argue about MSBuild because I don’t know internals of the project but I wanted you to be aware of things pointed by author of that blog.

As far as documentation is concern it is better now but I still have a filling not everything is covered (for example info on how to log is in some issue – or maybe I’m not searching properly). I will try to follow docs next time I will be using this library and maybe add something if it will be missing. Generally I prefer to have all the docs on main page but this is me.

Ps. I don’t know it is connected with MSBuild mentioned on the beginning but recently I had some issue but I was not sure it is an Issue or it is not doable and now it came to my mind again. I had a class that was decorated with attribute to generate other class basing on it – I could get method names, parameters and parameters types but when I tried to get type symbol of parameter predecessor (base class) I get null so it looks like I can’t go deep all class hierarchy. If this looks like an issue I can create one if it is not doable please give me some info.

BenjaBobs commented 4 years ago

Hi.

The roslyn repo has a relatively new Source Generators Proposal, different from the original. I'm not sure if any of their thoughts would be useful, but I thought I'd chime in in case you hadn't noticed the new proposal.

/goes back to lurking

AArnott commented 4 years ago

The link to the current source generators proposal is here: https://github.com/dotnet/roslyn/blob/features/source-generators/docs/features/source-generators.md

mwpowellhtx commented 4 years ago

Concerning the Uno discussion, re: Uno, generally I agree. However, the last time I heard anything about pure Roslyn CGs, the talk was about an extremely narrow use case, I think, the NPCs, if you will, NotifyPropertyChangXYZ (Changing, Changed, etc). This is a narrow use case, when what I really need, which motivated forking my own, is to also generate at an assembly level. But more over, to generate CompilationUnits themselves, and drop the namespace discovery issue. That's none of the CG's concern, that is the concern of the generator author where his or her generated assets land.

amis92 commented 4 years ago

last time I heard anything about pure Roslyn CGs

Well, the current proposal linked by @AArnott just above your post has a bit more details; it doesn't have much to do with INPC you mention, although it keeps being additive-only.

In parallel however, the LDT is discussing allowing partial on all members, including those with return type - they would need to be implemented, but it'd be much closer to "replace/original" discussed earlier in that the developer would still control declaration of member (name, parameter names, additional attributes etc.) - and the member body would be provided by a generator.

All very interesting stuff, definitely out of scope for this project here.

BenjaBobs commented 4 years ago

Introducing C# Source Generators

AArnott / CodeGeneration.Roslyn

[RFC] Redesign (UX+extensibility oriented) #162

Terms

UX

New framework design

Plugin discovery

Pipeline building

Pipeline execution

Work plan