Semantic Snippets: Console snippet first look #59303

akhera99 · 2022-02-04T23:33:22Z

Here's a first look at snippets!
I wanted to work on a basic example first, the console writeline snippet.

Here's a short video showcasing how it works:

Console.Snippet.Recording.mp4

Design
Snippet.cs:
Stores information for the type of snippet it is (what is displayed on the completion list), the TextChange that is created by this snippet, the position we want the cursor to end up, and locations we would want to call upon a rename.

SnippetData.cs:
Acts as a middle-man for ProvideCompletionsAsync() and GetChangesAsync() in the AbstractSnippetCompletionProvider. We do not want complete all the logic to determine the new TextChange and cursor position at the point that we are simply determining if the completions should exist at a certain position. This is then used to retrieve the actual Snippet.

ISnippetService.cs:
Interface implemented by the CSharpSnippetService which gets available snippets at a particular position.

ISnippetProvider.cs:
Interface implemented by the AbstractSnippetProvider

AbstractSnippetCompletionProvider.cs:
Looks at the context and calls upon the ISnippetService to return all available snippets at that location to be added to the completion list. GetChangesAsync() calls upon the ISnippetService to get the ISnippetProvider related to the CompletionItem that was selected. The ISnippetProvider retrieves the corresponding Snippet to create the CompletionChange.

AbstractSnippetProvider:
To be implemented by every snippet we want created. Generates the Snippet. Each snippet will need to implement a method to generate the snippet TextChange, annotate the newly added syntax for the cursor location, annotate the newly added syntax to be reformatted, get the integer position we want the cursor to appear, and return all the rename spans if applicable.

Todo:
-Need to figure out how to have several possible shortcuts to invoke the completion item
-Need to make sure all cases are being hit
-Need to make sure formatting of the original document stays the same after inserting the snippet
-Possibly should look within the document for something that can be written in the WriteLine?
-Add comments

DoctorKrolic · 2022-02-05T14:02:41Z

This is just an example, isn't it? Because if it is not, I see no reason for making a separate overcomplicated snippet to just write to the console. But it would be excellent, if all existing snippets are transformed to such context dependent form.

CyrusNajmabadi · 2022-02-05T16:26:53Z

@DoctorKrolic as mentioned, it's a first look.

jmarolf · 2022-02-06T03:22:09Z

@DoctorKrolic For more background on what we are trying to do ther I would recommend reading the document here: https://github.com/dotnet/roslyn/blob/features/semantic-snippets/docs/ide/specs/semantic_snippets.md. Please feel free to post your thoughts on this issue #56541

jmarolf

Great start! for this feature branch I am hoping that we can do a bunch of small PRs instead of a one big one. In that vein I would ensure that CI passes and that these items are done before merging (which I am interpreting as adding tests :)):

-Need to make sure all cases are being hit
-Need to make sure formatting of the original document stays the same after inserting the snippet
-Add comments

For these I think we can do them in a separate PR

-Need to figure out how to have several possible shortcuts to invoke the completion item
-Possibly should look within the document for something that can be written in the WriteLine?

akhera99 · 2022-02-07T18:05:38Z

Great start! for this feature branch I am hoping that we can do a bunch of small PRs instead of a one big one. In that vein I would ensure that CI passes and that these items are done before merging (which I am interpreting as adding tests :)):

-Need to make sure all cases are being hit
-Need to make sure formatting of the original document stays the same after inserting the snippet
-Add comments

For these I think we can do them in a separate PR

-Need to figure out how to have several possible shortcuts to invoke the completion item
-Possibly should look within the document for something that can be written in the WriteLine?

Yeah that's just an overall TODO list.
Tests are written, just need to make sure it iterates over every possible case.

It seems that CI is looking at some stale code. I need to figure out why it's looking at some snippet completion providers that I had removed for the PR.

CyrusNajmabadi · 2022-02-07T18:27:45Z

Ping me when you would like a review.

src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs

CyrusNajmabadi · 2022-03-09T17:28:26Z

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

+            if (await IsValidSnippetLocationAsync(document, position, cancellationToken).ConfigureAwait(false))
+            {
+                return new SnippetData(SnippetDisplayName, SnippetIdentifier);
+            }
+
+            return null;


Suggested change

if (await IsValidSnippetLocationAsync(document, position, cancellationToken).ConfigureAwait(false))

{

return new SnippetData(SnippetDisplayName, SnippetIdentifier);

}

return null;

if (!await IsValidSnippetLocationAsync(document, position, cancellationToken).ConfigureAwait(false))

{

return null;

}

return new SnippetData(SnippetDisplayName, SnippetIdentifier);

general idea here being to be consistent with how you bail out or return values. it makes tracking overal logic and data flow easier.

CyrusNajmabadi · 2022-03-09T17:33:52Z

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

+        protected virtual Task<ImmutableArray<TextSpan>> GetRenameLocationsAsync(Document document, int position, CancellationToken cancellationToken)
+        {
+            return Task.FromResult(ImmutableArray<TextSpan>.Empty);
+        }


i woudl remove this from the PR for now.

CyrusNajmabadi · 2022-03-09T17:34:34Z

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

