Snippets: If snippet and use of LSP's SnippetExpander #60860

akhera99 · 2022-04-20T17:09:02Z

In this PR:

Uses reflection to call upon the LanguageServerSnippetExpander
Adds a Placeholders piece to the SnippetChange
Adds a service to convert the data held by the SnippetChange into an LSP formatted snippet
Adds testing to determine if the LSP formatted snippet is something that we expect the LanguageServerSnippetExpander to handle
Adds an if snippet
Adds functionality to put trivia on all of the tokens of the generated snippet syntax so that it may be formatted accordingly
Adds an overload to Create in the CompletionChange that holds onto the LSP formatted snippet (internal)

…to features/snippets_2

CyrusNajmabadi · 2022-05-05T19:49:25Z

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

+                    lspSnippetString.Append(str);
+
+                    // Skip past the entire identifier in the TextChange text
+                    i += placeholderInfo.identifier.Length;


something feels very funky here. in the above if check, you end with a ++... but then you lookup in this dictionary using that position that you mutated. so if you have a caretposition and placeholder at the same position, it seems like this will fail.

DO you have testws for that?

CyrusNajmabadi · 2022-05-05T19:49:48Z

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

+        /// Preprocesses the list of placeholders into a dictionary that maps the insertion position
+        /// in the string to the placeholder's identifier and the priority associated with it.
+        /// </summary>
+        private static void GetMapOfSpanStartsToLSPStringItem(ref PooledDictionary<int, (string identifier, int priority)> dictionary, ImmutableArray<SnippetPlaceholder> placeholders, int textChangeStart)


Suggested change

private static void GetMapOfSpanStartsToLSPStringItem(ref PooledDictionary<int, (string identifier, int priority)> dictionary, ImmutableArray<SnippetPlaceholder> placeholders, int textChangeStart)

private static void GetMapOfSpanStartsToLSPStringItem(Dictionary<int, (string identifier, int priority)> dictionary, ImmutableArray<SnippetPlaceholder> placeholders, int textChangeStart)

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

CyrusNajmabadi · 2022-05-05T19:51:28Z

src/Features/Core/Portable/Snippets/SnippetChange.cs

        {
            if (textChanges.IsEmpty)
            {
-                throw new ArgumentException($"{ textChanges.Length } must not be empty");
+                throw new ArgumentException($"textChanges must not be empty");


Suggested change

throw new ArgumentException($"textChanges must not be empty");

throw new ArgumentException($"{nameof(textChanges)} must not be empty");

src/Features/Core/Portable/Snippets/SnippetPlaceholder.cs

CyrusNajmabadi · 2022-05-05T19:53:43Z

src/Features/Core/Portable/Snippets/SnippetPlaceholder.cs

+        /// Example:
+        /// For loop would have two placeholders:
+        /// for (var {1:i} = 0; {1:i} &lt; {2:length}; {i}++)
+        /// Identifier: i, 3 associated  positions 


Suggested change

/// Identifier: i, 3 associated positions

/// Identifier: i, 3 associated positions

CyrusNajmabadi · 2022-05-05T19:53:50Z

src/Features/Core/Portable/Snippets/SnippetPlaceholder.cs

+        /// For loop would have two placeholders:
+        /// for (var {1:i} = 0; {1:i} &lt; {2:length}; {i}++)
+        /// Identifier: i, 3 associated  positions 
+        /// IdentifierL length, 1 associated position


Something is weird here.

CyrusNajmabadi · 2022-05-05T19:54:35Z

src/Features/Core/Portable/Snippets/SnippetProviders/AbstractConsoleSnippetProvider.cs

-            syntaxFacts.GetPartsOfArgumentList(argumentListNode, out var openParenToken, out _, out _);
-            return openParenToken.Span.End;
+            var openParenToken = GetOpenParenToken(caretTarget, syntaxFacts);
+            Contract.ThrowIfNull(openParenToken);


why are we contract-throwing here? how do you know this must insert an open paren? (consdier VB, which would not have an open paren here).

(Actually, the VB comment applies to if-statements, not console.writeline) (though in VB the parens are optinoal there too :))

I'm creating the syntax, so I can ensure there are parentheses created. Not sure what you mean by it applying to if-statements, I don't get any open parentheses tokens in the if statement provider.

CyrusNajmabadi · 2022-05-05T19:54:50Z

src/Features/Core/Portable/Snippets/SnippetProviders/AbstractConsoleSnippetProvider.cs

+
+            syntaxFacts.GetPartsOfArgumentList(argumentListNode, out var openParenToken, out _, out _);
+
+            return openParenToken;


i don't get why this method is nullable just to have the caller throw if it gets null.

CyrusNajmabadi · 2022-05-05T19:58:41Z

Done with pass.

CyrusNajmabadi · 2022-05-06T23:17:20Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CommitManager.cs

+            {
+                var lspSnippetText = change.Properties[SnippetCompletionItem.LSPSnippetKey];
+
+                _roslynLSPSnippetExpander.TryExpand(change.TextChange.Span, lspSnippetText, _textView, triggerSnapshot);


nit: if return value is not checked. consider just making "Expand" and not "TryExpand"

CyrusNajmabadi · 2022-05-06T23:17:38Z

src/EditorFeatures/Core/Snippets/RoslynLSPSnippetExpander.cs

+            }
+        }
+
+        public void TryExpand(TextSpan textSpan, string lspSnippetText, ITextView textView, ITextSnapshot textSnapshot)


