Roslyn: Code Fixes/refactorings should add using statements if necessary and possible

[YairHalberstadt]

For example, when pull members up to base class is used, and the base class is in a separate document without a suitable using statement, the following happens:

interface Interface
{
    public System.Threading.Tasks.Task PulledUp(System.Guid guid);
}

Instead I think it would be best if pull members up tried to preserve the names as they appeared in the original document. This means that it should add using statements if doing so will not cause conflicts.

Thus it should result in:

using System;
using System.Threading.Tasks;

interface Interface
{
    public Task PulledUp(Guid guid);
}

Similiar applies to Implement members, generate variable, etc.

[sharwell]

I was recently hoping the code fix APIs supported an annotation like Simplifier.Annotation that does this, but it doesn't seem to be in place yet.

[CyrusNajmabadi]

We investigated this at one point. Main problem was changes in semantics. We could try an approach that complexifies/simplifies in the presence of the new usings though. I think our work predated having that system.

[YairHalberstadt]

I believe resharper does this, so it should be possible in theory at least.

[CyrusNajmabadi]

I believe resharper does this, so it should be possible in theory at least.

Our last investigation (which admittedly was years ago) showed resharper producing far more bugs in these sorts of cases. That was def something i wanted to avoid as a constant theme we were hearing from people was "we want to be able to trust these refactorings". It's def possible the landscape has changed though (i.e. the tools have gotten better, or people are more willing to just accept the more desirable result, even if it causes problems a higher percentage of times).

[YairHalberstadt]

The naive approach would be to choose which namespace you want to add, build a hashset of all types mentioned in the document, and check if any members of the namespace are in the hashset of types in the document. If not, you add the using, if yes, you don't.

That would be overly conservative, but significantly better than the current approach.

I have no idea what the performance impact would be.

[CyrusNajmabadi]

It's unfortunately more complex than this. For example, because of extension methods. Also (and this is really crazy, but does happen in practice), but adding/removing theses usings can still break things because it can affect error paths in lambdas. Meaning that the types from the usings might not even be used in teh final compiled code, but touching them can change meaning.

Note: that's if you want to ensure no changed semantics. I'd be ok saying that the benefit here is good enough that we can accept minor breaks in edge cases.

[YairHalberstadt]

The lambdas could be solved by looking for type names at the syntactic level, not the symbol level. So any expression which can be a type you put the name of in the hashset.

Extension methods, especially for things like deconstructs etc. is more complex.

For pull members up it may be possible to try a completely different approach. Just try it, and then diff the operation tree for all other methods to see if it changed them. I have no idea if that would work though.

[CyrusNajmabadi]

see if it changed them

This also gets complex, and usually requires custom logic. i.e. by definition the code is now different (things like the 'this' reference in the method being an obvious case). So you need to write and define what equality means.

note: none of this is meant to try to say this can't be done. just outlining the size of hte task :)

Extension methods, especially for things like deconstructs etc. is more complex.

Definitely. And i imagine it will get worse in later language versions. i.e. if we do shapes, and if witness types and the like are brought in through usings, etc. etc. :)

[sharwell]

Design review conclusion:

• We already have a service to add missing using directives, but the service does not automatically run during the post-processing of code fixes.
• We need a new public-facing annotation which means "allow simplifying this name by adding a using directive". The annotation would not be applied by default in the syntax factory APIs, but could be applied manually (or possibly in the generator/editor APIs) during a code fix computation.
• The code fix post-processing steps would consider this annotation during the simplification steps, leveraging existing services for adding using directives.

[CyrusNajmabadi]

The annotation would not be applied by default in the syntax factory APIs, but could be applied manually (or possibly in the generator/editor APIs) during a code fix computation.

Design suggestion:

Add an AddImportAnnotation that can be applied to a syntax node. The semantics of this annotation are:

1. walk downwards to find all SymbolAnnotations at that node or below.
2. use those SymbolAnnotations to find the ITypeSymbols that have been generated.
3. Use those ITypeSymbols to determine the set of namespaces to import.

Note that step '3' requires more than finding the containing namespcae for the symbol. i.e. if we generate the type syntax for Foo[], we want the namespace for 'Foo' not for System.Array.

[YairHalberstadt]

@sharwell

How would this address the issue of adding using statements changing the semantics of the code?

[CyrusNajmabadi]

I can think of a couple of approaches:

1. Complexify, then add the usings, then simplify.
2. Check if the usings pull in names that would conflict with names already in scope. If so, don't add.

[YairHalberstadt]

Would we want to centralise the logic to check if the Imports are safe to add at the point where the annotations are added, or where they're read and the imports are added?

[jnm2]

Would it be hard to respect existing namespace aliases so that if using Diag = System.Diagnostics; is there, using System.Diagnostics; is not added and the alias Diag is used instead of fully qualifying?

[YairHalberstadt]

@jnm2

Currently I don't think I have time to do that. However if you want to open a PR against my branch, feel free to do so. Alternatively you could wait till this is merged and then add it in as an enhancement.

In terms of whether it would be hard - I think the change would be very localized, but I have no idea how hard the actual logic to detect that case would be.

[YairHalberstadt]

Closing as done

[sharwell]

@YairHalberstadt yeah it is. Also I love this feature!