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

Features/source generators #40162

Merged

Conversation

chsienki
Copy link
Contributor

@chsienki chsienki commented Dec 5, 2019

Add source generators working doc

@chsienki
Copy link
Contributor Author

@chsienki chsienki commented Dec 5, 2019

the compilation before running, as well as access any _additional files_, enabling generators to
introspect both user C# code and generator specific files.

> **Note**: This proposal is separate from the [previous generator design](generators.md)
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

💡 It would be good to also update the other page to mention its relation to this one.

@@ -0,0 +1,285 @@
# Source Generators


Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

Do we have a way to indicate at the start that this is just a proposal, and not actually a feature that is already implemented?

Copy link
Member

@jasonmalinowski jasonmalinowski Dec 11, 2019

Choose a reason for hiding this comment

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

It's in the feature branch, I think that implies it?

Copy link
Member

@jaredpar jaredpar Dec 11, 2019

Choose a reason for hiding this comment

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

When @sharwell initially made this comment the PR was targeting master. @chsienki then changed to target the feature branch, partially in response to this. That's why the comment seems confusing.

Copy link
Member

@jaredpar jaredpar Dec 11, 2019

Choose a reason for hiding this comment

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

At this point though it should be clear this isn't a shipped feature though given the entirety of the documentation is in a feature branch 😄

### High Level Design Goals

- Generators produce one or more strings that represent C# source code to be added to the compilation.
- Explicitly _additional_ only. Generators can add new code to a compilation but may **not** modify existing user code.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

Suggested change
- Explicitly _additional_ only. Generators can add new code to a compilation but may **not** modify existing user code.
- Explicitly _additive_ only. Generators can add new code to a compilation but may **not** modify existing user code.
``` #Resolved

- Generators produce one or more strings that represent C# source code to be added to the compilation.
- Explicitly _additional_ only. Generators can add new code to a compilation but may **not** modify existing user code.
- May access _additional files_, that is, non-C# source texts.
- Run _un-ordered_, each generator will see the same input compilation.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

Suggested change
- Run _un-ordered_, each generator will see the same input compilation.
- Run _un-ordered_, each generator will see the original input compilation, without access to files which may have been generated by other source generators run previously.
``` #Resolved

- Generators produce one or more strings that represent C# source code to be added to the compilation.
- Explicitly _additional_ only. Generators can add new code to a compilation but may **not** modify existing user code.
- May access _additional files_, that is, non-C# source texts.
- Run _un-ordered_, each generator will see the same input compilation.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

💡 Consider mentioning that source generators run before analyzers, and analyzers will be able to analyze code generated by the source generator.

We discussed embedding this directly in the source text via `#pragma` but
this would require language changes and limit the feature to a specific version
of C#. Other considerations could be specially formed comments or `#if FALSE --`
blocks. In general a 'side-channel' approach seems preferable to specially crafted
grammar in the generated text.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

This block is unexpectedly indented #Resolved

in the original document this was generated from. This would allow the
compiler API to track e.g. a generated `Symbol` as having an `OriginalDefinition`
that represents a span of third party source text (such as a Razor tag in a
`.cshtml` file).
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