nit: as a void method, consider just namign 'Expand'.

CyrusNajmabadi · 2022-05-06T23:18:19Z

src/EditorFeatures/Core/Snippets/RoslynLSPSnippetExpander.cs

+                if (expandMethodResult is null)
+                {
+                    throw new Exception("The result of the invoked LSP snippet expander is null.");
+                }


Suggested change

if (expandMethodResult is null)

{

throw new Exception("The result of the invoked LSP snippet expander is null.");

}

if (expandMethodResult is not boolean resultValue)

{

throw new Exception("The result of the invoked LSP snippet expander was not a boolean.");

}

CyrusNajmabadi · 2022-05-06T23:18:33Z

src/EditorFeatures/Core/Snippets/RoslynLSPSnippetExpander.cs

+                if (!(bool)expandMethodResult)
+                {
+                    throw new Exception("The invoked LSP snippet expander came back as false.");
+                }


Suggested change

if (!(bool)expandMethodResult)

{

throw new Exception("The invoked LSP snippet expander came back as false.");

}

if (!resultValue)

{

throw new Exception("The invoked LSP snippet expander came back as false.");

}

CyrusNajmabadi · 2022-05-06T23:19:43Z

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

+            }
+
+            return lspSnippetString.ToString();
+        }


CyrusNajmabadi · 2022-05-06T23:20:36Z

src/Features/Core/Portable/Snippets/SnippetPlaceholder.cs

+            if (identifier.Length == 0)
+            {
+                throw new ArgumentException($"{nameof(identifier)} must not be an empty string.");
+            }


CyrusNajmabadi · 2022-05-10T05:46:46Z

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

