Semantic snippets - for loop snippet #62990

akhera99 · 2022-07-27T19:50:47Z

The next snippet in the long list of snippets to implement :)

This also checks for the option set for var for built in types in the editorconfig.

davidwengier · 2022-07-27T22:40:16Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+
+            // Creating the variable declaration based on if the user has
+            // 'var for built in types' set in their editorconfig.
+            var variableDeclarationSyntax = varBuiltInType


Could the conditional be used to create the varIdentifier? Seems like everything other than the first parameter to SyntaxFactory.VariableDeclaration here is identical.

davidwengier · 2022-07-27T22:40:51Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                            indexVariable,
+                            argumentList: null,
+                            SyntaxFactory.EqualsValueClause((ExpressionSyntax)generator.LiteralExpression(0)))))
+                : SyntaxFactory.VariableDeclaration(compilation.GetSpecialType(SpecialType.System_Int32).GenerateTypeSyntax(allowVar: false),


I'm curious, what does allowVar do here? Can it just be set to varBuildInType and does the syntax generator do magic for you then?

@davidwengier To my knowledge, it adds DoNotAllowVarAnnotation.Annotation. This annotation is used by CSharpUseImplicitTypeHelper.

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.cs

Lines 60 to 61 in a84cae0

if (!allowVar)

syntax = syntax.WithAdditionalAnnotations(DoNotAllowVarAnnotation.Annotation);

roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/TypeStyle/CSharpUseImplicitTypeHelper.cs

Lines 38 to 41 in a84cae0

if (typeName.HasAnnotation(DoNotAllowVarAnnotation.Annotation))

{

return default;

}

One important use of CSharpUseImplicitTypeHelper is CSharpVarReducer, which is run after every CodeAction is applied.

I think no code actions are involved here, so most likely this annotation will do nothing?

I'm curious though why we will ever want to put a DoNotAllowVarAnnotation.Annotation

@davidwengier Note that if this was a CodeAction (e.g, in a CodeFixProvider or CodeRefactoringProvider), we can always generate a non-var TypeSyntax and add Simplifier.Annotation, and it will magically get converted to var if user prefers it.

See dotnet/roslyn-analyzers#6023 :)

Ooh snippets are calling CleanupDocumentAsync:

roslyn/src/Features/Core/Portable/Snippets/SnippetProviders/AbstractSnippetProvider.cs

Line 106 in a84cae0

var reformattedDocument = await CleanupDocumentAsync(formatAnnotatedSnippetDocument, cancellationToken).ConfigureAwait(false);

So things I was saying specific to CodeActions might be applicable to snippets as well.

@akhera99 Can you try to unconditionally produce compilation.GetSpecialType(SpecialType.System_Int32).GenerateTypeSyntax(allowVar: true).WithAdditionalAnnotations(Simplifier.Annotation)?

The CleanupDocumentAsync call should be able to convert to var if user prefers that.

@Youssef1313 yep you were correct, thanks :)

davidwengier · 2022-07-27T23:06:08Z

...SharpTest/Completion/CompletionProviders/Snippets/CSharpForSnippetCompletionProviderTests.cs

