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

Changes from 2 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
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