Fix auto-complete inserting brackets around function names (#895)

* Fix reserved words not to be bracket quoted

* wip

* More fixes

* Better name

* Format files and update test

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/AutoCompleteHelper.cs

@@ -28,7 +28,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices
     {
         private static CompletionItem[] emptyCompletionList = new CompletionItem[0];
 
-        private static readonly string[] DefaultCompletionText = new string[]
+        public static readonly string[] DefaultCompletionText = new string[]
         {
             "abs",
             "acos",
@@ -488,8 +488,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices
         /// </summary>
         internal static bool IsReservedWord(string text)
         {
-            int pos = Array.IndexOf(DefaultCompletionText, text.ToLower());
-            return pos > -1;
+            return DefaultCompletionText.Contains(text, StringComparer.InvariantCultureIgnoreCase);
         }
 
         /// <summary>

src/Microsoft.SqlTools.ServiceLayer/LanguageServices/Completion/SqlCompletionItem.cs

@@ -19,9 +19,10 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion
     public class SqlCompletionItem
     {
         private static Regex ValidSqlNameRegex = new Regex(@"^[\p{L}_@#][\p{L}\p{N}@$#_]{0,127}$");
-        private static DelimitedIdentifier BracketedIdentifiers = new DelimitedIdentifier { Start = "[", End = "]"};
+        private static DelimitedIdentifier BracketedIdentifiers = new DelimitedIdentifier { Start = "[", End = "]" };
+        private static DelimitedIdentifier FunctionPostfix = new DelimitedIdentifier { Start = "", End = "()" };
         private static DelimitedIdentifier[] DelimitedIdentifiers =
-            new DelimitedIdentifier[] { BracketedIdentifiers, new DelimitedIdentifier {Start = "\"", End = "\"" } };
+            new DelimitedIdentifier[] { BracketedIdentifiers, new DelimitedIdentifier { Start = "\"", End = "\"" } };
 
         /// <summary>
         /// Create new instance given the SQL parser declaration
@@ -48,14 +49,39 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion
         private void Init()
         {
             InsertText = DeclarationTitle;
-            DelimitedIdentifier delimitedIdentifier = GetDelimitedIdentifier(TokenText);
             Label = DeclarationTitle;
+            DelimitedIdentifier delimitedIdentifier = GetDelimitedIdentifier(TokenText);
 
-            if (delimitedIdentifier == null && !string.IsNullOrEmpty(DeclarationTitle) && 
-                (!ValidSqlNameRegex.IsMatch(DeclarationTitle) || AutoCompleteHelper.IsReservedWord(InsertText)))
+            // If we're not already going to quote this then handle special cases for various
+            // DeclarationTypes
+            if (delimitedIdentifier == null && !string.IsNullOrEmpty(DeclarationTitle))
             {
-                InsertText = WithDelimitedIdentifier(BracketedIdentifiers, DeclarationTitle);
+                switch (this.DeclarationType)
+                {
+                    case DeclarationType.Server:
+                    case DeclarationType.Database:
+                    case DeclarationType.Table:
+                    case DeclarationType.Column:
+                    case DeclarationType.View:
+                    case DeclarationType.Schema:
+                        // Only quote if we need to - i.e. if this isn't a valid name (has characters that need escaping such as [) 
+                        // or if it's a reserved word
+                        if (!ValidSqlNameRegex.IsMatch(DeclarationTitle) || AutoCompleteHelper.IsReservedWord(InsertText))
+                        {
+                            InsertText = WithDelimitedIdentifier(BracketedIdentifiers, DeclarationTitle);
+                        }
+                        break;
+                    case DeclarationType.BuiltInFunction:
+                    case DeclarationType.ScalarValuedFunction:
+                    case DeclarationType.TableValuedFunction:
+                        // Functions we add on the () at the end since they'll always have them
+                        InsertText = WithDelimitedIdentifier(FunctionPostfix, DeclarationTitle);
+                        break;
+                }
             }
+
+            // If the user typed a token that starts with a delimiter then always quote both
+            // the display label and text to be inserted
             if (delimitedIdentifier != null)
             {
                 Label = WithDelimitedIdentifier(delimitedIdentifier, Label);
@@ -184,7 +210,7 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion
 
         private bool HasDelimitedIdentifier(DelimitedIdentifier delimiteIidentifier, string text)
         {
-            return text != null && delimiteIidentifier != null && text.StartsWith(delimiteIidentifier.Start) 
+            return text != null && delimiteIidentifier != null && text.StartsWith(delimiteIidentifier.Start)
                 && text.EndsWith(delimiteIidentifier.End);
         }
 
@@ -193,11 +219,11 @@ namespace Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion
             return text != null ? DelimitedIdentifiers.FirstOrDefault(x => text.StartsWith(x.Start)) : null;
         }
 
-        private string WithDelimitedIdentifier(DelimitedIdentifier delimiteIidentifier, string text)
+        private string WithDelimitedIdentifier(DelimitedIdentifier delimitedIdentifier, string text)
         {
-            if (!HasDelimitedIdentifier(delimiteIidentifier, text))
+            if (!HasDelimitedIdentifier(delimitedIdentifier, text))
             {
-                return string.Format(CultureInfo.InvariantCulture, "{0}{1}{2}", delimiteIidentifier.Start, text, delimiteIidentifier.End);
+                return string.Format(CultureInfo.InvariantCulture, "{0}{1}{2}", delimitedIdentifier.Start, text, delimitedIdentifier.End);
             }
             else
             {

test/Microsoft.SqlTools.ServiceLayer.UnitTests/LanguageServer/SqlCompletionItemTests.cs

@@ -4,7 +4,9 @@
 //
 
 using System;
+using System.Linq;
 using Microsoft.SqlServer.Management.SqlParser.Intellisense;
+using Microsoft.SqlTools.ServiceLayer.LanguageServices;
 using Microsoft.SqlTools.ServiceLayer.LanguageServices.Completion;
 using Microsoft.SqlTools.ServiceLayer.LanguageServices.Contracts;
 using Xunit;
@@ -13,303 +15,6 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer
 {
     public class SqlCompletionItemTests
     {
-        private static readonly string[] ReservedWords = new string[]
-        {
-            "all",
-            "alter",
-            "and",
-            "apply",
-            "as",
-            "asc",
-            "at",
-            "backup",
-            "begin",
-            "binary",
-            "bit",
-            "break",
-            "bulk",
-            "by",
-            "call",
-            "cascade",
-            "case",
-            "catch",
-            "char",
-            "character",
-            "check",
-            "checkpoint",
-            "close",
-            "clustered",
-            "column",
-            "columnstore",
-            "commit",
-            "connect",
-            "constraint",
-            "continue",
-            "create",
-            "cross",
-            "current_date",
-            "cursor",
-            "cursor_close_on_commit",
-            "cursor_default",
-            "data",
-            "data_compression",
-            "database",
-            "date",
-            "datetime",
-            "datetime2",
-            "days",
-            "dbcc",
-            "dec",
-            "decimal",
-            "declare",
-            "default",
-            "delete",
-            "deny",
-            "desc",
-            "description",
-            "disabled",
-            "disk",
-            "distinct",
-            "double",
-            "drop",
-            "drop_existing",
-            "dump",
-            "dynamic",
-            "else",
-            "enable",
-            "encrypted",
-            "end",
-            "end-exec",
-            "exec",
-            "execute",
-            "exists",
-            "exit",
-            "external",
-            "fast_forward",
-            "fetch",
-            "file",
-            "filegroup",
-            "filename",
-            "filestream",
-            "filter",
-            "first",
-            "float",
-            "for",
-            "foreign",
-            "from",
-            "full",
-            "function",
-            "geography",
-            "get",
-            "global",
-            "go",
-            "goto",
-            "grant",
-            "group",
-            "hash",
-            "hashed",
-            "having",
-            "hidden",
-            "hierarchyid",
-            "holdlock",
-            "hours",
-            "identity",
-            "identitycol",
-            "if",
-            "image",
-            "immediate",
-            "include",
-            "index",
-            "inner",
-            "insert",
-            "instead",
-            "int",
-            "integer",
-            "intersect",
-            "into",
-            "isolation",
-            "join",
-            "json",
-            "key",
-            "language",
-            "last",
-            "left",
-            "level",
-            "lineno",
-            "load",
-            "local",
-            "locate",
-            "location",
-            "login",
-            "masked",
-            "maxdop",
-            "merge",
-            "message",
-            "modify",
-            "move",
-            "namespace",
-            "native_compilation",
-            "nchar",
-            "next",
-            "no",
-            "nocheck",
-            "nocount",
-            "nonclustered",
-            "none",
-            "norecompute",
-            "not",
-            "now",
-            "null",
-            "numeric",
-            "object",
-            "of",
-            "off",
-            "offsets",
-            "on",
-            "online",
-            "open",
-            "openrowset",
-            "openxml",
-            "option",
-            "or",
-            "order",
-            "out",
-            "outer",
-            "output",
-            "over",
-            "owner",
-            "partial",
-            "partition",
-            "password",
-            "path",
-            "percent",
-            "percentage",
-            "period",
-            "persisted",
-            "plan",
-            "policy",
-            "precision",
-            "predicate",
-            "primary",
-            "print",
-            "prior",
-            "proc",
-            "procedure",
-            "public",
-            "query_store",
-            "quoted_identifier",
-            "raiserror",
-            "range",
-            "raw",
-            "read",
-            "read_committed_snapshot",
-            "read_only",
-            "read_write",
-            "readonly",
-            "readtext",
-            "real",
-            "rebuild",
-            "receive",
-            "reconfigure",
-            "recovery",
-            "recursive",
-            "recursive_triggers",
-            "references",
-            "relative",
-            "remove",
-            "reorganize",
-            "required",
-            "restart",
-            "restore",
-            "restrict",
-            "resume",
-            "return",
-            "returns",
-            "revert",
-            "revoke",
-            "rollback",
-            "rollup",
-            "row",
-            "rowcount",
-            "rowguidcol",
-            "rows",
-            "rule",
-            "sample",
-            "save",
-            "schema",
-            "schemabinding",
-            "scoped",
-            "scroll",
-            "secondary",
-            "security",
-            "select",
-            "send",
-            "sent",
-            "sequence",
-            "server",
-            "session",
-            "set",
-            "sets",
-            "setuser",
-            "simple",
-            "smallint",
-            "smallmoney",
-            "snapshot",
-            "sql",
-            "standard",
-            "start",
-            "started",
-            "state",
-            "statement",
-            "static",
-            "statistics",
-            "statistics_norecompute",
-            "status",
-            "stopped",
-            "sysname",
-            "system",
-            "system_time",
-            "table",
-            "take",
-            "target",
-            "then",
-            "throw",
-            "time",
-            "timestamp",
-            "tinyint",
-            "to",
-            "top",
-            "tran",
-            "transaction",
-            "trigger",
-            "truncate",
-            "try",
-            "tsql",
-            "type",
-            "uncommitted",
-            "union",
-            "unique",
-            "uniqueidentifier",
-            "updatetext",
-            "use",
-            "user",
-            "using",
-            "value",
-            "values",
-            "varchar",
-            "version",
-            "view",
-            "waitfor",
-            "when",
-            "where",
-            "while",
-            "with",
-            "within",
-            "without",
-            "writetext",
-            "xact_abort",
-            "xml",
-        };
 
         [Fact]
         public void InsertTextShouldIncludeBracketGivenNameWithSpace()
@@ -477,19 +182,18 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer
         }
 
         [Fact]
-        public void InsertTextShouldIncludeBracketGivenReservedName()
+        public void InsertTextShouldNotIncludeBracketGivenReservedName()
         {
-            foreach (string word in ReservedWords)
+            foreach (string word in AutoCompleteHelper.DefaultCompletionText)
             {
                 string declarationTitle = word;
-                string expected = "[" + declarationTitle + "]";
-                DeclarationType declarationType = DeclarationType.Table;
+                DeclarationType declarationType = DeclarationType.ApplicationRole;
                 string tokenText = "";
                 SqlCompletionItem item = new SqlCompletionItem(declarationTitle, declarationType, tokenText);
                 CompletionItem completionItem = item.CreateCompletionItem(0, 1, 2);
 
                 Assert.Equal(completionItem.Label, word);
-                Assert.Equal(completionItem.InsertText, expected);
+                Assert.Equal(completionItem.InsertText, word);
                 Assert.Equal(completionItem.Detail, word);
             }
         }
@@ -539,6 +243,49 @@ namespace Microsoft.SqlTools.ServiceLayer.UnitTests.LanguageServer
             Assert.Equal(completionItem.Detail, expected);
         }
 
+        [Theory]
+        [InlineData(DeclarationType.BuiltInFunction)]
+        [InlineData(DeclarationType.ScalarValuedFunction)]
+        [InlineData(DeclarationType.TableValuedFunction)]
+        public void FunctionsShouldHaveParenthesesAdded(DeclarationType declarationType)
+        {
+            foreach (string word in AutoCompleteHelper.DefaultCompletionText)
+            {
+                string declarationTitle = word;
+                string tokenText = "";
+                SqlCompletionItem item = new SqlCompletionItem(declarationTitle, declarationType, tokenText);
+                CompletionItem completionItem = item.CreateCompletionItem(0, 1, 2);
+
+                Assert.Equal(declarationTitle, completionItem.Label);
+                Assert.Equal($"{declarationTitle}()", completionItem.InsertText);
+                Assert.Equal(declarationTitle, completionItem.Detail);
+            }
+
+        }
+
+        [Theory]
+        [InlineData(DeclarationType.Server)]
+        [InlineData(DeclarationType.Database)]
+        [InlineData(DeclarationType.Table)]
+        [InlineData(DeclarationType.Column)]
+        [InlineData(DeclarationType.View)]
+        [InlineData(DeclarationType.Schema)]
+        public void NamedIdentifiersShouldBeBracketQuoted(DeclarationType declarationType)
+        {
+            string declarationTitle = declarationType.ToString();
+            // All words - both reserved and not - should be bracket quoted for these types
+            foreach (string word in AutoCompleteHelper.DefaultCompletionText.ToList().Append("NonReservedWord"))
+            {
+                string tokenText = "";
+                SqlCompletionItem item = new SqlCompletionItem(declarationTitle, declarationType, tokenText);
+                CompletionItem completionItem = item.CreateCompletionItem(0, 1, 2);
+
+                Assert.Equal(declarationTitle, completionItem.Label);
+                Assert.Equal($"[{declarationTitle}]", completionItem.InsertText);
+                Assert.Equal(declarationTitle, completionItem.Detail);
+            }
+        }
+
         [Fact]
         public void KindShouldBeModuleGivenSchemaDeclarationType()
         {