1670070 - Show closest `public:`/`private:`/`protected:` line in context bar for class declarations

Status: NEW

(Webtools :: Searchfox, enhancement)

Description

[Kris Maglione [:kmag]]

I noticed that Phabricator does this in the diff view. It makes it easier to figure out the visibility of a given member without having to scroll up to find the context.

Would probably be helpful for parent/child/both lines in IPDL declarations too...

Comment 1

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

I just briefly looked at the clang AST stuff for this and, quoting myself from matrix:

• It looks like it's an AccessSpecDecl which is a type of Decl: https://clang.llvm.org/doxygen/classclang_1_1AccessSpecDecl.html
• So it looks like there should be a TraverseAccessSpecDecl method per https://clang.llvm.org/doxygen/RecursiveASTVisitor_8h_source.html#l01533 (and the DEF_TRAVERSE_DECL) above it that we should use.

The AccessSpecDecl explicitly has a getSourceRange method, but it very much looks like it's only about the access specifier. https://searchfox.org/llvm/rev/b55d55ecd9b2ce99b98bbb2595a1feb957d02a28/clang/lib/Sema/SemaDeclCXX.cpp#3058-3067

/// ActOnAccessSpecifier - Parsed an access specifier followed by a colon.
bool Sema::ActOnAccessSpecifier(AccessSpecifier Access, SourceLocation ASLoc,
                                SourceLocation ColonLoc,
                                const ParsedAttributesView &Attrs) {
  assert(Access != AS_none && "Invalid kind for syntactic access specifier!");
  AccessSpecDecl *ASDecl = AccessSpecDecl::Create(Context, Access, CurContext,
                                                  ASLoc, ColonLoc);
  CurContext->addHiddenDecl(ASDecl);
  return ProcessAccessDeclAttributeList(ASDecl, Attrs);
}

Note that addHiddenDecl doesn't seem too concerning as the doc says "Add the declaration D to this context without modifying any lookup tables." which seems reasonable for this use-case and like we'd still traverse it.

Comment 2

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

Uh, which is to say, the main issue then with dealing with this is that, under our current nesting implementation, we still need to infer a nestingRange (documented in the source record section) for our presentation purposes that covers until the next AccessSpecDecl or the end of the class.

Something we might do here to simplify things in general is to not require that nestingRange be used here, but instead define a mechanism so that we'd add a property like "lazyNesting": "access" which has semantics of:

• create a nesting range that goes until we pop the containing nestingRange or we encounter another lazyNesting value.
• The value here of "access" would just be debugging metadata and maybe as a way to hyperlink to https://en.cppreference.com/book/intro/access_specifiers or some other reference doc. You could imagine a situation where we might support multiple levels of lazyNesting, but then you could shake your head and say "no, that seems like a recipe for headaches if a language had orthogonal mark-up attributes for that".

Note that the current nesting popping logic is not entirely ready for this, as it implies some type of analysis-stream lookahead (which currently is not using an iterator), but that might not be insane. That said, it could end up being easier for MozsearchIndexer to just lazily flush the access specifier when it runs into a new one or the end of the current class traversal. There's no requirement that analysis records be emitted in order; we sort them before processing them everywhere, and the AST visitor should generally be fine with this.

Comment 3

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

And as another addendum... we have been thinking about adding additional passes through the analysis data in our linking phase, so it's possible that we could still do "lazyNesting", but rather than handling it in format.rs we could instead handle it in the linking phase when we're doing clever high level things.