Add incremental generator specification #52847
Conversation
|
@dotnet/roslyn-compiler for review please |
| the generator host, the performance benefits are only really seen when the | ||
| driver can cache the outputs from one pipeline step to the next. While we have | ||
| generally said that the execute method in an `ISourceGenerator` should be | ||
| deterministic, incremental generators actively _require_ this property to be |
There was a problem hiding this comment.
What failure modes do you imagine will occur when developers get this wrong? Should there be a "debug/checked" mode of some kind that incremental generators can run in so that issues around nondeterminism in user code can be caught more easily?
| ```csharp | ||
| public static partial class IncrementalValueSourceExtensions | ||
| { | ||
| // join 1 => many ((source1[0], source2), (source1[0], source2), ...) |
There was a problem hiding this comment.
this comment doesn't look right. did you mean source1[1]? Also, you should should show after the ... that we do source2[0]...source2[1] etc. Right now this comment just feels confusing and misleading.
| item of data from `source1` combined with the entire batch of data from | ||
| `source2`. While this is somewhat low-level, when combined with subsequent | ||
| transforms, it gives the author the ability to combine an arbitrary number of | ||
| data sources into any shape they require. |
There was a problem hiding this comment.
oh. maybe hte comment is correct. this is... interesting...
|
|
||
| ```csharp | ||
| IncrementalValueSource<(AdditionalText, Compilation)> transformed = combined.Transform(pair => (pair.Item1, pair.Item2.Single())); | ||
| ``` |
There was a problem hiding this comment.
the .Single here is super sucky. I wonder if we can have a better Combine that reflects this.
There was a problem hiding this comment.
Yeah, that was the original reasoning for the Single/MultiValueSource stuff, you got the 'single' for free via the type system. We could have a CombineWithSingle() that just throws at runtime if the second argument isn't a single thing.
| ```csharp | ||
| IncrementalValueSource<(AdditionalText, MetadataReference)> combined = context.Sources.AdditionalTexts | ||
| .Combine(context.Sources.MetadataReference) | ||
| .TransformMany(pair => pair.Item2.Select(item2 => (pair.Item1, item2)); |
There was a problem hiding this comment.
fwiw, this became compeltely impossible for me to reason about. :) It might be worthwhile to hide behind a helper. So .AdditionalTexts.CrossJoin(context.Sources.MetadataReferences).
| **OPEN QUESTION**: Combine is pretty low level, but does allow you to do | ||
| everything needed when used in conjunction with transform. Should we provide | ||
| some higher level extension methods that chain together combine and transform | ||
| out of the box for common operations? |
| static partial class IncrementalValueSourceExtensions | ||
| { | ||
| // helper for filtering values | ||
| public static IncrementalValueSource<T> Filter<T>(this IncrementalValueSource<T> source, Func<T, bool> filter) => ... |
There was a problem hiding this comment.
we should probablyjust call this 'Where'.
| ### WithComparer | ||
|
|
||
| For a user provided result to be comparable across iterations, there needs to be some concept of equivalence. Rather than requiring types returned from transformations implement `IEquatable<T>`, there exists an extension method that allows the author to supply a comparer that should be used when comparing values for the given transformation: | ||
|
|
There was a problem hiding this comment.
do we specify what semantics you get without this? is it EqualityComparer.Default?
| .WithComparer(myComparer); | ||
| ``` | ||
|
|
||
| Note that the comparer is on a per-transformation basis, meaning an author can specify different comparers for different parts of the pipeline. |
There was a problem hiding this comment.
for our combinators (like cross join and whatnot) do we grab the comparers from the underlying sources to build the default one for the combined tansform?
|
|
||
| // register the output context.RegisterOutput(output); }); } } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Think you need an example which shows usage of a SemanticModel and SourceText here. That is one of the most common forms of generators, basically looking for an attribute trigger. Customers going from V1 to V2 are going to want to see that type of example that they can look to for guidance.
There was a problem hiding this comment.
Yeah thats the 'syntax nodes' bit down below. The problem is I'm still trying to figure out exactly what that actually looks like. We have some ideas, but still trying to find the final shape for it :/
There was a problem hiding this comment.
Just read through this and I don't have the context so I might have skipped something, but this is the biggest question for me as well.
Basically, I need to know
- Was this type annotated with this particular attribute
- Did any fields/properties change in the type
(2) may be easy to do via a simple overly-aggressive "is this text the same", (1) I might be able to cheat and just ask if the name of the attribute is the same, but theoretically the binding for the attribute could be different. Am I supposed to care? And if I do grab the symbol and ask if two symbols are the same, presumably that's not valid because you can't compare symbols from two compilations. So what do I do? Ask if they're the same fully-qualified name?
It would be good to spell this out (especially the part about not being able to compare symbols).
There was a problem hiding this comment.
This is also my main question. Without the SemanticModel being part of the input (and ideally portions of it cached), I'm not sure this would give us much savings in our use of generators.
Ideally, this would work as follows:
- Even if input syntax does not change, but one of the dependent symbols looked-up did, the pipeline would re-execute.
- If input syntax does not change, and all dependent symbols stay the same; the previous output would be re-used.
| @@ -0,0 +1,478 @@ | |||
| # Incremental Generators | |||
|
|
|||
| ## Summary | |||
There was a problem hiding this comment.
Potential additions or follow-ups:
- when should I use an incremental generator vs. a plain source generator? (I assume the answer is the incremental is recommended)
- how do I convert my existing source generator to an incremental generator?
| } | ||
| ``` | ||
|
|
||
| This pipeline is not directly executed, but instead consists of a set of steps |
| // apply a 1-to-1 transform on each text, which represents extracting the path | ||
| IncrementalValueSource<string> transformed = additionalTexts.Transform(static text => text.Path); | ||
|
|
||
| }); |
There was a problem hiding this comment.
nit: consider adding an ellipsis on line after declaration of transformed
| public static partial class IncrementalValueSourceExtensions | ||
| { | ||
| // 1 => many (or none) | ||
| public static IncrementalValueSource<U> TransformMany<T, U>(this IncrementalValueSource<T> source, Func<T, IEnumerable<U>> func) => ... |
There was a problem hiding this comment.
nit: in the examples, consider using TInput (or TSource) and TResult for type parameters
| pipeline, but an output cannot be further transformed once it is created. | ||
|
|
||
| Splitting the outputs by type produced allows the host (such as the IDE) to not | ||
| run outputs that are not required. For instance artifacts are only produced as |
| ```csharp | ||
| IValueSource<string> transform = context.Sources.AdditionalTexts | ||
| .Transform(static t => t.Path) | ||
| .Transform(static p => "prefix_" + p); |
There was a problem hiding this comment.
nit: although I understand this is for illustration purpose, I'm a bit nervous about using such trivial steps and encouraging caching trivial computations.
For follow-up: we'll want to provide some guidance on granularity of steps.
| ## Summary | ||
|
|
||
| Incremental generators are intended to be a new API that exists alongside | ||
| [source generators](generators.md) to allow users to specify generation |
There was a problem hiding this comment.
| [source generators](generators.md) to allow users to specify generation | |
| [source generators](source-generators.md) to allow users to specify generation |
Not finished yet, but looking to get feedback on what is there so far.