[clang-tidy] misc-avoid-std-io-outside-main: a new check

[mgartmann]

Summary

Problem Description

The iostream header file defines multiple global objects like std:cin, std::cout, etc.

The pitfall of using these global objects outside of the main function is that it negatively affects the code's testability. They cannot be replaced with a mocked input stream during testing. Therefore, std::cin, std::cout etc. should only be used inside the main function. If other functions need an input or an output stream, one is encouraged to use the std::istream and std::ostream interfaces and to receive the stream object files as function parameters.
Thus, during testing, mocked stream objects can be handed to the function.

What this Check Does

The goal of this check is to find any uses of predefined standard stream objects (i.e., cout, wcout, cerr, wcerr, cin, wcin) and to check whether they are used inside the main function or not. If any uses are found outside the main function, they get flagged.

The use of clog and wclog does not get flagged by this check. The rationale for this is that code with logging functionality rarely needs to be tested.

Context

The idea for this check is adopted from the checks implemented in Cevelop IDE.

This contribution is part of a project which aims to port Cevelop's built-in checks to other IDEs.

We are happy to receive any suggestions for improvement.

[mgartmann]

I am working on fixing the failing build.

[riccibruno]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

25 ↗

(On Diff #334382)

Will this match my_namespace::cin?

47 ↗

(On Diff #334382)

You can use FunctionDecl::isMain. Additionally you might want to also use FunctionDecl::isMSVCRTEntryPoint for the Windows-specific main functions.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

61 ↗

(On Diff #334382)

Please add newline.

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.h

24 ↗

(On Diff #334382)

Check must work only for C++. Please add isLanguageVersionSupported.

39 ↗

(On Diff #334382)

Please add newline.

clang-tools-extra/docs/ReleaseNotes.rst

120

Please highlight cin and other with double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/misc-std-stream-objects-outside-main.rst

6 ↗

(On Diff #334382)

Please synchronize with Release Notes and omit The check.

46 ↗

(On Diff #334382)

Please add newline.

[njames93]

Is it not wise to also check the c standard library.
So check for function refs to these names in the global or std namespace.
printf, vprintf, puts, putchar, scanf, scanf, getchar and gets
It may be a bit of a pain checking for usages of stdin and stdout due to them being defined as macros.

If you make this change the name of the check would likely need updating as they aren't stream objects.

Going on a tangent here, but on POSIX platforms you can redirect stdin, stdout, and stderr using dup and dup2 (Windows is _dup and _dup2 respectively), gtest uses this functionality.

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

42 ↗

(On Diff #334382)

This all seems unnecessary, you can add unless(forFunction(isMain())) to your declRefExpr matcher instead.

clang-tools-extra/test/clang-tidy/checkers/misc-std-stream-objects-outside-main.cpp

1 ↗

(On Diff #334382)

Why won't this work in c++11 mode?

[mgartmann]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

25 ↗

(On Diff #334382)

Yes, at the moment this would be matched as well.

42 ↗

(On Diff #334382)

Thanks for pointing this out to me. forFunction() is exactly what I was initially looking for. Unfortunately, I did not find it.

I now refactored the code to use this matcher instead.

I guess isMain() would not match if a MSVCRT entry point (see @riccibruno's answer on line 47) is used, is it? Do you think it makes sense to also match this kind of entry point? How could this be done? I was not able to find anything related in the AST Matcher Reference.

47 ↗

(On Diff #334382)

Due to @njames93's suggestion to use a unless(forFunction(isMain())) matcher, this function is not needed anymore.

Thank you anyways for pointing this out to me.

clang-tools-extra/test/clang-tidy/checkers/misc-std-stream-objects-outside-main.cpp

1 ↗

(On Diff #334382)

When not specifying C++14 as the standard, the corresponding tests failed.
Some errors in the test output pointed out that e.g., auto return without trailing return types is a C++14 extension.

I created a gist (click me) which contains the test ouput of this clang-tidy check. There, all occured errors can be seen.

IMHO, those errors seem to come from the directly or indirectly included header files rather than from this check. Please correct me if I am wrong.

After adding -std=c++14-or-later, no errors occured anymore. That is the reason why I added it.

Anyways, I now refactored the test file, creating a std namespace and adding my own "mocked" stream objects. I adpoted this approach from existing tests (e.g., readability-string-compare).

Thus, no include is needed anymore and -std=c++14-or-later can be omitted.

If I overlooked something, I would be glad if you could point this out.

[Eugene.Zelenko]

It looks like new patch was not uploaded.

[mgartmann]

Refactored the code and documentation files according to the feedback received on the first diff.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.h

30 ↗

(On Diff #334527)

Semicolon is not needed and should cause -Wextra-semi warnings.

[mgartmann]

Removed superfluous semicolon in StdStreamObjectsOutsideMainCheck.cpp according to feedback.

[mgartmann]

Add isInStdNamespace to matcher so that only global objects in namespace std are matched and add corresponding tests.

[mgartmann]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

25 ↗

(On Diff #334382)

Thinking about it, in my opinion, only matching those objects if they are coming form the std namespace would make more sense and lead to less "false-positive" diagnostics.
Furthermore, Cevelop IDE's check, which is where the idea for this check came form, also behaves like this.

Thus, I added the isInStdNamespace() matcher in the latest diff.

@riccibruno What is your opinion on this? Would it make more sense to also match e.g., my_namespace::cin in your opinion?

[riccibruno]

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

25 ↗

(On Diff #334382)

That was my point. I don't think that my_namespace::cin should be matched.

[mgartmann]

In D99646#2661651, @njames93 wrote:

Is it not wise to also check the c standard library.
So check for function refs to these names in the global or std namespace.
printf, vprintf, puts, putchar, scanf, scanf, getchar and gets
It may be a bit of a pain checking for usages of stdin and stdout due to them being defined as macros.

Hi @njames93,
I can see your point, I am going to add this functionality.

However, I do not completely understand what you mean with check for function refs to these names in the global or std namespace.
Could you explain this a bit further?

E.g., should all calls to these functions be flagged if they happen inside the std namespace or in the global namespace?
And if they happen inside my_namespace e.g., they should not be flagged. Am I understanding this correctly?
How should the check behave if the functions are called inside main()?

Would it make more sense to put this functionality into a separat check?

Thanks for your effort in advance.
Looking forward to hearing from you soon.

[njames93]

In D99646#2663733, @mgartmann wrote:

Hi @njames93,
I can see your point, I am going to add this functionality.

However, I do not completely understand what you mean with check for function refs to these names in the global or std namespace.
Could you explain this a bit further?

E.g., should all calls to these functions be flagged if they happen inside the std namespace or in the global namespace?
And if they happen inside my_namespace e.g., they should not be flagged. Am I understanding this correctly?
How should the check behave if the functions are called inside main()?

I mean any DeclRefExpr that reference those functions.
We don't actually want to only match on call expressions to those functions, we also want the other ways they can be called.

auto Print = &puts;
Print("This is using stdio");

I could imagine this is the kind of matcher expression you need.

declRefExpr(
     hasDeclaration(functionDecl(
         anyOf(hasDeclContext(translationUnitDecl()), isInStdNamespace()),
         hasAnyName("printf", "vprintf", ...))),
     unless(forFunction(isMain())))

As for flagging the same rules should apply for references to cin, cout etc.

Would it make more sense to put this functionality into a separat check?

Thanks for your effort in advance.
Looking forward to hearing from you soon.

You could include it in this check, then maybe rename this check to a more general name like misc-avoid-stdio-outside-main.

clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.cpp

22 ↗

(On Diff #334632)

should probably use varDecl here instead of namedDecl.

[mgartmann]

• Added two new matchers to flag uses of stdio.h/csdtio functions outside of main.
• Renamed the check to fit those new functionalities.

Rationale:

When analyzing the AST dump of a program using stdio.h's printf() e.g., I found that those functions are not declared directly under translationUnit, but under translationUnitDecl/linkageSpecDecl. A hasDeclContext(linkageSpecDecl()) matcher worked with both stdio.h and cstdio functions.
In the check's test, I tried to include a own header file from the Inputs directory. However, the functions declared in this header file were then direct AST children of translationUnitDecl and not of linkageSpecDecl.
Due to the fact that I was not able to reproduce the original AST in the tests, I decided to omit this part of the matcher.

Furthermore, when using cstdio's std::printf() e.g., these functions are in fact UsingShadowDecls. Despite them being declared in the std namespace, a call of std::printf("...") does not match with a isInStdNamespace() matcher.
Thus, this part of the matcher was omitted as well.

The resulting matcher matches any reference to a function which is called "printf", "vprintf", "puts", "putchar", "scanf", "getchar" or "gets".

@njames93: I would be interested to hear your opinion on this newest update. If I overlooked something, I would be more than happy to incorporate any further feedback.

[mgartmann]

Corrected check's entry in list.rst after renaming the check.

[mgartmann]

clang-tools-extra/clang-tidy/misc/AvoidStdIoOutsideMainCheck.cpp

23

Would there be a way to extract these names (cin, cout, ...) into a separate variable? In my opinion, this would make the AST matchers cleaner and easier to read.

E.g., I was not able to find an overload of hasAnyName() which takes a std:.vector as argument.

Looking forward to hearing from you.
Thanks for any feedback in advance.

[mgartmann]

Friendly ping :)

[njames93]

clang-tools-extra/clang-tidy/misc/AvoidStdIoOutsideMainCheck.cpp

23

hasAnyName can take an ArrayRef<StringRef> So If you have a std::vector<StringRef> that would work. If you have std::vector<std::string> you can still sort of use it

auto StringClassMatcher = cxxRecordDecl(hasAnyName(SmallVector<StringRef, 4>(
    StringLikeClasses.begin(), StringLikeClasses.end())));

[mgartmann]

Extracted STD IO stream and C-like IO function names into vectors.

[mgartmann]

@njames93 thanks a lot for your answer! I extracted the STD IO stream and C-like function names according to your comment.

In your opinion, is there something else in this patch that has to be improved to make this request mergeable and to get a LGTM?

[mgartmann]

Remove trailing whitespaces from documentation file.

[mgartmann]

Updated diff to fix the build.

[mgartmann]

Change encoding of patch to UTF-8 in order to fix build.

[mgartmann]

Revert ReleaseNotes.rst to a point where the build worked.

[mgartmann]

Revert ReleaseNotes.rst to its initial content in a try to fix the pre-build tests.

[mgartmann]

Re-add description for this check in ReleaseNotes.rst.
Adjust AvoidStdIoOutsideMainCheck.cpp third matcher to call hasAnyName() with a vector.

[mgartmann]

Remove any parentheses and slashes from the check's section in ReleaseNotes.rst in order to try to fix the build.

[mgartmann]

This comment was removed by mgartmann.

[mgartmann]

Fetched new commits from upstream main branch and resolved merge conflicts.

[mgartmann]

Dear reviewers,

Since this work was conducted as part of a Bachelor's thesis, which has to be
handed in on the 18th of June, 2021, we wanted to add a friendly ping to this
patch.

It would make us, the thesis team, proud to state that some of our work was
accepted and included into the LLVM project.

Therefore, we wanted to ask if we can further improve anything in this patch
for it to get a LGTM?

Thanks a lot in advance!
Yours faithfully,
Fabian & Marco

clang-tools-extra/clang-tidy/misc/AvoidStdIoOutsideMainCheck.h

+//===--- AvoidStdIoOutsideMainCheck.h - clang-tidy --------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_AVOIDSTDIOOUTSIDEMAINCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_AVOIDSTDIOOUTSIDEMAINCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <string>
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds predefined standard stream objects like ``cin``, ``wcin``, ``cout``,
+/// ``wcout``, ``cerr`` or ``wcerr``, that are used outside the ``main``
+/// function. It also finds uses of ``cstdio`` and ``stdio.h`` functions like
+/// ``printf()`` outside the ``main`` function.
+///
+/// For the user-facing documentation and examples see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-avoid-std-io-outside-main.html
+class AvoidStdIoOutsideMainCheck : public ClangTidyCheck {
+public:
+  AvoidStdIoOutsideMainCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::vector<std::string> StdIOStreams = {"cin",   "wcin", "cout",
+                                                 "wcout", "cerr", "wcerr"};
+  const std::vector<std::string> CLikeIOFunctions = {
+      "printf", "vprintf", "puts", "putchar", "scanf", "getchar", "gets"};
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_AVOIDSTDIOOUTSIDEMAINCHECK_H

clang-tools-extra/clang-tidy/misc/AvoidStdIoOutsideMainCheck.cpp

+//===--- AvoidStdIoOutsideMainCheck.cpp - clang-tidy ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidStdIoOutsideMainCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void AvoidStdIoOutsideMainCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declRefExpr(to(varDecl(hasAnyName(SmallVector<StringRef>(
+                                 StdIOStreams.begin(), StdIOStreams.end())),
+                             isInStdNamespace())),
+                  unless(forFunction(isMain())))
+          .bind("StdStreamObject"),
+      this);
+
+  Finder->addMatcher(
+      declRefExpr(hasDeclaration(functionDecl(hasAnyName(SmallVector<StringRef>(
+                      CLikeIOFunctions.begin(), CLikeIOFunctions.end())))),
+                  unless(forFunction(isMain())))
+          .bind("CLibFunction"),
+      this);
+
+  /// Matcher for indirect stdio uses:
+  /// \code
+  ///   auto Print = &puts;
+  ///   Print("This is using stdio");
+  /// \endcode
+  Finder->addMatcher(
+      declRefExpr(
+          hasDeclaration(varDecl(hasDescendant(declRefExpr(
+              hasDeclaration(functionDecl(hasAnyName(SmallVector<StringRef>(
+                  CLikeIOFunctions.begin(), CLikeIOFunctions.end())))))))),
+          unless(forFunction(isMain())))
+          .bind("CLibFunction"),
+      this);
+}
+
+void AvoidStdIoOutsideMainCheck::check(const MatchFinder::MatchResult &Result) {
+
+  if (const auto *MatchedStreamObj =
+          Result.Nodes.getNodeAs<DeclRefExpr>("StdStreamObject")) {
+    diag(MatchedStreamObj->getLocation(),
+         "predefined standard stream objects should "
+         "not be used outside the main function");
+    return;
+  }
+
+  if (const auto *MatchedCLibFunc =
+          Result.Nodes.getNodeAs<DeclRefExpr>("CLibFunction")) {
+    diag(MatchedCLibFunc->getLocation(),
+         "cstdio functions should not be used outside the main function");
+    return;
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyMiscModule
+  AvoidStdIoOutsideMainCheck.cpp
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoRecursionCheck.cpp
   NonCopyableObjects.cpp
   NonPrivateMemberVariablesInClassesCheck.cpp
   RedundantExpressionCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AvoidStdIoOutsideMainCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoRecursionCheck.h"
 #include "NonCopyableObjects.h"
 #include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AvoidStdIoOutsideMainCheck>(
+        "misc-avoid-std-io-outside-main");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
 
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
 - New :doc:`bugprone-unhandled-exception-at-new
   <clang-tidy/checks/bugprone-unhandled-exception-at-new>` check.
 
   Finds calls to ``new`` with missing exception handler for ``std::bad_alloc``.
 
+- New :doc:`misc-avoid-std-io-outside-main
+  <clang-tidy/checks/misc-avoid-std-io-outside-main>` check.
+
+  Finds predefined standard stream objects like ``cin``, ``wcin``, ``cout``,
+  ``wcout``, ``cerr`` or ``wcerr``, that are used outside the ``main``
+  function. It also finds uses of ``cstdio`` and ``stdio.h`` functions like
+  ``printf()`` outside the ``main`` function.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-pos47-c
   <clang-tidy/checks/cert-pos47-c>` to
   :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
    `llvmlibc-callee-namespace <llvmlibc-callee-namespace.html>`_,
    `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
+   `misc-misc-avoid-std-io-outside-main <misc-avoid-std-io-outside-main.html>`_,
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
    `misc-redundant-expression <misc-redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc-static-assert.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc-avoid-std-io-outside-main.rst

+.. title:: clang-tidy - misc-avoid-std-io-outside-main
+
+misc-avoid-std-io-outside-main
+=============
+
+Finds predefined standard stream objects like ``cin``, ``wcin``, ``cout``,
+``wcout``, ``cerr`` or ``wcerr``, that are used outside the ``main``
+function. It also finds uses of ``cstdio`` and ``stdio.h`` functions like
+``printf()`` outside the ``main`` function.
+
+For instance, in the following code, the use of ``std::cout`` and ``printf()``
+outside of ``main()`` would get flagged whereas the use of them inside
+``main()`` is not flagged:
+
+.. code-block:: c++
+
+  #include <iostream>
+  #include <cstdio>
+
+  void some_function() {
+    std::cout << "This triggers the check."; // NOK
+    std::printf("This triggers the check."); // NOK
+  }
+
+  int main() {
+    std::cout << "This does not trigger the check."; // OK
+    std::printf("This does not trigger the check."); // OK
+  }
+
+Since the predefined standard stream objects are global objects, their use
+outside of ``main()`` worsens a program's testability and is thus discouraged.
+Instead, those objects should only be used inside ``main()``. They can then be
+passed as arguments to other functions like so:
+
+.. code-block:: c++
+
+  #include <iostream>
+
+  void some_function(std::istream & in, std::ostream & out) {
+    out << "This does not trigger the check.";
+
+    int i{0};
+    in >> i;
+  }
+
+  int main() {
+    some_function(std::cin, std::cout);
+  }
+
+In contrast to using ``std::cin`` and ``std::cout`` directly, in the above
+example, it is possible to inject mocked stream objects into
+``some_function()`` during testing.