1856762 - [templates] searchfox doesn't have semantic information for lambda arguments whose type is determined by the caller (and auto is involved?) in BroadcastAllWorkers helper

Status: RESOLVED FIXED

(Webtools :: Searchfox, defect)

Description

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

We have this interesting helper idiom in workers:

template <typename Func>
void RuntimeService::BroadcastAllWorkers(const Func& aFunc) {
  AssertIsOnMainThread();

  AutoTArray<WorkerPrivate*, 100> workers;
  {
    MutexAutoLock lock(mMutex);

    AddAllTopLevelWorkersToArray(workers);
  }

  for (const auto& worker : workers) {
    aFunc(*worker);
  }
}

void RuntimeService::UpdateAllWorkerContextOptions() {
  BroadcastAllWorkers([](auto& worker) {
    worker.UpdateContextOptions(sDefaultJSSettings->contextOptions);
  });
}

In the call to BroadcastAllWorkers we end up having no semantic data for UpdateContextOptions. Using vscode-clangd's "show AST" magic shows that worker.UpdateContextOptions is only covered by a CXXDependentScopeMember and its only child is a DeclRef covering the worker. The tooltip for the CXXDependentScopeMember notably says '<dependent type>' lvalue .UpdateContextOptions. I don't see any way to get a serialized version of that info from the UI though.

Comment 1

[Botond Ballo [:botond]]

CXXDependentScopeMember is what I would expect there, since the generic lambda basically expands to the body of a templated function, i.e. we can think of

[](auto& worker) {
    worker.UpdateContextOptions(sDefaultJSSettings->contextOptions);
  }

as shorthand for

(struct __Unnamed__ {
  template <typename T>
  void operator()(T& worker) {
    worker.UpdateContextOptions(sDefaultJSSettings->contextOptions);
  }
})()

