Add a refactoring to convert a common for-loop form to a foreach-statement. #25469

CyrusNajmabadi · 2018-03-14T19:40:45Z

Todo:

VB
Tests

Neme12 · 2018-03-14T19:45:11Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+            //  x += constant_1
+
+            ExpressionSyntax operand;
+            if (incrementor.Kind() == SyntaxKind.PostIncrementExpression)


switch? #Resolved

Neme12 · 2018-03-14T19:47:28Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+                if (TypeStyleHelper.IsImplicitStylePreferred(
+                        optionSet, isBuiltInTypeContext, isTypeApparentContext: false))
+                {
+                    typeNode = SyntaxFactory.IdentifierName("var");


duplication #Resolved

sorry, don't know what this means. #Resolved

could you use ?:? #Resolved

typeNode = is duplicated in each branch #Resolved

it feels like it would be very large and unweildy. I found this to read better. #Resolved

we don't have a general rule of avoiding hte pattern of:

SomeVar v; // initialize through branchign code // use v

Devs decide on a case by case basis. #Resolved

this is nice. I should reuse this.

Neme12 · 2018-03-14T19:49:02Z

src/Workspaces/CSharp/Portable/CodeStyle/TypeStyle/TypeStyleHelper.cs


+        private static bool IsImplicitStylePreferred(TypeStylePreference stylePreferences,


i thought we weren't supposed to format this way?

Neme12 · 2018-03-14T19:56:32Z

...ures/Core/Portable/ConvertForToForEach/AbstractConvertForToForEachCodeRefactoringProvider.cs

+
+            // position has to be inside the 'for' span, or if there is a selection, it must
+            // match the 'for' span exactly.
+            if (context.Span.IsEmpty && !token.Span.IntersectsWith(context.Span.Start))


i would personally also expect this offered after ')'

Neme12 · 2018-03-14T19:57:37Z

...ures/Core/Portable/ConvertForToForEach/AbstractConvertForToForEachCodeRefactoringProvider.cs

+            }
+
+            var forStatement = token.Parent as TForStatementSyntax;
+            if (forStatement == null)


pattern matching?

CyrusNajmabadi · 2018-03-14T19:57:49Z

@Neme12 This has not been submitted for review. it is a WIP (work in progress). I will let you know when it's ready :)

CyrusNajmabadi · 2018-03-14T20:10:38Z

Feature looks like:

it also supports figuring out the foreach variable if the user declared it in the for loop:

CyrusNajmabadi · 2018-03-14T20:52:53Z

@heejaechang I've extracted out the helper to generate a unique name token in a specific context. you can port that over if you want.

CyrusNajmabadi · 2018-03-14T21:41:17Z

VB side:

CyrusNajmabadi · 2018-03-14T22:11:31Z

VB is done. Tests coming now.

@dotnet/roslyn-ide @heejaechang @Neme12 @jinujoe this is ready for review now.

Neme12 · 2018-03-14T23:13:54Z

...ures/Core/Portable/ConvertForToForEach/AbstractConvertForToForEachCodeRefactoringProvider.cs

+                return;
+            }
+
+            var forStatement = token.Parent.AncestorsAndSelf().OfType<TForStatementSyntax>().FirstOrDefault();


this is a little weird. isn't there something like FirstAncestorOrSelf<> or GetAncestorOrSelf<> you could use? #Resolved

yup. #resolved #Resolved

Neme12 · 2018-03-14T23:15:49Z

src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSemanticFactsService.vb

@@ -290,5 +293,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic

            Return {semanticModel.GetDeclaredSymbol(memberDeclaration, cancellationToken)}
        End Function
+
+        Private Function ISemanticFactsService_GenerateUniqueName(semanticModel As SemanticModel, location As SyntaxNode, containerOpt As SyntaxNode, baseName As String, cancellationToken As CancellationToken) As SyntaxToken Implements ISemanticFactsService.GenerateUniqueName


line way too long

in this type, considering so many methods are simply to just represent the interface signature, we don't wrap and just forward things along. I agree it's not consistent, but it helps keep this type from literally becoming 5x as long and having the signatures mask what's actually relevant.

Neme12 · 2018-03-14T23:17:30Z

src/Workspaces/Core/Portable/LanguageServices/SemanticsFactsService/ISemanticFactsService.cs

