-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Make method abstract and force subclasses to implement #72145
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,43 +5,39 @@ | |
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Microsoft.CodeAnalysis | ||
| namespace Microsoft.CodeAnalysis; | ||
|
|
||
| internal partial class SolutionCompilationState | ||
| { | ||
| internal partial class SolutionCompilationState | ||
| /// <summary> | ||
| /// Represents a change that needs to be made to a <see cref="Compilation"/>, <see cref="GeneratorDriver"/>, or both in response to | ||
| /// some user edit. | ||
| /// </summary> | ||
| private abstract partial class CompilationAndGeneratorDriverTranslationAction | ||
| { | ||
| public abstract Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i far preferred subclasses having to be intentional about what they should do here. i already did something similar for CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput previously.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now, the only non-abstract method is TryMergeWithPrior. And that one is fine to stay virtual as it's only used as a perf optimization, not a correctness one. |
||
|
|
||
| /// <summary> | ||
| /// Represents a change that needs to be made to a <see cref="Compilation"/>, <see cref="GeneratorDriver"/>, or both in response to | ||
| /// some user edit. | ||
| /// Whether or not <see cref="TransformCompilationAsync" /> can be called on Compilations that may contain | ||
| /// generated documents. | ||
| /// </summary> | ||
| private abstract partial class CompilationAndGeneratorDriverTranslationAction | ||
| { | ||
| public virtual Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
| { | ||
| return Task.FromResult(oldCompilation); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Whether or not <see cref="TransformCompilationAsync" /> can be called on Compilations that may contain | ||
| /// generated documents. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Most translation actions add or remove a single syntax tree which means we can do the "same" change | ||
| /// to a compilation that contains the generated files and one that doesn't; however some translation actions | ||
| /// (like <see cref="ReplaceAllSyntaxTreesAction"/>) will unilaterally remove all trees, and would have unexpected | ||
| /// side effects. This opts those out of operating on ones with generated documents where there would be side effects. | ||
| /// </remarks> | ||
| public abstract bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput { get; } | ||
| /// <remarks> | ||
| /// Most translation actions add or remove a single syntax tree which means we can do the "same" change | ||
| /// to a compilation that contains the generated files and one that doesn't; however some translation actions | ||
| /// (like <see cref="ReplaceAllSyntaxTreesAction"/>) will unilaterally remove all trees, and would have unexpected | ||
| /// side effects. This opts those out of operating on ones with generated documents where there would be side effects. | ||
| /// </remarks> | ||
| public abstract bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput { get; } | ||
|
|
||
| public abstract GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver); | ||
| public abstract GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver); | ||
|
|
||
| /// <summary> | ||
| /// When changes are made to a solution, we make a list of translation actions. If multiple similar changes happen in rapid | ||
| /// succession, we may be able to merge them without holding onto intermediate state. | ||
| /// </summary> | ||
| /// <param name="priorAction">The action prior to this one. May be a different type.</param> | ||
| /// <returns>A non-null <see cref="CompilationAndGeneratorDriverTranslationAction" /> if we could create a merged one, null otherwise.</returns> | ||
| public virtual CompilationAndGeneratorDriverTranslationAction? TryMergeWithPrior(CompilationAndGeneratorDriverTranslationAction priorAction) | ||
| => null; | ||
| } | ||
| /// <summary> | ||
| /// When changes are made to a solution, we make a list of translation actions. If multiple similar changes happen in rapid | ||
| /// succession, we may be able to merge them without holding onto intermediate state. | ||
| /// </summary> | ||
| /// <param name="priorAction">The action prior to this one. May be a different type.</param> | ||
| /// <returns>A non-null <see cref="CompilationAndGeneratorDriverTranslationAction" /> if we could create a merged one, null otherwise.</returns> | ||
| public virtual CompilationAndGeneratorDriverTranslationAction? TryMergeWithPrior(CompilationAndGeneratorDriverTranslationAction priorAction) | ||
| => null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,10 +55,13 @@ internal sealed class TouchAdditionalDocumentAction(AdditionalDocumentState oldS | |
| private readonly AdditionalDocumentState _newState = newState; | ||
|
|
||
| // Changing an additional document doesn't change the compilation directly, so we can "apply" the | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping | ||
| // the compilation with stale trees around, answering true is still important. | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping the | ||
| // compilation with stale trees around, answering true is still important. | ||
| public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput => true; | ||
|
|
||
| public override Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for cases where there was an existing comment explaining things, i didn't comment. |
||
| => Task.FromResult(oldCompilation); | ||
|
|
||
| public override CompilationAndGeneratorDriverTranslationAction? TryMergeWithPrior(CompilationAndGeneratorDriverTranslationAction priorAction) | ||
| { | ||
| if (priorAction is TouchAdditionalDocumentAction priorTouchAction && | ||
|
|
@@ -198,12 +201,14 @@ public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generat | |
|
|
||
| internal sealed class AddOrRemoveAnalyzerReferencesAction(string language, ImmutableArray<AnalyzerReference> referencesToAdd = default, ImmutableArray<AnalyzerReference> referencesToRemove = default) : CompilationAndGeneratorDriverTranslationAction | ||
| { | ||
|
|
||
| // Changing analyzer references doesn't change the compilation directly, so we can "apply" the | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping | ||
| // the compilation with stale trees around, answering true is still important. | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping the | ||
| // compilation with stale trees around, answering true is still important. | ||
| public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput => true; | ||
|
|
||
| public override Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
| => Task.FromResult(oldCompilation); | ||
|
|
||
| public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver) | ||
| { | ||
| if (!referencesToRemove.IsDefaultOrEmpty) | ||
|
|
@@ -222,12 +227,14 @@ public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generat | |
|
|
||
| internal sealed class AddAdditionalDocumentsAction(ImmutableArray<AdditionalDocumentState> additionalDocuments) : CompilationAndGeneratorDriverTranslationAction | ||
| { | ||
|
|
||
| // Changing an additional document doesn't change the compilation directly, so we can "apply" the | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping | ||
| // the compilation with stale trees around, answering true is still important. | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping the | ||
| // compilation with stale trees around, answering true is still important. | ||
| public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput => true; | ||
|
|
||
| public override Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
| => Task.FromResult(oldCompilation); | ||
|
|
||
| public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver) | ||
| { | ||
| return generatorDriver.AddAdditionalTexts(additionalDocuments.SelectAsArray(static documentState => documentState.AdditionalText)); | ||
|
|
@@ -236,12 +243,14 @@ public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generat | |
|
|
||
| internal sealed class RemoveAdditionalDocumentsAction(ImmutableArray<AdditionalDocumentState> additionalDocuments) : CompilationAndGeneratorDriverTranslationAction | ||
| { | ||
|
|
||
| // Changing an additional document doesn't change the compilation directly, so we can "apply" the | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping | ||
| // the compilation with stale trees around, answering true is still important. | ||
| // translation (which is a no-op). Since we use a 'false' here to mean that it's not worth keeping the | ||
| // compilation with stale trees around, answering true is still important. | ||
| public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput => true; | ||
|
|
||
| public override Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
| => Task.FromResult(oldCompilation); | ||
|
|
||
| public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver) | ||
| { | ||
| return generatorDriver.RemoveAdditionalTexts(additionalDocuments.SelectAsArray(static documentState => documentState.AdditionalText)); | ||
|
|
@@ -252,6 +261,11 @@ internal sealed class ReplaceGeneratorDriverAction(GeneratorDriver oldGeneratorD | |
| { | ||
| public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput => true; | ||
|
|
||
| // Replacing the generator doesn't change the non-generator compilation. So we can just return the old | ||
| // compilation as is. | ||
| public override Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken) | ||
| => Task.FromResult(oldCompilation); | ||
|
|
||
| public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver _) | ||
| { | ||
| // The GeneratorDriver that we have here is from a prior version of the Project, it may be missing state changes due | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view with whitespace off.