[Sema] Warn about unused functions even when they're inline

[aaronpuchert]

Summary

Being inline doesn't change anything about a function having internal
linkage, see [basic.link]p3 for 'static' and p4 for unnamed namespaces.
An internal linkage function can only be used in the same translation
unit, so if it's not being used or needed, that's suspicious.

Also don't recommend "static inline" since for all intents and purposes
it has the same effect as just using "static", which should thus be
preferred.

[• Quuxplusone]

I agree with your reasoning, but in practice I still see a lot of people using static inline for functions (especially function templates) in .h files. I'm not sure exactly why — maybe to reduce the burden on the linker, which would otherwise have to make sure all the symbols had the same address, whereas with static it doesn't have to worry about that?

Have you run this patch over Clang's own codebase, and over libc++? How many positives are there, and do they fall into any thought-provoking patterns?

@mpark, you added some static inline to libc++ in D68840 — any thoughts on this subject?

[efriedma]

I'm not sure this direction really makes sense.

There's a long history of defining utility functions in headers as "static inline". Non-static inline functions in C have confusing semantics. In C++, the semantics are less confusing, but "static inline" still isn't rare. The warning is designed to be compatible with that reality: it allows people to define "static inline" functions, and still get warnings about other functions that might be unused unintentionally. I don't think the warning is realistically usable if it doesn't allow "static inline" functions in headers.

The fact that the "inline" keyword isn't really significant on a "static inline" function doesn't necessarily mean the warning shouldn't exist in its current form.

Have you tried to see what the practical impact of this change is on real-world codebases?

[aaronpuchert]

In D92886#2441290, @Quuxplusone wrote:

I agree with your reasoning, but in practice I still see a lot of people using static inline for functions (especially function templates) in .h files.

That's also what I was seeing, and then I wondered why our warnings don't catch this.

I'm not sure exactly why — maybe to reduce the burden on the linker, which would otherwise have to make sure all the symbols had the same address, whereas with static it doesn't have to worry about that?

I don't think it helps, but perhaps I'm missing something. There are essentially two cases:

• The function is inlined everywhere. Then it's going to be pruned from the object file (because inline functions need to have a definition in all translation units where they're odr-used or something like that), and the linker will never see it.
• The function is not inlined everywhere, so it will be emitted. Then you're right that the linker doesn't have to merge the emitted functions, but that might actually make linking slower, because relocations and so on have to be done for every copy.

My theory is that especially in OOP-heavy code bases programmers develop a habit of thinking in class scope, where static has a completely different meaning. At least that's my impression from our code base.

Have you run this patch over Clang's own codebase, and over libc++? How many positives are there, and do they fall into any thought-provoking patterns?

That's likely a good idea, I will try to run over some code bases.

In D92886#2441410, @efriedma wrote:

There's a long history of defining utility functions in headers as "static inline".

Indeed, I wouldn't want to warn about something that never happens anyway.

Non-static inline functions in C have confusing semantics.

I'm not terribly familiar with C, but judging from C11 6.7.4 it seems that C is a bit more permissive, but then says that certain things are implementation-defined. Whereas in C++ it's forbidden by the one-definition rule that an inline function has another external definition, it's allowed in C but implementation-defined which variant is called.

So if you do non-static inline functions like you'd do them in C++, I think you should be fine. But perhaps I'm missing something, in that case feel free to give me a pointer.

The warning is designed to be compatible with that reality: it allows people to define "static inline" functions, and still get warnings about other functions that might be unused unintentionally. I don't think the warning is realistically usable if it doesn't allow "static inline" functions in headers.

Though it does warn about static non-inline functions, and at least for C++ that has the same effect as "static inline" (perhaps even for C). It would be strange if the warning would hinge on a keyword that doesn't actually change anything in that case, and it would suggest to developers that it's a meaningful change.

Have you tried to see what the practical impact of this change is on real-world codebases?

I'd expect a lot of warnings on our code base at least. We have a bit of a problem with binary sizes, and I think part of that is our usage of internal linkage in header files. (I'm not talking about sure-to-be-inlined functions, but rather templates or functions that shouldn't be in a header.)