+    {
+        protected abstract ISyntaxFactsService SyntaxFactsService { get; }
+
+        public SyntaxToken GenerateUniqueName(


would a better place for this be inside NameGenerator?

NameGenerator is designed to be Roslyn agnostic. i.e. it's literally jsut string manipulation. we've deliberately kept it that way so that partners can use it/copy it trivially.

Neme12 · 2018-03-14T23:25:22Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+        protected override bool TryGetForStatementComponents(
+            ForStatementSyntax forStatement,
+            out SyntaxToken iterationVariable, out ExpressionSyntax initializer,
+            out MemberAccessExpressionSyntax memberAccess, out ExpressionSyntax stepValue,


it's a little confusing to use 'stepValue' for both the value and the expression it's calculated from, consider adding 'expression' to the name or differentiating it in some other way #Resolved

Sure. #Resolved

#resolved #Resolved

Neme12 · 2018-03-14T23:32:08Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+                            identifierName.Identifier.ValueText == iterationVariable.ValueText &&
+                            binaryExpression.Right is MemberAccessExpressionSyntax)
+                        {
+                            memberAccess = (MemberAccessExpressionSyntax)binaryExpression.Right;


i see waht's going on here and it's not very nice, but i can't think of any way how to improve this #WontFix

#won'tfix #Resolved

can these move to abstract one and use IOperation rather than doing it on language specific way?

I don't think so. Vb and C# literally uses different types here (IForToLoop vs IForLoop). So you'd have to write custom code anyways. Given that, i don't mind explicitly writing this at the language level since each language is different.

Also, i tried out hte IOperation approach and i had to jump through lots of hoops due to conversion operations in the code. It was actually more annoying.

Neme12 · 2018-03-14T23:33:16Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+                            memberAccess = (MemberAccessExpressionSyntax)binaryExpression.Right;
+
+                            var incrementor = forStatement.Incrementors[0];
+                            if (TryGetStepValue(iterationVariable, incrementor, out stepValue, cancellationToken))


can you do return TryStepGetValue?

Neme12 · 2018-03-14T23:35:41Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+            ExpressionSyntax operand;
+            switch (incrementor.Kind())
+            {
+            case SyntaxKind.PostIncrementExpression:


it makes me sad that there's no UnaryExpressionSyntax that would let us merge these cases #WontFix

#won'tfix #Resolved

Neme12 · 2018-03-14T23:42:13Z

...ures/CSharp/Portable/ConvertForToForEach/CSharpConvertForToForEachCodeRefactoringProvider.cs

+
+            return operand is IdentifierNameSyntax identifierName &&
+                identifierName.Identifier.ValueText == iterationVariable.ValueText;
+        }


few ideas:

could you use pattern matching instead of the casts? for assignment you'd have to use when .IsKind(), do you think that to be an improvement or not?

i don't like this imperative style with variables outside assigned from each case with a break everywhere, it seems like this would deserve maybe a local function or a method that could have a single return from each case with a tuple of (operand, stepValue)... in default it would return a null operand and the operand is IdentifierNameSyntax check would still work as expected... but this is all up to you, I'm just throwing some suggestions/ideas

i'm ok with it as it is. If we had a match expression, i might consider using that. But i'm ok with things as they are here.

CyrusNajmabadi · 2018-03-15T00:59:55Z

tests added. @dotnet/roslyn-ide please review at your earliest convenience.

CyrusNajmabadi · 2018-03-15T01:12:33Z

Note: one of the best things i could get from reviewers are potentially interesting cases i missed in both languages. VB can be especially surprising with all the forms it supports :)

CyrusNajmabadi · 2018-03-15T01:15:51Z

@heejaechang Can you review? (in exchange for the reviews i did :))

CyrusNajmabadi · 2018-03-15T20:01:45Z

@jcouv Woudl appreciate your eyes here.

CyrusNajmabadi · 2018-03-15T20:02:09Z

@Pilchie This refactoring seems very safe for 15.7. Woudl you want it there, or would 15.8 make sense?

Pilchie · 2018-03-15T20:26:28Z

I believe we want it in 15.7 as long as it's code reviewed and buddy tested in within the next week. @jinujoseph / @kuhlenh

CyrusNajmabadi · 2018-03-15T20:27:40Z

That would be great! The ball's in your court :)

jcouv · 2018-03-15T21:18:26Z

src/EditorFeatures/CSharpTest/ConvertForToForEach/ConvertForToForEachTests.cs

+    {
+        [||]for (int i = 0; i < array.Length; i++)
+        {
+            Console.WriteLine(array[i]);


array[i] [](start = 30, length = 8)

Consider adding a test where array[i] appears more than once in the foreach block.

CyrusNajmabadi · 2018-03-22T19:50:18Z

Overall LGTM. I have some comments about handling certain additional (and uncommon) bail out cases. Also had some comments about use of IOperation when analyzing semantics to simplify code, but existing code is also fine. Great work @CyrusNajmabadi!

Thanks! I'm going to add some more checks and warnings for potential mutations to the collection. I think that's very reasonable!

CyrusNajmabadi · 2018-03-22T21:19:13Z

@jinujoseph PR feedback addressed. this can go in once green.

CyrusNajmabadi · 2018-03-23T00:47:05Z

@dotnet/roslyn-infrastructure I'm getting a failure on the new spanish unit test build:

error MSB6002: The command-line for the "CreatePkgDef" task is too long. Command-lines longer than 32000 characters are likely to fail. Try reducing the length of the command-line by breaking down the call to "CreatePkgDef" into multiple calls with fewer parameters per call. [D:\j\workspace\windows_debug---51589b87\src\VisualStudio\IntegrationTest\TestSetup\VisualStudioIntegrationTestSetup.csproj]

Does anyone know what this is about?

CyrusNajmabadi · 2018-03-23T00:48:15Z

@tmeschter @jasonmalinowski do you know anything about that msbuild error?

jasonmalinowski · 2018-03-23T01:19:07Z

@CyrusNajmabadi That queue is still experimental, safe to ignore.

CyrusNajmabadi · 2018-03-23T01:20:30Z

oh thanks. i missed that.

CyrusNajmabadi · 2018-03-23T01:23:37Z

retest windows_debug_unit32_prtest please

jinujoseph · 2018-03-23T02:27:09Z

@Pilchie for ask mode approval

CyrusNajmabadi · 2018-03-23T08:08:52Z

@jinujoseph This has passed and can be merged in. All the failures are the current issue with "error MSB6002: The command-line for the "CreatePkgDef" task is too long."

jinujoseph · 2018-03-23T08:11:20Z

thanks @CyrusNajmabadi , this is pending approval from @Pilchie

Pilchie · 2018-03-23T15:49:24Z

Approved - thanks @CyrusNajmabadi

jinujoseph · 2018-03-23T16:36:43Z

@333fred test failures expected ?

jinujoseph · 2018-03-23T16:41:42Z

test windows_release_vs-integration_prtest

CyrusNajmabadi · 2018-03-23T18:12:17Z

@jinujoseph It's not a test failure. The error with integration tests (as i mentioned above) is:

error MSB6002: The command-line for the "CreatePkgDef" task is too long.
Command-lines longer than 32000 characters are likely to fail.

I don't know what can be done about this. Thoughts @333fred @jasonmalinowski

--

Jason, this is not the spanish queue. This is hte integration test queue. I'm not sure i feel comfortable having my stuff not go through integration testing, even if htat queue isn't required.

jasonmalinowski · 2018-03-23T18:17:37Z

The integration test queues are in need of the same fix as the Spanish queue. You can disregard those failures.

CyrusNajmabadi · 2018-03-23T18:25:26Z

Thanks @jasonmalinowski . @jinujoseph This can be merged in when convenient for you.

CyrusNajmabadi · 2018-03-23T18:30:32Z

Thanks all. This is going to be a great release!

Pilchie · 2018-03-23T18:38:16Z

Agreed - so excited.

CyrusNajmabadi requested a review from a team as a code owner March 14, 2018 19:40

Neme12 reviewed Mar 14, 2018

View reviewed changes

CyrusNajmabadi changed the title ~~WIP - Initial work to support converting a for-statement to a foreach.~~ Add a refactoring to convert a common for-loop form to a foreach-statement. Mar 14, 2018

Neme12 reviewed Mar 14, 2018

View reviewed changes

jinujoseph added the Area-IDE label Mar 15, 2018

jinujoseph added this to the 15.7 milestone Mar 15, 2018

jcouv reviewed Mar 15, 2018

View reviewed changes

CyrusNajmabadi added 2 commits March 22, 2018 12:58

Use IOperation check.

414221e

Add checks for unexpected usages of hte collection.

d833c69

CyrusNajmabadi added 2 commits March 22, 2018 16:32

Fix tests with incorrect baselines.

0fb504f

Fix tests with incorrect baselines.

905b2ba

jinujoseph closed this Mar 23, 2018

jinujoseph reopened this Mar 23, 2018

Pilchie added the Approved to merge label Mar 23, 2018

jinujoseph merged commit 0b35308 into dotnet:dev15.7.x Mar 23, 2018

This was referenced Mar 27, 2018

Fix failure to initialize RemoteWorkspace in the out-of-process host #25691

Merged

Preview3 rollup #25747

Merged

kuhlenh mentioned this pull request Mar 30, 2018

Analyzers and code fixes for converting Loop Family #8953

Closed

10 tasks

CyrusNajmabadi deleted the convertForToForEach branch January 25, 2020 00:46


		private static bool IsImplicitStylePreferred(TypeStylePreference stylePreferences,

Add a refactoring to convert a common for-loop form to a foreach-statement. #25469

Add a refactoring to convert a common for-loop form to a foreach-statement. #25469

Conversation

CyrusNajmabadi commented Mar 14, 2018 • edited Loading

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

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 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 14, 2018

CyrusNajmabadi commented Mar 14, 2018 • edited Loading

CyrusNajmabadi commented Mar 14, 2018

CyrusNajmabadi commented Mar 14, 2018

CyrusNajmabadi commented Mar 14, 2018

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

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 by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

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 by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 14, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Neme12 Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 15, 2018

CyrusNajmabadi commented Mar 15, 2018

CyrusNajmabadi commented Mar 15, 2018

CyrusNajmabadi commented Mar 15, 2018

CyrusNajmabadi commented Mar 15, 2018

Pilchie commented Mar 15, 2018

CyrusNajmabadi commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 22, 2018 • edited Loading

CyrusNajmabadi commented Mar 22, 2018

CyrusNajmabadi commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

jasonmalinowski commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

jinujoseph commented Mar 23, 2018

CyrusNajmabadi commented Mar 23, 2018

jinujoseph commented Mar 23, 2018

CyrusNajmabadi commented Mar 14, 2018 •

edited

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 14, 2018 •

edited

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Mar 14, 2018 •

edited by jcouv

Loading

Neme12 Mar 14, 2018 •

edited

Loading

CyrusNajmabadi commented Mar 22, 2018 •

edited

Loading