[PATCH] D105904: [clangd] Support `#pragma mark` in the outline

Wed Jul 14 08:49:29 PDT 2021

dgoldman added a comment.

In D105904#2877184 <https://reviews.llvm.org/D105904#2877184>, @kadircet wrote:

> So first of all, it is not clear to me if this is a useful enough improvement to justify all the new code, but looking at your investment I'll assume this is really common in ObjC land and gonna be useful for developers working with ObjC. That's enough of a justification for me, but I'd be more than happy to see some real data if you have any.

Yep, I can you send you some searches internally with how prevalent they are.

> That being said, reading the code it seems like the intent is enabling `#pragma mark` directives the introduce symbols that are either leaves or containers:
>
> - Leaf case is obvious and straightforward, it is just like a normal declaration happening at that particular location. It is unclear which kind should be used. It seems you've went with `File` but I don't think it is suitable in this case. I would suggest using `Null`.
> - Container case is more sophisticated. One thing is for sure, they are nested under the same symbol a declaration occuring at that point would be. They also seem to contain all the symbols that would nest under current subtree and spelled after the mark and spelled before any other marks. Hence there is no way to explicitly terminate the scope of a mark. They are implicitly terminated by hitting the end of the container containing the mark or hitting a new mark directive. This also implies one cannot have multiple levels of nesting just by pragma directives, but it can be achieved if there are usual containers with more mark directives (see my comment around tests for an example).
>
> Can you verify that I understood the intent correctly and make it more explicit in the comments as well?

Yep, although there are some TODOs there, we could allow letting leaf nodes to be contained in a group parent as well as potentially merging them in the `#pragma mark -` followed by `#pragma mark Foo` case.

There's no good kind at the moment IMO, I don't really have a preference as long as it doesn't render awkwardly in VS Code.

> (Terminating containers implicitly sounds like an unfortunate limitation to me. I'd rather only enable termination of them explicitly) but I suppose there's a lot of code that's already existing out in the world with the assumption of XCode's implicit termination rules, so this is probably not gonna be helpful in reality. Hence we can't do much.)

I don't think this an Xcode limitation really - more of an LSP spec limitation. If we create a group inside of another symbol, we are effectively contained in it and cannot escape without violating the spec, right? To be clear, Xcode rules are a bit different - see the link above - the `#pragma mark -` just creates a divider which we can't mirror.

> Before diving into the details of the implementation it looks like we are doing some post-processing to incorporate mark pragmas into the tree, why can't we just insert them while building the tree as we do with macros? for example:
>
> - when inserting a symbol we look for a possible mark-pragma container (similar to macro case), a mark pragma is only eligible as a container if it is contained within the range of the parent && spelled before current declaration at hand && is a group.

This assumes (like this code today) that the nodes are sorted properly and the ranges of the parent are accurate. I was thinking of adding a proper sort in a post-processing phase but maybe we could do it + range fix up during insertion. Seems complicated to move everything over to insertion, but if you think that's reasonable, I can try that out.

And that case generally works but we need to be sure to handle multiple pragmas next to each other. `#pragma mark - Foo\n#pragma mark - Bar` turns Foo into an empty group and Bar takes decls after. And then, currently, a `#pragma mark Third` would end the group as well.

> - as for insertion of leaves, after traversal of all the children we can just add any mark pragmas that occured within the current symbol's range as a child. we can also drop any mark pragmas occuring inside symbols that doesn't require child traversal this way.

I think it's a bit more complicated since the leaf nodes terminate the groups. As long as we do that it should be fine, assuming editors don't require it to be sorted.

> This approach relies on the fact that `range` calculated by `declToSym` will be correct enough. I think it should be good in most cases, but might require some adjustments around macros, as usual (can't think of a problematic example right now though).
> This brings out the question about ordering of `children` in `DocumentSymbol`, lsp doesn't say anything about it today, but during rendering we can just order these based on the `selectionRange` (probably during `build`) and hope that clients will preserve it.
>
> I believe implementing it as part of the traversal rather than as post processing both makes code easier to reason about and more efficient. So WDYT about going in that direction?

We could - I considered it, but with the existing behavior and potential subtle ordering issues I thought it would be simpler, but less efficient to do it after. I took a look at the Macro Expansion code and didn't understand it + potential edge cases well enough to add it there.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105904/new/

https://reviews.llvm.org/D105904