1728376 - Function overrides no longer shown

Status: RESOLVED FIXED

(Webtools :: Searchfox, task)

Description

[Andrew McCreight [:mccr8]]

jrmuizel pointed out in IRC that the function override stuff doesn't appear to be working.

For instance, if you search for nsIVariant::GetAsInt32(), it only shows the base definition and the uses of it, not any of the overrides in subclasses:
https://searchfox.org/mozilla-central/search?q=symbol:_ZN10nsIVariant10GetAsInt32EPi&redirect=false

Here's one of the overrides: https://searchfox.org/mozilla-central/search?q=symbol:_ZN10XPCVariant10GetAsInt32EPi&redirect=false

The latter search only shows the definition and declaration of the method, and no uses of it.

Comment 1

[Andrew McCreight [:mccr8]]

Andrew, I noticed that you landed a few changes recently. Maybe this is a regression from one of those?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Ah, yeah, this is. This was a foreseen change with partially desired side-effects (ex: fixes bug 1466692) that didn't get translated to a must-have action item prior to landing because of the fuzzy question in my mind whether router.py should exactly maintain the existing behavior or if there should be UI changes to either provide for some level of faceting or optional search re-writing to provide the old behavior as an upsell. And testing-wise, unfortunately my changes in bug 1707282 didn't get to adding router.py coverage because the local index "server" wouldn't have access to the memory map stuff we currently only do in python in router.py and I didn't want to try and implement that until performing the rustc_serialize to serde_json transition.

I'll see what I can do in terms of pragmatically addressing this without back-out, as one of the stronger motivations of getting bug 1641372 landed was expediting landing some of the template WIPs I have that build on that like making MakeRefPtr show the constructor usages at the invocation points of MakeRefPtr.

I'll probably solicit some feedback in #searchfox on chat.mozilla.org in terms of UX for this scenario.

Excerpts of prior discussion for prior thoughts

Relevant bit of my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1641372#c0:

• Having only the single symbol in the "source" record means that the searches menu either needs help to build the search symbol list at formatting time or needs the search page to perform the overrides traversal itself.
  • Arguably a UX improvement here would be for the context menu to provide explicit search options for the method and the methods it's overriding as explicitly different options. This would necessitate access to the crossref database in format.rs or just a more structured cheat sheet than the jumps information provides. I'd argue for crossref database access.

Unfortunately most of my comment at https://github.com/mozsearch/mozsearch/pull/408#issue-592720474:

https://bugzilla.mozilla.org/show_bug.cgi?id=1641372#c0 describes the overall set of changes here and the current fallout, which is why this isn't yet a review request. Specifically, the last bullet which is:

• Having only the single "target" record means that the "identifiers" mapping file only maps the pretty name for a symbol to its directly corresponding symbol. This means router.py would need to perform this traversal itself or we would want to intentionally change the search results here.

In particular, if you compare the example from https://bugzilla.mozilla.org/show_bug.cgi?id=1466692:

• https://searchfox.org/mozilla-central/search?q=nsdisplayoutline%3A%3Acreat&path= with all of the overloaded superclass results
• https://asuth.searchfox.org/mozilla-central/search?q=nsdisplayoutline%3A%3Acreat&path= with just the 1 result because we don't walk up the superclass / overrides automatically.

It'd be straightforward to give the existing result, but this might be a case where we should very slightly feature creep things at least so that the existing search endpoint can either separate the results or have a means of selecting which results are displayed.

Either way I think it's necessary that I add some kind of check script check that covers a similar situation to this for the test repo.

my q on UX possibilities in https://bugzilla.mozilla.org/show_bug.cgi?id=1466692#c2:

• The existing https://searchfox.org/mozilla-central/search?q=nsdisplayoutline%3A%3Acreat&path= has all of the overloaded superclass/overrides results you didn't want
• The new https://asuth.searchfox.org/mozilla-central/search?q=nsdisplayoutline%3A%3Acreat&path= has just the 1 result grouped by "Definitions (nsDisplayOutline::CreateWebRenderCommands)" because we don't walk up the superclass / overrides automatically.

Would you only want the new behavior in this case, or would you want some combination of:

• Showing the parent override (but not all of its other overrides)
  • In a separate group, so we have "Definitions (nsDisplayItem::CreateWebRenderCommands)" in addition to the existing "Definitions (nsDisplayOutline::CreateWebRenderCommands)"
  • By having the group header have some interactive pieces like "Definitions ([x] nsDisplayOutline::CreateWebRenderCommands, [ ] overridden nsDisplayItem::CreateWebRenderCommands, [ ] nsDisplayItem::CreateWebRenderCommands overrides)"
• Optionally showing all of those other overrides, possibly via a search upsell link that is just an entirely new search
• Something else!

mstange's response in https://bugzilla.mozilla.org/show_bug.cgi?id=1466692#c3:

The new behavior would definitely work for me.
In addition, linking to separate searches for the superclass implementation and for the overrides would be nice but I don't need it very often.

If you really want to show information about nsDisplayItem::CreateWebRenderCommands on the same search page, then both the definition and the declarations of nsDisplayOutline::CreateWebRenderCommands should come first, above that additional section.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1728376 - Searchfox should treat pure virtual declarations as definitions. r=jrmuizel

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aec264ab32b9
Searchfox should treat pure virtual declarations as definitions. r=jrmuizel

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: mozsearch UI and indexer changes with test coverage

Comment 6

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/aec264ab32b9

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Attachment: PR: Landed much later increase the merge heuristic limit to 100 from 50 for nsIAsyncInputStream