Skip to content

Don't treat SyntaxProvider transform step inputs as cached. #58935

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
jkoritzinsky merged 3 commits into from
Jan 20, 2022
Merged

Don't treat SyntaxProvider transform step inputs as cached. #58935

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
66d8b75
Enable using cached results of the transform function for a SyntaxPro…
jkoritzinsky Jan 19, 2022
d197ad1
Never treat the transform input as cached as one of the inputs, the S…
jkoritzinsky Jan 20, 2022
8631167
Update tests.
jkoritzinsky Jan 20, 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
79 changes: 41 additions & 38 deletions src/Compilers/CSharp/Test/Semantic/SourceGeneration/SyntaxAwareGeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,16 +946,26 @@ class C
Compilation compilation = CreateCompilation(source1, options: TestOptions.DebugDll, parseOptions: parseOptions);
compilation.VerifyDiagnostics();

List<string> syntaxFilterVisited = new();
var testGenerator = new PipelineCallbackGenerator(context =>
{
var source = context.SyntaxProvider.CreateSyntaxProvider((c, _) => c is FieldDeclarationSyntax fds, (c, _) => ((FieldDeclarationSyntax)c.Node).Declaration.Variables[0].Identifier.ValueText).WithTrackingName("Fields");
var source = context.SyntaxProvider.CreateSyntaxProvider((c, _) =>
{
if (c is FieldDeclarationSyntax fds)
{
syntaxFilterVisited.Add(fds.Declaration.Variables[0].Identifier.ValueText);
return true;
}
return false;
}, (c, _) => ((FieldDeclarationSyntax)c.Node).Declaration.Variables[0].Identifier.ValueText);
context.RegisterSourceOutput(source, (spc, fieldName) =>
{
spc.AddSource(fieldName, "");
});
});

GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { new IncrementalGeneratorWrapper(testGenerator) }, parseOptions: parseOptions, driverOptions: new GeneratorDriverOptions(IncrementalGeneratorOutputKind.None, trackIncrementalGeneratorSteps: true));
// Don't enable incremental tracking here as incremental tracking disables the "unchanged compilation implies unchanged syntax trees" optimization.
Copy link
Contributor

@cston cston Jan 20, 2022

Choose a reason for hiding this comment

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

incremental tracking disables the "unchanged compilation implies unchanged syntax trees" optimization

Do we need to support this optimization with incremental tracking? (Is this the TODO in DriverStateTable.Builder.GetSyntaxInputTable()?) If so, is there an issue logged to track that?

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jan 20, 2022

Choose a reason for hiding this comment

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

We don't need to support this optimization with incremental tracking as incremental tracking is meant to be a test-only feature, but it would be a "nice to have" feature if the implementation of it becomes simpler in a future refactoring.

This is the TODO in GetSyntaxInputTable(). There currently isn't an issue logged for it as it's not a required feature.

GeneratorDriver driver = CSharpGeneratorDriver.Create(new[] { new IncrementalGeneratorWrapper(testGenerator) }, parseOptions: parseOptions);
driver = driver.RunGenerators(compilation);

var results = driver.GetRunResult();
Expand All @@ -964,14 +974,9 @@ class C
Assert.EndsWith("fieldA.cs", results.GeneratedTrees[0].FilePath);
Assert.EndsWith("fieldB.cs", results.GeneratedTrees[1].FilePath);
Assert.EndsWith("fieldC.cs", results.GeneratedTrees[2].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.New), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.New), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.New), output)));
Assert.Equal(new[] { "fieldA", "fieldB", "fieldC" }, syntaxFilterVisited);

syntaxFilterVisited.Clear();
// run again on the *same* compilation
driver = driver.RunGenerators(compilation);
results = driver.GetRunResult();
Expand All @@ -980,13 +985,7 @@ class C
Assert.EndsWith("fieldA.cs", results.GeneratedTrees[0].FilePath);
Assert.EndsWith("fieldB.cs", results.GeneratedTrees[1].FilePath);
Assert.EndsWith("fieldC.cs", results.GeneratedTrees[2].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Cached), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)));
Assert.Empty(syntaxFilterVisited);

// now change the compilation, but don't change the syntax trees
compilation = compilation.WithAssemblyName("newCompilation");
Expand All @@ -997,13 +996,7 @@ class C
Assert.EndsWith("fieldA.cs", results.GeneratedTrees[0].FilePath);
Assert.EndsWith("fieldB.cs", results.GeneratedTrees[1].FilePath);
Assert.EndsWith("fieldC.cs", results.GeneratedTrees[2].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Cached), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)));
Assert.Equal(new[] { "fieldA", "fieldB", "fieldC" }, syntaxFilterVisited);
}

