Improve Searchfox's ability to resolve symbol definitions in templated C++ code
Categories
(Webtools :: Searchfox, enhancement)
Tracking
(Not tracked)
People
(Reporter: botond, Assigned: botond)
References
(Blocks 5 open bugs)
Details
(Keywords: meta)
Providing accurate code navigation in templated C++ code is a hard problem. Many symbol names in templated code are "dependent names" (meaning, they depend in some way on a template parameter), which the compiler only resolves to a symbol definition during instantiation -- but we read and write and want to navigate uninstantiated code.
Tools typically employ heuristics to provide navigation features in templated code, and Searchfox has some good ones already.
As someone who also uses clangd to navigate and edit mozilla-central locally, I've noticed that there are some cases that clangd's heuristics handle but Searchfox's currently do not.
I'd like to port over the missing pieces of clangd's template heuristics (many of which I implemented in clangd) to Searchfox. Since clangd and Searchfox are both based on clang's C++ API, Searchfox should in principle be able to implement anything that clangd does.
Here are some examples of symbol names in templated code which Searchfox currently does not resolve to a symbol definition but clangd does:
- Function/method names
ProjectPoint()
hereHasPositiveWCoord()
hereFromUnknownMatrix()
andToUnknownMatrix()
here- (Other similar examples in our Matrix library, these likely have a small set of common root causes.)
TransformPoint()
here- Here, clangd's current behaviour isn't ideal either (it offers all the overloads of the target function, not just the relevant one), but better than nothing
- Cases using the
template
disambiguator, such asdowncast
here format
here (maybe related to the use of a union?)
- Type names
Identity
here (I filed bug 1621789 about this earlier)const_iterator
hereArgType
here
(Note 1: I provided permalinks to protect against code rot, but testing Searchfox's navigation behaviour requires clicking "Go to latest revision" first, since only the latest revision is indexed semantically.)
(Note 2: There are also cases where Searchfox is able to resolve a definition that clangd doesn't! In some cases, this is due to known limitations of clangd, such as only indexing code on one platform at a time. In other cases, I imagine Searchfox knows some tricks that clangd doesn't, which I'm hoping to learn about and port back over to clangd in turn :))
In this bug, I plan to focus on Searchfox's "go to definition" feature. "Find references" in templated code could also use improvement (e.g. bug 1535149 and related), but I suspect that's less a matter of porting over heuristics from clangd (which unfortunately also doesn't handle this very well), and more a matter of getting Searchfox to index template instantiations.
Over the course of this work, I would also like to keep an eye out for possible opportunities to "upstream" code implementing relevant heuristics to a place that both clangd and Searchfox can reuse (e.g. libclangAST/libclangSema).
Assignee | ||
Comment 1•2 years ago
|
||
My plan here is to fix this incrementally in dependent bugs, treating this as a meta bug.
(In reply to Botond Ballo [:botond] from comment #0)
Here are some examples of symbol names in templated code which Searchfox currently does not resolve to a symbol definition but clangd does:
- Function/method names
ProjectPoint()
here
Reduced as bug 1799579.
Assignee | ||
Comment 2•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
ProjectPoint()
here
Confirmed fixed by bug 1799579.
Assignee | ||
Comment 3•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
HasPositiveWCoord()
here
Reduced as bug 1828223.
Assignee | ||
Comment 4•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
FromUnknownMatrix()
andToUnknownMatrix()
here
Reduced ToUnknownMatrix()
as bug 1833072.
Assignee | ||
Comment 5•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
FromUnknownMatrix()
andToUnknownMatrix()
here
Reduced FromUnknownMatrix()
as bug 1833073.
Assignee | ||
Comment 6•1 year ago
|
||
Assignee | ||
Comment 7•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
- Cases using the
template
disambiguator, such asdowncast
here
With bug 1833272, this is now highlighted as a symbol use, but the only (semantic) context menu item offered for it is "search", and while the symbol name displayed in that context menu item (mozilla::SafeRefPtr::downcast
) looks correct, the only search result is the original use itself, not a definition or declaration (or other uses).
Unfortunately, this has resisted my attempts to reduce it to a minimal testcase for debugging purposes (i.e. single-file testcases that I've tried to prepare with code that has similar structure do not exhibit this issue).
I did spot check other uses of the template disambiguator (such as this one, this one, this one, and this one), and those seem to be working well, so I'm going to defer this particular one for future investigation, without being concerned that we're overlooking an important category of template uses.
Assignee | ||
Comment 8•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
format
here (maybe related to the use of a union?)
Reduced as bug 1834859.
Note that, while I listed this under "function/method names" in comment 0, this is actually a field name, and the issue is likely related to the fact that the heuristics added for previous cases handle methods but not fields.
Assignee | ||
Comment 9•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
Identity
here (I filed bug 1621789 about this earlier)
Posted reduced testcase in bug 1621789. (I'm not sure why I categorized this one under "type names"; this is an enumerator name, not a type name.)
Assignee | ||
Comment 10•1 year ago
|
||
Assignee | ||
Comment 11•1 year ago
|
||
(In reply to Botond Ballo [:botond] from comment #0)
ArgType
here
This one is fixed by the patch in bug 1835696 as well.
Assignee | ||
Comment 12•1 year ago
|
||
As the examples listed in comment 0 have now been addressed, I'm going to close this bug. There are opportunities for further improvement in this area, some of which are tracked in the bugs this one is marked as blocking.
A couple of closing notes:
(In reply to Botond Ballo [:botond] from comment #0)
(Note 2: There are also cases where Searchfox is able to resolve a definition that clangd doesn't! In some cases, this is due to known limitations of clangd, such as only indexing code on one platform at a time. In other cases, I imagine Searchfox knows some tricks that clangd doesn't, which I'm hoping to learn about and port back over to clangd in turn :))
The main trick that Searchfox knows that clangd doesn't that I've come across is the "AutoTemplateContext" machinery to traverse known instantiations of templates and associate uses in the template body with the targets they resolve to in those instantiations (what I've been calling "concrete results" in places like bug 1833552).
Clangd would benefit from machinery like this as well, and I plan to explore adding that as time permits.
Over the course of this work, I would also like to keep an eye out for possible opportunities to "upstream" code implementing relevant heuristics to a place that both clangd and Searchfox can reuse (e.g. libclangAST/libclangSema).
HeuristicResolver is a nice standalone component that Searchfox currently uses a copy of, which would be nice to upstream from clangd to the clang libraries and make it public API, allowing Searchfox to consume the copy inside the clang libraries directly. I plan to explore this upstreaming as time permits. A prerequisite for this is writing a dedicated test suite for HeuristicResolver
, whose code is currently tested indirectly via tests that exercise its clients in clangd (and Searchfox).
Assignee | ||
Updated•1 year ago
|
Description
•