Is this using needed?

sort.

I really wish if we wanted usings sorted it would at least fail the correctness test

</rant>

Emm, in which case could 'type' be null here?

i also don't really understand what is going on here. you're finding all the declared symbols... and then doing either COnvertToType or GetMemberType... because? I don't really get the logic of what's going on here.

consider indenting this too.

It is a comment for all the tests.
Could you write tests that imports some other namespaces? (Not 'System', but like 'System.Linq, or System.Threading')

This is because System is so widely used, at this line, if the symbol is a method, then it would return the 'System.Func',
which cause 'System' is imported later.
However, because all your tests now are importing 'System', if the member is
void Bar() { var c = Uri.TryCreate() ... }
'System' could be imported by 'Func', not 'Uri'

Also, semanticModel.GetAllDeclaredSymbols will only returns the 'declared' symbol,
If I have a method like
void bar() { Uri.TryCreate(...) }
I really worry 'Uri' won't be passed in this method

This other way to do it gets all descendant syntax nodes, and adds any that have a valid type. One issue is that this is possibly slow/inefficient, in which case I think we can filter on only Identifier nodes, which would require converting this to a language service.

I think it's worse than this. First, we want to check any start of expression (which could be identifiers, generic names, etc.). Second, things like extension methods are brought in by using statements, so we need to bind methods.

AN alternate approach that sidesteps this (though introduces its own problems is), is to avoid this kind of binding entirely.

Instead:

1. pull all the usings up to the destination location that are not already in the destination.
2. mark all existing usings in teh destination location somehow (or, alternatively, mark only the usings that were pulled up.
3. remove all unused usings as the final step but filter that only to the set that was added.

Effectively, this will leave us with only the set that was needed to bind the code that was moved. THe downside is though that some of the added usings may cause conflicts in the destination due to ambiguous references. However, that was already possible with the approach we're talking here, so maybe it's not too bad.

: ) Just one point added, It is ok if you finally need to convert to non-static language service. I remember when this class is originally written, we have a discussion around whether it should be a static helper or abstract class -> C# impl/VB impl.

Also, a document is passed in. So you can always acquire language services from that.

One other option to consider would be to use the simplifier to expand (and later collapse) the moved nodes into using their fully qualified names. This might get around the destination conflicts problem as well, although that may need more testing.

I haven't followed the logic here, but is implicit object creation dropped intentionally?

this won't work for things like List<int>

yes. please don't parenthesize single parameters.

this will not work for extension methods.

this may be an enormous number of annotations. i'm not comfortable with this.

Remove if possible.

this is a requirement now. no new code can be nullable disabled.

place abstract after constructors plz :)

we don't need a special service for thsi. these services can be removed and ISyntaxFactsService or ISyntaxKindsService can be used instead from the original code to make this determination for both C# and VB.

i didn't see vb tests in this PR.

The parenthesis is not needed : )

The parenthesis is not needed : )

Also could be simplified to

Also I remembered DescendantNodesAndSelf() has an overload takes a predicate, so you could merge this where clause.

@Cosifne I think most of the time, Where isn't an equivalent for the overload taking a descendIntoChildren, which IIRC just stops going deep in the tree once the condition is false. But since the node we're looking for is either the root (compilation unit), or a child of the root (namespace declaration), using this overload may work.

@Youssef1313 thanks for your further explanation. ( I just omit to explain the recursively part of that function) : )

this is not quite right. consider something like:

namespace Outer {
  namespace Inner {
     class StartingPoint { ... }
  }
}

This will likely create using Inner instead of using Outer.Inner. to resolve this, don't use syntax this way. Instead, get the containing namespace symbol for the type you're in. ANd then just generate the name for that and the corresponding namespace import. this can all be done in teh base class.

is there some code somewhere that validates teh destination type's language is the same as the source type's language?

It looks like the quick action is not offered when the base type is in another language, as I can't seem to find it when the base class is written in VB and derived in C#. However, I can't find any line of code that validates this (here is the closest thing), or any tests that confirm this behavior, but it's possible that there is a check.

I think the move to base class shouldn't work at all when the languages are different, as the generator takes an option which tells it to reuse the existing member syntax, which should already be in its language-specific form. Moving to an interface should probably still work, since the generator rewrites the member signature from scratch. Would you suggest I add some sort of check here, or would that be another issue entirely?

interesting. are both crlfs needed?

so we already have:

interface ISyntaxFacts 
{
        SyntaxList<SyntaxNode> GetMembersOfNamespaceDeclaration(SyntaxNode namespaceDeclaration);
        SyntaxList<SyntaxNode> GetMembersOfCompilationUnit(SyntaxNode compilationUnit);
}

We shoudl add equivalent GetImportsOfNamespaceDeclaration (and ocmpilation unit).

Then we can just use ISyntaxFacts, and remove the IMemberPullService entirely.

also add test for filescopednamespace case.

(note: you'll need to fixup IsNamespaceDeclaration. i'm doing that in one of my PRs. but it's ok if you do that as well.