Bug 1799579 - Handle OverloadExpr in MozSearchIndexer

[theres-waldo]

Posting a WIP patch for bug 1799579.

The patch partially fixes the bug, in that the call to Project is now clickable and offers the menu item "Search for function Foo::Project", but it does not yet offer the menu item "Go to definition of Foo::Project", which we'd like as well.

I will continue investigating why it doesn't offer that second menu item, but in the meantime I wanted to post what I have.

---

Also, a note on why I didn't use the AutoTemplateContext machinery:

While OverloadExpr is (usually, and in this testcase in particular) dependent, it's a bit different from CXXDependentScopeMemberExpr in that candidate declarations being referenced are available, and it's only the selection among the candidates that's deferred until instantiation (whereas, with CXXDependentScopeMemberExpr, candidate declarations are not available prior to instantiation because the scope in which name lookup would have to be performed to find the candidates is itself not available).

As a result, we can provide heuristic results for OverloadExpr without looking at instantiations (namely, just provide all the candidates), and I think this is useful enough for a first pass.

I think there's an opportunity to improve the results by using the AutoTemplateContext machinery to discover which candidates are actually used by instantiations and potentially narrow the results that way, but I'm proposing to defer that to a future improvement.

(I should definitely expand the testcase to include a scenario with multiple overloads to illustrate the behaviour, I will do that in an upcoming revision of this patch.)

[asutherland]

Re: being a decl and not a def, assuming the logic is analogous to a pure virtual decl being basically a def, we'd want to add an additional case to https://searchfox.org/mozilla-central/rev/e66cff951667dacd0faa95dfde830564a58a8a3f/build/clang-plugin/mozsearch-plugin/MozsearchIndexer.cpp#1621-1622

// We treat pure virtual declarations as definitions.
Kind = (D2->isThisDeclarationADefinition() || D2->isPure()) ? "def" : "decl";

[theres-waldo]

Ahh, ok, I'm just being silly then and failed to notice that showing the menu item requires an actual definition and not just a declaration. (I'm used to clangd which interprets "go to definition" broadly and will take you to a declaration if that's all it can find.)

I don't think there's a need to special-case anything here.

[asutherland]

Ah, so your branch probably would benefit from being rebased to the current tip. I added "Go to declaration" as a new functionality of the context menu in https://bugzilla.mozilla.org/show_bug.cgi?id=1776522 but it's not present in your branch. In your branch, there's only support for "go to def" and "search".

Actually, just as a general awareness thing, as part of that change, I added the concept of binding slots. This is generally really only relevant for IDLs and language bindings where we're dealing with the same conceptual symbol expressed in different languages (interface definition, C++, JS, usually)... I expect for templates we'd generally be using superclass/overrides relationships, but there may be cases where it could make sense. The docs in the code there provide some context, but also, concretely:

If we run curl -H "Accept: application/json" 'https://searchfox.org/mozilla-central/sorch?q=nsIXPCTestParams::TestOctet&path=' | jq '.semantic | map(.meta)' to see our synthetic XPIDL symbol and its C++ binding which has the same "pretty" identifier:

[
  {
    "structured": 1,
    "pretty": "nsIXPCTestParams::testOctet",
    "sym": "XPIDL_nsIXPCTestParams_testOctet",
    "kind": "method",
    "implKind": "idl",
    "sizeBytes": null,
    "bindingSlots": [
      {
        "slotKind": "method",
        "slotLang": "cpp",
        "sym": "_ZN16nsIXPCTestParams9TestOctetEhPhS0_"
      },
      {
        "slotKind": "method",
        "slotLang": "js",
        "sym": "#testOctet"
      }
    ],
    "supers": [],
    "methods": [],
    "fields": [],
    "overrides": [],
    "props": []
  },
  {
    "structured": 1,
    "pretty": "nsIXPCTestParams::TestOctet",
    "sym": "_ZN16nsIXPCTestParams9TestOctetEhPhS0_",
    "kind": "method",
    "parentsym": "T_nsIXPCTestParams",
    "slotOwner": {
      "slotKind": "method",
      "slotLang": "cpp",
      "sym": "XPIDL_nsIXPCTestParams_testOctet"
    },
    "implKind": "",
    "sizeBytes": null,
    "bindingSlots": [],
    "supers": [],
    "methods": [],
    "fields": [],
    "overrides": [],
    "props": [
      "instance",
      "virtual",
      "user"
    ],
    "overriddenBy": [
      "_ZN15nsXPCTestParams9TestOctetEhPhS0_"
    ]
  }
]

