New generator apis #74127

chsienki · 2024-06-24T17:42:25Z

Adds APIs to convert ISourceGenerators to IIncrementalGenerators and allows the generator driver to only update a sub-set of generators via a new filter parameter.

Fixes #74039
Fixes #74086

chsienki · 2024-06-24T17:43:50Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

+        /// <param name="generatorFilter">An optional filter that specifies which generators to run. If <c>null</c> all generators will run.</param>
+        /// <param name="cancellationToken">Used to cancel an in progress operation.</param>
+        /// <returns>An updated driver that contains the results of the generators running.</returns>
+        public GeneratorDriver RunGenerators(Compilation compilation, Func<GeneratorFilterContext, bool>? generatorFilter = null, CancellationToken cancellationToken = default)


I couldn't figure out a way to make generatorFilter non-optional (as discussed in the API review) without breaking our back compat guidelines.

Another option would be to just make this a differently named method, like RunGeneratorsWithFilter

I don't see the problem - what guidelines would be broken?

Our analyzer requires that we don't have multiple overloads with optional parameters: https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

That means that the signatures when we make generatorFilter required are:

RunGenerators(Compilation, CancellationToken) RunGenerators(Compilation, Func<> filter, CancellationToken = default)

Anyone who was calling RunGenerators(Compilation) will now be source broken, because we no longer have an overload with only a single required parameter.

Note: It also says that we shouldn't add new optional parameters in the middle of a signature for source-compat. Imagine if you had RunGenerator(compilation, default) that might bind to a different overload now. So we're in a bit of tricky situation.

We can either: make the filter optional, and bend the 'don't put optional in the middle rule', or we can make it non-optional, and introduce a third overload of RunGenerators that only takes a Compilation parameter.

Tagging @333fred for any API shape thoughts on this one?

we can make it non-optional, and introduce a third overload of RunGenerators that only takes a Compilation parameter.

This sounds like the best option to me.

What is the problem with the signature in the current form? Yes it's optional but it just gets filled in as essentially "run everything". That behavior is compatible with the existing overload so that all seems to track for me. What am I missing?

chsienki · 2024-06-24T17:44:32Z

@dotnet/roslyn-compiler for review please.

jaredpar · 2024-06-27T04:03:03Z

@jjonescz PTAL

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

jjonescz · 2024-06-27T07:49:47Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

+        /// <param name="generatorFilter">An optional filter that specifies which generators to run. If <c>null</c> all generators will run.</param>
+        /// <param name="cancellationToken">Used to cancel an in progress operation.</param>
+        /// <returns>An updated driver that contains the results of the generators running.</returns>
+        public GeneratorDriver RunGenerators(Compilation compilation, Func<GeneratorFilterContext, bool>? generatorFilter = null, CancellationToken cancellationToken = default)


I don't see the problem - what guidelines would be broken?

src/Compilers/Core/Portable/SourceGeneration/GeneratorExtensions.cs

jjonescz · 2024-06-27T08:08:27Z

src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs

+
+            // step didn't run because the generator was already up to date
+            Assert.False(stepRan);
+        }


RegisterPostInitializationOutput should run even if the generator is filtered out, right? Consider testing that.

Hmmm, this is tricky. It seems logical to say that we shouldn't even init a generator if it's filtered out, which would skip the post init stage.

However, we also have a step there where we re-parse the post init docs if the parse options change. Right now that's a single flag as it's assumed all the generators run. If we skip a generator on a parse options change then it'll not get them reparsed and cause errors the next time we run.

We would need to track per-generator if they have had their parse options updated or not, so that we can re-parse them as needed. The alternative is to just always do the init + postinit steps for a generator even if its requested to be skipped.

Let me play around a little and see how hard the per-generator flag would be.

So, the per generator flag is actually trivial, as we can just compare the produced trees with the latest parse options.

I've updated it so that the generator doesn't init, and doesn't take part in the compilation at all if it's filtered out + added tests to cover those cases.

Add tests to ensure we re-parse init only trees if the parse options changed between filtered runs

jjonescz · 2024-06-28T07:53:26Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorExtensions.cs

@@ -47,7 +47,7 @@ public static Type GetGeneratorType(this IIncrementalGenerator generator)
        /// Converts an <see cref="IIncrementalGenerator"/> into an <see cref="ISourceGenerator"/> object that can be used when constructing a <see cref="GeneratorDriver"/>
        /// </summary>
        /// <param name="incrementalGenerator">The incremental generator to wrap</param>


Suggested change

/// <param name="incrementalGenerator">The incremental generator to wrap</param>

/// <param name="incrementalGenerator">The incremental generator to convert</param>

jjonescz · 2024-06-28T08:23:47Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorState.cs