Suggested change
`.cshtml` file).
`.cshtml` file).
``` #Resolved

It is expected that generators will need some form of configuration system, and we intend to allow
certain properties to flow through from MSBuild to facilitate this.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

💡 It may be sufficient to pass in the .editorconfig representation from the compiler

Ultimately, the performance of the feature is going to be somewhat dependent on the performance of the
generators written by customers. Progressive opt-in, and build-time only by-default will allow the IDE
to mitigate many of the potential performance problems posed by third party generators. However, there
is still a risk that third-party generators will cause unacceptable performance problems for the IDE,
and the design of the feature will need to keep this in mind.
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

This feature should be implemented under the assumption that third-party source generators have taken all reasonable measures to ensure their generators perform well when invoked. It is not acceptable to mitigate a non-existent problem by forcibly degrading the developer experience for someone using third-party generators, so this should be rephrased to simply say that third-party generator authors are responsible for ensuring the generators perform well. With that in mind:

  • We need to be able to identify cases where a third-party generator is actively causing performance problems (so users don't blame us for something we didn't cause).
  • We need to minimize the overhead of the source generator driver itself to ensure that third-party generator authors have the ability to address performance issues that arise

Choose a reason for hiding this comment

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

In many cases, the performance of the generator is very tricky to define, as it is extremely dependent on the number of files to process (think XAML to cs generation for instance). Being able to view very clearly which generator is taking up the generation time is a must have.

Good behavior for generators, such as avoiding .ToDisplayString() to perform symbols lookups and comparisons, should be documented as best practices.

- Resx file generation
- [System.CommandLine](https://github.com/dotnet/command-line-api)
- Serializers
- [SWIG](http://www.swig.org/)
Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

💡 Compiler compilers, such as ANTLR

💡 Language conversion, such as generation of a CUDA or OpenCL program as a string from a managed function

Copy link
Member

@jkoritzinsky jkoritzinsky Dec 7, 2019

Choose a reason for hiding this comment

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

Another example is the SharpGenTools project at https://github.com/SharpGenTools/SharpGenTools.

{
public interface ISourceGenerator
{
void Execute(SourceGeneratorContext context);
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 5, 2019

Choose a reason for hiding this comment

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

synchronous? non-cancellable? how does that design impact compilations?

Copy link
Member

@sharwell sharwell Dec 5, 2019

Choose a reason for hiding this comment

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

synchronous?

This code doesn't read from disk (files were already read) or write to disk (compiler writes content to disk if necessary), so it's synchronous like analyzers are synchronous.

non-cancellable?

context.CancellationToken

Choose a reason for hiding this comment

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

Generators very frequently need to read from the disk, particularly if their input is a mix of the semantic model and external files. Allowing for async may help for performance ?

Copy link
Member

@jaredpar jaredpar Dec 9, 2019

Choose a reason for hiding this comment

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

@jeromelaban

Generators that read from disk in the context of source generators are an anti-pattern that must be avoided. To properly fit into our build model all inputs to the compiler, and by consequence generators, must be visible to MSBuild. If they're not then changes to those files will not result in compilation being re-done but rather MSBuild considering the compilation up to date and hence skipping it entirely.

Source generators that want to read non-source files must do so through the /additionalFiles options. This is the same approach that we take for analyzers.

Choose a reason for hiding this comment

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

@jaredpar Thanks! I do not understand the rationale behind the anti-pattern. Reading files from the disk and performing up-to-date checks are two separate tasks. Could you explain a bit more about it?

The additional files option is also very limiting in its current form. While it provides access to the files content, it removes access to metadata provided by msbuild (most notably Link or others like nuget source) which can be very useful in some situations. AdditionalText also a synchronous API, which may force generators to spawn tasks to read content when there are many files (think many hundreds) and do parallel work when needed.

Reading through additional files doc, it does have the ability to tie a specific set of input files to a specific generator, which will force to rerun all generators. For a first version, it may be good enough, or could be done through msbuild metadata.

One information that could prove very useful for additional files is the diff with a previous run with the same compilation instance. Generators could use it to partially generate source or content, or reuse previously generated content. Diffing of compilations may not be useful at all, as determining what's changed could be challenging.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 10, 2019

Choose a reason for hiding this comment

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

One information that could prove very useful for additional files is the diff with a previous run with the same compilation instance.

That would require state tracking, which could easily lead to broken behavior.

also a synchronous API, which may force generators to spawn tasks to read content when there are many files (think many hundreds) and do parallel work when needed.

This is precisely the sort of perf cliff that terrifies me. VS performance has already nose-dived in recent releases for me, and we basically had to stop using analyzers over here because of the massive cost they added to a build. This just seems to be exacerbating the issue.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 10, 2019

Choose a reason for hiding this comment

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

Thanks! I do not understand the rationale behind the anti-pattern. Reading files from the disk and performing up-to-date checks are two separate tasks. Could you explain a bit more about it?

If you need to read in contents of a file, then you need to specify that file as a dependency. Otherwise you simply will not be run if that file does not change. Adding that file as a dependency can be done with .AdditionalFiles. This informs the entire system about this dependency so that it can properly be used when computing what actually needs to run/update during a build.

Copy link

@jeromelaban jeromelaban Dec 10, 2019

Choose a reason for hiding this comment

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

This is precisely the sort of perf cliff that terrifies me. VS performance has already nose-dived in recent releases for me, and we basically had to stop using analyzers over here because of the massive cost they added to a build. This just seems to be exacerbating the issue.

Performance is definitely a concern, and having generators running at every keystroke is scenario that most likely won't scale. For instance, it is frequent that some complex generators can take 15+ seconds to run, though part of it is just creating the initial Compilation that roslyn already has.

Running generators at build time, then refresh intellisense afterwards is a scenario that can be reasoned with pretty easily (this is what Uno.SourceGeneration does at this point), once explained.

Copy link
Member

@jaredpar jaredpar Dec 11, 2019

Choose a reason for hiding this comment

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

Reading files from the disk and performing up-to-date checks are two separate tasks. Could you explain a bit more about it?

The way MSBuild evaluates up to date checks is to essentially analyze the inputs to Targets, check to see if they're unchanged and if so not run the target. That means for the compiler to properly participate in up to date checks all of the files used by the compiler must be listed in the Targets. This applies to both the core compiler and any analyzer or source generator plugin.

The /additionalFiles parameter is how we allow analyzers, and now source generators, participate into this process.

Further an at API level the compiler must be able to represent an immutable snapshot of a compilation. This is a core part of our model and that includes removing the state of the file system from the equation. Hence all file inputs to the compiler end up getting represented as SourceText instances including the input to /additionalFiles.

If source generators, or analyzers, directly read from disk they're violating at least the immutable snapshot property of the model and / or the MSBuild targets model.

this would require language changes and limit the feature to a specific version
of C#. Other considerations could be specially formed comments or `#if FALSE --`
blocks. In general a 'side-channel' approach seems preferable to specially crafted
grammar in the generated text.
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 5, 2019

