Foreach to For refactoring #25460

heejaechang · 2018-03-14T01:16:58Z

currently only C# implementation. working on VB.

Customer scenario

User has foreach loop and want to convert it to for loop. user puts caret on foreach loop and refactoring Lightbulb shows up and user invokes the refactoring.

Bugs this fixes

related to #8953
vso Bug : VSO bug : https://devdiv.visualstudio.com/DevDiv/_workitems/edit/590185

Workarounds, if any

No

Risk

No risk except it being new feature

Performance impact

One more refactoring that can run on caret move. but I believe it is quite cheap

Is this a regression from a previous update?

No

Root cause analysis

it is new feature

How was the bug found?

N/A

Neme12 · 2018-03-14T01:23:55Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+                // use original text
+                var foreachVariableString = foreachStatement.Identifier.ToString();
+
+                // create varialbe statement


typo: variable

Neme12 · 2018-03-14T01:25:07Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+
+        private StatementSyntax GetForLoopBody(ForeachInfo foreachInfo, ParseOptions parseOption, string collectionVariableName, string indexString)
+        {
+            var foreachStatement = (ForEachStatementSyntax)foreachInfo.ForeachStatement;


what about ForEachVariableStatementSyntax? it would be nice if that worked too, and created a deconstruction

not supported yet. will do vb first and then for to foreach and then might come back for tuple.

Neme12 · 2018-03-14T01:26:43Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+            }
+
+            // create new index varialbe name
+            var indexString = CreateUniqueMethodName(model, foreachStatement.Statement, "i");


what if "i" already exists? it's not uncommon that this would happen in a nested loop

i is just base. it will add number to make it unique.

oh i see, i don't know how i overlooked that, sorry (but CreateUniqueMethodName might be a little misleading because it's not just for methods)

agree. I should change name of the method.

Neme12 · 2018-03-14T01:27:44Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+                var foreachVariableString = foreachStatement.Identifier.ToString();
+
+                // create varialbe statement
+                var variableStatement = AddElasticAnnotation(SyntaxFactory.ParseStatement($"var {foreachVariableString} = {collectionVariableName}[{indexString}];", options: parseOption), SyntaxFactory.ElasticMarker);


please consider wrapping lines that go past github (130 characters)

also, please do not parse to make strings. just construct the tree manually using SyntaxFactory or SyntaxGenerator.

why? I dont see any benefit going tree generation here. and much straight-forward on what I am getting here.

Well, for one thing, you then had to fix things up by adding elastic annotations.

For another, we already have really good APIs for this. Like SyntaxGenerator.LocalDeclarationStatement.

For another, because your approach hardcodes in 'var' instead of respecting the user's setting.

And, finally, because no other code does this. please do not add code to the IDE that needlessly behaves differently from every other refactoring/fix we've created so far.

Neme12 · 2018-03-14T01:31:27Z

src/Features/Core/Portable/ForeachToFor/AbstractForeachToForCodeRefactoringProvider.cs

+
+            string explicitCast;
+            string countName;
+            GetInterfaceInfo(semanticFact, model, foreachVariable, foreachCollection, out explicitCast, out countName);


why not out var?

sure :) I just used extract method. I guess we need to update extract method to use new language features.

Neme12 · 2018-03-14T01:34:56Z

src/Features/Core/Portable/ForeachToFor/AbstractForeachToForCodeRefactoringProvider.cs

+                return false;
+            }
+
+            return !dataFlow.WrittenInside.Contains(foreachVariable);


how can a foreach iteration variable be written to?

it is an error in foreach. you can't write to foreach variable. but not an error in for loop (at least the way I converted it)

so, if foreach had an error and refactor to for loop, it suddenly become no error (semantic changes), so, this look for such case and not offer refactoring in such case.

actually, I'm not sure if that's a good enough reason to disallow this. This would only fix code was previously an error, and this is just a refactoring, not an analyzer, so it wouldn't be obtrusive. I can imagine this scenario: what if someone is trying to modify the iteration variable, then realizes that's not possible and therefore wants to convert it to a for loop?