@@ -124,5 +125,7 @@ public GeneratorState WithError(Exception exception, Diagnostic error, TimeSpan
        internal ImmutableDictionary<string, ImmutableArray<IncrementalGeneratorRunStep>> OutputSteps { get; }

        internal ImmutableArray<(string Key, string Value)> HostOutputs { get; }
+
+        internal bool RequiresPostInitReparse(ParseOptions parseOptions) => PostInitTrees.Any(t => t.Tree.Options != parseOptions);


Consider using static lambda if possible:

Suggested change

internal bool RequiresPostInitReparse(ParseOptions parseOptions) => PostInitTrees.Any(t => t.Tree.Options != parseOptions);

internal bool RequiresPostInitReparse(ParseOptions parseOptions) => PostInitTrees.Any(static (t, parseOptions) => t.Tree.Options != parseOptions, parseOptions);

jjonescz · 2024-06-28T08:26:54Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverState.cs

@@ -20,8 +20,7 @@ namespace Microsoft.CodeAnalysis
                                      DriverStateTable stateTable,
                                      SyntaxStore syntaxStore,
                                      GeneratorDriverOptions driverOptions,
-                                      TimeSpan runtime,
-                                      bool parseOptionsChanged)
+                                      TimeSpan runtime)


Can we remove the internal readonly bool ParseOptionsChanged; field as well?

jaredpar · 2024-06-28T15:18:45Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorAdaptor.cs

+        /// A dummy extension that is used to indicate this adaptor was created outside of the driver.
+        /// </summary>
+        public const string DummySourceExtension = ".dummy";
+
        private readonly string _sourceExtension;


Is this a value that is controllable by customers? If so have reservations about using a magic string. Had trouble tracking this through all the APIs to see where it originates.

jaredpar · 2024-06-28T15:20:52Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

+        /// <param name="generatorFilter">An optional filter that specifies which generators to run. If <c>null</c> all generators will run.</param>
+        /// <param name="cancellationToken">Used to cancel an in progress operation.</param>
+        /// <returns>An updated driver that contains the results of the generators running.</returns>
+        public GeneratorDriver RunGenerators(Compilation compilation, Func<GeneratorFilterContext, bool>? generatorFilter = null, CancellationToken cancellationToken = default)


What is the problem with the signature in the current form? Yes it's optional but it just gets filled in as essentially "run everything". That behavior is compatible with the existing overload so that all seems to track for me. What am I missing?

jaredpar · 2024-06-28T15:23:04Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

@@ -195,7 +205,7 @@ public GeneratorDriverTimingInfo GetTimingInfo()
            return new GeneratorDriverTimingInfo(_state.RunTime, generatorTimings);
        }

-        internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, DiagnosticBag? diagnosticsBag, CancellationToken cancellationToken = default)
+        internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, DiagnosticBag? diagnosticsBag, Func<GeneratorFilterContext, bool>? generatorFilter = null, CancellationToken cancellationToken = default)


Rather than having the filter be optional here why not normalize it in the constructor to be non-null? Essentially somethnig like

generatorFilter ??= static _ => true;

Believe that should be amortized to a single alloc, if not could cache as a static readonly on the type

chsienki added 2 commits June 21, 2024 11:54

Add AsIncrementalGenerator API

77f382e

Implement generator filtering and add tests

96cdcc7

chsienki added Area-Compilers New Feature - Source Generators Source Generators labels Jun 24, 2024

chsienki requested review from a team as code owners June 24, 2024 17:42

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 24, 2024

chsienki commented Jun 24, 2024

View reviewed changes

chsienki added 2 commits June 24, 2024 11:00

Note API removal as part of back compat

26800b6

Merge branch 'main' into new_generator_apis

b675fbb

333fred approved these changes Jun 26, 2024

View reviewed changes

jaredpar assigned 333fred and jjonescz Jun 27, 2024

jjonescz reviewed Jun 27, 2024

View reviewed changes

chsienki added 3 commits June 27, 2024 09:25

Update docs

fd12ffa

Don't init generators if they're filtered

a316de1

Add tests to ensure we re-parse init only trees if the parse options changed between filtered runs

Remove unused field

28299c6

chsienki force-pushed the new_generator_apis branch from 1a24208 to 28299c6 Compare June 27, 2024 23:45

jjonescz reviewed Jun 28, 2024

View reviewed changes

jaredpar reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New generator apis #74127

New generator apis #74127

chsienki commented Jun 24, 2024

chsienki Jun 24, 2024

jjonescz Jun 27, 2024

chsienki Jun 27, 2024

jjonescz Jun 28, 2024

jaredpar Jun 28, 2024

chsienki commented Jun 24, 2024

jaredpar commented Jun 27, 2024

jjonescz Jun 27, 2024

jjonescz Jun 27, 2024

chsienki Jun 27, 2024

chsienki Jun 27, 2024 •

edited

Loading

jjonescz Jun 28, 2024

jjonescz Jun 28, 2024

jjonescz Jun 28, 2024

jaredpar Jun 28, 2024

jaredpar Jun 28, 2024

jaredpar Jun 28, 2024

	/// <param name="incrementalGenerator">The incremental generator to wrap</param>
	/// <param name="incrementalGenerator">The incremental generator to convert</param>

	internal bool RequiresPostInitReparse(ParseOptions parseOptions) => PostInitTrees.Any(t => t.Tree.Options != parseOptions);
	internal bool RequiresPostInitReparse(ParseOptions parseOptions) => PostInitTrees.Any(static (t, parseOptions) => t.Tree.Options != parseOptions, parseOptions);

New generator apis #74127

Are you sure you want to change the base?

New generator apis #74127

Conversation

chsienki commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented Jun 24, 2024

jaredpar commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki Jun 27, 2024 •

edited

Loading