Choose a reason for hiding this comment

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

this actually seems less preferable. This will be "another" way to convey this information. Why can't we use the existing mechanisms that already exists anad are baked in throughout roslyn for this?

Copy link
Contributor Author

@chsienki chsienki Dec 6, 2019

Choose a reason for hiding this comment

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

How do you mean?

We don't currently have a way of providing that information to the compiler from the generator which is what this proposes. We'd use the existing roslyn APIs for retrieving that information.


In reply to: 354540456 [](ancestors = 354540456)

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 7, 2019

Choose a reason for hiding this comment

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

I was responding to this:

The current thinking is to have some form of 'side-channel' available to
the generator. As the generator emits source text, it would indicate where
in the original document this was generated from.

Don't we have that already with #line and whatnot? We already use that in Razor today, and have built hte tooling around using this information to tie things together. My point is that it seems bad that we'd now have to support this old way (unless we're willing to rip it out entirely) and whatever new way we're discussing to duplicate this functionality.

Copy link
Contributor Author

@chsienki chsienki Dec 11, 2019

Choose a reason for hiding this comment

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

We don't believe #line isn't high enough definition to be able to convey the information with the level of precision we need (razor has tried this before). As mentioned we don't want to bake this into the language itself as that ties our hands to a specific language version.


In reply to: 355146903 [](ancestors = 355146903)

- [gRPC](https://docs.microsoft.com/en-us/aspnet/core/grpc/?view=aspnetcore-3.1)
- Resx file generation
- [System.CommandLine](https://github.com/dotnet/command-line-api)
- Serializers
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi Dec 5, 2019

Choose a reason for hiding this comment

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

how do these interact? it was stated above that there is no ordering, but it seems like it will be hard to remove that. i.e. how do 'serializers' and 'blazor/razor' interact? will one never be dependent on hte other? #Resolved

Copy link
Contributor Author

@chsienki chsienki Dec 6, 2019

Choose a reason for hiding this comment

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

That's the plan. Here serializers are really aimed at adding potential compile time specialization for generation. Razor/Blazor is for transforming cshtml. Razor/Blazor does use serialization but would just be faster if the generator for the serializer it's using is present.


In reply to: 354541360 [](ancestors = 354541360)


**Security Review**:

Do generators create any new security risks not already posed via analyzers and nuget?
Copy link
Contributor

@gafter gafter Dec 6, 2019

Choose a reason for hiding this comment

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

Certainly they do introduce new security risks. See my "insert a trojan horse virus into the program at compile-time" generator. It is open-source and as you can see by examining its source it does nothing nefarious. See https://www.cs.cmu.edu/~rdriley/487/papers/Thompson_1984_ReflectionsonTrustingTrust.pdf for its design document. #Resolved

Copy link
Member

@jaredpar jaredpar Dec 6, 2019

Choose a reason for hiding this comment

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

How is this a new security risk though? Or more accurately how is the security risk of a source generator any different than that of analyzers? A misbehaving analyzer could use reflection tricks to do virtually anything a source generator could do. Certainly a bad actor wouldn't baulk at using reflection just because it wasn't supported.

Copy link
Contributor Author

@chsienki chsienki Dec 6, 2019

Choose a reason for hiding this comment

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

As far as we understand, there is nothing a generator can do that an analyzer couldn't have achieved previously via private reflection. Ultimately we've been allowing arbitrary code execution from packages during compilation for some time (especially via nuget), we're just making it explicit now. This is a note that we need to chat with the security folks to ensure our assumptions are correct.


In reply to: 354613044 [](ancestors = 354613044)

Copy link
Member

@jaredpar jaredpar Dec 6, 2019

Choose a reason for hiding this comment

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

Analyzers come via package references and package references can do basically anything. They can install props, targets, random non-.NET binaries, play the he-man song on repeat, etc ... Even in the case we assume they're checked (they're not) analyzers are code running with full trust on the machine.

Copy link
Member

@jaredpar jaredpar Dec 9, 2019

Choose a reason for hiding this comment

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

For context, this is the he-man song: http://aka.ms/davkean


- ASP.Net: Improve startup time
- Blazor and Razor: Massively reduce tooling burden
- Azure Functions: regex compilation during startup
Copy link
Contributor

@pakrym pakrym Dec 6, 2019

Choose a reason for hiding this comment

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

We (Azure SDK) team would like to experiment with this feature too.

@mgravell
Copy link

@mgravell mgravell commented Dec 7, 2019

Question/observation re SupportedAdditionalFileExtensions - the example given of gRPC makes me observe that file extensions, even for the same format, are not unique / uncontested. To be specific re gRPC - presumably this is talking about .proto schema codegen. There are multiple handlers for ".proto" protobuf schemas, so the design would ideally consider this possibility. It may be necessary to consider what happens if multiple handlers report the same extension.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

comment

**Should we share more with the analyzer type hierarchy?**:

We would still need to differentiate analyzers from generators, since
they would be generated at different times (generator diagnostics only on
Copy link
Member

@agocke agocke Dec 8, 2019

Choose a reason for hiding this comment

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

Suggested change
they would be generated at different times (generator diagnostics only on
they would be run at different times (generator diagnostics only on
``` #Resolved

to produce events for as many nodes as possible, even with errors, as generators
will often work in the presence

- The events that we use today for analyzers may require may more work to produce,
Copy link
Member

@agocke agocke Dec 8, 2019

Choose a reason for hiding this comment

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

By "more work", specifically, doing the minimal amount of work needed to produce an event, i.e. creating a Compilation should not need to do any binding.


To support the use case where a user wishes to generate the source text, then commit
the generated files to source control, we will allow changing the location of the
generated files via an appropriate command line switch, and matching MSBuild property
Copy link
Member

@agocke agocke Dec 8, 2019

Choose a reason for hiding this comment

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

Something to consider: if we can figure out the output names of the generator, and have a list of output items with the same name, we can decide whether or not they are up-to-date based on file stamps.

We might want to try to get this in V1, since it may be difficult to add after-the-fact. #Resolved

Copy link
Contributor Author

@chsienki chsienki Dec 11, 2019

Choose a reason for hiding this comment

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

Added to the doc to track


In reply to: 355151636 [](ancestors = 355151636)

@YairHalberstadt
Copy link
Contributor

@YairHalberstadt YairHalberstadt commented Dec 8, 2019

This looks like a very interesting proposal, and will certainly be valuable for many use cases.

I think one of the commonly requested use cases for source generators is generating calls to NotifyPropertyChanged from an auto property, without having to create a full property, via usage of an attribute.

Given that this proposal only allows generating new files, not modifying existing files, it looks like such use cases are for now out of reach.

Is there any long term path to increasing the power of source generators to make this possible, or is that not on the horizon?

@GSPP
Copy link

@GSPP GSPP commented Dec 8, 2019

In what process will the generators be run? In devenv.exe? csc.exe? That seems very brittle. There could be global state corruption, resource over-usage, conflicts in the runtime and libraries required.

Copy link

@jeromelaban jeromelaban left a comment

Very nice to see this feature making a simple comeback :)

By default, a generator implementing only `ISourceGenerator` would see no IDE integration
and only be correct at build time. Based on conversations with 1st party customers,
there are several cases where this would be enough.

Choose a reason for hiding this comment

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

@sharwell in most scenarios I've been able to deal with, actual generation time is generally insignificant, and the determination of the API to generate takes up most of the time for larger generators.

copies of the generated source texts (for 'Find all references', breakpoints etc.) and
periodically flush any changes to disk.

To support the use case where a user wishes to generate the source text, then commit

Choose a reason for hiding this comment

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

This kind of scenario is very problematic, as the user generally does not control when the generators are run (when run from the IDE). This may create many sorts of "my modifications went away" scenarios that frustrate users. Having a non-negotiable scenario of "files are always re-generated on build" is easier to reason with.


### MSBuild Integration

It is expected that generators will need some form of configuration system, and we intend to allow

Choose a reason for hiding this comment

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

This feature is critical. In a general case, a IDictionary<string, IList<(string, IDictionary<string, string>)> (for ItemGroups and their metadata) and IDictionary<string, IDictionary<string, string>> (for properties and metadata) would be the best way to propagate this information in such a way that it would not require generators to forcibly ship with an msbuild props file that defines which properties they are interested in.

This is also the way to get the proper way input files from msbuild (and not just rely on some arbitrary enumeration on files and folders based on the project's location, having to skip bin/obj files properly in user code).

Copy link
Member

@jaredpar jaredpar Dec 9, 2019

Choose a reason for hiding this comment

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

@jeromelaban

Part of the issue here is the sheer amount of data that gets stored in MSBuild properties and item metadata. Forcing all of it into the compilation by default would decrease performance as well as impacting our ability to avoid unnecessary rebuilds as literally everything in MSBuild would be considered an input to the compilation.

Instead we've chosen this approach where analyzers / generators can pragmatically determine the item metadata and properties that are interesting to them.

Ultimately, the performance of the feature is going to be somewhat dependent on the performance of the
generators written by customers. Progressive opt-in, and build-time only by-default will allow the IDE
to mitigate many of the potential performance problems posed by third party generators. However, there
is still a risk that third-party generators will cause unacceptable performance problems for the IDE,
and the design of the feature will need to keep this in mind.

Choose a reason for hiding this comment

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

In many cases, the performance of the generator is very tricky to define, as it is extremely dependent on the number of files to process (think XAML to cs generation for instance). Being able to view very clearly which generator is taking up the generation time is a must have.

Good behavior for generators, such as avoiding .ToDisplayString() to perform symbols lookups and comparisons, should be documented as best practices.


### Language Changes

This design does not currently propose altering the language, it is purely a compiler feature.

Choose a reason for hiding this comment

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

👏 I agree, even if it would have been a very nice feature to have. The ramifications of such changes are very complex to reason with.


**IDependsOnCompilationGenerator**:

We did discuss if there should be an IDependsOnCompilationGenerator to formally state

Choose a reason for hiding this comment

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

Should this be an attribute ?

- [SWIG](http://www.swig.org/)


## Discussion / Open Issues / TODOs:
Copy link

@jeromelaban jeromelaban Dec 8, 2019

Choose a reason for hiding this comment

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

The high level design goals mentions that ordered execution is out of scope. A DependsOn / Before / After model is very important as generators very frequently need to depend on the generated code of other generators. If that may help, here's how we implemented it in Uno.SourceGeneration, it's using literal strings to enable that as we haven't found a better way to do that.

{
public interface ISourceGenerator
{
void Execute(SourceGeneratorContext context);

Choose a reason for hiding this comment

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

Generators very frequently need to read from the disk, particularly if their input is a mix of the semantic model and external files. Allowing for async may help for performance ?

@Suchiman
Copy link
Contributor

@Suchiman Suchiman commented Dec 8, 2019

If concrete scenarios are later shown that can’t be achieved with the V1 approach we would consider allowing modification as a V2.

As @YairHalberstadt already said, i think one of the most common scenario's for this feature i've heard (IRC and gitter) are to support automatic INotifyPropertyChanged implementation (since having builtin support for that keeps getting declined). Without replace and original this falls flat, again. It looks like we shall be punished into all eternity to write INPC manually.

@jeromelaban
Copy link

@jeromelaban jeromelaban commented Dec 8, 2019

If concrete scenarios are later shown that can’t be achieved with the V1 approach we would consider allowing modification as a V2.

As @YairHalberstadt already said, i think one of the most common scenario's for this feature i've heard (IRC and gitter) are to support automatic INotifyPropertyChanged implementation (since having builtin support for that keeps getting declined). Without replace and original this falls flat, again. It looks like we shall be punished into all eternity to write INPC manually.

It can still be implemented, but in a different way, e.g.:

partial class M
{
  [Property(NotifyChanged: true)]
  private string _myProperty;

  partial void OnMyPropertyChanged(string previousValue, string newValue)
  {
  }
}

Which generates:

partial class M : INotifyPropertyChanged
{
  public string MyProperty
  {
    get => _myProperty;
    set
    {
      var previous = _myProperty;
      _myProperty = value;
      PropertyChanged?.Invoke(new PropertyChangedEventArgs(nameof(MyProperty)));
      OnMyPropertyChanged(previous, value);
    }

    partial void OnPropertyChanged(string previousValue, string newValue);
}

Adding the property auto adds INotifyPropertyChanged, and the required properties implementations. The only requirement is to have a partial modifier on the class (for which I'd love to see partial be the default, it'd help so much with those generation scenarios without any replace keyword).

Additional scenarios could add equality checks, etc...

> **Note**: This is still under design and open to change.

### Performance targets
Copy link
Contributor

@cartermp cartermp Dec 9, 2019

Choose a reason for hiding this comment

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

Should we consider out of process from the start given the nature of generating very very large sources, even if the generator was written well?

Copy link
Member

@jaredpar jaredpar Dec 9, 2019

Choose a reason for hiding this comment

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

For IDE we're looking at a minimal bring up scenario to allow the compiler work to proceed. It's not intended to be the final shipping solution. This reduced their cost significantly and unblocked our ability to move the compiler forward. Expectation is that for shipping we'll move it out of proc just as we do for analyzers.

Copy link
Contributor

@cartermp cartermp Dec 9, 2019

Choose a reason for hiding this comment

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

Much yeet.

@orthoxerox
Copy link
Contributor

@orthoxerox orthoxerox commented Dec 9, 2019

Are convenience features explicitly out of scope? That is, do you expect generator writers to use third-party libraries to implement things like "invoke this method for every type declaration adorned with FooAttribute" at least at the MVP stage?

@jaredpar
Copy link
Member

@jaredpar jaredpar commented Dec 9, 2019

@GSPP

In what process will the generators be run? In devenv.exe? csc.exe?

The primary use case will be running the generators inside the compilation process which is generally VBCSCompiler.exe. The IDE though will end up running them in their out of process containers.

That seems very brittle. There could be global state corruption, resource over-usage, conflicts in the runtime and libraries required.

In what way are source generators different from analyzers in this respect? True all of these results can happen from running code in process but analyzers have the same problems and so far we've been able to sufficiently mitigate the issues.

@chsienki chsienki changed the base branch from master to features/source-generators Dec 9, 2019
@chsienki
Copy link
Contributor Author

@chsienki chsienki commented Dec 11, 2019

@mgravell: We're not really sure what these extension points are going to look like yet, and this is mainly just a placeholder. In terms of this particular design, just because a generator gets informed of changes for a given extension, it doesn't have to provide any source for it. Given two handlers for ".proto" one might look at the file and decide not to generate anything, while the other does. There are cases where both might want to generate something, but solving that doesn't really depend on the triggering mechanism. Currently the MVP model just assumes you'd globally disable one generator, but it's an interesting question to think about the granularity of it (I'll add it in the follow ups).


In reply to: 562892606 [](ancestors = 562892606)

@chsienki
Copy link
Contributor Author

@chsienki chsienki commented Dec 11, 2019

@orthoxerox

Are convenience features explicitly out of scope? That is, do you expect generator writers to use third-party libraries to implement things like "invoke this method for every type declaration adorned with FooAttribute" at least at the MVP stage?

For the MVP stage design, yes. However we're also investigating a 'push' model where generators can react to events, similar to how analyzers work today.

agocke
agocke approved these changes Dec 11, 2019
@chsienki chsienki merged commit 993d498 into dotnet:features/source-generators Dec 12, 2019
15 of 18 checks passed
@Ciantic
Copy link

@Ciantic Ciantic commented Jan 22, 2020

Can we have an GitHub issue to follow the work? I'm eager to see how far this effort is, what is the timeframe here. It's has been mentioned several times (see 1, 2) in ASP.NET community stand up recently, but no easy way to follow the effort.

Rust beats the C# in the code clarity (sometimes) with code generation features. I wait patiently to someone implement similar things as Rust's #[derive] for C#. This could make writing immutable data types much easier for C#.

@jeromelaban
Copy link

@jeromelaban jeromelaban commented Jan 22, 2020

@Ciantic the rust feature looks similar to this: https://github.com/unoplatform/Uno.CodeGen#uno-codegen, where [GenerateEquality] or [GenerateImmutable] can be used.

@Ciantic
Copy link

@Ciantic Ciantic commented Jan 22, 2020

There is no shortage of code generation for C#, but having an official API for it will make it more attractive in longer term, right now it's wild west out there.

P.S. I just want to subscribe to an issue so I can follow the effort, merits of the code generation is well known by the people in this merge.

@jeromelaban
Copy link

@jeromelaban jeromelaban commented Jan 22, 2020

@Ciantic fair, though the implementation is very close the specifications mentioned here (if it does not change too much). The only part that may not be available in Roslyn is dependent generators.

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

Successfully merging this pull request may close these issues.

None yet