1833695 - "Go to definition" may not appear in context menu for templated functions because of duplicate "def" target records

Status: RESOLVED FIXED

(Webtools :: Searchfox, defect)

Description

[Botond Ballo [:botond]]

STR

1. Go to https://searchfox.org/mozilla-central/source/widget/Theme.cpp#1080
2. Click on DoDrawWidgetBackground
3. Observe that the first item in the context menu is "go to declaration", and there is no "go to definition" item
4. Perform "search" (second context menu item) and observe that the results include the function's definition

Expected results

If searchfox knows where the function's definition is, the context menu includes an item to navigate to it.

Comment 1

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

The problem seems to be that there are 2 definition records (as accessed from the raw-analysis for Theme.cpp), one with a context/contextsym and one without:

{"loc":"01118:12-34","target":1,"kind":"def","pretty":"mozilla::widget::Theme::DoDrawWidgetBackground","sym":"_ZN7mozilla6widget5Theme22DoDrawWidgetBackgroundERT_P8nsIFrameNS_15StyleAppearanceERK6nsRectN8nsITheme12DrawOverflowE","context":"mozilla::widget::Theme::DoDrawWidgetBackground","contextsym":"_ZN7mozilla6widget5Theme22DoDrawWidgetBackgroundERT_P8nsIFrameNS_15StyleAppearanceERK6nsRectN8nsITheme12DrawOverflowE","peekRange":"1118-1122"}
{"loc":"01118:12-34","target":1,"kind":"def","pretty":"mozilla::widget::Theme::DoDrawWidgetBackground","sym":"_ZN7mozilla6widget5Theme22DoDrawWidgetBackgroundERT_P8nsIFrameNS_15StyleAppearanceERK6nsRectN8nsITheme12DrawOverflowE","peekRange":"1118-1122"}

The jumpref heuristics only emit a definition if there is a single definition, but this is technically 2 definitions. The "search" endpoint only shows one of them because of its suppression logic. (And as noted, we don't currently try and merge target records, just suppress exactly identical versions.)

It seems like the reported contextsym may be the symbol itself, which seems like a bug.

Comment 2

[Timothy Nikkel (:tnikkel)]

Note that this seems to happen almost everytime, the specific symbol reported is just one specific one to get a bug going with a specific case.

Comment 3

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

Note that this seems to happen almost everytime, the specific symbol reported is just one specific one to get a bug going with a specific case.

I assume you mean almost every time a templated function is targeted, as opposed to all functions.

Comment 4

[Timothy Nikkel (:tnikkel)]

Hmm, maybe you are correct. I might just be remembering the times it does not work and forgetting the ones when it does.

Comment 5

[Botond Ballo [:botond]]

Reduced testcase:

struct Theme {
  void CreateWebRenderCommandsForWidget() {
    DoDrawWidgetBackground(0);
  }

  template <typename T>
  void DoDrawWidgetBackground(T);
};

template <typename T>
void Theme::DoDrawWidgetBackground(T) {}

Comment 6

[Nicolas Guichard]

I've tracked this down to TraverseFunctionDecl and friends calling TraverseFunctionDecl for the definition with a wrong CurDeclContext when traversing templated code. For instance:

   1   │ struct Theme {
   2   │   void CreateWebRenderCommandsForWidget() {
   3   │     DoDrawWidgetBackground(0);
   4   │   }
   5   │ 
   6   │   template <typename T>
   7   │   void DoDrawWidgetBackground(T); // ← when traversing this CXXMethodDecl, we call TraverseFunctionDecl for the definition below, but CurDeclContext == Theme::DoDrawWidgetBackground
   8   │ };
   9   │ 
  10   │ template <typename T>
  11   │ void Theme::DoDrawWidgetBackground(T) {} // ← TraverseCXXMethodDecl is called again here, with CurDeclContext == nullptr
  12   │ 

Resetting the CurDeclContext before the TraverseFunctionDecl calls (and setting it back after) seems to fix the issue. I don't think this can fail, because out-of-line member function definitions can't be nested in another struct/class/function definition.

I'll send a Mozsearch PR for this later this week.

Comment 7

[Nicolas Guichard]

Attachment: GitHub Pull Request

Comment 8

[Nicolas Guichard]

Attachment: Bug 1833695 - Reset DeclContext stack when analysing out-of-line templated function. r=asuth!

Without this, OutOfLineTemplateShouldntHaveContextSym had two
definition target records that disagreed on the contextsym:

1. The first one was generated when traversing the declaration, because
   it forwards to the definition if out-of-line (see comment in
   TraverseFunctionDecl).
   We didn't reset CurDeclContext, so it looked as-if the definition
   appeared inside the declaration, leading to a bogus contextsym
   (pointing to the function itself).
2. The second one was generated later on when traversing the definition
   itself. This one had a correct contextsym (ie. none).

This introduces a ValueRollback helper to make sure we don't forget to
rollback CurDeclContext afterwards. (inspired by QScopedValueRollback)

See https://github.com/mozsearch/mozsearch/pull/820

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f2d76d7c0f1
Reset DeclContext stack when analysing out-of-line templated function. r=asuth

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1f2d76d7c0f1