Perhaps this is a different issue though, and the proper fix is rather to suppress unused-* warnings in headers entirely and have a different warning about using internal linkage in header files.

[aaronpuchert]

In D92886#2443176, @aaronpuchert wrote:

Non-static inline functions in C have confusing semantics.

I'm not terribly familiar with C, but judging from C11 6.7.4 it seems that C is a bit more permissive, but then says that certain things are implementation-defined. Whereas in C++ it's forbidden by the one-definition rule that an inline function has another external definition, it's allowed in C but implementation-defined which variant is called.

So if you do non-static inline functions like you'd do them in C++, I think you should be fine. But perhaps I'm missing something, in that case feel free to give me a pointer.

Ah, I missed 6.9p5:

If an identifier declared with external linkage is used in an expression [...], somewhere in the entire program there shall be exactly one external definition for the identifier; [...]

So you also have to provide an external definition if you actually intend to use the function. A lot like variables actually, if you declare one in a header you also have to provide a definition in some TU.

Thus C is different from C++ in that C has no notion of weak linkage, is that right? Which means you can't really have header-only libraries in C. Not if that's a concern, I don't think anybody was doing that anyway.

Still I think that internal linkage in headers is inherently a contradiction. Why mark something as internal to a translation unit if you put it in a header file?

In D92886#2443176, @aaronpuchert wrote:

Perhaps this is a different issue though, and the proper fix is rather to suppress unused-* warnings in headers entirely and have a different warning about using internal linkage in header files.

I am growing fond of this idea. I've seen lots of programmers mightily confused by internal declarations in headers being supposedly unused and they would show me where they were used and complain about false positives. The compiler was right, but in the case of -Wunused-function the warning doesn't talk about linkage at all.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_unused_member_function : Warning<"unused member function %0">,
   InGroup<UnusedMemberFunction>, DefaultIgnore;
 def warn_used_but_marked_unused: Warning<"%0 was marked unused but was used">,
   InGroup<UsedButMarkedUnused>, DefaultIgnore;
 def warn_unneeded_internal_decl : Warning<
   "%select{function|variable}0 %1 is not needed and will not be emitted">,
   InGroup<UnneededInternalDecl>, DefaultIgnore;
 def warn_unneeded_static_internal_decl : Warning<
-  "'static' function %0 declared in header file "
-  "should be declared 'static inline'">,
+  "function %0 declared in header file should "
+  "%select{be declared 'inline' instead of|not be declared}1 "
+  "%select{in an anonymous namespace|'static'}2">,
   InGroup<UnneededInternalDecl>, DefaultIgnore;
 def warn_unneeded_member_function : Warning<
   "member function %0 is not needed and will not be emitted">,
   InGroup<UnneededMemberFunction>, DefaultIgnore;
 def warn_unused_private_field: Warning<"private field %0 is not used">,
   InGroup<UnusedPrivateField>, DefaultIgnore;
 def warn_unused_lambda_capture: Warning<"lambda capture %0 is not "
   "%select{used|required to be captured for this use}1">,

clang/lib/Sema/Sema.cpp