(ignoring for a moment that you can't explicitly define a template inline like that).

The heuristic resolution step is not going to offer any results here, because we have no information about the type of worker, just T.

However, I would expect the TemplateStack mechanism to kick in and give us a better result based on the instantiation, so the thing to investigate would be, why does that not happen.

Comment 2

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

Thanks for the context! Just for posterity, in a discussion about not having an easy way to get a serialized rep of the AST tree, I wrote up this little function that can be pasted into the VS code devtools to dump a sketchy serialized version of the AST tree:

function astDump(elem) {
  let pieces = [];
  // The ariaLabels have supersede the text nodes so just use them.
  // Each aria-label is in there twice; once on the ".monaco-list-row" root an once
  // in its descendent.  The descendant as a usefull offsetLeft, so we favor that
  // one.
  if (elem.ariaLabel && !elem.classList.contains("monaco-list-row")) {
    let trimmed = elem.ariaLabel.trim();
    let whitespace = " ".repeat(Math.floor(elem.offsetLeft / 8));
    pieces.push(`${whitespace}- ${elem.ariaLabel}\n`);
  }
  for (const kid of elem.children) {
    let kidStr = astDump(kid);
    if (kidStr.length) {
       pieces.push(kidStr);
    }
  }
  if (pieces.length) {
    return pieces.join("");
  }
  return "";
}
console.log(astDump(document.querySelector('h3.title[title="AST"]').closest('.pane').querySelector('.pane-body')))

This produces output that looks like (note that I'm not putting this in backticks because we want the bugzilla rendering to make it more useful, but you can look at the raw version of this message if you want):

• AST
  • CXXMethodDecl 0x7fca04112e98 parent 0x7fcb19163440 prev 0x7fcb1bdca710 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1797:1, line:1801:1> line:1797:22 used UpdateAllWorkerContextOptions 'void ()'
    • TypeSpec specifier
      • QualType 0x7fcb191635d0 'mozilla::dom::workerinternals::RuntimeService'
    • QualType 0x7fcb19160820 'void ()'
      • QualType 0x7fcb1aeba920 'void'
    • CompoundStmt 0x7fca04114330 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1797:54, line:1801:1>
      • ExprWithCleanups 0x7fca04114318 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:3, line:1800:4> 'void'
        • CXXMemberCallExpr 0x7fca04114200 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:3, line:1800:4> 'void'
          • MemberExpr 0x7fca041141a0 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:3> '<bound member function type>' ->BroadcastAllWorkers 0x7fca04114048
            • CXXThisExpr 0x7fca04114190 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:3> 'mozilla::dom::workerinternals::RuntimeService *' implicit this
          • MaterializeTemporaryExpr 0x7fca04114240 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23, line:1800:3> 'const (lambda at /home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23)' lvalue
            • ImplicitCastExpr 0x7fca04114228 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23, line:1800:3> 'const (lambda at /home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23)' <NoOp>
              • LambdaExpr 0x7fca04113d30 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23, line:1800:3> '(lambda at /home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:23)'
                • TemplateTypeParmDecl 0x7fca04113258 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:26, col:32> col:32 implicit class depth 0 index 0 worker:auto
                • ParmVarDecl 0x7fca04113368 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:26, col:32> col:32 referenced worker 'auto &'
                  • QualType 0x7fca04113330 'auto &'
                    • QualType 0x7fca041132b0 'auto'
                • CompoundStmt 0x7fca04113878 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1798:40, line:1800:3>
                  • CallExpr 0x7fca04113850 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1799:5, col:67> '<dependent type>'
                    • CXXDependentScopeMemberExpr 0x7fca041136b0 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1799:5, col:12> '<dependent type>' lvalue .UpdateContextOptions
                      • DeclRefExpr 0x7fca04113690 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1799:5> 'auto' lvalue ParmVar 0x7fca04113368 'worker' 'auto &'
                    • MemberExpr 0x7fca04113820 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1799:33, col:53> 'JS::ContextOptions':'JS::ContextOptions' lvalue ->contextOptions 0x7fcb1ad90828
                      • CXXOperatorCallExpr 0x7fca041137f0 </home/visbrero/rev_control/fgit/gd-pine/dom/workers/RuntimeService.cpp:1799:33, col:51> 'Pointer':'mozilla::dom::workerinternals::JSSettings *' '->'

Comment 3

[Nicolas Guichard]

From this test code:

struct Struct0 {
    void method() {}
};

struct Struct1 {
    void method() const {}
};

void test() {
    const auto lambda = [](auto &&t) {
        t.method();
    };

    lambda(Struct0{});
    lambda(Struct1{});
}

Here is the result of a LambdaExpr::dumpColor on the lambda (well, you won't get the color here):

LambdaExpr 0x2dd85868 'class (lambda at plugin-test-source/lambdas.cpp:10:25)'
|-CXXRecordDecl 0x2dd65280  implicit class definition
| |-DefinitionData generic lambda pass_in_registers empty standard_layout trivially_copyable literal can_const_default_init
| | |-DefaultConstructor defaulted_is_constexpr
| | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveConstructor exists simple trivial needs_implicit
| | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | |-MoveAssignment
| | `-Destructor simple irrelevant trivial
| |-FunctionTemplateDecl 0x2dd65650  operator()
| | |-TemplateTypeParmDecl 0x2dd65478  implicit class depth 0 index 0 t:auto
| | |-CXXMethodDecl 0x2dd653c0  constexpr operator() 'auto (auto &&) const' inline
| | | |-ParmVarDecl 0x2dd655d0  referenced t 'auto &&'
| | | `-CompoundStmt 0x2dd65950
| | |   `-CallExpr 0x2dd65930 '<dependent type>'
| | |     `-CXXDependentScopeMemberExpr 0x2dd658e8 '<dependent type>' lvalue .method
| | |       `-DeclRefExpr 0x2dd658c8 'auto' lvalue ParmVar 0x2dd655d0 't' 'auto &&'
| | |-CXXMethodDecl 0x2dd85f28  used constexpr operator() 'void (struct Struct0 &&) const' implicit_instantiation inline
| | | |-TemplateArgument type 'struct Struct0'
| | | | `-RecordType 0x2dd64b20 'struct Struct0'
| | | |   `-CXXRecord 0x2dd64a88 'Struct0'
| | | |-ParmVarDecl 0x2dd85e10  used t 'struct Struct0 &&'
| | | `-CompoundStmt 0x2dd861c0
| | |   `-CXXMemberCallExpr 0x2dd860b0 'void'
| | |     `-MemberExpr 0x2dd86050 '<bound member function type>' .method 0x2dd64cc0
| | |       `-DeclRefExpr 0x2dd86030 'struct Struct0' lvalue ParmVar 0x2dd85e10 't' 'struct Struct0 &&'
| | `-CXXMethodDecl 0x2dd89188  used constexpr operator() 'void (struct Struct1 &&) const' implicit_instantiation inline
| |   |-TemplateArgument type 'struct Struct1'
| |   | `-RecordType 0x2dd64e20 'struct Struct1'
| |   |   `-CXXRecord 0x2dd64d90 'Struct1'
| |   |-ParmVarDecl 0x2dd89070  used t 'struct Struct1 &&'
| |   `-CompoundStmt 0x2dd89408
| |     `-CXXMemberCallExpr 0x2dd89328 'void'
| |       `-MemberExpr 0x2dd892b0 '<bound member function type>' .method 0x2dd64fc0
| |         `-ImplicitCastExpr 0x2dd89310 'const struct Struct1' lvalue <NoOp>
| |           `-DeclRefExpr 0x2dd89290 'struct Struct1' lvalue ParmVar 0x2dd89070 't' 'struct Struct1 &&'
| |-FunctionTemplateDecl 0x2dd856e8  implicit operator auto (*)(type-parameter-0-0 &&)
| | |-TemplateTypeParmDecl 0x2dd65478  implicit class depth 0 index 0 t:auto
| | `-CXXConversionDecl 0x2dd85638  implicit constexpr operator auto (*)(type-parameter-0-0 &&) 'auto (*(void) const noexcept)(auto &&)' inline
| |-FunctionTemplateDecl 0x2dd857f8  implicit __invoke
| | |-TemplateTypeParmDecl 0x2dd65478  implicit class depth 0 index 0 t:auto
| | `-CXXMethodDecl 0x2dd85748  implicit __invoke 'auto (auto &&)' static inline
| |   `-ParmVarDecl 0x2dd855d0  t 'auto &&'
| `-CXXDestructorDecl 0x2dd85890  implicit referenced ~(lambda at plugin-test-source/lambdas.cpp:10:25) 'void (void) noexcept' inline default trivial
`-CompoundStmt 0x2dd65950
  `-CallExpr 0x2dd65930 '<dependent type>'
    `-CXXDependentScopeMemberExpr 0x2dd658e8 '<dependent type>' lvalue .method
      `-DeclRefExpr 0x2dd658c8 'auto' lvalue ParmVar 0x2dd655d0 't' 'auto &&'

All the data is there, but hidden behind the lambda's implicit class definition, which isn't traversed by default.

Comment 4

[Nicolas Guichard]

Attachment: GitHub Pull Request

Comment 5

[Nicolas Guichard]

Attachment: Bug 1856762 - Visit implicit code inside lambdas. r=asuth!

This reuses the AutoSetContext machinery initially implemented for
constructors to visit the implicit code generated for lambdas too,
giving us the data required to understand the template-dependent code
in lambdas.

AutoSetContext is adapted to support AutoSetContext::Decl == nullptr
because, contrary to CXXConstructorDecl, LambdaExprs aren't NamedDecls.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0230eecc05d
Visit implicit code inside lambdas. r=asuth

Comment 7

[Sandor Molnar[:smolnar]]

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

Comment 8

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

There are some interesting consequences to contextsym values with this change. For example, a symbol search for mozilla::dom::FetchChild::Shutdown goes from showing a context for its use within a lambda of mozilla::dom::FetchChild::CreateForWorker to mozilla::dom::FetchChild::CreateForWorker(WorkerPrivate *, RefPtr<Promise>, RefPtr<AbortSignalImpl>, RefPtr<FetchObserver>)::(anonymous class)::operator(). But there is no crossref entry for that symbol (_ZZN7mozilla3dom10FetchChild15CreateForWorkerEPNS0_13WorkerPrivateE6RefPtrINS0_7PromiseEES4_INS0_15AbortSignalImplEES4_INS0_13FetchObserverEEENK3) so our diagram functionality breaks (working diagram on a.sf.o, broken diagram on sf.o.)

I think there is value in searchfox having insight into the existence of lambdas, but I wonder if for now it would be good for us to make lambdas invisible for context(sym) purposes. For example, for the motivating use of UpdateContextOptions in comment 0 we are showing a context of mozilla::dom::workerinternals::RuntimeService::UpdateAllWorkerContextOptions()::(anonymous class)::operator() but arguably just showing mozilla::dom::workerinternals::RuntimeService::UpdateAllWorkerContextOptions is more useful because the use of a sync lambda is irrelevant[1].

1: I think there are some cases where we could do something interesting with the lambda for context display, but that would be more for cases like MozPromise use for DoAsyncThing()->Then(lambda) where the interesting relationship is that we're logically chained to DoAsyncThing(), but that isn't something the static contextsym will really provide us; we'd only know the argument type being passed into the lambda that may or may not be distinct to DoAsyncThing, but usually will be a generic type. The sync/async delineation would also matter for diagram purposes; I think we would want to show lambdas that are invoked asynchronously as potentially visually distinct from their containing function. But that would be presumably be something where we initially assume everything is sync and just use an ontology mapping. And then the longer term thing could roughly be a clang static analyzer escape analysis pass.