This would turn an error into correct code but not in a way that's unexpected (I don't see any other interpretation of how that code could have been fixed). Many refactorings do this already. I personally think this would be ok.

we can remove this restriction later. I worry people without realizing changing meaning of code.

I'm with @Neme12 on this. The code was in error before. I think it's fine to allow the refactoring. If you're concerned about them not realizing this, you can add a ConflictAnnotation at the places where the variable is written to.

we can get feedback to see what people wants. while I was coding, I encountered this and I see benefit on not offering refactoring here.

There is no feedback that can be provided here. We've already seen thsi over and over again. People simply assume that the product is busted when they do not get features but cannot tell why. If they've written into their foreach-variable there is no reason that that should prevent htem from being able to convert to a for.

Can you please clarify why you think it's bad to let someone do this? It seems like we don't need the code at all. And, if we're going to check for this, because we do think it's important, we should at least do the right thing and add the conflict annotation rather than supressing the feature.

I am not sure why adding warning is right thing? we can open an issue and talk about it and change if we want to. but not here.

Neme12 · 2018-03-14T01:42:47Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+        var array = new int[] { 1, 3, 4 };
+        foreach [||] (var a in array)
+        {
+            a = 1;


as far as I know, this code doesn't actually compile (foreach variable can't be written to). what's the reason for this? edit: resolved

we offer refactoring for code that doesn't compile as well. I wrote the reason above.

we offer refactoring for code that doesn't compile as well

yes, that's what I would expect, but this test actually prevents that

offering refactoring on broken code and refactoring changing meaning of code is different thing. refactoring is not supposed to change meaning of code.

refactoring is not supposed to change meaning of code

actually the vast majority of refactorings do that in one way or another

In general, the only rule we really have here is that a 'large scale refactoring' really should preserve semantics. "large scale" means that it was too large enough too reasonably expect a user to have to examine the code to validate that the refactoring didn't subtly change anything. Basically, the only examples of this are "rename" and, to a lesser extent, "replace prop with methods (and vice versa).

Almost everything else we have is not really 'large scale', and is much more of a "locally targeted code transformation". In that regard we have a lot more leeway with what we offer. And we can definitely still be offered even in broken code scenarios.

This is really only transforming a the top of a 'foreach' to a 'for', and possibly introducing a variable. That's so small and targeted in scope that we can offer it even in cases where a user happened to start with the following:

foreach (var a in list) { a = 0; }

sure. everyone has their reasoning. I opened an issue tracking all design questions or handling additional cases.

#25667

should refactoring be offered even if refactoring can change semantic

I don' tthink this should even be shown as a warning, I don't really see this as "changing the semantics". Yes, if you're trying to modify the variable, the code is invalid, But the resulting code would have the exact same semantics as reassigning the foreach variable if it was allowed. The fact that you can't modify the foreach variable is just an arbitrary limitation. There's no semantics to be concerned with.

If on the other hand, we were talking about changing assignment of an array element to assignment of a local then yes, that would be a really important semantic change but I don't see how this is relevant at all. You just couldn't reassign a local before and now you can. What's the problem with that? There's no surprises here.

I am fine with going permissive way.

if we decide refactoring is more about reducing work for code transformation, then I would go even more than this actually. like there is no reason why I dont offer refactoring because type or variable, or collection is missing. or because next statement's control variable is different than foreach variable and etc.

I dont even need to check whether length or indexer is there, I can offer the refactoring and do code transformation.

still less work for users to fix it up later.

so this is where principle comes in I believe.

