[ASTMatchers] Add matcher for functions that are effectively inline

[Trass3r]

Summary

The existing isInline matcher only catches the ones explicitly marked inline.

[Trass3r]

fix test

[Trass3r]

fix formatting

[ziqingluo-90]

clang/include/clang/ASTMatchers/ASTMatchersMacros.h

96 ↗

(On Diff #466882)

I was wondering if this change is necessary. This definition is so general that it could affect a massive matchers. So any change to it should be very careful and unnecessary changes may be avoided.

Other than that, this patch looks good to me.

[Trass3r]

clang/include/clang/ASTMatchers/ASTMatchers.h

7791

Is there any value in adding variables?
I guess that would mainly target constexpr vars, for which there is isConstexpr.

clang/include/clang/ASTMatchers/ASTMatchersMacros.h

96 ↗

(On Diff #466882)

Just came across it. Since it's a macro-internal class I couldn't see any potential breakage. Nobody should inherit from that. But I can take it out.

[aaron.ballman]

Thanks for the new matcher, can you also add a release note for the addition? One question I have is: what's the need for adding this matcher? Do you plan to use it for some in-tree needs? We usually only add new matchers where there's an immediate need for them because of how expensive AST matchers are to compile (and each matcher adds a fair number of template instantiations to the final binary as well, so there's a bit of runtime cost too).

clang/include/clang/ASTMatchers/ASTMatchers.h

7787–7788

I think it'd be interesting to show some other cases here -- like when the inline keyword is specified on a declaration without a visible definition or when the inline keyword is used on a declaration with a non-inline definition. (And update the comment below accordingly with what is matched.)

7791

I don't see a need for covering variables yet, but I wouldn't be opposed if there was a use case.

clang/include/clang/ASTMatchers/ASTMatchersMacros.h

96 ↗

(On Diff #466882)

I'd remove it as it's unrelated to the changes in the patch, but it might be a reasonable NFC change (especially if there's some positive impact to the change in terms of codegen).

clang/lib/ASTMatchers/Dynamic/Registry.cpp

435

This should be kept in alphabetical order.

clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

310–311

I think you should also test that g() does not match and the additional tests I suggested in the documentation.

[Trass3r]

In D135690#3852362, @aaron.ballman wrote:

what's the need for adding this matcher? Do you plan to use it for some in-tree needs? We usually only add new matchers where there's an immediate need for them because of how expensive AST matchers are to compile (and each matcher adds a fair number of template instantiations to the final binary as well, so there's a bit of runtime cost too).

Didn't realize it has a big cost. Looking inside the AST_MATCHER and REGISTER_MATCHER macros I can't see any unique instantiations, should be memoized?
I created it a while ago for use in a clang-tidy check. Oddly I can't find that code right now.
It might have been for finding inline ctors/dtors: https://github.com/llvm/llvm-project/issues/51577.
https://chromium.googlesource.com/chromium/src/tools/clang/+/refs/heads/main/plugins/FindBadConstructsConsumer.cpp#495

[aaron.ballman]

In D135690#3856863, @Trass3r wrote:

In D135690#3852362, @aaron.ballman wrote:

what's the need for adding this matcher? Do you plan to use it for some in-tree needs? We usually only add new matchers where there's an immediate need for them because of how expensive AST matchers are to compile (and each matcher adds a fair number of template instantiations to the final binary as well, so there's a bit of runtime cost too).

Didn't realize it has a big cost. Looking inside the AST_MATCHER and REGISTER_MATCHER macros I can't see any unique instantiations, should be memoized?

IIRC, the cost may depend on the compiler. I know we had to enable /bigobj when building with MSVC because each template instantiation here was being added to its own section in the object file and we'd wind up with tens of thousands of sections.

I created it a while ago for use in a clang-tidy check. Oddly I can't find that code right now.
It might have been for finding inline ctors/dtors: https://github.com/llvm/llvm-project/issues/51577.
https://chromium.googlesource.com/chromium/src/tools/clang/+/refs/heads/main/plugins/FindBadConstructsConsumer.cpp#495

I think it might make more sense to use a local matcher if you need it only for one clang-tidy check. If we find we need it in more checks or there's a wider need for it, we can hoist it up to ASTMatchers.h so it's exposed more generally at that time. WDYT?

[Trass3r]

In D135690#3858541, @aaron.ballman wrote:

In D135690#3856863, @Trass3r wrote:

Didn't realize it has a big cost. Looking inside the AST_MATCHER and REGISTER_MATCHER macros I can't see any unique instantiations, should be memoized?

IIRC, the cost may depend on the compiler. I know we had to enable /bigobj when building with MSVC because each template instantiation here was being added to its own section in the object file and we'd wind up with tens of thousands of sections.

Interesting, was that before/without -Zc:inline?

I think it might make more sense to use a local matcher if you need it only for one clang-tidy check. If we find we need it in more checks or there's a wider need for it, we can hoist it up to ASTMatchers.h so it's exposed more generally at that time. WDYT?

Yeah a quick search revealed 2 places where it might be useful:
https://github.com/llvm/llvm-project/blob/b6c6933e9548f17d567a20c83eede16b3fcb0c2b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp#L100-L101
https://github.com/llvm/llvm-project/blob/22e4203df813a8051b40adb3e2872e30fdbe1bbe/clang-tools-extra/clang-move/Move.cpp#L243-L245

Of course as shown there you can always do additional checks after matching when writing a check in C++.
But for tools like clang-query the matcher might be useful. Maybe there are also similar code search tools out there based on matchers?

[aaron.ballman]

In D135690#3858621, @Trass3r wrote:

In D135690#3858541, @aaron.ballman wrote:

In D135690#3856863, @Trass3r wrote:

Didn't realize it has a big cost. Looking inside the AST_MATCHER and REGISTER_MATCHER macros I can't see any unique instantiations, should be memoized?

IIRC, the cost may depend on the compiler. I know we had to enable /bigobj when building with MSVC because each template instantiation here was being added to its own section in the object file and we'd wind up with tens of thousands of sections.

Interesting, was that before/without -Zc:inline?

Pretty sure that was after /Zc:inline (IIRC that existed in MSVC 2015 which was roughly when I recall we were hitting this).

I think it might make more sense to use a local matcher if you need it only for one clang-tidy check. If we find we need it in more checks or there's a wider need for it, we can hoist it up to ASTMatchers.h so it's exposed more generally at that time. WDYT?

Yeah a quick search revealed 2 places where it might be useful:
https://github.com/llvm/llvm-project/blob/b6c6933e9548f17d567a20c83eede16b3fcb0c2b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp#L100-L101
https://github.com/llvm/llvm-project/blob/22e4203df813a8051b40adb3e2872e30fdbe1bbe/clang-tools-extra/clang-move/Move.cpp#L243-L245

Of course as shown there you can always do additional checks after matching when writing a check in C++.
But for tools like clang-query the matcher might be useful. Maybe there are also similar code search tools out there based on matchers?

The trouble with clang-query is that it can be used as a reason to expose *any* matcher, because it's specifically about exploring how to match the AST. I do know there are folks who use search tools based on matchers that aren't clang-tidy, but we don't usually support those as part of the matcher interface without a compelling reason.

The two examples you found do look like plausible reasons to expose this matcher though. Would you mind proving that out by switching those over to use the new matcher (as a second patch building on top of this one)? If it looks like we can easily use the new matcher in those checks, then I think it's a good enough reason to add the matcher (and update those checks).

[Trass3r]

In D135690#3862112, @aaron.ballman wrote:

Pretty sure that was after /Zc:inline (IIRC that existed in MSVC 2015 which was roughly when I recall we were hitting this).

I see, yeah /Zc:inline got added slightly earlier:
https://github.com/llvm/llvm-project/commit/f846557eb89a913c69b54653b9bece04b0473233
than /bigobj:
https://github.com/llvm/llvm-project/commit/969b1a50a04e579146f1a78c0c10547a8175577c
Though interestingly it seems to build just fine without the bigobj flag.

fwiw on Linux it adds the following to Registry.cpp.o:

0000000000000000 0000000000000042 W clang::ast_matchers::isEffectivelyInline()
0000000000000000 000000000000002d W clang::ast_matchers::internal::matcher_isEffectivelyInlineMatcher::~matcher_isEffectivelyInlineMatcher()
0000000000000000 000000000000000a W clang::ast_matchers::internal::matcher_isEffectivelyInlineMatcher::matches(clang::FunctionDecl const&, clang::ast_matchers::internal::ASTMatchFinder*, clang::ast_matchers::internal::BoundNodesTreeBuilder*) const
0000000000000000 0000000000000038 V vtable for clang::ast_matchers::internal::matcher_isEffectivelyInlineMatcher

The two examples you found do look like plausible reasons to expose this matcher though. Would you mind proving that out by switching those over to use the new matcher (as a second patch building on top of this one)? If it looks like we can easily use the new matcher in those checks, then I think it's a good enough reason to add the matcher (and update those checks).

Ok 👍

[Trass3r]

clang/include/clang/ASTMatchers/ASTMatchers.h

7787–7788

inline keyword is specified on a declaration without a visible definition

According to https://learn.microsoft.com/en-us/cpp/build/reference/zc-inline-remove-unreferenced-comdat that's not standard-compliant?
But anyways those sound more like use-cases of the existing isInline matcher to me.

This one is more focused on those implicit inline methods which you can't find easily with a simple text search. I've also considered an alternative isImplicitlyInline matcher which would exclude explicit inline usage but I'm not sure which design is better.
It seems I ended up with this inclusive design because I wanted to match the opposite: cxxMethodDecl(unless(isEffectivelyInline()).

clang/docs/LibASTMatchersReference.html

Given
   }
   inline int Foo = 5;
 functionDecl(isInline()) will match ::f().
 namespaceDecl(isInline()) will match n::m.
 varDecl(isInline()) will match Foo;
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('isEffectivelyInline0')"><a name="isEffectivelyInline0Anchor">isEffectivelyInline</a></td><td></td></tr>
+<tr><td colspan="4" class="doc" id="isEffectivelyInline0"><pre>Matches function declarations that are either marked with
+the inline keyword or are implicitly inline.
+
+Given
+  class A {
+    void f() {}
+    void g();
+  };
+functionDecl(isEffectivelyInline()) will match ::f().
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('isMain0')"><a name="isMain0Anchor">isMain</a></td><td></td></tr>
 <tr><td colspan="4" class="doc" id="isMain0"><pre>Determines whether the function is "main", which is the entry point
 into an executable program.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('isNoReturn0')"><a name="isNoReturn0Anchor">isNoReturn</a></td><td></td></tr>
 <tr><td colspan="4" class="doc" id="isNoReturn0"><pre>Matches FunctionDecls that have a noreturn attribute.

clang/include/clang/ASTMatchers/ASTMatchers.h

if (const auto *FD = dyn_cast<FunctionDecl>(&Node))
     return FD->isInlineSpecified();
   if (const auto *NSD = dyn_cast<NamespaceDecl>(&Node))
     return NSD->isInline();
   if (const auto *VD = dyn_cast<VarDecl>(&Node))
     return VD->isInline();
   llvm_unreachable("Not a valid polymorphic type");
 }
 
+/// Matches function declarations that are either marked with
+/// the inline keyword or are implicitly inline.
+///
+/// Given
+/// \code
+///   class A {
+///     void f() {}
+///     void g();
+///   };
+/// \endcode
+/// functionDecl(isEffectivelyInline()) will match f().
+AST_MATCHER(FunctionDecl, isEffectivelyInline) {
+  return Node.isInlined();
+}
+
 /// Matches anonymous namespace declarations.
 ///
 /// Given
 /// \code
 ///   namespace n {
 ///   namespace {} // #1
 ///   }
 /// \endcode

clang/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(isFinal);
   REGISTER_MATCHER(isPrivateKind);
   REGISTER_MATCHER(isFirstPrivateKind);
   REGISTER_MATCHER(isImplicit);
   REGISTER_MATCHER(isInStdNamespace);
   REGISTER_MATCHER(isInTemplateInstantiation);
   REGISTER_MATCHER(isInitCapture);
   REGISTER_MATCHER(isInline);
+  REGISTER_MATCHER(isEffectivelyInline);
   REGISTER_MATCHER(isInstanceMessage);
   REGISTER_MATCHER(isInstanceMethod);
   REGISTER_MATCHER(isInstantiated);
   REGISTER_MATCHER(isInstantiationDependent);
   REGISTER_MATCHER(isInteger);
   REGISTER_MATCHER(isIntegral);
   REGISTER_MATCHER(isLambda);
   REGISTER_MATCHER(isListInitialization);

clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp

TEST(IsInlineMatcher, IsInline) {
   EXPECT_TRUE(matches("void g(); inline void f();",
                       functionDecl(isInline(), hasName("f"))));
   EXPECT_TRUE(matches("namespace n { inline namespace m {} }",
                       namespaceDecl(isInline(), hasName("m"))));
   EXPECT_TRUE(matches("inline int Foo = 5;",
                       varDecl(isInline(), hasName("Foo")), {Lang_CXX17}));
 }
 
+TEST(IsInlineMatcher, IsEffectivelyInline) {
+  EXPECT_TRUE(matches("class X { void f() {} void g(); };",
+                      functionDecl(isEffectivelyInline(), hasName("f"))));
+  EXPECT_TRUE(matches("constexpr void f() {}",
+                      functionDecl(isEffectivelyInline(), hasName("f")), {Lang_CXX11}));
+}
+
 // FIXME: Figure out how to specify paths so the following tests pass on
 // Windows.
 #ifndef _WIN32
 
 TEST(Matcher, IsExpansionInMainFileMatcher) {
   EXPECT_TRUE(matches("class X {};",
                       recordDecl(hasName("X"), isExpansionInMainFile())));
   EXPECT_TRUE(notMatches("", recordDecl(isExpansionInMainFile())));