Page MenuHomePhabricator

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

Authored by mgartmann on Mar 31 2021, 3:09 AM.

Details

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.

Diff Detail

Event Timeline

mgartmann created this revision.Mar 31 2021, 3:09 AM
mgartmann requested review of this revision.Mar 31 2021, 3:09 AM

I am working on fixing the failing build.

riccibruno added inline comments.
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 added inline comments.
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
101

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.

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 marked 10 inline comments as done.Mar 31 2021, 1:55 PM
mgartmann added inline comments.
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.

It looks like new patch was not uploaded.

mgartmann updated this revision to Diff 334527.Mar 31 2021, 2:03 PM
mgartmann marked 4 inline comments as done.

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

Eugene.Zelenko added inline comments.Mar 31 2021, 2:19 PM
clang-tools-extra/clang-tidy/misc/StdStreamObjectsOutsideMainCheck.h
30 ↗(On Diff #334527)

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

Removed superfluous semicolon in StdStreamObjectsOutsideMainCheck.cpp according to feedback.

mgartmann updated this revision to Diff 334632.Apr 1 2021, 3:12 AM
mgartmann marked an inline comment as done.

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

mgartmann added inline comments.Apr 1 2021, 3:17 AM
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 added inline comments.Apr 1 2021, 3:38 AM
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.

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.

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 updated this revision to Diff 335750.Apr 7 2021, 2:36 AM
mgartmann retitled this revision from [clang-tidy] misc-std-stream-objects-outside-main: a new check to [clang-tidy] misc-avoid-std-io-outside-main: a new check.
  • 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 updated this revision to Diff 335839.Apr 7 2021, 9:20 AM

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

mgartmann added inline comments.Tue, Apr 13, 11:35 PM
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.

Friendly ping :)

njames93 added inline comments.Mon, May 3, 3:17 AM
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 updated this revision to Diff 343911.Sun, May 9, 7:21 AM

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

mgartmann marked an inline comment as done.Sun, May 9, 7:25 AM

@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 updated this revision to Diff 344013.Mon, May 10, 3:00 AM

Remove trailing whitespaces from documentation file.

mgartmann updated this revision to Diff 344023.Mon, May 10, 5:05 AM

Updated diff to fix the build.

mgartmann updated this revision to Diff 344030.Mon, May 10, 5:57 AM

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

mgartmann updated this revision to Diff 344043.Mon, May 10, 6:34 AM

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

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

mgartmann updated this revision to Diff 344711.Wed, May 12, 1:25 AM

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

mgartmann updated this revision to Diff 344726.Wed, May 12, 2:20 AM

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

mgartmann updated this revision to Diff 344731.Wed, May 12, 2:32 AM
This comment was removed by mgartmann.