Escape strings correctly in snippets (#14582)

anthony-c-martin · web-flow · commit f1c7212e0066 · 2024-07-18T11:06:02.000-04:00
Use the syntax factory + pretty printer to emit snippets rather than ad-hoc string formatting. This avoids having to deal with edge cases like escaping. Closes #14578 ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/14582)
diff --git a/src/Bicep.Core.Samples/Files/baselines/InvalidModules_LF/Completions/moduleBodyCompletions.json b/src/Bicep.Core.Samples/Files/baselines/InvalidModules_LF/Completions/moduleBodyCompletions.json
@@ -73,7 +73,7 @@
     "detail": "Required properties",
     "documentation": {
       "kind": "markdown",
-      "value": "```bicep\n{\n\tname: \n\tparams: {\n\t\tarrayParam: \n\t\tobjParam: {\n\t\t}\n\t\tstringParamB: \n\t}\n}\n```  \n"
+      "value": "```bicep\n{\n\tname: \n\tparams: {\n\t\tarrayParam: \n\t\tobjParam: {}\n\t\tstringParamB: \n\t}\n}\n```  \n"
     },
     "deprecated": false,
     "preselect": true,
@@ -82,7 +82,7 @@
     "insertTextMode": "adjustIndentation",
     "textEdit": {
       "range": {},
-      "newText": "{\n\tname: $1\n\tparams: {\n\t\tarrayParam: $2\n\t\tobjParam: {\n\t\t}\n\t\tstringParamB: $3\n\t}\n}$0"
+      "newText": "{\n\tname: $1\n\tparams: {\n\t\tarrayParam: $2\n\t\tobjParam: {}\n\t\tstringParamB: $3\n\t}\n}$0"
     },
     "command": {
       "title": "module body completion snippet",
diff --git a/src/Bicep.LangServer.IntegrationTests/CompletionTests.cs b/src/Bicep.LangServer.IntegrationTests/CompletionTests.cs
@@ -4899,5 +4899,102 @@ await RunCompletionScenarioTest(TestContext, ServerWithNamespaceProvider, fileWi
                 completionList.Should().SatisfyRespectively(i => i.Label.Should().Be("*"));
             });
         }
+
+        [TestMethod]
+        public async Task Strings_in_required_property_completions_are_correctly_escaped()
+        {
+            var fileWithCursors = """
+@discriminator('odata.type')
+type alertType = alertWebtestType | alertResourceType | alertMultiResourceType
+type alertResourceType = {
+  'odata.type': 'Microsoft.Azure.Monitor.SingleResourceMultipleMetricCriteria'
+  allof: array
+}
+type alertMultiResourceType = {
+  'odata.type': 'Microsoft.Azure.Monitor.MultipleResourceMultipleMetricCriteria'
+  allof: array
+}
+type alertWebtestType = {
+  'odata.type': 'Microsoft.Azure.Monitor.WebtestLocationAvailabilityCriteria'
+  componentId: string
+  failedLocationCount: int
+  webTestId: string
+}
+
+param myAlert alertType = |>
+""";
+
+            var (text, cursor) = ParserHelper.GetFileWithSingleCursor(fileWithCursors, "|>");
+            var file = await new ServerRequestHelper(TestContext, ServerWithExtensibilityEnabled).OpenFile(text);
+
+            var completions = await file.RequestCompletion(cursor);
+
+            var updatedFile = file.ApplyCompletion(completions, "required-properties-Microsoft.Azure.Monitor.WebtestLocationAvailabilityCriteria");
+            updatedFile.Should().HaveSourceText("""
+@discriminator('odata.type')
+type alertType = alertWebtestType | alertResourceType | alertMultiResourceType
+type alertResourceType = {
+  'odata.type': 'Microsoft.Azure.Monitor.SingleResourceMultipleMetricCriteria'
+  allof: array
+}
+type alertMultiResourceType = {
+  'odata.type': 'Microsoft.Azure.Monitor.MultipleResourceMultipleMetricCriteria'
+  allof: array
+}
+type alertWebtestType = {
+  'odata.type': 'Microsoft.Azure.Monitor.WebtestLocationAvailabilityCriteria'
+  componentId: string
+  failedLocationCount: int
+  webTestId: string
+}
+
+param myAlert alertType = {
+  componentId: $1
+  failedLocationCount: $2
+  'odata.type': 'Microsoft.Azure.Monitor.WebtestLocationAvailabilityCriteria'
+  webTestId: $3
+}|
+""");
+        }
+
+        [TestMethod]
+        public async Task Nested_tab_stops_are_correctly_ordered_in_required_properties_snippet()
+        {
+            var fileWithCursors = """
+type nestedType = {
+  foo: string
+  bar: {
+    bar: string
+  }
+  baz: string
+}
+
+param test nestedType = |>
+""";
+
+            var (text, cursor) = ParserHelper.GetFileWithSingleCursor(fileWithCursors, "|>");
+            var file = await new ServerRequestHelper(TestContext, ServerWithExtensibilityEnabled).OpenFile(text);
+
+            var completions = await file.RequestCompletion(cursor);
+
+            var updatedFile = file.ApplyCompletion(completions, "required-properties");
+            updatedFile.Should().HaveSourceText("""
+type nestedType = {
+  foo: string
+  bar: {
+    bar: string
+  }
+  baz: string
+}
+
+param test nestedType = {
+  bar: {
+    bar: $1
+  }
+  baz: $2
+  foo: $3
+}|
+""");
+        }
     }
 }
