Feature/add import annotation #37228
Conversation
AddImportService now uses the context for better choice of whee to add imports. AddImportService first checks which namespaces need to be added, then adds them. This will be necessary when we want to make the addition safe, to avoid doing more work than we need to.
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)] | ||
| [Fact(Skip = "AddImportsService adds into hidden region"), Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)] |
There was a problem hiding this comment.
i assume these will become unskipped in later commits?
There was a problem hiding this comment.
The issue here is with the AddImportsService, which is an existing service, and does add into hidden regions. I wanted to know whether this was correct or incorrect behaviour before deleting the test or fixing the service.
| ' {CodeAnalysisResources.InMemoryAssembly} | ||
| #End Region | ||
| Imports System.Runtime.CompilerServices |
There was a problem hiding this comment.
is this a desirable change?
There was a problem hiding this comment.
There seems to be a bug with the simplifier that removes imports used by attributes in the metadata as source. The AbstractImportAdder didn't add a Simplifier annotation to added imports, so this didn't cause an issue.
I don't know whether the bug should be fixed in this PR or a seperate issue created.
| return result; | ||
| } | ||
|
|
||
| if (!result.CanAddUsingDirectives(_cancellationToken)) |
There was a problem hiding this comment.
do these checks still exist in the final code? i'm not certain that hteir cases are tested well enough.
| return null; | ||
| } | ||
|
|
||
| protected override INamespaceSymbol GetContainedNamespace(SyntaxNode node, SemanticModel model) |
There was a problem hiding this comment.
nit: consider naming TryGetContainedNamespace to plainly indicate it can return null
| return c.WithAdditionalAnnotations(SymbolAnnotation.Create(symbol), Simplifier.Annotation); | ||
| return c; | ||
| } | ||
| ); |
| var symbol = model.GetSymbolInfo(o).Symbol; | ||
| if (symbol != null) | ||
| return c.WithAdditionalAnnotations(SymbolAnnotation.Create(symbol), Simplifier.Annotation); | ||
| return c; |
There was a problem hiding this comment.
nit: consider ? : expression.
| private async Task TestAllAsync(string initialText, string importsAddedText, string simplifiedText, Func<OptionSet, OptionSet> optionsTransform = null) | ||
| { | ||
| await TestAddImportsFromSyntaxesAsync(initialText, importsAddedText, simplifiedText, optionsTransform); | ||
| await TestAddImportsFromSymbolAnnotationsAsync(initialText, importsAddedText, simplifiedText, optionsTransform); |
| /// <summary> | ||
| /// Adds namespace imports / using directives for namespace references found in the document. | ||
| /// </summary> | ||
| public static async Task<Document> AddImportsFromSymbolAnnotationAsync(Document document, OptionSet options = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
i'd be ok with these being internal to start.
| bool isInSpan(SyntaxNodeOrToken nodeOrToken) => | ||
| spansTree.HasIntervalThatOverlapsWith(nodeOrToken.FullSpan.Start, nodeOrToken.FullSpan.Length); | ||
| bool isInSpan(SyntaxNode node) => | ||
| spansTree.HasIntervalThatOverlapsWith(node.FullSpan.Start, node.FullSpan.Length); |
There was a problem hiding this comment.
nit:
- fix naming
IsInSpan. - move below main code.
| /// <summary> | ||
| /// Gets the namespace <paramref name="node"/> is contained in, or null otherwise | ||
| /// </summary> | ||
| protected abstract INamespaceSymbol GetContainedNamespace(SyntaxNode node, SemanticModel model); |
There was a problem hiding this comment.
so we already have a SemanticModelExtension.GetEnclosingNamespace. Will that not work here?
| { | ||
| return generator | ||
| .NamespaceImportDeclaration(namespaceSymbol.ToDisplayString(SymbolDisplayFormats.NameFormat)) | ||
| .WithAdditionalAnnotations(Simplifier.Annotation, Formatter.Annotation); |
There was a problem hiding this comment.
Note: my recollection is that when it comes to imports we prefer to not simplify them. That's because users want to be able to look at the using X.Y.Z and understand waht X.Y.Z is without having to go "wait... waht does X resolve off of from a higher up using".
There was a problem hiding this comment.
I think that it is necessary to simplify them, as import annotations can sometimes be added unnecessarily, and we need to be able to remove them.
There was a problem hiding this comment.
Ok. I'm ok with the entire 'import/using' being removed. We just have to make sure we don't simplify the 'name' on the RHS of the import/using. If this is only about the former (which we should doc), then this is fine :)
| IAddImportsService addImportsService, | ||
| SyntaxGenerator generator, | ||
| CancellationToken cancellationToken | ||
| ) |
| } | ||
|
|
||
| protected abstract INamespaceSymbol GetExplicitNamespaceSymbol(SyntaxNode node, SemanticModel model); | ||
| private (IEnumerable<SyntaxNode> imports, SyntaxNode context) GetImportDirectivesFromSyntaxesAsync( |
There was a problem hiding this comment.
would prefer that this be (ImmutableArray<SyntaxNode> imports, ...)
| protected abstract INamespaceSymbol GetExplicitNamespaceSymbol(SyntaxNode node, SemanticModel model); | ||
| private (IEnumerable<SyntaxNode> imports, SyntaxNode context) GetImportDirectivesFromSyntaxesAsync( | ||
| IEnumerable<SyntaxNode> syntaxNodes, | ||
| ref SyntaxNode root, |
| importsToAdd.Add(namespaceSyntax); | ||
| } | ||
|
|
||
| if (nodesToSimplify.Count is 0) |
There was a problem hiding this comment.
i hate this. plz use ==. this is just not idiomatic for the codebase :)
| var first = root.DescendantNodesAndSelf().First(x => x.HasAnnotation(annotation)); | ||
| var last = root.DescendantNodesAndSelf().Last(x => x.HasAnnotation(annotation)); | ||
|
|
||
| return (importsToAdd, first.GetCommonRoot(last)); |
There was a problem hiding this comment.
TODO(cyrusn): review this function.
| { | ||
| SyntaxNode first = null; | ||
| SyntaxNode last = null; | ||
| var importsToAdd = new List<SyntaxNode>(); |
There was a problem hiding this comment.
nit: prefer ArrayBuilder, with ToImmutableAndFree.
| case SpecialType.System_Double: | ||
| case SpecialType.System_String: | ||
| case SpecialType.System_Nullable_T: | ||
| return true; |
There was a problem hiding this comment.
can this bre replaced with type.IsSpecialType()?
There was a problem hiding this comment.
As an aside, the name of that function is confusing, since it implies that Type.SpecialType is not SpecialType.None, but actually it could be eg. DateTime
| } | ||
| } | ||
|
|
||
| return (importsToAdd, first?.GetCommonRoot(last)); |
There was a problem hiding this comment.
todo(cyrusn): review this function.
|
|
||
| public override SyntaxNode VisitIdentifierName(IdentifierNameSyntax node) | ||
| { | ||
| //Only visit leading trivia, as we only care about xml doc comments |
There was a problem hiding this comment.
applies to other methods in this class as well.
There was a problem hiding this comment.
i'm confused how an identifier can have xml doc comments.
There was a problem hiding this comment.
If it's a return type.
There was a problem hiding this comment.
gotcha. don't particularly like this (as it seems unnecessary for 99.9% of identifiers). Can we instead do this for specific nodes that can have doc comments?
There was a problem hiding this comment.
note: this isn't hugely important. but i thought maybe a few brain cycles on this might be nice.
| private class Rewriter : CSharpSyntaxRewriter | ||
| { | ||
| private Workspace _workspace; | ||
| private CancellationToken _cancellationToken; |
There was a problem hiding this comment.
not readonly for tehse two?
|
I've fixed the case sensitivity issue, but unfortunately that's broken the build. It should be trivial to fix (just remove the declaration of semantic model in abstract add imports service which I left in by mistake), but I can't do it on a phone, and I'm on holiday for the next two weeks, so I this will have to wait till then. Feel free to make the change yourself if it's more convenient, and it's blocking merging/reviewing :-) |
Remove Undeclared variable left in AbstractAddImportsService accidentally
|
CI is now passing, so I think this is ready for all the changes I've made to be reviewed. |
|
Is this waiting on a design review for further progress, or is there anything I can do to help push this PR forward? Thanks |
|
Hi Is it possible to give this a bit of a nudge please? Thanks |
|
Hey @jinujoseph @sharwell . This is a super awesome community PR from @YairHalberstadt that has a lot of applicability in several roslyn features. As an example, this would have been great to have for @jnm2's recent Things seem to have stalled out on the Roslyn side though with the last communiation from the team almost two months ago in July. Can this be prioritized so that @YairHalberstadt can make forward progress on this? Thanks! |
|
@YairHalberstadt Sorry for the delay here, This is marked for design review and we will pick this up next week and get back on it soon. |
|
Thanks, @jinu, that would be great. |
|
Is there anything this needs from me at the moment @sharwell, @JoeRobich? Thanks |
|
Hi @YairHalberstadt, I should have updated this last week with the notes from the review. In all we like this change, but had a few questions. Question:
|
jinujoseph
removed
the
Need Design Review
|
Certainly both could be done. Is that what you prefer? |
|
@YairHalberstadt Unless there is an immediate need for the methods to be public, let's make only the Annotation public and we would like to make the Annotation available from the Simplifier. If a need for these method arises later, we can expose them at that time. |
|
I've made the requested changes and merged master in. |
|
|
||
| /// <summary> | ||
| /// <see cref="ImportAdderService"/> adds <see cref="Simplifier.Annotation"/> to Import Directives it adds, | ||
| /// which causes the <see cref="Simplifier"/> to remove import directives when thety are only used by attributes. |
|
Thanks @YairHalberstadt! |
|
Thanks @JoeRobich. Glad it's finally merged 😅. Looking forward to using it in the next version of VS. |
6 participants
Closes #35806
The aim of this PR is to improve the ability of Roslyn to add imports to code, in particular with regards to code generated by code actions.
This PR consists of 4 commits (EDIT: plenty more commits added now, but the first 4 contain most of the interesting stuff :-)).
The first commit merges the ImportAdderService which previously just added imports based on QualifiedNameSyntaxes, with the AbstractImportAdder which previously added imports based upon the SymbolAnnotations. This way both approaches gain the benefits of the changes in the next commit, whilst reducing code duplication and technical debt. We also first calculate all using statements to add before adding them, and are stricter about not adding unnecessary using statements. This allows us to make the changes in the next commit more efficient, since we can skip them if there are no imports to add.
The second commit adds a safe flag to the AddImportsService. When this flag is set to true, we will try to complexify code as necessary in order to prevent the addition of namespaces from changing the semantic meaning of the code.
The third commit adds a static
Annotationproperty toImportAdder's public API. When a subtree is marked with this annotation, the post processing stage of a code action will add import statements to nodes in the subtree which have SymbolAnnotations attached.The fourth commit adds this annotation by default in the AbstractCodeGenerationService APIs. It can be switched off by Setting AddImports in CodeGenerationOptions to false.