[Fact]
Expand Down Expand Up @@ -1070,11 +1063,11 @@ class D
Assert.EndsWith("fieldE.cs", results.GeneratedTrees[4].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldD", IncrementalStepRunReason.New), output)),
step => Assert.Collection(step.Outputs,
Expand Down Expand Up @@ -1149,11 +1142,11 @@ class D
Assert.EndsWith("fieldC.cs", results.GeneratedTrees[2].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldD", IncrementalStepRunReason.Removed), output)),
step => Assert.Collection(step.Outputs,
Expand Down Expand Up @@ -1273,6 +1266,7 @@ class F
}", parseOptions);
compilation = compilation.ReplaceSyntaxTree(firstTree, newTree);

fieldsCalledFor.Clear();
// now re-run the drivers
driver = driver.RunGenerators(compilation);
results = driver.GetRunResult();
Expand All @@ -1287,9 +1281,10 @@ class F
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldD", IncrementalStepRunReason.Modified), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)));
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Unchanged), output)));
Assert.Equal("fieldD", Assert.Single(fieldsCalledFor));
}

[Fact]
Expand Down Expand Up @@ -1330,7 +1325,8 @@ class E
}
return false;
},
(c, _) => ((FieldDeclarationSyntax)c.Node).Declaration.Variables[0].Identifier.ValueText).WithTrackingName("Fields");
(c, _) => ((FieldDeclarationSyntax)c.Node).Declaration.Variables[0].Identifier.ValueText).WithTrackingName("Syntax")
.Select((s, ct) => s).WithTrackingName("Fields");

context.RegisterSourceOutput(source, (spc, fieldName) =>
{
Expand Down Expand Up @@ -1367,7 +1363,8 @@ class E
.ReplaceSyntaxTree(lastTree, firstTree)
.ReplaceSyntaxTree(dummyTree, lastTree);

// now re-run the drivers and confirm we didn't actually run
// now re-run the drivers and confirm we didn't actually run the node selector for any nodes.
// Verify that we still ran the transform.
syntaxFieldsCalledFor.Clear();
driver = driver.RunGenerators(compilation);
results = driver.GetRunResult();
Expand All @@ -1376,6 +1373,13 @@ class E
Assert.EndsWith("fieldA.cs", results.GeneratedTrees[0].FilePath);
Assert.EndsWith("fieldB.cs", results.GeneratedTrees[1].FilePath);
Assert.EndsWith("fieldC.cs", results.GeneratedTrees[2].FilePath);
Assert.Collection(results.Results[0].TrackedSteps["Syntax"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Unchanged), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Unchanged), output)));
Assert.Collection(results.Results[0].TrackedSteps["Fields"],
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldA", IncrementalStepRunReason.Cached), output)),
Expand All @@ -1393,8 +1397,8 @@ class E
.ReplaceSyntaxTree(lastTree, firstTree)
.ReplaceSyntaxTree(dummyTree, newLastTree);

// now re-run the drivers and confirm we only ran for the 'new' syntax tree
// but then stopped when we got the same value out
// now re-run the drivers and confirm we only ran the selector for the 'new' syntax tree
// but then cached the result after the transform when we got the same value out
syntaxFieldsCalledFor.Clear();
driver = driver.RunGenerators(compilation);
results = driver.GetRunResult();
Expand All @@ -1409,9 +1413,8 @@ class E
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldB", IncrementalStepRunReason.Cached), output)),
step => Assert.Collection(step.Outputs,
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Unchanged), output)));
Assert.Single(syntaxFieldsCalledFor);
Assert.Equal("fieldC", syntaxFieldsCalledFor[0]);
output => Assert.Equal(("fieldC", IncrementalStepRunReason.Cached), output)));
Assert.Equal("fieldC", Assert.Single(syntaxFieldsCalledFor));
}

[Fact]
Expand Down
6 changes: 5 additions & 1 deletion src/Compilers/Core/Portable/SourceGeneration/Nodes/SyntaxInputNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ public void VisitTree(Lazy<SyntaxNode> root, EntryState state, SemanticModel? mo
var value = new GeneratorSyntaxContext(node, model);
var transformed = _owner._transformFunc(value, cancellationToken);

if (state == EntryState.Added || !_transformTable.TryModifyEntry(transformed, _owner._comparer, stopwatch.Elapsed, noInputStepsStepInfo, state))
// The SemanticModel we provide to GeneratorSyntaxContext is never guaranteed to be the same between runs,
// so we never consider the input to the transform as cached.
Copy link
Contributor

@cston cston Jan 20, 2022

Choose a reason for hiding this comment

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

Should the caller of VisitTree() be responsible for passing the correct EntryState in state instead? (Why is the incoming state ever set to Cached if that value is ignored?)

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jan 20, 2022 •
edited by jaredpar
Loading

Choose a reason for hiding this comment

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

The passed-in EntryState value can be cached for the "filter" step as it represents the "what is the state of the syntax tree represented by root".

The GeneratorSyntaxContext basically functions as a Combine node of a SyntaxNode and the Compilation, but since we already handle the "Compilation is cached" situation previously, we don't need to handle it here in the same way. We can assume that if we get here, that the Compilation changed, so we need to make the "input state" for the transform Modified as one of the inputs (the semantic model/compilation) has changed.

var transformInputState = state == EntryState.Cached ? EntryState.Modified : state;

if (transformInputState == EntryState.Added || !_transformTable.TryModifyEntry(transformed, _owner._comparer, stopwatch.Elapsed, noInputStepsStepInfo, transformInputState))
{
_transformTable.AddEntry(transformed, EntryState.Added, stopwatch.Elapsed, noInputStepsStepInfo, EntryState.Added);
}
Expand Down