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

Replace ImageExtensions.Save.tt with Source Generator #2237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CollinAlpert
Copy link

@CollinAlpert CollinAlpert commented Sep 19, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This PR removes the ImageExtensions.Save T4 template and replaces it with a Source Generator.
The generator (and all future generators) is placed in the ImageSharp.Generators project, since generators need to be in a separate project than the code they generate for.
I have also added <LangVersion>10</LangVersion> to the generator, in order to use features like file-scoped namespaces and such. It is completely safe to use a language version as high as the project which consumes the generator.

Everything still builds and it is still possible to navigate to source for the extension methods the generator replaces. Let me know what you think!

@CLAassistant
Copy link

CLAassistant commented Sep 19, 2022

CLA assistant check
All committers have signed the CLA.

@CollinAlpert
Copy link
Author

I see the build is failing due to the fact that the ImageSharp.Generators project does not have a net6.0 target. Where is this limitation coming from? SGs must target netstandard2.0.

@JimBobSquarePants
Copy link
Member

Urgh......

So we explicitly tell dotnet build which target framework we want to build against during CI. This lets us identify JIT issues which we have an uncanny ability to uncover.

I wonder why the compiler doesn't multi target?

@CollinAlpert
Copy link
Author

I see! I will try to multi target the SG later and see what happens. Should work fine.

@JimBobSquarePants
Copy link
Member

I actually don't think it will because our build process isolates each target within any multi target declarations. Not quite sure what to do. 🤔

@CollinAlpert
Copy link
Author

That's fine, the generator will build and work correctly under .NET 6 as well. It just won't show up in IntelliSense, which doesn't matter for a CI build. This is why it is discouraged to use a higher version than netstandard2.0.

@Sergio0694
Copy link
Member

Sergio0694 commented Sep 20, 2022

I don't understand the point of this PR. T4 templates are better suited for this purpose. Source generators are not a replacement for T4 generators and they're not inherently better, they're just meant for different use cases, and this doesn't look like one.

@CollinAlpert
Copy link
Author

@Sergio0694 This PR was meant as a demonstration for the ImageSharp maintainers so they could evaluate the maintenance costs of abandoning T4 templates.

@Sergio0694
Copy link
Member

Right, but I'm saying there's just no reason to do so at all. T4 templates are the right tool for this. Not to mention, using source generators with a local project reference is unsupported, and has broken tooling. If you clean the repository (eg. git clean -fdx) and open the solution, IntelliSense will be completely broken until you rebuild the source generator and close and reopen VS again. And even without this, using a source generator is just more maintenance effort for literally no gain here.

@antonfirsov
Copy link
Member

antonfirsov commented Sep 20, 2022

We should only merge this if we are absolutely sure this is the way forward and we are ready to migrate our entire T4 logic to source generators, especially the massive usage around PixelFormats:

TBH I was always assuming that we should switch to source generators at some point, since it's "the modern tool" for generating more source code based on existing sources, which is how our use case looks like (or should actually look like): generate chore code for all the pixel types in the repo automatically, generate Load/Save overloads based on existing overloads and/or some other hints etc.

they're just meant for different use cases

@Sergio0694 can you elaborate what do you mean by this considering our use-cases?

Not to mention, using source generators with a local project reference is unsupported, and has broken tooling.

I guess ... it's a show stopper for the adaption then? Though I really believed source generators are in better shape already. How do other projects deal with these problems? Or they don't have them?

@JimBobSquarePants
Copy link
Member

Very curious to hear more expert opinion here. This is all very new to me!

@Sergio0694
Copy link
Member

"can you elaborate what do you mean by this considering our use-cases?"

The advantage of source generators is that they can dynamically generate code based on what code you're using in your own project, but that really only makes sense when there's some kind of introspection being done on said code from the generator end. Say, you're using System.Text.Json, and the generator is inspecting your data models to generate serialization code for you based on those models. In your case instead, there's no introspection being done, and everything is just done in a generator initialization step. That is literally just a T4 template but with extra steps, and it's not really recommended because it just adds way more complexity for no benefit. Not to mention that, currently, there's really no proper tooling support for locally referenced generators, so working on them is a bit of a pain because IntelliSense is pretty much always broken.

The point is: source generators are not meant to replace T4 templates, they're just meant for different scenarios. If you need to generate some fixed templated code, then a T4 template is a perfectly reasonable, and much simpler, choice. Eg. I'm heavily using T4 templates in ComputeSharp too to generate all the HLSL primitive projection types for C#, not generators.

In your case, eg. in those pixel operations files that Anton linked, that setup looks perfectly reasonable. If you wanted to make it more compact, you could just use an array of all pixel format types, and generate all the partial types in a single file 🙂

@antonfirsov
Copy link
Member

antonfirsov commented Sep 27, 2022

In your case instead, there's no introspection being done

This is how we implemented stuff with T4, but in theory it would be more robust if we could enumerate & inspect the existing pixel types, and existing Image API-s in the generator instead -- at least this is how I imagined it before @Sergio0694 came to ruin my naïve world 😆

If I get the whole picture right, you basically say: use Source Generator if you really need a reusable tool to generate more code based on existing code in a flexible manner. Use T4 if you want to generate code for a single project you control, otherwise you'll just end up adding even more complexity and maintenance burden.

I think we should stick with T4 then? Or are any counter-opinions in the community which are based on experience with bigger projects?

@CollinAlpert
Copy link
Author

Yeah, I definitely agree that SGs are meant for scenarios where more of the code is actually analyzed/introspected to generate the result. I was just under the impression that the tooling around T4 templates (across all operating systems and IDEs) is also not great and that this would cancel out the Source Generators' shortcomings.

@Sergio0694
Copy link
Member

"Use T4 if you want to generate code for a single project you control, otherwise you'll just end up adding even more complexity and maintenance burden."

I would say that in some cases it might make sense to still use a source generator for internal use, as long as the benefit is worth the extra work, eg. if you have lots of different uses on random types where the introspection makes sense due to the high variety of scenarios that are supported, but even then it's something you should think about on a case by case basis.

One rule of thumb I can definitely give you though is: if your generator is only using RegisterPostInitializationOutput, then that's really just T4 with extra steps 😄


// <auto-generated />";

private static readonly string[] ImageFormats =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have some sort of attribute on the encoder type that can be used to generate the methods instead?

That would make it more generator worthy would it not @Sergio0694 ?

It would be amazing if we could add that to a base class (ImageEncoder See #2269) and have these methods generated automatically without us ever having to touch this file again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this would be possible and probably a better suited scenario for a Source Generator. You'd still have the issues around local projects with SGs which @Sergio0694 mentioned in #2237 (comment) and would need to decide if this is okay for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to mention, using source generators with a local project reference is unsupported, and has broken tooling

This one? I'm still blown away by that. How does someone test such a thing if locally referencing the project is unsupported? It seems incredibly short sighted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to be honest I haven't noticed this as much. I rarely clean my repository like that.
I have multiple SGs in a local project at work and only get red squiggly lines when modifying the SG code, which rarely happens. Even then closing and reopening the solution fixes it. Other than that, all is good.

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

Successfully merging this pull request may close these issues.

5 participants