Neme12 · 2018-03-14T02:02:43Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    void Method()
+    {
+        var array = new int[] { 1, 3, 4 };
+        for (var {|Rename:i|} = 0; i < array.Length; i++)


is post-increment what we want here?

also: does this respect var/explicit type preferences? what if someone prefers explicit type on predefined types, therefore on int as an index, but prefered var for the element type?

for now, I made it to always use "var". didn't spend time to see how other feature did. if there is common code around this, I will follow those. #Closed

"is post-increment what we want here?"

what style do you prefer?

Keep post increment. It is idiomatic and appropriate for C#.

Shouldn't we just carry whatever type was spelled out in the for statement?
If you have for (int i ... then we'd produce foreach (int i in ... and if you have for (var i ... then we'd produce foreach (var i in ....

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

If you have for (int i ... then we'd produce foreach (int i in ...

No. These types are unrelated. one is the type of the index (normally an int). The other is the type that that the iterator returns (something random). #Closed

D'oh. My bad

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

CyrusNajmabadi · 2018-03-14T05:56:39Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.ForeachToFor
+{
+    public partial class ForeachToForTests : AbstractCSharpCodeActionTest


It's SyntaxKind.ForEachKeyword. So this should be "ForEachToFor" in all naming.

CyrusNajmabadi · 2018-03-14T05:59:20Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+{
+    void Method()
+    {
+        foreach[||](var a in new int[] { 1, 3, 4 })


have test where 'foreach' is an embedded statement. i.e. directly parented by an if statement.

that's good one. for now, I probably won't offer refactoring in that case.

why not offer?

Note: if the collection is written into, i still offer ConvertForToForEach, i just put a warning in the code.

because it needs to convert destination to block based on context. expression body, embedded statement and etc. what this is doing is basically what introduce variable refactoring is doing. currently that service is too tightly coupled to refactoring. we probably support it by refactoring that service and use that service here to extract out to local and work on that code.

tracking in this issue - #25667

CyrusNajmabadi · 2018-03-14T05:59:42Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    {
+        var list = 1;
+
+        var {|Rename:list1|} = new int[] { 1, 3, 4 };


i feel like the name should be "array" not "list".

well, I can change it to collection.

Please do not. it is more idiomatic for List/ReadOnlyList to be called "list" and for Array/ImmutableArray to be called "array".

do we have name generator that generate proper name?

CyrusNajmabadi · 2018-03-14T06:00:24Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    void Method()
+    {
+        var array = new int[] { 1, 3, 4 };
+        [||] foreach (var a in array)


this seems undesirable. i would require the caret be on the foreach.

could be. but will leave it as it is for now.

Please do not. We do not want features liek this to light up in too large a span. We've done a lot of work to minimize that sort of thing. For example, this would make the feature show up in a comment on top of the foreach. That's not desirable. The feature should appear when the caret intersects 'foreach'.

Optionally, it can also appear inside [|foreach (|] and also potentially after the close paren of the foreach. But it should not appear inside the span of the locals/condition/increment.

I opened issue tracking this design question.
#25667

CyrusNajmabadi · 2018-03-14T06:00:39Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    void Method()
+    {
+        var array = new int[] { 1, 3, 4 };
+        foreach (var a in array) [||] 


also seems undesirable.

Same reason as above. We did a lot of work to narrow down all our refactorings that were providing too large a span to be activated on.

tracking here - #25667

CyrusNajmabadi · 2018-03-14T06:01:27Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    }
+}
+
+class List : IList


why did you need these?

so, I can test IList case

I don't understand. IList is part of the framework. Why did you need to duplicate all this code here?

because I want to test IList. one in the framework implements multiple interfaces.

Sorry, i'm not getting it. If you want to test IList, then just use IList. IList doesn't implement anything, it's an interface. In otherwords, just write it like so:

+class Test +{ + void Method() + { + var list = default(IList); + foreach [||](var a in list) + { + Console.WriteLine(a); + } + + } +}

This point still stands.

I am fine with the test.

I have test where list is interface type as well.

CyrusNajmabadi · 2018-03-14T06:02:18Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    void Method()
+    {
+        var list = new Explicit();
+        var {|Rename:list1|} = (IReadOnlyList<int>)(list);


parens around '(list)' should not be there. Incorrect simplification?

need to put simplification and nosimpliciation annotation. didn't tweak much there yet.

please add that.

CyrusNajmabadi · 2018-03-14T06:02:42Z

src/EditorFeatures/CSharpTest/ForeachToFor/ForeachToForTests.cs

+    }
+}
+
+class Explicit : IReadOnlyList<int>, IReadOnlyList<string>


please extract all this duplication into a helper string you can insert into these tests.

this is part of test. without this, how user know what it is testing?

CyrusNajmabadi · 2018-03-14T06:03:05Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+    [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = nameof(CSharpForeachToForCodeRefactoringProvider)), Shared]
+    internal sealed class CSharpForeachToForCodeRefactoringProvider : AbstractForeachToForCodeRefactoringProvider
+    {
+        public CSharpForeachToForCodeRefactoringProvider()


constructor not needed.

CyrusNajmabadi · 2018-03-14T06:03:29Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+            }
+
+            // support refactoring only if caret is in between "foreach" and ")"
+            var scope = TextSpan.FromBounds(foreachStatement.ForEachKeyword.Span.Start, foreachStatement.CloseParenToken.Span.End);


i would only allow it if it intersects the foreach span.

why? I am not sure which other refactoring work that way?

For example: convert if-to-switch:

// To prevent noisiness, only show this feature on the 'if' keyword of the if-statement. var token = ifStatement.GetFirstToken(); if (!token.Span.Contains(context.Span)) { return; }

And teh reason is as i specified above. because otherwise it is too noisy.

tracking issue - #25667

Note: if you do this, can you please show what the experience is if someone goes to the "var" in "foreach (var x in y)" and brings up the lightbulb. We want to make sure that the "convert to named type" refactoring is above the "convert foreach to for" refactoring. Note: this is about the refactorings. not anyhting to do with analyzers.

good one to add to design question.

added one more design question - #25667 (comment)

now we offering refactoring LB same way we offering code fix, it might be good to add PreferableSpan to Refactoring api so that we can order refactoring as we do for fixers.

CyrusNajmabadi · 2018-03-14T06:04:33Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+
+            // check whether there is any comments between foreach and ) tokens
+            // if they do, we don't support conversion.
+            foreach (var trivia in foreachStatement.DescendantTrivia(n => n == foreachStatement || scope.Contains(n.FullSpan)))


i don't think this is necessary. we can support it. it'll just lose some comments. As this is a refactoring and not a fix, i don't think that's an issue. If user's run into reasonable comment cases that are worth supporting, we can adapt the code to do the right thing if it's important enough.

again, we can see how many people complaining about it and support it.

This is not how we generally do IDE features. We don't go and explicitly disable waiting for feedback. We are permissive except for the issue that we know we can't address. IN this case, we can support these cases, but you've gone and added code to explicitly disable the feature. Very few features do this, and only for extremely important cases where the fix would actually be very bad.

Why not support conversion? If really you want to preserve the comments, just put them anywhere, it's doesn't really matter that much. I don't see a problem here. For example:

foreach /*a*/ (/*b*/ var /*c*/ x /*d*/ in /*e*/ list /*f*/) /*g*/

could be converted to:

for /*a*/ (/*b*/ int i = 0; i < list.Count; i++ /*f*/) /*g*/ { var /*c*/ x /*d*/ = /*e*/ list[i]; }

(it could even be different to whatever makes the code that handles this nicer)

If users don't like what we did, they can adjust the result themselves but we could still save half their work by doing the conversion. Once people know about this refactoring, they'll expect it to show up everywhere and blame the IDE for not working if it's not offered.

tracking issue here - #25667

CyrusNajmabadi · 2018-03-14T06:05:36Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+            cancellationToken.ThrowIfCancellationRequested();
+
+            var parseOption = model.SyntaxTree.Options;
+            var foreachStatement = (ForEachStatementSyntax)foreachInfo.ForeachStatement;


why is ForeachStatement not typed as ForEachStatementSyntax? (Also, to reiterate a previous point. We should use the name ForEach in all this code as that's the Roslyn naming style for this construct).

because type in Core can't use language specific type.

You are in CSharpForeachToForCodeRefactoringProvider.cs

Please look at our other refactorings. You are not following the patterns or practices set out in them.

CyrusNajmabadi · 2018-03-14T06:06:08Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+            var collectionVariableName = foreachCollectionExpression.ToString();
+            if (foreachInfo.RequireCollectionStatement)
+            {
+                collectionVariableName = CreateUniqueMethodName(model, foreachStatement, "list");


the name should likely be based on the type of the item we were foreaching. So "list" or "array".

well. that can be improved later. I will rename it as collection.

Please do not. If you want to actually add this code to the IDE then we should be doing teh appropriate level of work to create features that fit what users will want. We should name these "list" and "array" as appropriate.

do we have code that return best name based on context?

CyrusNajmabadi · 2018-03-14T06:06:47Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+
+                var collectionStatement = SyntaxFactory.LocalDeclarationStatement(
+                    SyntaxFactory.VariableDeclaration(
+                        SyntaxFactory.IdentifierName("var"),


Should use var/type-name depending on the user's preference.

Note: consider just using SyntaxGenerator to simplify this a lot.

I looked SyntaxGenerator.LocalDeclaration and it didn't use Options so that seems still not pick "var" or user types based on options. for now, I didn't do "var" or explicit type thing since I couldn't find one central stuff that does it for me. will look UseExplicitType refactoring rather to see what it does.

I will use the TypeStyleHelper stuff you added in your PR once that is checked in

CyrusNajmabadi · 2018-03-14T06:06:56Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+                        SyntaxFactory.SingletonSeparatedList(
+                            SyntaxFactory.VariableDeclarator(
+                                SyntaxFactory.Identifier(collectionVariableName).WithAdditionalAnnotations(RenameAnnotation.Create()),
+                                default,


named argument.

CyrusNajmabadi · 2018-03-14T06:07:09Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+                                SyntaxFactory.Identifier(collectionVariableName).WithAdditionalAnnotations(RenameAnnotation.Create()),
+                                default,
+                                SyntaxFactory.EqualsValueClause(
+                                    foreachInfo.RequireExplicitCast ?


ide style is ? and : on hte next line, aligned.

CyrusNajmabadi · 2018-03-14T06:07:32Z

src/Features/CSharp/Portable/ForeachToFor/CSharpForeachToForCodeRefactoringProvider.cs

+                                SyntaxFactory.EqualsValueClause(
+                                    foreachInfo.RequireExplicitCast ?
+                                        SyntaxFactory.CastExpression(
+                                            SyntaxFactory.ParseTypeName(foreachInfo.ExplicitCastInterface).WithAdditionalAnnotations(Simplifier.Annotation),


Please dont' parse type names. We can always go from a type symbol to a TypeSyntax using existing helpers.

I am not sure why you dont want to parse? there is nothing wrong parsing snippet of code rather than building them using SyntaxFactory.

Because it does not do the right thigns (like add the appropriate annotations). Parsing means your'e dependent on what ToDisplayString produces. it means that things like "AddUsing" don't work properly. And whatnot.

We already built entire APIs to support this scenario and we use them in all our refactorings and analyzers. Please do not create code that deviates from the patterns and practices of hte IDE for no reason.

These APIs work well and they do the right thing wrt to the entire code-action stack. They put the right simplification annotations in place and they are where we can go and fix things such that they then apply to the entire codebase.

For example, if you use .LocalDeclarationStatement then that is a place where we can update it to use 'var' or an explicit-type, and then all features benefit.

there is nothing wrong

Yes there is. features that are coded differently and work differently than the rest of the IDE act as a tax on future maintainers. It means that instead of having uniform patterns and approaches to solving problems, the maintainer has to figure out why things are done differently. That's why we create supplementary APIs to address these common patterns, and why we want all our features to use them. please do not be different here. This feature is not special and does not warrant side-stepping the infrastructure we've spent over a decade building.

This is the philosophy we take with all our code action features (regardless of whether they are internally or externally contributed).

and that's why I telling people it is completely fine parsing some snippet rather than creating this verbose not intuitive chunk of tree building.

And that's why I telling people it is completely fine parsing some snippet

I literally listed several reasons above why it is not "completely fine". You yourself encountered one of those reasons yourself when your parsed snippet wouldn't format properly because it didn't contain the right trivia on it that the rest of our stack expects and works with.

Again, 100% of roslyn IDE code works this way. This is the way we recommend to users that things be done. It's the way that ensures that our stack works as uniformly and consistently as possible. Please, do not deviate from the rest of the IDE code. It is deliberate and intentional how it works, and deviations just end up being bugs and costs down hte line for whoever has to maintain 'ForEachToFor'.

CyrusNajmabadi · 2018-03-28T18:31:50Z

all looks legal but will check.

Thanks! If legal, then i'm ok with it.

I am not changing GenerateNameForExpression (http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/LanguageServices/SemanticsFactsService/ISemanticFactsService.cs,f1de32a188c50d1c,references) it is using to generate the name.

I'm not sure i understand. If you're not changing it... then why did the tests change? :)

heejaechang · 2018-03-28T18:51:47Z

GenerateNameForExpression creates base name => getHashCode from given node. and then CreateUniqueLocalName ensures that the name is unique which in old code, returns getHashCode1. now it returns getHashCode since it doesn't care about method symbol.

…eachrefactoring

heejaechang · 2018-03-28T23:23:45Z

@jinujoseph okay to checkin with integration failure? or do I need to make integration test to pass? I dont think the failure is related to mine.

CyrusNajmabadi · 2018-03-28T23:30:05Z

There is still outstanding PR feedback to be addressed.

jinujoseph · 2018-03-28T23:40:51Z

integration test failures is known issue so ok to checkin
But looks like this is still few things called out to be addressed.

@CyrusNajmabadi trying to find a mid path between this feature takes care of every scenario vs you get nothing. Unless there is a broken feature experience, must fix... would prefer to do a iterative approach and bring the changes in next PR. Let me know what you think.

CyrusNajmabadi · 2018-03-28T23:51:50Z

@jinujoseph There are basic PR thigns i would like to still see done. Like making sure that file names and folders match.

@CyrusNajmabadi trying to find a mid path between this feature takes care of every scenario vs you get nothing.

I'm not asking for all or nothing :)

There are def things i can think come later. (like if we want to really punt on trivia). However, there are very basic things i've mentioned in the PR that just make sense to do now as they're trivial and just more appropriate/correct.

In other words, instead of having extra code to do things not-quite-right, i've proposed changes that get things better while being more idiomatic and in line with how we generally have done transforming refactorings.

CyrusNajmabadi · 2018-03-28T23:59:18Z

src/EditorFeatures/VisualBasicTest/ForeachToFor/ForeachToForTests.vb

+' Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+
+Imports Microsoft.CodeAnalysis.CodeRefactorings


Still applies

heejaechang · 2018-03-29T00:01:25Z

git on windows doesn't picking up case changes. tried and didn't work. I am not going to spend too much time just to change casing. I tried it and git is not picking that up.

CyrusNajmabadi · 2018-03-29T00:01:44Z

...ures/CSharp/Portable/ConvertForEachToFor/CSharpConvertForEachToForCodeRefactoringProvider.cs

+                model, generator, foreachInfo, foreachCollectionExpression, cancellationToken);
+
+            var collectionStatementType = foreachInfo.RequireExplicitCastInterface
+                ? generator.GetTypeExpression(foreachInfo.Options, foreachInfo.ExplicitCastInterface) :


IDE almost always aligns ? and : when wrapping ternaries. can you please keep this style like that?

CyrusNajmabadi · 2018-03-29T00:05:37Z

@jinujoseph I went through and checked and only had some code consistency issues, as well as some questions about some of the changes. I think i'm fine if all of those can be addressed/answered.

CyrusNajmabadi · 2018-03-29T00:06:36Z

git on windows doesn't picking up case changes. tried and didn't work. I am not going to spend too much time just to change casing. I tried it and git is not picking that up.

I can make the changes for you. Let me fetch things. Give me a few minutes.

CyrusNajmabadi · 2018-03-29T00:14:27Z

PR with the changes is here: heejaechang#7

Rename files.

heejaechang · 2018-03-29T00:16:41Z

thank you! for rename.

heejaechang · 2018-03-29T00:17:39Z

That's just formatting....

CyrusNajmabadi · 2018-03-29T00:18:32Z

Classified as Microsoft Highly Confidential

Uh oh!! :D

CyrusNajmabadi · 2018-03-29T00:40:03Z

If this is just indentaiton changes, i'm ok with this going in. I really do hope we actually address a lot of hte design feedback soonish, and we don't jsut punt it down hte line .

jinujoseph · 2018-03-29T06:15:07Z

Thanks much @CyrusNajmabadi for review and working through this.

heejaechang · 2018-03-29T09:13:18Z

retest windows_release_unit32_prtest please

Pilchie · 2018-03-29T15:24:45Z

Classified as Microsoft Highly Confidential

Uh oh!! :D

It's an Outlook plugin that MSIT installed for us to try to get people to categorize mails. They had it configured to default to "Highly Confidential" yesterday for some reason...

heejaechang · 2018-03-30T08:32:15Z

ping @jinujoseph ?

jinujoseph · 2018-03-30T19:07:45Z

Thanks @CyrusNajmabadi
@heejaechang lets merge once all the feedback and concern has been captures in issue

CyrusNajmabadi · 2018-03-30T19:16:16Z

Sorry. Forgot to actually sign-off :)

* dotnet/dev15.7.x: Foreach to For refactoring (dotnet#25460) added more logging for completion to track some test failures where completion doesn't show up when it should have. (dotnet#25801) Add suggested stackalloc initializer tests (dotnet#25656) Addressed PR feedback. [SQLite] Fix continuous probing for sqlite Add missing file, update test for clearer output. Addressed PR feedback, updated ILocalFunctionOperation to final design specified in review. Added both ExpressionBody and BlockBody to the underlying BoundNode for local functions, and exposed both to IOperation consumers. Update PATH to our xcopy CLI

heejaechang added 3 commits March 13, 2018 15:16

foreach to for for C#

4ee5d12

use different method

0689099

no ctor needed

eabf902

heejaechang requested a review from a team as a code owner March 14, 2018 01:16

Neme12 reviewed Mar 14, 2018

View reviewed changes

jcouv added the Area-IDE label Mar 14, 2018

Neme12 reviewed Mar 14, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 14, 2018

View reviewed changes

heejaechang added 2 commits March 28, 2018 12:01

Merge branch 'dev15.7.x' of https://github.com/dotnet/roslyn into for…

41b0dd9

…eachrefactoring

fix up test failures due ot GenerateUniqueLocalName changes.

1d06d58

CyrusNajmabadi requested changes Mar 28, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 29, 2018

View reviewed changes

Rename files.

e2e3e2d

Merge pull request #7 from CyrusNajmabadi/foreachrefactoring

c3201d0

Rename files.

CyrusNajmabadi approved these changes Mar 30, 2018

View reviewed changes

heejaechang merged commit bd72846 into dotnet:dev15.7.x Mar 30, 2018

kuhlenh mentioned this pull request Mar 30, 2018

Analyzers and code fixes for converting Loop Family #8953

Open

10 tasks

		' Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.


		Imports Microsoft.CodeAnalysis.CodeRefactorings

Foreach to For refactoring #25460

Foreach to For refactoring #25460

Conversation

heejaechang commented Mar 14, 2018 • edited by jinujoseph

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 22, 2018 • edited

Choose a reason for hiding this comment

heejaechang Mar 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

heejaechang Mar 14, 2018 • edited by jcouv

Choose a reason for hiding this comment

heejaechang Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 14, 2018 •

edited by jinujoseph

Neme12 Mar 14, 2018 •

edited

Neme12 Mar 14, 2018 •

edited

Neme12 Mar 14, 2018 •

edited

Neme12 Mar 14, 2018 •

edited

Neme12 Mar 14, 2018 •

edited

CyrusNajmabadi Mar 14, 2018 •

edited

Neme12 Mar 22, 2018 •

edited

heejaechang Mar 23, 2018 •

edited

Neme12 Mar 14, 2018 •

edited

heejaechang Mar 14, 2018 •

edited by jcouv

heejaechang Mar 14, 2018 •

edited

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Neme12 Mar 14, 2018 •

edited