since linq to foreach is moving from expression to statement. won't it be simpler if it just create a new method and replace linq expression to method call?

like var q = from ... select ..;
to
var q = GetX(....);

so for something like q = (from .. select ...).ToList();
it becomes
q = (GetX(...)).ToList();

so, it will first do extract method on the linq expression and then rewirte body of the extracted method. #Resolved

and body will be something like this

foreach(int n1 in nums)
        {
            foreach(int n2 in nums)
            {
                yield return n1;
            }
        }

much simpler to do so since the method only contains linq expression in it. #Resolved

otherwise, I think you always need to introduce intermediate collection like list and then pull all items eagarly like

var list = new List<...>();
foreach(int n1 in nums)
        {
            foreach(int n2 in nums)
            {
                list.Add(n1);
            }
        }

and replace linq expression with list
var q = (from ... select ...).ToList() => var q = (list).ToList();

which is not good I think since you loose all laziness by doing so. #Resolved

yield return n1; [](start = 16, length = 16)

yield return n1; [](start = 16, length = 16)

This refactoring doesn't look right #Closed

yield return n1; [](start = 16, length = 16)

yield return n1; [](start = 16, length = 16)

Same here #Closed

Count [](start = 28, length = 5)

Count [](start = 28, length = 5)

Is this a method call? #Closed

indentation. #Resolved

@heejaechang@outlook.com, could you please help?

In reply to: 175267336 [](ancestors = 175267336)

setting format annotation on block or { } tokens doesn't work? I believe it is happening because none of "{", "}", "C...;" is generated or have formatting annotionats, so formatter is not touching them. #Resolved

Thank you! Just applied. It helped.

In reply to: 176885406 [](ancestors = 176885406)

this local function should probably be named contextually to some better. #Resolved

Any ideas?

In reply to: 175267354 [](ancestors = 175267354)

it looks like we have this.
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/LanguageServices/SemanticsFactsService/ISemanticFactsService.cs,75

improve that method? #Resolved

Not sure I understand. Could you please explain?

In reply to: 176885977 [](ancestors = 176885977)

this method "GenerateNameForExpression" tries to create a contextual name from given syntax node. by looking at the code, it tries to see whether things are like variable, method call and etc, and tries to create name from it. you can add query expression support there so that it gives you good name like, if query expression ends with select n1. it can give back GetN1s or something like that rather than localFunction.

also, I believe we have humenizer or something that fix up plurals and stuff? not sure where that code is though. #Resolved

works great! thank you!

In reply to: 176894908 [](ancestors = 176894908)

probably fine to just say "Convert 'query' to 'foreach'. #Resolved

resource string should match english. so this should be Convert_LINQ_query_to_foreach. #Resolved

this feels like logic that would exist for VB or C#? why is it in the C# layer? #Resolved

why disable if there's comments? THat's very nondiscoverable by the user and will just limit them if/when they wnat to use this feature. #Resolved

Let us consider them separately. Talked to @heejaechang@outlook.com who mentioned it could be a non-trivial task to migrate comments from one code to another.

#25639

In reply to: 175267407 [](ancestors = 175267407)

I'm fine with us creating tracking work to do this better...

However, my overall point is that until that work is done we should default to supporting the feature, not disabling it. #Resolved

I'm not sure that we support comments automatically with this conversion. To avoid breaking user (= losing comments), likely we should disable this by default.

In reply to: 175950029 [](ancestors = 175950029)

likely we should disable this by default.

Sorry i wasn't clear. By default, the IDE defaults to offering tehse sorts of features, even if they have comments. That's because we don't want users to not understand why they're not showing up. We do this even when features have comments and even when we do not know how to carry those comments over.

This is because refactoring is just a helper tool. The user wants to go to a location and make it happen, often even when it is not perfect. They can manually tweak things afterwards if necessary, or just not apply the refactoring if they don't like it.

However, if the feature doesn't even appear tehy just feel like the product is broken as there is no reason for them to think "oh, because i had a comment, that will make things unavailable".

To avoid breaking user (= losing comments)

Note: we do disable when code would truly 'break'. i..e if there's a pp-directive interspersed with the code that we don't know what to do with. However, not perfectly carrying a comment over is not considered a 'break', and we will give users the refactoring, allowing them to decide if the change is acceptable or not.

--

In other words: provide the help, allow the users to decide if they're ok with the change. Making the decision for them just makes the product seem flakey in ways that they cannot diagnose. "it works for me in this large query over here.... but not over here... well, guess this is just a buggy feature."

Note: if you are really worried about this, you can add "Warning annotations" on the comments, calling the user's attention to the fact that their comments might go missing. This is what i did with "convert for to foreach" when we allowed the user to run the feature even in circumstances that might potentially have deeper impact than they might realize. #Resolved

Thank you! There will be a design discussion soon to agree a consistent approach to this. I'll address this in the next PR after the discussion complete.

In reply to: 176271009 [](ancestors = 176271009)

multiply-assignment? that seems like a strange case to handle. #Resolved

it seems like you shoudl probably do your checks here and then call back up to 'TryConvert'. Consider, for example: return (from ...). It looks like with your current logic, you wouldn't support this. #Resolved