for (UnusedFileScopedDeclsType::iterator
           DiagD = FD;
         if (DiagD->isDeleted())
           continue; // Deleted functions are supposed to be unused.
         if (DiagD->isReferenced()) {
           if (isa<CXXMethodDecl>(DiagD))
             Diag(DiagD->getLocation(), diag::warn_unneeded_member_function)
                 << DiagD;
           else {
-            if (FD->getStorageClass() == SC_Static &&
-                !FD->isInlineSpecified() &&
-                !SourceMgr.isInMainFile(
+            if (!SourceMgr.isInMainFile(
                     SourceMgr.getExpansionLoc(FD->getLocation())))
               Diag(DiagD->getLocation(),
                    diag::warn_unneeded_static_internal_decl)
-                  << DiagD;
+                  << DiagD << FD->isInlineSpecified()
+                  << (FD->getStorageClass() == SC_Static);
             else
               Diag(DiagD->getLocation(), diag::warn_unneeded_internal_decl)
                   << /*function*/ 0 << DiagD;
           }
         } else {
           if (FD->getDescribedFunctionTemplate())
             Diag(DiagD->getLocation(), diag::warn_unused_template)
                 << /*function*/ 0 << DiagD;

clang/lib/Sema/SemaDecl.cpp

if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     // instantiated; it's the out-of-line declaration that we're interested in.
     if (FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization &&
         FD->getMemberSpecializationInfo() && !FD->isOutOfLine())
       return false;
 
     if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
       if (MD->isVirtual() || IsDisallowedCopyOrAssign(MD))
         return false;
-    } else {
-      // 'static inline' functions are defined in headers; don't warn.
-      if (FD->isInlined() && !isMainFileLoc(*this, FD->getLocation()))
-        return false;
     }
 
     if (FD->doesThisDeclarationHaveABody() &&
         Context.DeclMustBeEmitted(FD))
       return false;
   } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // Constants and utility variables are defined in headers with internal
     // linkage; don't warn.  (Unlike functions, there isn't a convenient marker

clang/test/SemaCXX/warn-static-function-inheader.h

-static void thing(void) { // expected-warning {{'static' function 'thing' declared in header file should be declared 'static inline'}}
+static void thing(void) { // expected-warning {{function 'thing' declared in header file should be declared 'inline' instead of 'static'}}
 }
 
+static inline void inline_thing(void) { // expected-warning {{function 'inline_thing' declared in header file should not be declared 'static'}}
+}
+
+namespace {
+void anon_thing(void) { // expected-warning {{function 'anon_thing' declared in header file should be declared 'inline' instead of in an anonymous namespace}}
+}
+
+inline void anon_inline_thing(void) { // expected-warning {{function 'anon_inline_thing' declared in header file should not be declared in an anonymous namespace}}
+}
+} // namespace

clang/test/SemaCXX/warn-static-function-inheader.cpp

 #include "warn-static-function-inheader.h"
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
 // rdar://11202617
 
 static void another(void) { // expected-warning {{function 'another' is not needed and will not be emitted}}
 }
 
 template <typename T>
 void foo(void) {
-        thing();
-  another();
+  inline_thing();
+  anon_thing();
+  anon_inline_thing();
+  another();
 }

clang/test/SemaCXX/warn-unused-filescoped.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-template -Wunused-member-function -Wno-unused-local-typedefs \
 // RUN:            -Wno-c++11-extensions -Wno-c++14-extensions -std=c++98 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-template -Wunused-member-function -Wno-unused-local-typedefs -std=c++14 %s
 
 #ifdef HEADER
 
 static void headerstatic() {} // expected-warning{{unused function 'headerstatic'}}
-static inline void headerstaticinline() {}
+static inline void headerstaticinline() {} // expected-warning{{unused function 'headerstaticinline'}}
 
 namespace {
 void headeranon() {} // expected-warning{{unused function 'headeranon'}}
-inline void headerinlineanon() {}
+inline void headerinlineanon() {} // expected-warning{{unused function 'headerinlineanon'}}
 }
 
 namespace test7
 {
   template<typename T>
-  static inline void foo(T) { }
+  static inline void foo(T) { } // expected-warning{{unused function template 'foo'}}
 
-  // This should not emit an unused-function warning since it inherits
+  // This should emit an unused-function warning since it inherits
   // the static storage type from the base template.
   template<>
-  inline void foo(int) {  }
+  inline void foo(int) {  } // expected-warning{{unused function 'foo<int>'}}
 
   // Partial specialization
   template<typename T, typename U>
-  static inline void bar(T, U) { }
+  static inline void bar(T, U) { } // expected-warning{{unused function template 'bar'}}
 
   template<typename U>
   inline void bar(int, U) { }
 
   template<>
   inline void bar(int, int) { }
 };
 
 namespace pr19713 {
 #if __cplusplus >= 201103L
-  static constexpr int constexpr1() { return 1; }
+  static constexpr int constexpr1() { return 1; } // expected-warning{{unused function 'constexpr1'}}
   constexpr int constexpr2() { return 2; }
 #endif
 }
 
 #else
 #define HEADER
 #include "warn-unused-filescoped.cpp"