diff --git a/src/Bicep.LangServer/Snippets/SnippetsProvider.cs b/src/Bicep.LangServer/Snippets/SnippetsProvider.cs
@@ -3,9 +3,13 @@
 
 using System.Collections.Concurrent;
 using System.Collections.Immutable;
+using System.DirectoryServices.Protocols;
 using System.Text;
 using System.Text.RegularExpressions;
 using Bicep.Core;
+using Bicep.Core.Parsing;
+using Bicep.Core.PrettyPrint;
+using Bicep.Core.PrettyPrintV2;
 using Bicep.Core.Resources;
 using Bicep.Core.Syntax;
 using Bicep.Core.TypeSystem;
@@ -25,7 +29,7 @@ public class SnippetsProvider : ISnippetsProvider
     // The common properties should be authored consistently to provide for understandability and consumption of the code.
     // See https://github.com/Azure/azure-quickstart-templates/blob/master/1-CONTRIBUTION-GUIDE/best-practices.md#resources
     // for more information
-    private readonly ImmutableArray<string> propertiesSortPreferenceList = ["scope", "parent", "name", "location", "zones", "sku", "kind", "scale", "plan", "identity", "tags", "properties", "dependsOn"];
+    private static readonly ImmutableArray<string> PropertiesSortPreferenceList = ["scope", "parent", "name", "location", "zones", "sku", "kind", "scale", "plan", "identity", "tags", "properties", "dependsOn"];
 
     private static readonly SnippetCache snippetCache = SnippetCache.FromManifest();
 
@@ -119,96 +123,68 @@ private IEnumerable<Snippet> GetRequiredPropertiesSnippetsForDisciminatedObjectT
         }
     }
 
-    private Snippet? GetRequiredPropertiesSnippet(ObjectType objectType, string label, string? discriminatedObjectKey = null)
+    private static ObjectSyntax GetObjectSnippetSyntax(ObjectType objectType, ref int tabStopIndex, string? discriminatedObjectKey)
     {
-        int index = 1;
-        StringBuilder sb = new();
-
-        var sortedProperties = objectType.Properties.OrderBy(x =>
+        var typeProperties = objectType.Properties.Values.OrderBy(x =>
+            PropertiesSortPreferenceList.IndexOf(x.Name) switch {
+                -1 => int.MaxValue,
+                int index =>  index,
+            })
+            .Where(TypeHelper.IsRequired);
+
+        var objectProperties = new List<ObjectPropertySyntax>();
+        foreach (var typeProperty in typeProperties)
         {
-            var index = propertiesSortPreferenceList.IndexOf(x.Key);
+            // Here we deliberately want to iterate in the correct order, and use a DFS approach, to ensure that the tab stops are correctly ordered.
+            // For example, we want to ensure we output: {\n  foo: $1\n  nested: {\n    bar: $2\n  }\n  baz: $3\n}
+            // Instead of:                               {\n  foo: $1\n  nested: {\n    bar: $3\n  }\n  baz: $2\n}
+            objectProperties.Add(GetObjectPropertySnippetSyntax(typeProperty, ref tabStopIndex, discriminatedObjectKey));
+        }
 
-            return (index > -1) ? index : (propertiesSortPreferenceList.Length - 1);
-        });
+        return SyntaxFactory.CreateObject(objectProperties);
+    }
 