[theres-waldo]

Apologies for the long delay here; I just recently finished digging out from under a bunch of APZ work I had fallen behind on while being on PTO earlier in the year.

I rebased the patch and updated it to illustrate the behaviour in the presence of multiple overloads, which is currently pretty rudimentary: we just offer all of them as candidate targets, even if e.g. the number of parameters is off. There is room to improve on this, but I'd like to leave such improvements to a future patch as they're not completely trivial (the options are either (1) using the AutoTemplateContext machinery to see which candidates are used in practice after instantiation, or (2) using some heuristics to try to narrow down the set of candidates based on the dependent call site, e.g. based on arity).

One other small note: the reason the test file is named bug1781178.cpp rather than bug1799579.cpp is that I figured that as I go on fix other dependencies of bug 1781178, it would be convenient / avoid repetition to reuse some of the declarations in the same testcase file. I appreciate this is a question of style though, so if you'd prefer to have every bug have its own testcase file I can do that too.

The patch should be ready for review now.

[theres-waldo]

I rebased the patch and updated it to illustrate the behaviour in the presence of multiple overloads, which is currently pretty rudimentary: we just offer all of them as candidate targets, even if e.g. the number of parameters is off. There is room to improve on this, but I'd like to leave such improvements to a future patch as they're not completely trivial (the options are either (1) using the AutoTemplateContext machinery to see which candidates are used in practice after instantiation, or (2) using some heuristics to try to narrow down the set of candidates based on the dependent call site, e.g. based on arity).

(FWIW clangd also currently offers all candidates in a situation like this, so the patch gets us to parity with clangd.)

[asutherland]

I'm reviewing now, here's clang -Xclang -ast-dump -fsyntax-only template.cpp for my reference/everyone's reference, plus a screenshot where the pretty colors are intact:

TranslationUnitDecl 0xd1da78 <<invalid sloc>> <invalid sloc>
|-TypedefDecl 0xd1e2e0 <<invalid sloc>> <invalid sloc> implicit __int128_t '__int128'
| `-BuiltinType 0xd1e040 '__int128'
|-TypedefDecl 0xd1e350 <<invalid sloc>> <invalid sloc> implicit __uint128_t 'unsigned __int128'
| `-BuiltinType 0xd1e060 'unsigned __int128'
|-TypedefDecl 0xd1e6c8 <<invalid sloc>> <invalid sloc> implicit __NSConstantString '__NSConstantString_tag'
| `-RecordType 0xd1e440 '__NSConstantString_tag'
|   `-CXXRecord 0xd1e3a8 '__NSConstantString_tag'
|-TypedefDecl 0xd1e760 <<invalid sloc>> <invalid sloc> implicit __builtin_ms_va_list 'char *'
| `-PointerType 0xd1e720 'char *'
|   `-BuiltinType 0xd1db20 'char'
|-TypedefDecl 0xd63128 <<invalid sloc>> <invalid sloc> implicit __builtin_va_list '__va_list_tag[1]'
| `-ConstantArrayType 0xd630d0 '__va_list_tag[1]' 1 
|   `-RecordType 0xd1e850 '__va_list_tag'
|     `-CXXRecord 0xd1e7b8 '__va_list_tag'
|-ClassTemplateDecl 0xd632d0 <template.cpp:1:1, col:35> col:28 Point
| |-TemplateTypeParmDecl 0xd63180 <col:11> col:19 typename depth 0 index 0
| |-CXXRecordDecl 0xd63240 <col:21, col:35> col:28 struct Point definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | `-CXXRecordDecl 0xd63510 <col:21, col:28> col:28 implicit struct Point
| `-ClassTemplateSpecializationDecl 0xd93048 <col:1, col:35> col:28 struct Point definition
|   |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
|   | |-DefaultConstructor exists trivial constexpr defaulted_is_constexpr
|   | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
|   | |-MoveConstructor exists simple trivial
|   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveAssignment exists simple trivial needs_implicit
|   | `-Destructor simple irrelevant trivial needs_implicit
|   |-TemplateArgument type 'float'
|   | `-BuiltinType 0xd1dc80 'float'
|   |-CXXRecordDecl 0xd932a0 <col:21, col:28> col:28 implicit struct Point
|   |-CXXConstructorDecl 0xd93360 <col:28> col:28 implicit used constexpr Point 'void () noexcept' inline default trivial
|   | `-CompoundStmt 0xd937e8 <col:28>
|   |-CXXConstructorDecl 0xd93468 <col:28> col:28 implicit constexpr Point 'void (const Point<float> &)' inline default trivial noexcept-unevaluated 0xd93468
|   | `-ParmVarDecl 0xd93578 <col:28> col:28 'const Point<float> &'
|   `-CXXConstructorDecl 0xd93628 <col:28> col:28 implicit constexpr Point 'void (Point<float> &&)' inline default trivial noexcept-unevaluated 0xd93628
|     `-ParmVarDecl 0xd93738 <col:28> col:28 'Point<float> &&'
`-ClassTemplateDecl 0xd636e0 <line:3:1, line:15:1> line:4:8 Foo
  |-TemplateTypeParmDecl 0xd635b8 <line:3:11> col:19 typename depth 0 index 0
  `-CXXRecordDecl 0xd63650 <line:4:1, line:15:1> line:4:8 struct Foo definition
    |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
    | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
    | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
    | |-MoveConstructor exists simple trivial needs_implicit
    | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
    | |-MoveAssignment exists simple trivial needs_implicit
    | `-Destructor simple irrelevant trivial needs_implicit
    |-CXXRecordDecl 0xd63920 <col:1, col:8> col:8 implicit struct Foo
    |-FunctionTemplateDecl 0xd63d08 <line:5:3, line:6:27> col:8 Project
    | |-TemplateTypeParmDecl 0xd639b0 <line:5:13, col:22> col:22 referenced typename depth 1 index 0 F
    | `-CXXMethodDecl 0xd63c68 <line:6:3, col:27> col:8 Project 'void (Point<F>)'
    |   |-ParmVarDecl 0xd63b40 <col:16, col:23> col:24 'Point<F>'
    |   `-CompoundStmt 0xd93010 <col:26, col:27>
    |-FunctionTemplateDecl 0xd92e90 <line:8:3, line:9:37> col:8 Project
    | |-TemplateTypeParmDecl 0xd63d68 <line:8:13, col:22> col:22 referenced typename depth 1 index 0 F
    | `-CXXMethodDecl 0xd92df0 <line:9:3, col:37> col:8 Project 'void (Point<F>, Point<F>)'
    |   |-ParmVarDecl 0xd63e90 <col:16, col:23> col:24 'Point<F>'
    |   |-ParmVarDecl 0xd63f90 <col:26, col:33> col:34 'Point<F>'
    |   `-CompoundStmt 0xd93020 <col:36, col:37>
    `-CXXMethodDecl 0xd92f58 <line:11:3, line:14:3> line:11:8 Bar 'void ()'
      `-CompoundStmt 0xd93a60 <col:14, line:14:3>
        |-DeclStmt 0xd93940 <line:12:5, col:19>
        | `-VarDecl 0xd931d0 <col:5, col:18> col:18 referenced p 'Point<float>':'Point<float>' callinit
        |   `-CXXConstructExpr 0xd93918 <col:18> 'Point<float>':'Point<float>' 'void () noexcept'
        `-CallExpr 0xd93a38 <line:13:5, col:14> '<dependent type>'
          |-UnresolvedMemberExpr 0xd939b8 <col:5> '<bound member function type>' lvalue
          `-DeclRefExpr 0xd93a18 <col:13> 'Point<float>':'Point<float>' lvalue Var 0xd931d0 'p' 'Point<float>':'Point<float>'

[asutherland]

Sorry for the delay, I wanted to try and dig into what might be happening for the structured information for Foo in the example before and after these changes. I'm including my brief investigation notes below for future reference, but none of this impacts this review.

Brief Structured Info Investigation

