Don't treat SyntaxProvider transform step inputs as cached. #58935
Conversation
…vider when the SyntaxNode entry is cached.
There was a problem hiding this comment.
This actually breaks correctness. It's not possible to use the cached semantic results: we know that the syntax hasn't changed (which is why we do it two phase) but even with a cached syntax tree the things it refers too (for instance a class in another tree) can changed between runs, meaning we always have to run the transform step, even when the syntax tree hasn't changed.
|
That's because the SemanticModel is also provided as an input... Makes sense. I'll go back to the drawing board on this one. |
…emanticModel, is never cached between runs.
jkoritzinsky
changed the title
|
@chsienki I've taken an alternative approach here that shouldn't impact correctness and will accurately reflect the fact that we don't cache the SemanticModel between generator runs. Can you take another look? |
ghost
added this to the
|
@jkoritzinsky Just a heads up, we usually require two sign offs to merge into main. @cston Could you do a review + retroactive sign off? |
|
Sorry! I figured that if auto-merge was available that the requirements to merge had been met. |
| @@ -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. | |||
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
This fixes a case where we had a bad step state transition where we went from all "Cached" inputs to a "Modified" output, which is disallowed (this is how I found this bug) by never treating the input as "Cached" (which is also valid as one of the inputs is the
SemanticModel, which we don't cache between runs).This is blocking DllImportGenerator from adopting the Incremental Step Tracking API for our testing.