CppTools: More consistent fuzzy matching behavior

There were annoying inconsistencies, for instance:
    - A not fully matching declaration was found from the definition,
      but not vice versa.
    - An implementation MyClass::foo(int) would fall back to a
      declaration foo(), but an implementation MyClass::foo() would
      not fall back to a declaration foo(int).
These cases behave consistently now. To this end, the clang code model
now forwards to the built-in code model if a function lookup has failed.

Fuzzy matching for free functions has been limited, as the cons appear
to outweigh the pros. For instance:
    void foo(int);
    void foo(double) {}
Following the definition would lead to the non-matching declaration,
which the user most likely does not want.

As a side effect, redundant code has been removed in the SymbolFinder
class.

Fixes: QTCREATORBUG-20279
Change-Id: Ib97d6710c7e12fb0fdbc30b51a0067e09bfc2190
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/plugins/cppeditor/followsymbol_switchmethoddecldef_test.cpp

@@ -935,6 +935,16 @@ void CppEditorPlugin::test_FollowSymbolUnderCursor_data()
         "void Foo::foo(int) {}\n"
     );
 
+    QTest::newRow("matchFunctionSignature_Follow_3.5") << _(
+        "void foo(int);\n"
+        "void @$foo() {}\n"
+    );
+
+    QTest::newRow("matchFunctionSignature_Follow_3.6") << _(
+        "void foo(int);\n"
+        "void @$foo(double) {}\n"
+    );
+
     QTest::newRow("matchFunctionSignature_Follow_4") << _(
         "class Foo {\n"
         "    void foo(int);\n"
@@ -963,17 +973,33 @@ void CppEditorPlugin::test_FollowSymbolUnderCursor_data()
         "void Foo::@foo(int) {}\n"
     );
 
-    QTest::newRow("matchFunctionSignature_Follow_8") << _(
+    QTest::newRow("matchFunctionSignature_Follow_8_fuzzy") << _(
         "class Foo {\n"
-        "    void @$foo(int *);\n"
+        "    void @foo(int *);\n"
         "};\n"
-        "void Foo::foo(const int *) {}\n"
+        "void Foo::$foo(const int *) {}\n"
     );
 
-    QTest::newRow("matchFunctionSignature_Follow_9") << _(
+    QTest::newRow("matchFunctionSignature_Follow_8_exact") << _(
         "class Foo {\n"
-        "    void @$foo(int&);\n"
+        "    void @foo(int *);\n"
         "};\n"
+        "void Foo::foo(const int *) {}\n"
+        "void Foo::$foo(int *) {}\n"
+    );
+
+    QTest::newRow("matchFunctionSignature_Follow_9_fuzzy") << _(
+        "class Foo {\n"
+        "    void @foo(int&);\n"
+        "};\n"
+        "void Foo::$foo(const int&) {}\n"
+    );
+
+    QTest::newRow("matchFunctionSignature_Follow_9_exact") << _(
+        "class Foo {\n"
+        "    void @foo(int&);\n"
+        "};\n"
+        "void Foo::$foo(int&) {}\n"
         "void Foo::foo(const int&) {}\n"
     );
 
@@ -1171,6 +1197,48 @@ void CppEditorPlugin::test_FollowSymbolUnderCursor_multipleDocuments_data()
                                 "foo.cpp")
     );
 