+        var x = 5;
+        void LocalMethod()
+        {
+            for (global::System.Int32 i = 0; (i) < (length); i++)


Why does it look so different when in a local function?

@davidwengier Known bug #58135 😕

Youssef1313 · 2022-07-28T06:46:17Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+using Microsoft.CodeAnalysis.Snippets.SnippetProviders;
+using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
+using static Humanizer.In;


Is this used?

Youssef1313 · 2022-07-28T06:46:24Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+namespace Microsoft.CodeAnalysis.CSharp.Snippets
+{
+    [ExportSnippetProvider(nameof(ISnippetProvider), LanguageNames.CSharp), Shared]
+    internal class CSharpForLoopSnippetProvider : AbstractForLoopSnippetProvider


Suggested change

internal class CSharpForLoopSnippetProvider : AbstractForLoopSnippetProvider

internal sealed class CSharpForLoopSnippetProvider : AbstractForLoopSnippetProvider

Youssef1313 · 2022-07-28T07:08:19Z

src/Features/Core/Portable/FeaturesResources.resx

+  <data name="Insert_a_for_loop" xml:space="preserve">
+    <value>Insert a 'for' loop</value>
+  </data>


Will the same string be used for VB in future?

If not, consider moving to CSharpFeaturesResources

Youssef1313 · 2022-07-28T07:24:31Z

...SharpTest/Completion/CompletionProviders/Snippets/CSharpForSnippetCompletionProviderTests.cs

+        for (int i = 0; i < length; i++)
+        {$$
+        }


I think it is much better if the cursor can be placed like this:

for (int i = 0; i < length; i++) { $$ }

I agree, but it is a question of if the LSP Snippet API should insert that indentation or if we should manually insert spaces for blocks when generating the syntax.

i feel like we shoudl be able to tell them: you should place the caret on this line at the appropriate indentation.

CyrusNajmabadi · 2022-07-28T17:38:04Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+        {
+            var compilation = await document.Project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
+            var generator = SyntaxGenerator.GetGenerator(document);
+            var options = await document.GetAnalyzerConfigOptionsAsync(cancellationToken).ConfigureAwait(false);


why only the analyzer config options? not just all the otpions? then you don't need the null check below either.

@CyrusNajmabadi I think we really don't have to look at options at all. Always generating compilation.GetSpecialType(SpecialType.System_Int32).GenerateTypeSyntax(allowVar: true).WithAdditionalAnnotations(Simplifier.Annotation) should just work and respect the user option.

CyrusNajmabadi · 2022-07-28T17:38:32Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                varBuiltInType = options.GetOption(CSharpCodeStyleOptions.VarForBuiltInTypes).Value;
+            }
+
+            var indexVariable = generator.Identifier("i");


can we generate j/k/l etc. if we're already in a scope where that identifier is in use?

CyrusNajmabadi · 2022-07-28T17:39:34Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                        SyntaxFactory.VariableDeclarator(
+                            indexVariable,
+                            argumentList: null,
+                            SyntaxFactory.EqualsValueClause((ExpressionSyntax)generator.LiteralExpression(0)))));


the declarator/list is duplicated. extract out and only do once. you only need to vary the TypeSyntax for var/int

