Closed Bug 1728376 Opened 3 months ago Closed 3 months ago

Function overrides no longer shown

Categories

(Webtools :: Searchfox, task)

task

Tracking

(firefox93 fixed)

RESOLVED FIXED
Tracking Status
firefox93 --- fixed

People

(Reporter: mccr8, Assigned: asuth)

References

(Blocks 1 open bug, Regression)

Details

Attachments

(2 files)

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.

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

Flags: needinfo?(bugmail)

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:

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:

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.

Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(bugmail)
See Also: → 1466692
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/aec264ab32b9
Searchfox should treat pure virtual declarations as definitions. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Blocks: 1728652
Blocks: 1729164
Regressed by: 1641372
You need to log in before you can comment on or make changes to this bug.