-        foreach (var (key, value) in sortedProperties)
+    private static ObjectPropertySyntax GetObjectPropertySnippetSyntax(TypeProperty typeProperty, ref int tabStopIndex, string? discriminatedObjectKey)
+    {
+        var valueType = typeProperty.TypeReference.Type;
+        if (valueType is ObjectType objectType)
         {
-            string? snippetText = GetSnippetText(value, indentLevel: 1, ref index, discriminatedObjectKey);
-
-            if (snippetText is not null)
-            {
-                sb.Append(snippetText);
-            }
+            return SyntaxFactory.CreateObjectProperty(
+                typeProperty.Name,
+                GetObjectSnippetSyntax(objectType, ref tabStopIndex, null));
         }
-
-        if (sb.Length > 0)
+        else if (discriminatedObjectKey is {} &&
+            valueType is StringLiteralType stringLiteralType &&
+            stringLiteralType.Name == discriminatedObjectKey)
         {
-            // Insert open curly at the beginning
-            sb.Insert(0, "{\n");
-
-            // Insert final tab stop outside the top level object
-            sb.Append("}$0");
-
-            return new Snippet(sb.ToString(), CompletionPriority.Medium, label, RequiredPropertiesDescription);
+            return SyntaxFactory.CreateObjectProperty(
+                typeProperty.Name,
+                SyntaxFactory.CreateStringLiteral(stringLiteralType.RawStringValue));
+        }
+        else
+        {
+            var newTabStopIndex = tabStopIndex++;
+            return SyntaxFactory.CreateObjectProperty(
+                typeProperty.Name,
+                SyntaxFactory.CreateFreeformToken(TokenType.Unrecognized, GetTabStop(newTabStopIndex)));
         }
-
-        return null;
     }
 
-    private string? GetSnippetText(TypeProperty typeProperty, int indentLevel, ref int index, string? discrimatedObjectKey = null)
+    private static string GetTabStop(int index)
+        => $"${index}";
+
+    private Snippet? GetRequiredPropertiesSnippet(ObjectType objectType, string label, string? discriminatedObjectKey = null)
     {
-        if (TypeHelper.IsRequired(typeProperty))
+        if (!objectType.Properties.Values.Any(TypeHelper.IsRequired))
         {
-            StringBuilder sb = new();
-
-            if (typeProperty.TypeReference.Type is ObjectType objectType)
-            {
-                sb.AppendLine(GetIndentString(indentLevel) + typeProperty.Name + ": {");
-
-                indentLevel++;
-
-                foreach (KeyValuePair<string, TypeProperty> kvp in objectType.Properties.OrderBy(x => x.Key))
-                {
-                    string? snippetText = GetSnippetText(kvp.Value, indentLevel, ref index);
-                    if (snippetText is not null)
-                    {
-                        sb.Append(snippetText);
-                    }
-                }
-
-                indentLevel--;
-                sb.AppendLine(GetIndentString(indentLevel) + "}");
-            }
-            else
-            {
-                string value = ": $" + (index).ToString();
-                bool shouldIncrementIndent = true;
-
-                if (discrimatedObjectKey is not null &&
-                    typeProperty.TypeReference.Type is TypeSymbol typeSymbol &&
-                    typeSymbol.Name == discrimatedObjectKey)
-                {
-                    value = ": " + discrimatedObjectKey;
-                    shouldIncrementIndent = false;
-                }
-
-                sb.AppendLine(GetIndentString(indentLevel) + typeProperty.Name + value);
-
-                if (shouldIncrementIndent)
-                {
-                    index++;
-                }
-            }
-
-            return sb.ToString();
+            return null;
         }
 
-        return null;
-    }
+        var tabStopIndex = 1;
+        var syntax = GetObjectSnippetSyntax(objectType, ref tabStopIndex, discriminatedObjectKey);
 
-    private string GetIndentString(int indentLevel)
-    {
-        return new string('\t', indentLevel);
+        var output = PrettyPrinterV2.PrintValid(syntax, PrettyPrinterV2Options.Default with { IndentKind = IndentKind.Tab }) + GetTabStop(0);
+        return new Snippet(output, CompletionPriority.Medium, label, RequiredPropertiesDescription);
     }
 
     private Snippet GetEmptySnippet()
