Use overloaded documentation symbol for class methods/table props (#724)

Right now `getDocumentationSymbol` on an overloaded class method or table prop just gives the prop name, not prepended with `/overload/ + toString(overload)`. This causes the incorrect symbol to be used for `table.insert`, `BrickColor.new`, `Color3.new` etc. (present in https://raw.githubusercontent.com/MaximumADHD/Roblox-Client-Tracker/roblox/api-docs/en-us.json) Co-authored-by: vegorov-rbx <[email protected]>
luau-lang · Oct 25, 2022 · ad7d006 · ad7d006
1 parent e64d489
commit ad7d006
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 27 deletions.
diff --git a/Analysis/src/AstQuery.cpp b/Analysis/src/AstQuery.cpp
@@ -427,6 +427,36 @@ ExprOrLocal findExprOrLocalAtPosition(const SourceModule& source, Position pos)
     return findVisitor.result;
 }
 
+static std::optional<DocumentationSymbol> checkOverloadedDocumentationSymbol(
+    const Module& module, const TypeId ty, const AstExpr* parentExpr, const std::optional<DocumentationSymbol> documentationSymbol)
+{
+    if (!documentationSymbol)
+        return std::nullopt;
+
+    // This might be an overloaded function.
+    if (get<IntersectionTypeVar>(follow(ty)))
+    {
+        TypeId matchingOverload = nullptr;
+        if (parentExpr && parentExpr->is<AstExprCall>())
+        {
+            if (auto it = module.astOverloadResolvedTypes.find(parentExpr))
+            {
+                matchingOverload = *it;
+            }
+        }
+
+        if (matchingOverload)
+        {
+            std::string overloadSymbol = *documentationSymbol + "/overload/";
+            // Default toString options are fine for this purpose.
+            overloadSymbol += toString(matchingOverload);
+            return overloadSymbol;
+        }
+    }
+
+    return documentationSymbol;
+}
+
 std::optional<DocumentationSymbol> getDocumentationSymbolAtPosition(const SourceModule& source, const Module& module, Position position)
 {
     std::vector<AstNode*> ancestry = findAstAncestryOfPosition(source, position);
@@ -436,31 +466,7 @@ std::optional<DocumentationSymbol> getDocumentationSymbolAtPosition(const Source
 
     if (std::optional<Binding> binding = findBindingAtPosition(module, source, position))
     {
-        if (binding->documentationSymbol)
-        {
-            // This might be an overloaded function binding.
-            if (get<IntersectionTypeVar>(follow(binding->typeId)))
-            {
-                TypeId matchingOverload = nullptr;
-                if (parentExpr && parentExpr->is<AstExprCall>())
-                {
-                    if (auto it = module.astOverloadResolvedTypes.find(parentExpr))
-                    {
-                        matchingOverload = *it;
-                    }
-                }
-
-                if (matchingOverload)
-                {
-                    std::string overloadSymbol = *binding->documentationSymbol + "/overload/";
-                    // Default toString options are fine for this purpose.
-                    overloadSymbol += toString(matchingOverload);
-                    return overloadSymbol;
-                }
-            }
-        }
-
-        return binding->documentationSymbol;
+        return checkOverloadedDocumentationSymbol(module, binding->typeId, parentExpr, binding->documentationSymbol);
     }
 
     if (targetExpr)
@@ -474,14 +480,14 @@ std::optional<DocumentationSymbol> getDocumentationSymbolAtPosition(const Source
                 {
                     if (auto propIt = ttv->props.find(indexName->index.value); propIt != ttv->props.end())
                     {
-                        return propIt->second.documentationSymbol;
+                        return checkOverloadedDocumentationSymbol(module, propIt->second.type, parentExpr, propIt->second.documentationSymbol);
                     }
                 }
                 else if (const ClassTypeVar* ctv = get<ClassTypeVar>(parentTy))
                 {
                     if (auto propIt = ctv->props.find(indexName->index.value); propIt != ctv->props.end())
                     {
-                        return propIt->second.documentationSymbol;
+                        return checkOverloadedDocumentationSymbol(module, propIt->second.type, parentExpr, propIt->second.documentationSymbol);
                     }
                 }
             }

diff --git a/tests/AstQuery.test.cpp b/tests/AstQuery.test.cpp
@@ -72,6 +72,73 @@ TEST_CASE_FIXTURE(DocumentationSymbolFixture, "overloaded_fn")
     CHECK_EQ(symbol, "@test/global/foo/overload/(string) -> number");
 }
 
+TEST_CASE_FIXTURE(DocumentationSymbolFixture, "class_method")
+{
+    loadDefinition(R"(
+        declare class Foo
+            function bar(self, x: string): number
+        end
+    )");
+
+    std::optional<DocumentationSymbol> symbol = getDocSymbol(R"(
+        local x: Foo
+        x:bar("asdf")
+    )",
+        Position(2, 11));
+
+    CHECK_EQ(symbol, "@test/globaltype/Foo.bar");
+}
+
+TEST_CASE_FIXTURE(DocumentationSymbolFixture, "overloaded_class_method")
+{
+    loadDefinition(R"(
+        declare class Foo
+            function bar(self, x: string): number
+            function bar(self, x: number): string
+        end
+    )");
+
+    std::optional<DocumentationSymbol> symbol = getDocSymbol(R"(
+        local x: Foo
+        x:bar("asdf")
+    )",
+        Position(2, 11));
+
+    CHECK_EQ(symbol, "@test/globaltype/Foo.bar/overload/(Foo, string) -> number");
+}
+
+TEST_CASE_FIXTURE(DocumentationSymbolFixture, "table_function_prop")
+{
+    loadDefinition(R"(
+        declare Foo: {
+            new: (number) -> string
+        }
+    )");
+
+    std::optional<DocumentationSymbol> symbol = getDocSymbol(R"(
+        Foo.new("asdf")
+    )",
+        Position(1, 13));
+
+    CHECK_EQ(symbol, "@test/global/Foo.new");
+}
+
+TEST_CASE_FIXTURE(DocumentationSymbolFixture, "table_overloaded_function_prop")
+{
+    loadDefinition(R"(
+        declare Foo: {
+            new: ((number) -> string) & ((string) -> number)
+        }
+    )");
+
+    std::optional<DocumentationSymbol> symbol = getDocSymbol(R"(
+        Foo.new("asdf")
+    )",
+        Position(1, 13));
+
+    CHECK_EQ(symbol, "@test/global/Foo.new/overload/(string) -> number");
+}
+
 TEST_SUITE_END();
 
 TEST_SUITE_BEGIN("AstQuery");