Not too surprisingly, we currently don't emit anything for the ClassTemplateDecl for Foo or the CXXRecordDecl it wraps as indicated by adding a check of filter-analysis bug1781178.cpp -i Foo which nets us output of:

[
  {
    "loc": "00004:7-10",
    "source": 1,
    "nestingRange": "4:11-15:0",
    "syntax": "def,type",
    "pretty": "type Foo",
    "sym": "T_Foo"
  },
  {
    "loc": "00004:7-10",
    "target": 1,
    "kind": "def",
    "pretty": "Foo",
    "sym": "T_Foo",
    "peekRange": "4-4"
  }
]

We do have explicit handling of TraverseClassTemplateDecl for our existing template machinery which pushes a template context. And here is currently where we emitStructuredInfo for RecordDecl subclasses which explicitly bails out because there is a template context because:

1. our call to getASTRecordLayout would crash clang in cases where it obviously couldn't determine a concrete layout
2. My concept of how to express the relationship between templates was even more fuzzy then.
   • My most recent thinking is that it makes sense for the crossref process to accumulate sets of template arguments into named sets in a similar method to the derivation of the list of subclasses. So for RefPtr<T> we'd know the set of types that ever are instantiated for T. And for a situation like nsTArray<T> where we expect to have different layouts for nsTArray<float> versus nsTArray<double> we could use a similar hashing approach to keeping track of the platform variants. (This would of course require that we make sure that the layouts are expressed in terms of the template argument names and never the argument payloads, as that would impact the hash/equality. That information would be stored externally to the layout.)

[asutherland]

One thing I find that can be handy for new functionality is to use the snapshot mechanism to capture the state of the snapshot before and after the fix. This is also not too hard to do after the fact, so just for my reference, here is pre-fix for the new test case:

[
  {
    "loc": "00006:7-14",
    "source": 1,
    "nestingRange": "6:25-6:26",
    "syntax": "def,function",
    "type": "void (Point<F>)",
    "pretty": "function Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__E"
  },
  {
    "loc": "00006:7-14",
    "target": 1,
    "kind": "def",
    "pretty": "Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__E",
    "context": "Foo",
    "contextsym": "T_Foo",
    "peekRange": "6-6"
  },
  {
    "loc": "00009:7-14",
    "source": 1,
    "nestingRange": "9:35-9:36",
    "syntax": "def,function",
    "type": "void (Point<F>, Point<F>)",
    "pretty": "function Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_"
  },
  {
    "loc": "00009:7-14",
    "target": 1,
    "kind": "def",
    "pretty": "Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_",
    "context": "Foo",
    "contextsym": "T_Foo",
    "peekRange": "9-9"
  }
]

and the new bits that show up with the fix are:

  {
    "loc": "00013:4-11",
    "source": 1,
    "syntax": "use,function",
    "type": "void (Point<F>)",
    "pretty": "function Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__E"
  },
  {
    "loc": "00013:4-11",
    "source": 1,
    "syntax": "use,function",
    "type": "void (Point<F>, Point<F>)",
    "pretty": "function Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_"
  },
  {
    "loc": "00013:4-11",
    "target": 1,
    "kind": "use",
    "pretty": "Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__E",
    "context": "Foo::Bar",
    "contextsym": "_ZN3Foo3BarEv"
  },
  {
    "loc": "00013:4-11",
    "target": 1,
    "kind": "use",
    "pretty": "Foo::Project",
    "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_",
    "context": "Foo::Bar",
    "contextsym": "_ZN3Foo3BarEv"
  }

[asutherland]

This is the relevant JSON dump for the call line excerpted from clang -Xclang -ast-dump=json -fsyntax-only bug1781178.cpp and unindented. There isn't really that much more than in the textual presentation, but the extra verbosity is helpful to me usually:

{
  "id": "0x7def08",
  "kind": "CallExpr",
  "range": {
    "begin": {
      "offset": 226,
      "line": 13,
      "col": 5,
      "tokLen": 7
    },
    "end": {
      "offset": 235,
      "col": 14,
      "tokLen": 1
    }
  },
  "type": {
    "qualType": "<dependent type>"
  },
  "valueCategory": "prvalue",
  "inner": [
    {
      "id": "0x7dee88",
      "kind": "UnresolvedMemberExpr",
      "range": {
        "begin": {
          "offset": 226,
          "col": 5,
          "tokLen": 7
        },
        "end": {
          "offset": 226,
          "col": 5,
          "tokLen": 7
        }
      },
      "type": {
        "qualType": "<bound member function type>"
      },
      "valueCategory": "lvalue"
    },
    {
      "id": "0x7deee8",
      "kind": "DeclRefExpr",
      "range": {
        "begin": {
          "offset": 234,
          "col": 13,
          "tokLen": 1
        },
        "end": {
          "offset": 234,
          "col": 13,
          "tokLen": 1
        }
      },
      "type": {
        "desugaredQualType": "Point<float>",
        "qualType": "Point<float>"
      },
      "valueCategory": "lvalue",
      "referencedDecl": {
        "id": "0x7de6a0",
        "kind": "VarDecl",
        "name": "p",
        "type": {
          "desugaredQualType": "Point<float>",
          "qualType": "Point<float>"
        }
      }
    }
  ]
}

[asutherland]

I rebased the patch and updated it to illustrate the behaviour in the presence of multiple overloads, which is currently pretty rudimentary: we just offer all of them as candidate targets, even if e.g. the number of parameters is off. There is room to improve on this, but I'd like to leave such improvements to a future patch as they're not completely trivial (the options are either (1) using the AutoTemplateContext machinery to see which candidates are used in practice after instantiation, or (2) using some heuristics to try to narrow down the set of candidates based on the dependent call site, e.g. based on arity).

Yeah, this definitely seems like the right incremental step to take given the complexity here. And this is largely consistent with searchfox's previous treatment of C++ method overrides where searchfox would essentially conflate all the overrides in a set. (Note that this was due to both the C++ indexer's behavior plus identifier lookup policies.)

That said, before those changes, visitIdentifier used to take a vector of symbols so that it could emit a single source record with all the symbols combined. That would avoid the weirdness with the context menu which currently results in the VM having 2 sets of "Go to definition of", "Search for", etc. However, this shouldn't actually be a problem in practice for mozilla-central trees because the merge-analyses step will coalesce the 2 "source" records into a single "source" record with both symbols, so I don't think it makes sense to go back to taking a vector unless we expect that we'll never be able to address this specific situation.

For example, running tools/target/release/searchfox-tool '-o pretty --server=/home/vagrant/index/config.json --tree=tests merge-analyses bug1781178.cpp' from /vagrant in the VM, this is what I end up with for the uses of Project:

{             
  "loc": "00013:4-11",        
  "target": 1,                         
  "kind": "use",           
  "pretty": "Foo::Project",
  "sym": "_ZN3Foo7ProjectE5PointITL0__E",
  "context": "Foo::Bar",
  "contextsym": "_ZN3Foo3BarEv"
}              
{                        
  "loc": "00013:4-11",              
  "target": 1,     
  "kind": "use",         
  "pretty": "Foo::Project",
  "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_",
  "context": "Foo::Bar",
  "contextsym": "_ZN3Foo3BarEv"
}
{
  "loc": "00013:4-11",
  "source": 1,
  "syntax": "function,use",
  "pretty": "function Foo::Project",
  "sym": "_ZN3Foo7ProjectE5PointITL0__ES2_,_ZN3Foo7ProjectE5PointITL0__E",
  "type": "void (Point<F>)"
}

Note that there's only 1 source record because we explicitly coalesce source records that have the same "pretty" value and exist at the same location. (This is necessary for differences in symbols between platforms.)

[asutherland]

Consistent with the above, this looks good! Thank you!

I wonder if you should rebase this stack to no longer include the already landed #617 ? I'm not sure.

I invited you to be part of the mozsearch org so you can potentially just hit the merge button yourself.

Note that of course there's no automation to get this into m-c, so you'll need to copy the file over to there, sanity check you're not clobbering anything, and then push a review to phabricator, etc. I'm happy to r+ there too, although I don't know if that's strictly necessary given that this is canonical upstream, but it can't hurt.

[theres-waldo]

Thanks for the review!

I wonder if you should rebase this stack to no longer include the already landed #617 ?

Yep, rebased.

