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>

1 files changed, 52 insertions, 67 deletions

diff --git a/src/plugins/cpptools/symbolfinder.cpp b/src/plugins/cpptools/symbolfinder.cpp
index 34b81a8499..6ee966161c 100644
--- a/src/plugins/cpptools/symbolfinder.cpp
+++ b/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)
-        : _declaration(declaration)
+    explicit FindMatchingDefinition(Symbol *declaration, bool strict)
+        : _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))
-                _result.append(fun);
+            if (fun->match(decl)) {
+                _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();
-        if (!result.isEmpty()) {
-            LookupContext context(doc, snapshot);
-
-            QList<Function *> viableFunctions;
-
-            ClassOrNamespace *enclosingType = context.lookupType(declaration);
-            if (!enclosingType)
-                continue; // nothing to do
-
-            foreach (Function *fun, result) {
-                if (fun->unqualifiedName()->isDestructorNameId() != declaration->unqualifiedName()->isDestructorNameId())
-                    continue;
+        const QList<Hit> result = candidates.result();
+        if (result.isEmpty())
+            continue;
 
-                const QList<LookupItem> declarations = context.lookup(fun->name(), fun->enclosingScope());
-                if (declarations.isEmpty())
-                    continue;
+        LookupContext context(doc, snapshot);
+        ClassOrNamespace *enclosingType = context.lookupType(declaration);
+        if (!enclosingType)
+            continue; // nothing to do
 
-                const LookupItem best = declarations.first();
-                if (enclosingType == context.lookupType(best.declaration()))
-                    viableFunctions.append(fun);
-            }
+        for (const Hit &hit : result) {
+            QTC_CHECK(!strict || hit.exact);
 
-            if (viableFunctions.isEmpty())
+            const QList<LookupItem> declarations = context.lookup(hit.func->name(),
+                                                                  hit.func->enclosingScope());
+            if (declarations.isEmpty())
                 continue;
-
-            else if (!strict && viableFunctions.length() == 1)
-                return viableFunctions.first();
-
-            Function *best = nullptr;
-
-            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)
+            if (enclosingType != context.lookupType(declarations.first().declaration()))
                 continue;
 
-            if (!best)
-                best = viableFunctions.first();
-            return best;
+            if (hit.exact)
+                return hit.func;
+
+            if (!best.func || hit.func->argumentCount() == declarationTy->argumentCount())
+                best = hit;
         }
     }
 
-    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;
 }