+    QTest::newRow("matchFunctionSignatureFuzzy1Forward") << (QList<TestDocumentPtr>()
+        << TestDocument::create("class Foo {\n"
+                                "    void @foo(int);\n"
+                                "    void foo();\n"
+                                "};\n",
+                                "foo.h")
+        << TestDocument::create("#include \"foo.h\"\n"
+                                "void Foo::$foo() {}\n",
+                                "foo.cpp")
+    );
+
+    QTest::newRow("matchFunctionSignatureFuzzy1Backward") << (QList<TestDocumentPtr>()
+        << TestDocument::create("class Foo {\n"
+                                "    void $foo(int);\n"
+                                "};\n",
+                                "foo.h")
+        << TestDocument::create("#include \"foo.h\"\n"
+                                "void Foo::@foo() {}\n",
+                                "foo.cpp")
+    );
+
+    QTest::newRow("matchFunctionSignatureFuzzy2Forward") << (QList<TestDocumentPtr>()
+        << TestDocument::create("class Foo {\n"
+                                "    void foo(int);\n"
+                                "    void @foo();\n"
+                                "};\n",
+                                "foo.h")
+        << TestDocument::create("#include \"foo.h\"\n"
+                                "void Foo::$foo(int) {}\n",
+                                "foo.cpp")
+    );
+
+    QTest::newRow("matchFunctionSignatureFuzzy2Backward") << (QList<TestDocumentPtr>()
+        << TestDocument::create("class Foo {\n"
+                                "    void $foo();\n"
+                                "};\n",
+                                "foo.h")
+        << TestDocument::create("#include \"foo.h\"\n"
+                                "void Foo::@foo(int) {}\n",
+                                "foo.cpp")
+    );
+
     QTest::newRow("globalVar") << QList<TestDocumentPtr>{
             TestDocument::create("namespace NS { extern int @globalVar; }\n", "file.h"),
             TestDocument::create(

src/plugins/cpptools/symbolfinder.cpp

@@ -46,21 +46,30 @@ using namespace CppTools;
 
 namespace {
 
+struct Hit {
+    Hit(Function *func, bool exact) : func(func), exact(exact) {}
+    Hit() = default;
+
+    Function *func = nullptr;
+    bool exact = false;
+};
+
 class FindMatchingDefinition: public SymbolVisitor
 {
     Symbol *_declaration = nullptr;
     const OperatorNameId *_oper = nullptr;
-    QList<Function *> _result;
+    const bool _strict;
+    QList<Hit> _result;
 
 public:
-    explicit FindMatchingDefinition(Symbol *declaration)
+    explicit FindMatchingDefinition(Symbol *declaration, bool strict)
-        : _declaration(declaration)
+        : _declaration(declaration), _strict(strict)
     {
         if (_declaration->name())
             _oper = _declaration->name()->asOperatorNameId();
     }
 
-    QList<Function *> result() const { return _result; }
+    const QList<Hit> result() const { return _result; }
 
     using SymbolVisitor::visit;
 
@@ -69,11 +78,15 @@ public:
         if (_oper) {
             if (const Name *name = fun->unqualifiedName()) {
                     if (_oper->match(name))
-                        _result.append(fun);
+                        _result.append({fun, true});
             }
         } else if (Function *decl = _declaration->type()->asFunctionType()) {
-            if (fun->match(decl))
+            if (fun->match(decl)) {
-                _result.append(fun);
+                _result.prepend({fun, true});
+            } else if (!_strict
+                       && Matcher::match(fun->unqualifiedName(), decl->unqualifiedName())) {
+                _result.append({fun, false});
+            }
         }
 
         return false;
@@ -155,6 +168,7 @@ Function *SymbolFinder::findMatchingDefinition(Symbol *declaration,
         return nullptr;
     }
 
+    Hit best;
     foreach (const QString &fileName, fileIterationOrder(declFile, snapshot)) {
         Document::Ptr doc = snapshot.document(fileName);
         if (!doc) {
@@ -176,76 +190,38 @@ Function *SymbolFinder::findMatchingDefinition(Symbol *declaration,
                 continue;
         }
 
-        FindMatchingDefinition candidates(declaration);
+        FindMatchingDefinition candidates(declaration, strict);
         candidates.accept(doc->globalNamespace());
 
-        const QList<Function *> result = candidates.result();
+        const QList<Hit> result = candidates.result();
-        if (!result.isEmpty()) {
+        if (result.isEmpty())
+            continue;
+
         LookupContext context(doc, snapshot);
-
-            QList<Function *> viableFunctions;
-
         ClassOrNamespace *enclosingType = context.lookupType(declaration);
         if (!enclosingType)
             continue; // nothing to do
 
-            foreach (Function *fun, result) {
+        for (const Hit &hit : result) {
-                if (fun->unqualifiedName()->isDestructorNameId() != declaration->unqualifiedName()->isDestructorNameId())
+            QTC_CHECK(!strict || hit.exact);
-                    continue;
 
-                const QList<LookupItem> declarations = context.lookup(fun->name(), fun->enclosingScope());
+            const QList<LookupItem> declarations = context.lookup(hit.func->name(),
+                                                                  hit.func->enclosingScope());
             if (declarations.isEmpty())
                 continue;
-
+            if (enclosingType != context.lookupType(declarations.first().declaration()))
-                const LookupItem best = declarations.first();
-                if (enclosingType == context.lookupType(best.declaration()))
-                    viableFunctions.append(fun);
-            }
-
-            if (viableFunctions.isEmpty())
                 continue;
 
-            else if (!strict && viableFunctions.length() == 1)
+            if (hit.exact)
-                return viableFunctions.first();
+                return hit.func;
 
-            Function *best = nullptr;
+            if (!best.func || hit.func->argumentCount() == declarationTy->argumentCount())
-
+                best = hit;
-            foreach (Function *fun, viableFunctions) {
-                if (!(fun->unqualifiedName()
-                      && fun->unqualifiedName()->match(declaration->unqualifiedName()))) {
-                    continue;
-                }
-                if (fun->argumentCount() == declarationTy->argumentCount()) {
-                    if (!strict && !best)
-                        best = fun;
-
-                    const unsigned argc = declarationTy->argumentCount();
-                    unsigned argIt = 0;
-                    for (; argIt < argc; ++argIt) {
-                        Symbol *arg = fun->argumentAt(argIt);
-                        Symbol *otherArg = declarationTy->argumentAt(argIt);
-                        if (!arg->type().match(otherArg->type()))
-                            break;
-                    }
-
-                    if (argIt == argc
-                            && fun->isConst() == declaration->type().isConst()
-                            && fun->isVolatile() == declaration->type().isVolatile()) {
-                        best = fun;
-                    }
-                }
-            }
-
-            if (strict && !best)
-                continue;
-
-            if (!best)
-                best = viableFunctions.first();
-            return best;
         }
     }
 
-    return nullptr;
+    QTC_CHECK(!best.exact);
+    return strict ? nullptr : best.func;
 }
 
 Symbol *SymbolFinder::findMatchingVarDefinition(Symbol *declaration, const Snapshot &snapshot)
@@ -455,7 +431,16 @@ QList<Declaration *> SymbolFinder::findMatchingDeclaration(const LookupContext &
     QList<Declaration *> nameMatch, argumentCountMatch, typeMatch;
     findMatchingDeclaration(context, functionType, &typeMatch, &argumentCountMatch, &nameMatch);
     result.append(typeMatch);
-    result.append(argumentCountMatch);
+
+    // For member functions not defined inline, add fuzzy matches as fallbacks. We cannot do
+    // this for free functions, because there is no guarantee that there's a separate declaration.
+    QList<Declaration *> fuzzyMatches = argumentCountMatch + nameMatch;
+    if (!functionType->enclosingScope() || !functionType->enclosingScope()->isClass()) {
+        for (Declaration * const d : fuzzyMatches) {
+            if (d->enclosingScope() && d->enclosingScope()->isClass())
+                result.append(d);
+        }
+    }
     return result;
 }
 

src/tools/clangbackend/source/clangfollowsymbol.cpp

@@ -131,7 +131,11 @@ FollowSymbolResult FollowSymbol::followSymbol(CXTranslationUnit tu,
             return extractMatchingTokenRange(declCursor, declCursor.spelling());
         }
 
-        return extractMatchingTokenRange(cursor.canonical(), tokenSpelling);
+        const Cursor declCursor = cursor.canonical();
+        FollowSymbolResult result;
+        result.range = extractMatchingTokenRange(declCursor, tokenSpelling);
+        result.isResultOnlyForFallBack = cursor.isFunctionLike() && declCursor == cursor;
+        return result;
     }
 
     if (!cursor.isDeclaration()) {
@@ -150,12 +154,13 @@ FollowSymbolResult FollowSymbol::followSymbol(CXTranslationUnit tu,
         return result;
     }
 
+    const bool isFunction = cursor.isFunctionLike();
     cursor = cursor.definition();
     // If we are able to find a definition in current TU
     if (!cursor.isNull())
         return extractMatchingTokenRange(cursor, tokenSpelling);
 
-    return SourceRangeContainer();
+    return FollowSymbolResult({}, isFunction);
 }
 
 } // namespace ClangBackEnd