Skip to content

Fix post init trees parsing when parse options change #61934

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.

Sign up for GitHub

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

Merged
chsienki merged 3 commits into from
Sep 2, 2022
Merged

Fix post init trees parsing when parse options change #61934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2a97aa
Fix post init trees parsing when parse options change
chsienki Jun 15, 2022
84996a4
Explicity track changes, so they continue across multiple mutations.
chsienki Jun 15, 2022
0f3deae
Update tests
chsienki Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3149,5 +3149,52 @@ public void Returning_Null_From_SelectMany_Gives_Empty_Array()
Assert.Empty(result.GeneratedSources);
Assert.Empty(result.Diagnostics);
}

[Fact]
public void Post_Init_Trees_Are_Reparsed_When_ParseOptions_Change()
{
var source = "class C{}";
var postInitSource = @"
#pragma warning disable CS0169
class D { (int, bool) _field; }";

var parseOptions = TestOptions.RegularPreview;
Compilation compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);
compilation.VerifyDiagnostics();

var generator = new PipelineCallbackGenerator(ctx =>
{
ctx.RegisterPostInitializationOutput(c => c.AddSource("D", postInitSource));
}).AsSourceGenerator();

GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { generator }, parseOptions: parseOptions, driverOptions: new GeneratorDriverOptions(IncrementalGeneratorOutputKind.None, trackIncrementalGeneratorSteps: true));
driver = driver.RunGeneratorsAndUpdateCompilation(compilation, out compilation, out var diagnostics);
compilation.VerifyDiagnostics();
Assert.Empty(diagnostics);

// change the parse options so that the tree is no longer accepted
parseOptions = parseOptions.WithLanguageVersion(LanguageVersion.CSharp2);
compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);
driver = driver.WithUpdatedParseOptions(parseOptions);

// change some other options to ensure the parseOption change tracking flows correctly
driver = driver.AddAdditionalTexts(ImmutableArray<AdditionalText>.Empty);

driver = driver.RunGeneratorsAndUpdateCompilation(compilation, out compilation, out diagnostics);
diagnostics.Verify();
Copy link
Member

@jaredpar jaredpar Jul 8, 2022

Choose a reason for hiding this comment

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

There should be diagnostics at this stage though because the language version is csharp 1. Believe we should validate these diagonstics show up here, likely by just grabbing the diagnostics off the syntax tree in the compilation.

Copy link
Member Author

@chsienki chsienki Sep 1, 2022

Choose a reason for hiding this comment

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

These are the generator diagnostics. I'll add a check for the compilation.

compilation.VerifyDiagnostics(
// Microsoft.CodeAnalysis.Test.Utilities\Roslyn.Test.Utilities.TestGenerators.PipelineCallbackGenerator\D.cs(3,12): error CS8022: Feature 'tuples' is not available in C# 2. Please use language version 7.0 or greater.
// class D { (int, bool) _field; }
Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion2, "(int, bool)").WithArguments("tuples", "7.0").WithLocation(3, 12)
);

// change them back to something where it is supported
parseOptions = parseOptions.WithLanguageVersion(LanguageVersion.CSharp8);
compilation = CreateCompilation(source, options: TestOptions.DebugDll, parseOptions: parseOptions);
driver = driver.WithUpdatedParseOptions(parseOptions);
driver = driver.RunGeneratorsAndUpdateCompilation(compilation, out compilation, out diagnostics);
diagnostics.Verify();
compilation.VerifyDiagnostics();
}
}
}
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Test/Semantic/SourceGeneration/StateTableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,8 @@ private DriverStateTable.Builder GetBuilder(DriverStateTable previous, bool trac
SyntaxStore.Empty,
disabledOutputs: IncrementalGeneratorOutputKind.None,
runtime: TimeSpan.Zero,
trackIncrementalGeneratorSteps: trackIncrementalGeneratorSteps);
trackIncrementalGeneratorSteps: trackIncrementalGeneratorSteps,
parseOptionsChanged: false);

return new DriverStateTable.Builder(c, state, SyntaxStore.Empty.ToBuilder(c, ImmutableArray<SyntaxInputNode>.Empty, trackIncrementalGeneratorSteps, cancellationToken: default));
}
Expand Down
12 changes: 9 additions & 3 deletions src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ internal GeneratorDriver(GeneratorDriverState state)
internal GeneratorDriver(ParseOptions parseOptions, ImmutableArray<ISourceGenerator> generators, AnalyzerConfigOptionsProvider optionsProvider, ImmutableArray<AdditionalText> additionalTexts, GeneratorDriverOptions driverOptions)
{
var incrementalGenerators = GetIncrementalGenerators(generators, SourceExtension);
_state = new GeneratorDriverState(parseOptions, optionsProvider, generators, incrementalGenerators, additionalTexts, ImmutableArray.Create(new GeneratorState[generators.Length]), DriverStateTable.Empty, SyntaxStore.Empty, driverOptions.DisabledOutputs, runtime: TimeSpan.Zero, driverOptions.TrackIncrementalGeneratorSteps);
_state = new GeneratorDriverState(parseOptions, optionsProvider, generators, incrementalGenerators, additionalTexts, ImmutableArray.Create(new GeneratorState[generators.Length]), DriverStateTable.Empty, SyntaxStore.Empty, driverOptions.DisabledOutputs, runtime: TimeSpan.Zero, driverOptions.TrackIncrementalGeneratorSteps, parseOptionsChanged: true);
}