CyrusNajmabadi · 2022-07-28T17:39:44Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            var forLoopSyntax = SyntaxFactory.ForStatement(variableDeclarationSyntax,
+                SyntaxFactory.SeparatedList<ExpressionSyntax>(),


nit.

Suggested change

var forLoopSyntax = SyntaxFactory.ForStatement(variableDeclarationSyntax,

SyntaxFactory.SeparatedList<ExpressionSyntax>(),

var forLoopSyntax = SyntaxFactory.ForStatement(

variableDeclarationSyntax,

SyntaxFactory.SeparatedList<ExpressionSyntax>(),

CyrusNajmabadi · 2022-07-28T17:40:19Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                    generator.IdentifierName(indexVariable),
+                    // Using a temporary identifier name for now, could later be changed
+                    // to look for an iterable item in the scope of the insertion.
+                    generator.IdentifierName("length")),


should this be some sort of LSP marker that the user can then tab through?

what do you mean? It's a placeholder so the user can tab through and also rename?

I guess Cyrus means we should place this into the SnippetPlaceholder ?

CyrusNajmabadi · 2022-07-28T17:40:53Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+        protected override ImmutableArray<SnippetPlaceholder> GetForLoopSnippetPlaceholders(SyntaxNode node, ISyntaxFacts syntaxFacts)
+        {
+            using var _ = ArrayBuilder<SnippetPlaceholder>.GetInstance(out var arrayBuilder);
+            var placeHolderBuilder = new Dictionary<string, List<int>>();


is this just a MultiDictionary?

I guess so, wasn't aware this existed

CyrusNajmabadi · 2022-07-28T17:41:37Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            GetPartsOfForStatement(node, out var declaration, out var condition, out var incrementor, out var _1);
+
+            // Retrieves the placeholder present in the variable declaration as well as its location.
+            if (declaration != null)


when is teh decl, condition, or incrementor null? don't you always create all 3?

CyrusNajmabadi · 2022-07-28T17:41:48Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            if (declaration != null)
+            {
+                var variableDeclarator = ((VariableDeclarationSyntax)declaration).Variables.FirstOrDefault();
+                if (variableDeclarator != null)


when is this null? don't you always create it?

CyrusNajmabadi · 2022-07-28T17:42:05Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                if (variableDeclarator != null)
+                {
+                    var declaratorIdentifier = variableDeclarator.Identifier;
+                    placeHolderBuilder.Add(declaratorIdentifier.ToString(), new List<int>() { declaratorIdentifier.SpanStart });


Suggested change

placeHolderBuilder.Add(declaratorIdentifier.ToString(), new List<int>() { declaratorIdentifier.SpanStart });

placeHolderBuilder.Add(declaratorIdentifier.ValueText, new List<int>() { declaratorIdentifier.SpanStart });

CyrusNajmabadi · 2022-07-28T17:42:47Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+                    placeHolderBuilder.Add(operand.ToString(), incrementorList);
+                }
+
+                incrementorList.Add(operand.SpanStart);


MultiDictionary will make all of this a lot easier :)

CyrusNajmabadi · 2022-07-28T17:43:07Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            foreach (var kvp in placeHolderBuilder)
+            {
+                arrayBuilder.Add(new SnippetPlaceholder(kvp.Key, kvp.Value.ToImmutableArray()));
+            }


do you need to sort this array in any special way? or does it not matter?

It doesn't matter because when we construct the LSP string, we create a new dictionary that has a mapping of the position it should be inserted into the string to the identifier

CyrusNajmabadi · 2022-07-28T17:43:14Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            }
+
+            return arrayBuilder.ToImmutableArray();
+


Suggested change

CyrusNajmabadi · 2022-07-28T17:43:28Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+        {
+            GetPartsOfForStatement(caretTarget, out _, out _, out _, out var statement);
+            return statement.SpanStart + 1;
+


Suggested change

CyrusNajmabadi

Overall looking great!

...SharpTest/Completion/CompletionProviders/Snippets/CSharpForSnippetCompletionProviderTests.cs

Cosifne · 2022-08-02T23:38:27Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            var generator = SyntaxGenerator.GetGenerator(document);
+            var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+
+            var iteratorName = NameGenerator.GenerateUniqueName(


Is it possible to use this GenerateUniqueName to get a great name?

Perhaps, but what would be the difference?

Nop, I just mean if it is possible to reuse the code, if it doesn't fit your needs that's fine

Cosifne · 2022-08-09T20:24:37Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            // Creating the variable declaration based on if the user has
+            // 'var for built in types' set in their editorconfig.
+            var variableDeclarationSyntax =
+                 SyntaxFactory.VariableDeclaration(compilation.GetSpecialType(SpecialType.System_Int32).GenerateTypeSyntax(allowVar: false),


Why allowVar is false here? Is it possible to to link this to the editorconfig value here
https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp.CodeStyle/CSharpCodeStyleOptions.cs,cb8ef73fcae9e59f,references

Why allowVar is false here?

Yeah it should be true.

Is it possible to to link this to the editorconfig value here

WithAdditionalAnnotations(Simplifier.Annotation) is needed. That way the code cleanup will be responsible of respecting the user preference. This should happen here:

roslyn/src/Features/Core/Portable/Snippets/SnippetProviders/AbstractSnippetProvider.cs

Line 108 in 35e54c1

var reformattedDocument = await CleanupDocumentAsync(formatAnnotatedSnippetDocument, cancellationToken).ConfigureAwait(false);

(specifically the Simplifier.ReduceAsync call)

Cosifne · 2022-08-09T20:26:29Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+
+            // Retrieves the placeholder present in the variable declaration as well as its location.
+            // The declaration is constructed so it can't be null.
+            var variableDeclarator = ((VariableDeclarationSyntax)declaration!).Variables.FirstOrDefault();


small nit: if it can't be null or empty because we own the code generation logic, then just call First() or Single()

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

Cosifne · 2022-08-09T20:35:35Z

I feel this looks good, once all the existing comments are addressed it should be good to go

ryzngard · 2022-09-22T20:01:33Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+            declaration = forStatement.Declaration;
+            condition = forStatement.Condition;
+            // We can assume there will only be one incrementor since it is only constructed with one.
+            incrementor = forStatement.Incrementors.First();


Suggested change

incrementor = forStatement.Incrementors.First();

incrementor = forStatement.Incrementors.Single();

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

ryzngard · 2022-09-22T20:03:11Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+        protected override async Task<Document> AddIndentationToDocumentAsync(Document document, int position, ISyntaxFacts syntaxFacts, CancellationToken cancellationToken)
+        {
+            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+            var snippet = root.GetAnnotatedNodes(_findSnippetAnnotation).FirstOrDefault();


Suggested change

var snippet = root.GetAnnotatedNodes(_findSnippetAnnotation).FirstOrDefault();

var snippetNode = root.GetAnnotatedNodes(_findSnippetAnnotation).First();

ryzngard · 2022-09-22T20:05:03Z

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs

+
+            // Retrieves the placeholder present in the variable declaration as well as its location.
+            // The declaration is constructed so it can't be null.
+            var variableDeclarator = ((VariableDeclarationSyntax)declaration!).Variables.FirstOrDefault();


DoctorKrolic · 2023-05-26T10:12:47Z

@akhera99 Do you plan to continue working on this? If so, I have a little suggestion: since 'inline statement snippets' are a thing now (#67819) I think for can be implemented as inline statement snippet as well. So if there is code like someInt.$$, the for snippet can generate for (int i = 0; i < someInt; i++) {...} and not have a placeholder for someInt. On the other hand, if you don't want/have time to fully implement this snippet I would like to take it 🙂

akhera99 · 2023-05-30T16:23:49Z

@akhera99 Do you plan to continue working on this? If so, I have a little suggestion: since 'inline statement snippets' are a thing now (#67819) I think for can be implemented as inline statement snippet as well. So if there is code like someInt.$$, the for snippet can generate for (int i = 0; i < someInt; i++) {...} and not have a placeholder for someInt. On the other hand, if you don't want/have time to fully implement this snippet I would like to take it 🙂

@DoctorKrolic feel free to implement the snippet!

akhera99 added 8 commits June 28, 2022 11:55

wip

97ce91e

wip

c520012

wip

03a944b

works

222d422

more tests

60c160c

merge main

1d85689

works for var

a6ae4f5

fix changed file

eef3d5e

dotnet-issue-labeler bot added the Area-IDE label Jul 27, 2022

comments

d029d8c

akhera99 marked this pull request as ready for review July 27, 2022 21:35

akhera99 requested a review from a team as a code owner July 27, 2022 21:35

davidwengier reviewed Jul 27, 2022

View reviewed changes

Youssef1313 reviewed Jul 28, 2022

View reviewed changes

CyrusNajmabadi reviewed Jul 28, 2022

View reviewed changes

CyrusNajmabadi requested changes Jul 28, 2022

View reviewed changes

akhera99 added 3 commits August 1, 2022 12:19

feedback

95fb464

naming

e01f5fb

more naming stuff

8b0a462

akhera99 requested a review from CyrusNajmabadi August 2, 2022 20:41

fix

e40bfc6

Cosifne reviewed Aug 2, 2022

View reviewed changes

...SharpTest/Completion/CompletionProviders/Snippets/CSharpForSnippetCompletionProviderTests.cs Show resolved Hide resolved

Cosifne reviewed Aug 2, 2022

View reviewed changes

Cosifne reviewed Aug 9, 2022

View reviewed changes

src/Features/CSharp/Portable/Snippets/CSharpForLoopSnippetProvider.cs Outdated Show resolved Hide resolved

akhera99 added 7 commits August 23, 2022 10:40

feedback

1087e20

merge main

012b071

add indentation

855905f

fix test

ab2a9dc

Merge branch 'main' into snippets/for-snippet

ef1e89a

merge main

0e8210f

nit

24e2079

ryzngard approved these changes Sep 22, 2022

View reviewed changes

akhera99 removed the request for review from CyrusNajmabadi September 23, 2022 16:04

akhera99 added 5 commits November 14, 2022 11:55

Merge branch 'main' into snippets/for-snippet

f26af6a

merge main

d3a66df

test names

655abe1

fix naming

d8796f4

fixes

7da1dab

DoctorKrolic mentioned this pull request Jun 9, 2023

Semantic snippets - for and forr snippets #68508

Merged

akhera99 closed this in #68508 Jun 15, 2023

	if (!allowVar)
	syntax = syntax.WithAdditionalAnnotations(DoNotAllowVarAnnotation.Annotation);

	if (typeName.HasAnnotation(DoNotAllowVarAnnotation.Annotation))
	{
	return default;
	}

	internal class CSharpForLoopSnippetProvider : AbstractForLoopSnippetProvider
	internal sealed class CSharpForLoopSnippetProvider : AbstractForLoopSnippetProvider

		var forLoopSyntax = SyntaxFactory.ForStatement(variableDeclarationSyntax,
		SyntaxFactory.SeparatedList<ExpressionSyntax>(),

	placeHolderBuilder.Add(declaratorIdentifier.ToString(), new List<int>() { declaratorIdentifier.SpanStart });
	placeHolderBuilder.Add(declaratorIdentifier.ValueText, new List<int>() { declaratorIdentifier.SpanStart });

	incrementor = forStatement.Incrementors.First();
	incrementor = forStatement.Incrementors.Single();

	var snippet = root.GetAnnotatedNodes(_findSnippetAnnotation).FirstOrDefault();
	var snippetNode = root.GetAnnotatedNodes(_findSnippetAnnotation).First();

Semantic snippets - for loop snippet #62990

Semantic snippets - for loop snippet #62990

Conversation

akhera99 commented Jul 27, 2022

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

Youssef1313 Jul 28, 2022 • edited 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

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

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

CyrusNajmabadi left a 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

Youssef1313 Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Cosifne Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cosifne commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorKrolic commented May 26, 2023

akhera99 commented May 30, 2023

Youssef1313 Jul 28, 2022 •

edited

Loading

Youssef1313 Aug 9, 2022 •

edited

Loading

Cosifne Aug 9, 2022 •

edited

Loading