This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Improve handling of Objective-C protocols in types
ClosedPublic

Authored by dgoldman on Mar 19 2021, 1:25 PM.

Details

Summary

Improve support for Objective-C protocols for types/type locs

Diff Detail

Event Timeline

dgoldman created this revision.Mar 19 2021, 1:25 PM
dgoldman requested review of this revision.Mar 19 2021, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 1:25 PM
dgoldman added inline comments.Mar 21 2021, 7:34 AM
clang-tools-extra/clangd/FindTarget.cpp
435–441

Let me know if I'm missing something here, not exactly sure what the old comment meant - is there a problem since this ObjCObjectType can refer to multiple protocols?

819

Let me know if you have something else in mind to handle the ObjC case here

917

Doesn't look like that's needed, but if we wanted to decorate the type parameters in ObjCObjectTypeLoc with typeParameter would that just go in SemanticHighlighting.cpp?

Friendly ping, not sure if there's something in here and https://reviews.llvm.org/D99975 that I'm missing to properly support the one decl/type loc to multiple references here

LG apart from a few nits & reverting the whitespace changes to unrelated tests.

clang-tools-extra/clangd/FindTarget.cpp
435–441

Here's the way the AST should ideally look like (and what this comment is wishing for):

ObjCObjectTypeLoc C<Foo>
 - ObjCInterfaceTypeLoc C
 - ObjCProtocolLoc Foo // doesn't actually exist

And targetDecl() would work on the two children, but not the parent.

But there's no AST node for protocols. A piece of cleverness in this patch is getting go-to-definition to work on protocols by saying "if the cursor is on ObjCObjectType itself, then it must be on a protocol rather than the base (otherwise it'd be on the ObjCInterfaceType child)".

But it's a hack, as evidenced by the fact that operations that work on the *range* [[C<Foo>]] will still see the target as <Foo> (literally any of nothing, C, C+Foo would be better).

Nevertheless I think it's probably a net win.


Can you leave a comment here like

// There's no child node for just a protocol, so make all the protocols targets.
// But not the base type, which *does* have a child ObjCInterfaceTypeLoc.
// This structure is a hack, but it works well for go-to-definition.
825

this doesn't seem right: we now have a list, the Visit() call may have added things to the list, we need to adjust *all* the added things.

// Add qualifier for the newly-added refs.
for (unsigned I = InitialSize; I < Refs.size(); ++I) {
 // adjust Refs[I]
}
917

That's not something we want to do - ObjCObjectTypeLoc has type arguments, not parameters.
(Not trying to be pedantic with terminology, but the difference is critical here)

e.g. in the function call foo(42) we don't decorate 42 as a parameter, we decorate it as an integer.

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
960–971

can you add a second protocol so we can see the boundaries of this approach?

(I think C<[[Foo]], Bar> should resolve to both @protocol Foo and @protocol Bar)

1046–1047

there are a ton of whitespace changes in this patch, because the version at HEAD isn't clang-format clean :-(

Feel free to format it first in a separate commit, but I'd prefer not to reformat all the tests mixed in with other changes.

sammccall accepted this revision.Apr 23 2021, 3:35 PM

Oops, meant to approve with above comments.

This revision is now accepted and ready to land.Apr 23 2021, 3:35 PM
dgoldman updated this revision to Diff 340653.Apr 26 2021, 2:28 PM
dgoldman marked 2 inline comments as done.

Address review comments

dgoldman marked 3 inline comments as done.Apr 26 2021, 2:31 PM
dgoldman added inline comments.
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
1046–1047

Will do if it hasn't been done already

dgoldman updated this revision to Diff 340659.Apr 26 2021, 2:54 PM
dgoldman marked an inline comment as done.

rebase

dgoldman updated this revision to Diff 340834.Apr 27 2021, 7:17 AM

Rebase on top of formatting changes

This revision was landed with ongoing or failed builds.Apr 27 2021, 7:20 AM
This revision was automatically updated to reflect the committed changes.