Note that of course there's no automation to get this into m-c, so you'll need to copy the file over to there, sanity check you're not clobbering anything, and then push a review to phabricator, etc. I'm happy to r+ there too, although I don't know if that's strictly necessary given that this is canonical upstream, but it can't hurt.

IIUC, Searchfox's m-c index itself will pick up the change from this repo, right?

Perhaps, if there's no particularly pressing reason to get the changes into the m-c copy, I can do a batch cherry-pick of all the bug 1781178 fixes once they're all done?

[asutherland]

IIUC, Searchfox's m-c index itself will pick up the change from this repo, right?

No?

The in-tree copy of https://searchfox.org/mozilla-central/source/build/clang-plugin/mozsearch-plugin/MozsearchIndexer.cpp is what is used to build the taskcluster artifacts that get downloaded from taskcluster by fetch-tc-artifacts.sh. We only use the copy of MozsearchIndexer.cpp that's checked into this repository for configurations where we run the build process on the indexer, something we prefer to avoid. Currently that means only the following trees which do builds in their build script will see the changes:

• wubkat
• nss

Coincidentally, it also means that mozilla-beta and mozilla-release and mozilla-esr102 would only see changes if we uplift any landed changes to MozsearchIndexer.cpp. Usually this isn't a major problem and we also don't uplift, but it does mean that we always need to be very careful about any potentially breaking schema changes. Note that ESRs older than 102 don't have semantic indexing because the taskcluster jobs get turned off for them and we need them, so we change the scripts for the ESRs once they become unsupported.

Perhaps, if there's no particularly pressing reason to get the changes into the m-c copy, I can do a batch cherry-pick of all the bug 1781178 fixes once they're all done?

I have no strong opinion on this and would leave that call to @emilio. My weak opinion is that if you think you'll be able to address the overload ambiguity in the next ~month, it probably makes sense to intentionally delay propagating to m-c to avoid creating extra confusion from the changes in the behavior.

[theres-waldo]

IIUC, Searchfox's m-c index itself will pick up the change from this repo, right?

No?

The in-tree copy of https://searchfox.org/mozilla-central/source/build/clang-plugin/mozsearch-plugin/MozsearchIndexer.cpp is what is used to build the taskcluster artifacts that get downloaded from taskcluster by fetch-tc-artifacts.sh. [...]

Ah, my bad, I misunderstood this aspect of the setup.

Perhaps, if there's no particularly pressing reason to get the changes into the m-c copy, I can do a batch cherry-pick of all the bug 1781178 fixes once they're all done?

I have no strong opinion on this and would leave that call to @emilio.

Given my updated understanding that rolling a change out to users does require copying the change over into m-c, my updated preference is to copy the fixes over as soon as they're merged into this repo, for a couple of reasons:

1. It will be helpful to observe the behaviour of Searchfox on m-c with the patch in place, to help focus further improvements (i.e. see which other templatey use sites in m-c are fixed by this patch and which need additional fixes)
2. Users can start benefiting from each improvement as soon as it's ready

if you think you'll be able to address the overload ambiguity in the next ~month

My thinking is to look at the rest of the improvements described in bug 1781178 (which I'm fairly confident in as "porting the behaviour over from clangd" type of work) before exploring improvements related to the overload ambiguity issue (which is more of an unknown as something that clangd currently doesn't get right either), and thus I probably wouldn't bank on having the latter addressed within the next month.

it probably makes sense to intentionally delay propagating to m-c to avoid creating extra confusion from the changes in the behavior.

My sense is that going from (1) a symbol usage site having no associated semantic actions, only textual search (the current state), to (2) having an associated semantic search action which finds all the candidates, and then later to (3) also having a go-to-definition action that goes to a particular result, are both strictly improvements; I'm not particularly concerned about confusion from users having (2) for a while.

[asutherland]

Works for me!

[theres-waldo]

I think there's an opportunity to improve the results by using the AutoTemplateContext machinery to discover which candidates are actually used by instantiations and potentially narrow the results that way, but I'm proposing to defer that to a future improvement.

I filed bug 1833555 for this, or rather a refinement of this that takes into account the fact that (based on our discussion in #628) we wouldn't want to discard candidates that aren't used by known instantiations, but we would want to annotate them differently.