@@ -119,7 +119,7 @@ private static async Task<TextChange> ExtendSnippetTextChangeAsync(Document docu

            var documentText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
            var newString = documentText.ToString(extendedSpan);
-            var newTextChange = new TextChange(new TextSpan(extendedSpan.Start, 0), newString);
+            var newTextChange = new TextChange(TextSpan.FromBounds(extendedSpan.Start, extendedSpan.End), newString);


isn't TextSpan.FromBounds(extendedSpan.Start, extendedSpan.End) exactly the same as extendedSpan?

akhera99 added 27 commits March 16, 2022 09:51

Merge remote-tracking branch 'upstream/features/semantic-snippets' in…

05edfc3

…to features/snippets_2

start work on if snippet

4ee5f98

wip

f7ff1ad

wip

bf3685c

wip

74f5583

wip

fa96642

wip

3fbf4c7

tests

977529d

Merge remote-tracking branch 'upstream/features/semantic-snippets' in…

14de21f

…to features/snippets_2

wip

2c1ed38

wip

c705fd8

Merge remote-tracking branch 'upstream/features/semantic-snippets' in…

d84a6c5

…to features/snippets_2

lsp service now works

85c12cc

lots of changes

293e2b5

wip

b8593eb

Merge branch 'main' into features/snippets_2

105e658

remove added changes

e137363

fixes

e78d2d4

cleanup

9220332

comments

5e4aefe

remove unused file

334331b

revert change made

ddd0aab

update

f4f0aea

fix random file change

3230e0a

simplify

ddfedf3

cleanup/comments

29c41a6

remove unused attribute

61ffd0e

dotnet-issue-labeler bot added the Area-IDE label Apr 20, 2022

akhera99 changed the title ~~Snippets: If snippet and use of LanguageServerSnippetExpander~~ Snippets: If snippet and use of LSP's SnippetExpander Apr 20, 2022

Merge remote-tracking branch 'upstream/features/semantic-snippets' in…

fae94b9

…to features/snippets_2

CyrusNajmabadi reviewed May 5, 2022

View reviewed changes

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs Show resolved Hide resolved

CyrusNajmabadi reviewed May 5, 2022

View reviewed changes

src/Features/Core/Portable/Snippets/SnippetPlaceholder.cs Show resolved Hide resolved

CyrusNajmabadi reviewed May 5, 2022

View reviewed changes

akhera99 added 4 commits May 6, 2022 01:40

feedback

c244e00

way more tests

74111f9

handle same positions better

af2aada

comments

f36a262

CyrusNajmabadi reviewed May 6, 2022

View reviewed changes

src/Features/Core/Portable/Snippets/RoslynLSPSnippetConverter.cs

}

return lspSnippetString.ToString();

}

Copy link

Member

CyrusNajmabadi May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

CyrusNajmabadi reviewed May 6, 2022

View reviewed changes

akhera99 added 3 commits May 9, 2022 10:18

tests

c8f674f

pr feedback

6e33b9d

removed unnecessary check

2498eff

akhera99 enabled auto-merge May 10, 2022 01:06

akhera99 disabled auto-merge May 10, 2022 01:06

CyrusNajmabadi reviewed May 10, 2022

View reviewed changes

CyrusNajmabadi approved these changes May 10, 2022

View reviewed changes

fix

d02e1c2

akhera99 enabled auto-merge May 10, 2022 15:43

akhera99 merged commit f6212d7 into dotnet:features/semantic-snippets May 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snippets: If snippet and use of LSP's SnippetExpander #60860

Snippets: If snippet and use of LSP's SnippetExpander #60860

akhera99 commented Apr 20, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi May 5, 2022

akhera99 May 6, 2022

CyrusNajmabadi May 5, 2022

CyrusNajmabadi commented May 5, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 6, 2022

CyrusNajmabadi May 10, 2022

	private static void GetMapOfSpanStartsToLSPStringItem(ref PooledDictionary<int, (string identifier, int priority)> dictionary, ImmutableArray<SnippetPlaceholder> placeholders, int textChangeStart)
	private static void GetMapOfSpanStartsToLSPStringItem(Dictionary<int, (string identifier, int priority)> dictionary, ImmutableArray<SnippetPlaceholder> placeholders, int textChangeStart)

	throw new ArgumentException($"textChanges must not be empty");
	throw new ArgumentException($"{nameof(textChanges)} must not be empty");

	/// Identifier: i, 3 associated positions
	/// Identifier: i, 3 associated positions


		syntaxFacts.GetPartsOfArgumentList(argumentListNode, out var openParenToken, out _, out _);

		return openParenToken;

Snippets: If snippet and use of LSP's SnippetExpander #60860

Snippets: If snippet and use of LSP's SnippetExpander #60860

Conversation

akhera99 commented Apr 20, 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

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 commented May 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment