Page MenuHomePhabricator

[clang] Update isDerivedFrom to support Objective-C classes ๐Ÿ”
ClosedPublic

Authored by stephanemoore on Apr 10 2019, 3:13 PM.

Details

Summary

This change updates isDerivedFrom to support Objective-C classes by
converting it to a polymorphic matcher.

Notes:
The matching behavior for Objective-C classes is modeled to match the
behavior of isDerivedFrom with C++ classes. To that effect,
isDerivedFrom matches aliased types of derived Objective-C classes,
including compatibility aliases. To achieve this, the AST visitor has
been updated to map compatibility aliases to their underlying
Objective-C class.

isSameOrDerivedFrom also provides similar behaviors for C++ and
Objective-C classes. The behavior that
cxxRecordDecl(isSameOrDerivedFrom("X")) does not match
class Y {}; typedef Y X; is mirrored for Objective-C in that
objcInterfaceDecl(isSameOrDerivedFrom("X")) does not match either
@interface Y @end typedef Y X; or
@interface Y @end @compatibility_alias X Y;.

Test Notes:
Ran clang unit tests.

Diff Detail

Repository
rL LLVM

Event Timeline

stephanemoore created this revision.Apr 10 2019, 3:13 PM
Herald added a project: Restricted Project. ยท View Herald TranscriptApr 10 2019, 3:13 PM
Herald added a subscriber: cfe-commits. ยท View Herald Transcript
stephanemoore marked an inline comment as done.Apr 10 2019, 3:19 PM
stephanemoore added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

I am still uncertain about the naming.

isSubclassOf seemed too generic as that could apply to C++ classes.
objcIsSubclassOf seemed unconventional as a matcher name.
isSubclassOfObjCInterface and isSubclassOfObjCClass seemed awkwardly lengthy.
Creating a new namespace clang::ast_matchers::objc seemed unprecedented.

I am happy to change the name if you think another name would be more appropriate.

aaron.ballman added inline comments.Apr 11 2019, 9:38 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

Does ObjC use the term "derived" by any chance? We already have isDerivedFrom, so I'm wondering if we can use that to also match on an ObjCInterfaceDecl?

stephanemoore planned changes to this revision.Apr 15 2019, 3:49 PM
stephanemoore added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

Objective-C doesn't generally use the term "derived" (for example, see archive of Programming With Objective-C > Defining Classes). With that said, I don't think it's unreasonable or incorrect to use the term "derived" to describe inheritance in Objective-C. The behavior of this matcher is also consistent with the behavior of isDerivedFrom. In order to change isDerivedFrom, I would also need to update isSameOrDerivedFrom. That would probably be a good thing so that derivation matching feature set is consistent for C++ and Objective-C language variants.

Let me take a crack at merging this into isDerivedFrom.

Adding some ObjC experts in case they'd like to weigh in on the name isDerivedFrom in this context.

clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

I agree that if we go with isDerivedFrom, you should update isSameOrDerivedFrom at the same time.

jordan_rose added inline comments.Apr 17 2019, 11:03 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies can't mix, it _should_ be okay, right? They're analogous concepts.

aaron.ballman added inline comments.Apr 17 2019, 11:08 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies can't mix, it _should_ be okay, right? They're analogous concepts.

In the AST matchers, we try to overload the matchers that have similar behavior. My concern is that a user will reach for isSubclassOf() when they really mean isDerivedFrom() or vice versa, and only through annoying error messages learns about their mistake. Given that we already have isDerivedFrom() and renaming it would break code, I was trying to determine whether using that name for both C++ derivation and ObjC derivation would be acceptable.

jordan_rose added inline comments.Apr 17 2019, 11:10 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

Ah, I see what you mean. Yes, I guess it's more important to be consistent than to perfectly match the terminology. You will certainly confuse an ObjC-only developer at first by using "non-standard" terminology, but any developer has to learn a certain amount of compiler-isms anyway using AST matchers.

aaron.ballman added inline comments.Apr 17 2019, 11:15 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

Okay, so it sounds like it wouldn't be hugely problematic to go with isDerivedFrom(), just that it may sound a bit confusing at first to an ObjC developer. Are there some words you think we should add to the documentation to help an ObjC developer searching for this functionality?

jordan_rose added inline comments.Apr 17 2019, 11:22 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

I think just including "subclass" is sufficient. For example, the current doc comment is

/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base.

and it could be changed to something like

/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base, or Objective-C classes that directly or
/// indirectly subclass a class matching \c Base.

A little clunky, but you get what I mean.

aaron.ballman added inline comments.Apr 17 2019, 11:24 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
1479 โ†—(On Diff #194597)

Fantastic, that makes sense to me. Thank you for weighing in!

Present one potential option for making isDerivedFrom support Objective-C classes.

stephanemoore retitled this revision from [clang] Add matcher for subclasses of Objective-C interfaces ๐Ÿ” to [clang] Update isDerivedFrom to support Objective-C classes ๐Ÿ”.May 9 2019, 5:03 PM
stephanemoore edited the summary of this revision. (Show Details)

I did some digging and I believe there are two approaches that we can take to extend isDerivedFrom to support Objective-C classes.

Option 1: Match on Common Ancestor Declaration Type:
Convert isDerivedFrom to match on the common ancestor declaration type, NamedDecl, and return false for
declaration types other than CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
โ€ข Largely works in-place without requiring major changes to matchers built on top of isDerivedFrom.

Disadvantages:
โ€ข Allows nonsensical matchers, e.g., functionDecl(isDerivedFrom("X")).

Option 2: Convert to Polymorphic Matcher:
Convert isDerivedFrom to a polymorphic matcher supporting CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
โ€ข Restricts usage of isDerivedFrom to CXXRecordDecl and ObjCInterfaceDecl.

Disadvantages:
โ€ข Potentially requires many or all matchers using isDerivedFrom to refactor to adapt to the polymorphic matcher interface.

Evaluation
I have been going back and forth as to which approach is superior. Option 1 promotes source stability at the cost of usability while
option 2 seems to promote usability at the cost of source stability. I exported a portrayal of option 1 for consideration as it
required less invasive changes. I can see arguments supporting both approaches.

What do you think? Which of the two approaches do you think we should go with? Is there another approach that I have not thought of?

stephanemoore added inline comments.May 9 2019, 5:31 PM
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
566 โ†—(On Diff #198956)

(note that there are unresolved formatting issues that I intend to fix after I establish which approach to pursue)

I did some digging and I believe there are two approaches that we can take to extend isDerivedFrom to support Objective-C classes.

Option 1: Match on Common Ancestor Declaration Type:
Convert isDerivedFrom to match on the common ancestor declaration type, NamedDecl, and return false for
declaration types other than CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
โ€ข Largely works in-place without requiring major changes to matchers built on top of isDerivedFrom.

Disadvantages:
โ€ข Allows nonsensical matchers, e.g., functionDecl(isDerivedFrom("X")).

Option 2: Convert to Polymorphic Matcher:
Convert isDerivedFrom to a polymorphic matcher supporting CXXRecordDecl and ObjCInterfaceDecl.

Advantages:
โ€ข Restricts usage of isDerivedFrom to CXXRecordDecl and ObjCInterfaceDecl.

Disadvantages:
โ€ข Potentially requires many or all matchers using isDerivedFrom to refactor to adapt to the polymorphic matcher interface.

Evaluation
I have been going back and forth as to which approach is superior. Option 1 promotes source stability at the cost of usability while
option 2 seems to promote usability at the cost of source stability. I exported a portrayal of option 1 for consideration as it
required less invasive changes. I can see arguments supporting both approaches.

What do you think? Which of the two approaches do you think we should go with? Is there another approach that I have not thought of?

This is a great breakdown, thank you for providing it!

Out of curiosity, how invasive is Option 2 within our own code base? Does that option require fixing a lot of code? Are the breakages loud or silent? Is the code easy to modify, or are there corner cases where this option becomes problematic? I prefer Option 2 because it is a cleaner, more understandable design for the matchers. If it turns out that this option causes a hard break (rather than silently changing matcher behavior) and the changes needed to get old code to compile again are minimal, I think it may be a reasonable choice.

Adding some more AST matcher reviewers to see if there are other options that we've not considered.

stephanemoore planned changes to this revision.May 21 2019, 8:56 PM

Out of curiosity, how invasive is Option 2 within our own code base?

I am still working on getting something working but I don't anticipate anything notably invasive.

Does that option require fixing a lot of code?

I believe it doesn't require fixing a lot of code within our codebase. I think we just need to fix the overloaded isDerivedFrom matcher and both isSameOrDerivedFrom matchers.

Are the breakages loud or silent?

I expect that the breakages should be loud compilation failures.

Is the code easy to modify, or are there corner cases where this option becomes problematic?

I am still working on putting together something that works. I am still getting familiar with polymorphic matchers so I probably can't give a reliable assessment of the ease of the code changes just yet. I imagine that the changes in this commit to convert the overloaded isDerivedFrom matcher and the two isSameOrDerivedFrom matchers could be demonstrative to others as to how to resolve breakages. I am not confident enough to state that with certainty though.

I prefer Option 2 because it is a cleaner, more understandable design for the matchers. If it turns out that this option causes a hard break (rather than silently changing matcher behavior) and the changes needed to get old code to compile again are minimal, I think it may be a reasonable choice.

I agree that Option 2 is preferable if we are allowed to require a non-zero amount of migration work from downstream clients. I will continue working on Option 2 and will return once I have something working.

I prefer Option 2 because it is a cleaner, more understandable design for the matchers.

I agree with Aaron. Clang or Clang Tooling provide no promise of source stability. Of course we should not break things just because we can, but API coherence and maintainability of Clang Tooling code are good reasons.

Update isDerivedFrom and related matchers to polymorphic matchers.

stephanemoore marked an inline comment as done.

Add missing braces to multi-line if statements.

stephanemoore accepted this revision.May 24 2019, 8:36 PM

Okay I now have an implementation of Option 2 that works.

I was hoping to find a more elegant solution but since this is the first working implementation of Option 2 I was able to produce, I figured I would present it to get feedback. My lack of familiarity with polymorphic matchers could be causing me to overlook some potential options. If anyone can think of a more elegant solution, I would be happy to implement it.

This revision is now accepted and ready to land.May 24 2019, 8:36 PM
stephanemoore requested review of this revision.May 24 2019, 8:36 PM
aaron.ballman added inline comments.May 27 2019, 7:47 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2638 โ†—(On Diff #201385)

I'd prefer this be named RD so that it doesn't conflict with the name of the RecordDecl type.

2642โ€“2649 โ†—(On Diff #201385)

This should probably be done similar to how classIsDerivedFrom() works. For instance, there's some type alias matching logic that this does not replicate, but we probably want.

2667โ€“2672 โ†—(On Diff #201385)

How about:

const auto *InterfaceDecl = cast<ObjCInterfaceDecl>(&Node);
return Matcher...

We can rely on cast<> to assert if the node is of an unexpected type. Same suggestions below.

stephanemoore planned changes to this revision.May 28 2019, 8:23 PM

Thanks for the input! I will get started on making changes accordingly.

clang/include/clang/ASTMatchers/ASTMatchers.h
2642โ€“2649 โ†—(On Diff #201385)

Upon first glance I had determined that the type aliasing logic in classIsDerivedFrom() was particular to C++. On second consideration, we will probably want to make sure that compatibility aliases are handled correctly for Objective-C. I will take a look into making sure that works as expected.

stephanemoore marked 6 inline comments as done.
stephanemoore edited the summary of this revision. (Show Details)

Update isDerivedFrom to match aliased types and compatibility aliases of
derived Objective-C classes.

I spent some time becoming familiar with how isDerivedFrom behaves for C++ classes. I think that I have managed to get the behavior for Objective-C classes to mirror that of C++ classes. Please let me know if I overlooked anything.

clang/include/clang/ASTMatchers/ASTMatchers.h
2642โ€“2649 โ†—(On Diff #201385)

Done.

2667โ€“2672 โ†—(On Diff #201385)

Good call. Done.

stephanemoore planned changes to this revision.Aug 7 2019, 6:14 PM

Whoops; forgot to add test cases for isDirectlyDerivedFrom ๐Ÿคฆ Will do that shortly.

Add tests for isDirectlyDerivedFrom.

gribozavr accepted this revision.Aug 8 2019, 9:13 AM
gribozavr added inline comments.
clang/lib/ASTMatchers/ASTMatchFinder.cpp
820 โ†—(On Diff #214053)

unordered_set?

This revision is now accepted and ready to land.Aug 8 2019, 9:13 AM
aaron.ballman accepted this revision.Aug 8 2019, 12:37 PM

LGTM

clang/lib/ASTMatchers/ASTMatchFinder.cpp
820 โ†—(On Diff #214053)

or SmallPtrSet?

Use llvm::SmallPtrSet to store the compatible aliases instead of std::set.
Fix a stray unit test failure in RegistryTest.cpp.

stephanemoore marked 3 inline comments as done.Aug 8 2019, 6:31 PM
stephanemoore added inline comments.
clang/lib/ASTMatchers/ASTMatchFinder.cpp
820 โ†—(On Diff #214053)

I went with llvm::SmallPtrSet as that seems more common in clang sources compared to std::unordered_set.

gribozavr accepted this revision.Aug 8 2019, 11:15 PM
This revision was automatically updated to reflect the committed changes.
stephanemoore marked an inline comment as done.
Herald added a project: Restricted Project. ยท View Herald TranscriptAug 12 2019, 4:23 PM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript