Page MenuHomePhabricator

[clang] Add `ObjCProtocolLoc` to represent protocol references
ClosedPublic

Authored by dgoldman on Feb 9 2022, 11:58 AM.

Details

Summary

Add ObjCProtocolLoc which behaves like TypeLoc but for
ObjCProtocolDecl references.

RecursiveASTVisitor now synthesizes ObjCProtocolLoc during traversal
and the ObjCProtocolLoc can be stored in a DynTypedNode.

In a follow up patch, I'll update clangd to make use of this
to properly support protocol references for hover + goto definition.

Diff Detail

Event Timeline

dgoldman created this revision.Feb 9 2022, 11:58 AM
dgoldman requested review of this revision.Feb 9 2022, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2022, 11:58 AM
dgoldman updated this revision to Diff 407295.Feb 9 2022, 2:21 PM

Minor lint fixes

dgoldman edited the summary of this revision. (Show Details)Feb 9 2022, 2:26 PM
dgoldman added a reviewer: arphaman.

Thanks @akyrtzi @egorzhdan I was going to suggest that I think someone at Apple should probably look at changes here first.

The goal here is to ensure that any tokens in the code that reference a decl have some AST node (which can be stored in DynTypedNode).
This is provides a nice simple model which clangd's go-to-definition etc rely on: token -> ast node -> target.
Clang almost follows this rule but references to protocols have no dedicated AST node.

clang/include/clang/AST/RecursiveASTVisitor.h
331

RAV changes should have tests

clang/include/clang/AST/TypeLoc.h
2612

These fields shouldn't be public in AST nodes, provide accessors instead (I think they can be read-only).

2621

will we ever have invalid instances?

Thank you @dgoldman, this approach looks good to me.
I don't have anything to add other than what @sammccall has already commented.

dgoldman updated this revision to Diff 408928.Feb 15 2022, 9:36 AM
dgoldman marked an inline comment as done.

Add tests and public accessors

clang/include/clang/AST/TypeLoc.h
2621
dgoldman marked an inline comment as done.Feb 15 2022, 9:37 AM

PTAL, thanks!

I'm a bit concerned about the parent map vs ASTMatchFinder's view being out of sync: we'll have a ProtocolLoc node that has a parent but the parent doesn't have the child.

I'm not sure this breaks anything immediately, but I think we should also make these nodes visible to matchers, even if there's no specific matcher yet.

clang/include/clang/AST/RecursiveASTVisitor.h
1350

i => I etc

clang/include/clang/AST/TypeLoc.h
2611

Also add a public getLocation() accessor for ProtocolLoc? (That returns SourceLocation)

2617

Return non-const pointer, matching other AST nodes. (But method should be const)

2620

This comment doesn't really say anything.

Maybe replace with "the source range is just the protocol name"?

2621

From what I can tell, nope. This is explicitly used here though: https://github.com/llvm/llvm-project/blob/ceb5dc55c2e395c085cd9d16c4152c5a1923d7e2/clang/lib/AST/ParentMapContext.cpp#L405

Well it's not used today, you're adding the use in this patch.

It can be solved in some other way, like pulling out an isNull template and then overloading it for ProtocolLoc.
I don't think we should add API surface to AST nodes to satisfy brittle templates

dgoldman updated this revision to Diff 408970.Feb 15 2022, 11:08 AM
dgoldman marked 4 inline comments as done.

Template and variable casing fixes

I'm a bit concerned about the parent map vs ASTMatchFinder's view being out of sync: we'll have a ProtocolLoc node that has a parent but the parent doesn't have the child.

I'm not sure this breaks anything immediately, but I think we should also make these nodes visible to matchers, even if there's no specific matcher yet.

Hmm I can try to do it - what do I need to modify to make them visible to matchers?

clang/include/clang/AST/TypeLoc.h
2611

this was already there? did you want something else?

2617

Why - Is there a reason to make it mutable? TypeLoc's getTypePtr is const (although this is a decl not a type).

2621

Gotcha, I'm relatively new to templates but took a stab at isNull - LMK what you think/if there's a better way.

I'm a bit concerned about the parent map vs ASTMatchFinder's view being out of sync: we'll have a ProtocolLoc node that has a parent but the parent doesn't have the child.

I'm not sure this breaks anything immediately, but I think we should also make these nodes visible to matchers, even if there's no specific matcher yet.

Hmm I can try to do it - what do I need to modify to make them visible to matchers?

I don't remember specifically, I think ASTMatchFinderImpl has a RecursiveASTVisitor that you need to extend? I'm not sure actually how you would observe these nodes cleanly without matchers (hacks like has(hasParent()) maybe?) So maybe it's best to ignore it in this patch and add basic matchers in a next one

clang/include/clang/AST/TypeLoc.h
2611

Nope sorry I'm just going blind it seems

2617

A bunch of stuff operates on non-const nodes only, RecursiveASTVisitor for example.
And for whatever reason this is the convention, rather than a const+non-const variant of everything.

TypeLoc's getTypePtr is const (although this is a decl not a type).

Yeah types are always const, this is related to the fact that they're interned so mutating them wouldn't make sense.

clang/lib/AST/ParentMapContext.cpp
404

t -> Node (case)

404

These should be static I think so they're not exposed

405

No need for template<>, this can just be a normal function that forms an overload set along with the template

dgoldman updated this revision to Diff 409005.Feb 15 2022, 12:19 PM
dgoldman marked 3 inline comments as done.

Template fixes

dgoldman marked 3 inline comments as done.Feb 15 2022, 12:22 PM

I'm a bit concerned about the parent map vs ASTMatchFinder's view being out of sync: we'll have a ProtocolLoc node that has a parent but the parent doesn't have the child.

I'm not sure this breaks anything immediately, but I think we should also make these nodes visible to matchers, even if there's no specific matcher yet.

Hmm I can try to do it - what do I need to modify to make them visible to matchers?

I don't remember specifically, I think ASTMatchFinderImpl has a RecursiveASTVisitor that you need to extend? I'm not sure actually how you would observe these nodes cleanly without matchers (hacks like has(hasParent()) maybe?) So maybe it's best to ignore it in this patch and add basic matchers in a next one

It looks like ASTMatchFinder.cpp has a MatchASTVisitor which I think is what you meant, but yeah I think it's probably best to do in a follow up unless you think it'll break something here.

sammccall accepted this revision.Feb 18 2022, 3:08 AM

Great!
This is worth mentioning in the clang release notes. (Along with any matchers that match these new nodes)

This revision is now accepted and ready to land.Feb 18 2022, 3:08 AM
This revision was landed with ongoing or failed builds.Feb 18 2022, 12:25 PM
This revision was automatically updated to reflect the committed changes.