nit: would like this whole body extracted out. that way we can easily see the cases handled, but the handling is nicely separated. #Resolved

.IsParentKind #Resolved

really hard to read code that declares variables like this, but then sets them through some codepaths. Consider a pattern where you call something that comptues and returns these values, then use them if you got hem back. #Resolved

do you need to check if hte name is generic? i.e. (...).ToList<UhOh>() #Resolved

Thanks! ValueText for generic is still ToList. I've added a test case for this.

In reply to: 175267527 [](ancestors = 175267527)

these tyep of syntax checks would benefit form comments explaining what code pattern you're addressing. #Resolved

removed this. will use GetSymbolInfo for semantic checks

In reply to: 175267542 [](ancestors = 175267542)

ide style is to keep ); at end of previous line. #Resolved

nameof(IList.Add) #Resolved

this will not work unless "using System.Collections.Generic" is in scope. The better way to do this is to use SyntaxGenerator, or, get the type symbol you want, and call the extension .GenerateTypeSyntax on it. It will do the right thing wrt to simplification. #Resolved

Thank you! I definitely should use the full qualifier.

I tried SyntaxGenerator. It actually returns and accepts SyntaxNode not specific type, e.g. ExpressionSyntax. So, it seems to be weakly typed.

So, I think that using SyntaxFactory will be consistent. Then, we still apply Simplifier to clean up namespaces if usings are already applied.

What do you think?

In reply to: 175267578 [](ancestors = 175267578)

I tried SyntaxGenerator. It actually returns and accepts SyntaxNode not specific type,

That's a pro. Given taht it could let you use the same code for VB and C# potentially.

So, I think that using SyntaxFactory will be consistent. Then, we still apply Simplifier to clean up namespaces if usings are already applied.

Unfortunately, that's not how it works. The simplifier needs to know the original type symbols that created these types. That's what things like generator.TypeExpression do for you.

e.g. ExpressionSyntax. So, it seems to be weakly typed.

Note: in general you don't need to be strongly typed for this sort of feature. If you do need a stsrong type in some cases, you can just cast as necessary. #Resolved

#25639

In reply to: 175617788 [](ancestors = 175617788)

lastSelectExpressionTypeInfo.ConvertedType == lastSelectExpressionTypeInfo.Type [](start = 28, length = 80)

I am not sure what motivated this change. I thing this now does a reference comparison because it compares interfaces. In general, we should not assume that equal types are reference equal, this is often false for generic types, arrays, etc #Closed

Please see QueryInForEachWithConvertedType.
SymbolEquivalenceComparer returns true for C and for int.

In reply to: 178423789 [](ancestors = 178423789)

I thought we expect to differentiate C and int in this example. Correct?

In reply to: 178423950 [](ancestors = 178423950,178423789)

I thought we expect to differentiate C and int in this example. Correct?

Correct.
The comment was for the change that replaced SymbolEquivalenceComparer.Instance.Equals with ==.
But SymbolEquivalenceComparer.Instance.Equals would do what we want. I think == will fail for cases when we want the check to pass.

In reply to: 178423957 [](ancestors = 178423957,178423950,178423789)

SymbolEquivalenceComparer returns true for C and for int.

Now I am really confused. What does SymbolEquivalenceComparer.Instance.Equals do and why we are using it for the comparison below?

In reply to: 178424410 [](ancestors = 178424410,178423957,178423950,178423789)

Should we use Object.Equals to compare types instead? It looks like we are using SymbolEquivalenceComparer.Instance.Equals in several places in this file to compare types. The main point is - reference comparison is the wrong way. Object.Equals is better

In reply to: 178424954 [](ancestors = 178424954,178424410,178423957,178423950,178423789)

var declaredSymbol = _semanticModel.GetDeclaredSymbol(memberDeclarationNode); [](start = 16, length = 77)

We already found the symbol in FindParentMemberDeclarationNode, consider returning it from there as an out parameter. #Closed

if (_semanticModel.GetTypeInfo(selectClause.Expression).Type.ContainsAnonymousType()) [](start = 20, length = 85)

This feels too conservative, but we can discuss this and, if necessary, follow-up later. Just put this on the list #Closed

Added to #25639

In reply to: 178423888 [](ancestors = 178423888)

cSnnot [](start = 62, length = 6)

Typo "cSnnot"? #Closed

returnedType.OriginalDefinition?.SpecialType == SpecialType.System_Collections_Generic_IEnumerable_T [](start = 24, length = 100)

This is probably too conservative because there could be a conversion from IEnumerable<T> to the Converted type and things would work out. Let's put this on the list to discuss/follow up on later. #Closed

Thank you! added to #25639

In reply to: 178431931 [](ancestors = 178431931)

Equals(typeSymbol.OriginalDefinition, _semanticModel.Compilation.GetTypeByMetadataName(typeof(List<>).FullName)); [](start = 19, length = 113)

Do we understand why SymbolEquivalenceComparer.Instance didn't work for us? Was it not supposed to? Let's put this on the list to follow up, unless you have an answer. #Closed

Thank you! Added to #25639

In reply to: 178432101 [](ancestors = 178432101)