+    internal abstract class AbstractSnippetService : ISnippetService
+    {
+        private readonly ImmutableArray<Lazy<ISnippetProvider, LanguageMetadata>> _lazySnippetProviders;
+        private readonly Dictionary<string, ISnippetProvider> _snippetProviderDictionary = new();


with dictionaries, it's good to have the name indicate waht this is mapping from/to. e.g. identifierToProviderMap, etc.

CyrusNajmabadi · 2022-03-09T17:34:56Z

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

+    {
+        private readonly ImmutableArray<Lazy<ISnippetProvider, LanguageMetadata>> _lazySnippetProviders;
+        private readonly Dictionary<string, ISnippetProvider> _snippetProviderDictionary = new();
+        private ImmutableArray<ISnippetProvider> _snippetProviders = new();


new() here doesn't do anything. indeed, as a struct, you'll still just get the defautl immutable array.

CyrusNajmabadi · 2022-03-09T17:35:45Z

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

+
+        public ISnippetProvider GetSnippetProvider(string snippetIdentifier)
+        {
+            return _snippetProviderDictionary[snippetIdentifier];


this seems like it could completely fail. how do you know _snippetProviderDictionary is initialized? consider doc'ing/asserting things you think are true at thsi point.

This should never be called prior to GetSnippetsAsync. Is that sufficient or should there be some other checks at this point?

in this case, it woudl be good to have a Contract call like Contract.ThrowIfFalse(_snippetProviderDictionary.ContainsKey(snippetIdentifier));

CyrusNajmabadi · 2022-03-09T17:36:13Z

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

+
+        private ImmutableArray<ISnippetProvider> GetSnippetProviders(Document document)
+        {
+            using var _ = ArrayBuilder<ISnippetProvider>.GetInstance(out var arrayBuilder);


move this inside the if check.

CyrusNajmabadi · 2022-03-09T17:38:00Z

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

+                {
+                    var providerData = provider.Value;
+                    arrayBuilder.Add(providerData);
+                    _snippetProviderDictionary.Add(providerData.SnippetIdentifier, providerData);


this is definitely not threadsafe, and will throw if two threads are calling into GetSnippetProvider at the same time.

CyrusNajmabadi · 2022-03-09T17:38:18Z

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

+{
+    internal interface ISnippetProvider
+    {
+        string SnippetIdentifier { get; }


doc what happens if two snippet providers have hte same ID.

CyrusNajmabadi · 2022-03-09T17:38:25Z

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

+    {
+        string SnippetIdentifier { get; }
+
+        string SnippetDisplayName { get; }


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

CyrusNajmabadi · 2022-03-09T17:39:17Z

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

+        /// <summary>
+        /// The TextSpans that need to be renamed if we insert a snippet with values that the user may want to change
+        /// </summary>
+        public readonly ImmutableArray<TextSpan> RenameLocations;


consider removing for now.

CyrusNajmabadi · 2022-03-11T22:08:49Z

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

+
+        private async Task<TextChange> GenerateSnippetTextChangeAsync(Document document, int position, CancellationToken cancellationToken)
+        {
+            var symbol = await GetSymbolFromMetaDataNameAsync(document, cancellationToken).ConfigureAwait(false);


Contract.ThrowIfNull on this (mentioning we only have the snippet if we had this symbol).

CyrusNajmabadi · 2022-03-11T22:12:24Z

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

+                    {
+                        var providerData = provider.Value;
+                        arrayBuilder.Add(providerData);
+                        _identifierToProviderMap.Add(providerData.SnippetIdentifier, providerData);


this will throw if two providers ahve the same snippet id. Is that something we can ensure will never happen? are all snippet IDs under our control? or could a 3rd party add a snippet with teh same ID and crash the system? if this is all under our control, this is fine. but it would likely be better to have this be tolerant so that misauthored snippets don't kill things.

note: you can do a Debug.ASsert here so that if we screw up we at least know about it in our debug builds, but this won't fire and cause problems in release builds.

All snippet IDs should be under our control, but some tolerance can be added here.

CyrusNajmabadi · 2022-03-11T22:14:27Z

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

+        /// <summary>
+        /// The type of snippet, equivalent to what gets displayed in the Completion list
+        /// </summary>
+        public readonly string DisplayText;


do we need this? what do you use this for?

CyrusNajmabadi · 2022-03-11T22:14:49Z

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

+    /// <summary>
+    /// Encapsulates the information that makes up a Snippet.
+    /// </summary>
+    internal readonly struct Snippet


looking at this, perhaps you should call this SnippetChange to indicate that it's the change that needs to be made to apply the snippet to user code.

akhera99 added 10 commits November 11, 2021 10:21

snippets first loook

6e23a2e

Merge branch 'main' into features/snippets_1

a74697d

starting imp

6bda3a9

still figuring things out

592e07f

temp

cce51e9

console snippet

f1bf487

console snippet works

e96dfd8

tests + correct formatting :)

fb20055

more tests

01b7fef

more tests, removed old unusued code

e43efd1

akhera99 requested review from CyrusNajmabadi, jmarolf and ryzngard February 4, 2022 23:33

akhera99 self-assigned this Feb 4, 2022

ghost added the Area-IDE label Feb 4, 2022

This comment was marked as off-topic.

Sign in to view

jmarolf approved these changes Feb 7, 2022

View reviewed changes

added a few comments

e3d9029

akhera99 marked this pull request as ready for review February 7, 2022 19:09

akhera99 requested a review from a team as a code owner February 7, 2022 19:09

CyrusNajmabadi reviewed Feb 7, 2022

View reviewed changes

src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs Outdated Show resolved Hide resolved