public GeneratorDriver RunGenerators(Compilation compilation, CancellationToken cancellationToken = default)
Expand Down Expand Up @@ -145,7 +145,7 @@ public GeneratorDriver ReplaceAdditionalText(AdditionalText oldText, AdditionalT
public GeneratorDriver ReplaceAdditionalTexts(ImmutableArray<AdditionalText> newTexts) => FromState(_state.With(additionalTexts: newTexts));

public GeneratorDriver WithUpdatedParseOptions(ParseOptions newOptions) => newOptions is object
? FromState(_state.With(parseOptions: newOptions))
? FromState(_state.With(parseOptions: newOptions, parseOptionsChanged: true))
Copy link
Member

@jaredpar jaredpar Jul 8, 2022

Choose a reason for hiding this comment

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

While ParseOptions doesn't implement any equality all of the derived types do implement equality. Yet here we're unconditionally saying it changed. Do these changes happen frequently enough that we should plumb this through the derived types to let them say if the new ParseOptions are different than the current ones?

Copy link
Member

@jasonmalinowski jasonmalinowski Jul 8, 2022

Choose a reason for hiding this comment

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

I may have to implement equality checking in #62219 to make that approach work there, so if we don't do it here this may not last long. I have new bits to test that PR with so will hopefully know soon enough if it works or needs that change.

Copy link
Member Author

@chsienki chsienki Sep 1, 2022

Choose a reason for hiding this comment

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

We could, but re-parsing the init trees is cheap, and doesn't happen often enough that it'll be an issue. I think we should not block on this here, and can always improve it at a later date if we need to.

: throw new ArgumentNullException(nameof(newOptions));

public GeneratorDriver WithUpdatedAnalyzerConfigOptions(AnalyzerConfigOptionsProvider newOptions) => newOptions is object
Expand Down Expand Up @@ -248,6 +248,12 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
? new GeneratorState(postInitSources, inputNodes, outputNodes)
: SetGeneratorException(MessageProvider, GeneratorState.Empty, sourceGenerator, ex, diagnosticsBag, isInit: true);
}
else if (state.ParseOptionsChanged && generatorState.PostInitTrees.Length > 0)
{
// the generator is initalized, but we need to reparse the post-init trees as the parse options have changed
var reparsedInitSources = ParseAdditionalSources(sourceGenerator, generatorState.PostInitTrees.SelectAsArray(t => new GeneratedSourceText(t.HintName, t.Text)), cancellationToken);
generatorState = new GeneratorState(reparsedInitSources, generatorState.InputNodes, generatorState.OutputNodes);
}

// if the pipeline registered any syntax input nodes, record them
if (!generatorState.InputNodes.IsEmpty)
Expand Down Expand Up @@ -298,7 +304,7 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
}
}

state = state.With(stateTable: driverStateBuilder.ToImmutable(), syntaxStore: syntaxStoreBuilder.ToImmutable(), generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed);
state = state.With(stateTable: driverStateBuilder.ToImmutable(), syntaxStore: syntaxStoreBuilder.ToImmutable(), generatorStates: stateBuilder.ToImmutableAndFree(), runTime: timer.Elapsed, parseOptionsChanged: false);
return state;
}

Expand Down
15 changes: 12 additions & 3 deletions src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ internal GeneratorDriverState(ParseOptions parseOptions,
SyntaxStore syntaxStore,
IncrementalGeneratorOutputKind disabledOutputs,
TimeSpan runtime,
bool trackIncrementalGeneratorSteps)
bool trackIncrementalGeneratorSteps,
bool parseOptionsChanged)
{
Generators = sourceGenerators;
IncrementalGenerators = incrementalGenerators;
Expand All @@ -34,6 +35,7 @@ internal GeneratorDriverState(ParseOptions parseOptions,
DisabledOutputs = disabledOutputs;
RunTime = runtime;
TrackIncrementalSteps = trackIncrementalGeneratorSteps;
ParseOptionsChanged = parseOptionsChanged;
Debug.Assert(Generators.Length == GeneratorStates.Length);
Debug.Assert(IncrementalGenerators.Length == GeneratorStates.Length);
}
Expand Down Expand Up @@ -93,6 +95,11 @@ internal GeneratorDriverState(ParseOptions parseOptions,

internal readonly bool TrackIncrementalSteps;

/// <summary>
/// Tracks if the <see cref="ParseOptions"/> have been changed meaning post init trees will need to be re-parsed.
/// </summary>
internal readonly bool ParseOptionsChanged;

internal GeneratorDriverState With(
ImmutableArray<ISourceGenerator>? sourceGenerators = null,
ImmutableArray<IIncrementalGenerator>? incrementalGenerators = null,
Expand All @@ -103,7 +110,8 @@ internal GeneratorDriverState With(
ParseOptions? parseOptions = null,
AnalyzerConfigOptionsProvider? optionsProvider = null,
IncrementalGeneratorOutputKind? disabledOutputs = null,
TimeSpan? runTime = null)
TimeSpan? runTime = null,
bool? parseOptionsChanged = null)
{
return new GeneratorDriverState(
parseOptions ?? this.ParseOptions,
Expand All @@ -116,7 +124,8 @@ internal GeneratorDriverState With(
syntaxStore ?? this.SyntaxStore,
disabledOutputs ?? this.DisabledOutputs,
runTime ?? this.RunTime,
this.TrackIncrementalSteps
this.TrackIncrementalSteps,
parseOptionsChanged ?? this.ParseOptionsChanged
);
}
}
Expand Down