This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] New check: bugprone-suspicious-include
ClosedPublic

Authored by jroelofs on Feb 15 2020, 9:20 AM.

Details

Summary

Detects and fixes suspicious code like: #include "foo.cpp".

Inspired by: https://twitter.com/lefticus/status/1228458240364687360?s=20

Diff Detail

Event Timeline

jroelofs created this revision.Feb 15 2020, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2020, 9:20 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.EditedFeb 15 2020, 6:11 PM

How about .cc and .cxx extension? It'll be reasonable to provide option to configure list of extensions.

Please add documentation and mention new check in Release Notes.

clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
49 ↗(On Diff #244829)

Unnecessary empty line.

Eugene.Zelenko added a project: Restricted Project.

I have a feeling this check should be called something along the lines of bugprone-suspicous-include.

clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
62 ↗(On Diff #244829)

This replacement is dangerous, I have a feeling no fix-it should be provided or at least do a search of the include directories to see if file you are trying to include actually does exist. The correct file could be *.hpp like what boost uses for all its header files

jroelofs marked an inline comment as done.Feb 16 2020, 6:43 AM

I have a feeling this check should be called something along the lines of bugprone-suspicous-include.

That's a much better name, I like it.

clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
62 ↗(On Diff #244829)

Yeah, perhaps the FixIt should only be added if there is a single candidate replacement that exists on the -I path.

Another option is to not add FixIts at all, and instead emit a list of note:s suggesting each of the candidates.

njames93 added inline comments.Feb 16 2020, 7:25 AM
clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
11–13 ↗(On Diff #244829)

You don't need to include or use the AST matchers in a preprocessor only check.

50 ↗(On Diff #244829)

I have a feeling this is to suppress some unused parameter linting check. If it is then it should be removed as unused parameters in an overridden function shouldn't emit a warning.
Same for below.

Side note: File a bug with whichever linter tool gave you that warning (if there was one).

62 ↗(On Diff #244829)

How about the case if someone wants to (for whatever reason) include something like this:

#include "example.h"
#include "example.cpp"

That looks intentional and a fix it shouldn't be emitted.

clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.h
31 ↗(On Diff #244829)

This should be marked override.

jroelofs updated this revision to Diff 244883.Feb 16 2020, 8:16 AM
jroelofs marked an inline comment as done.
  • Renamed to bugprone-suspicious-include
  • Removed FixIts in favor of note: suggestions, iff the suggested file exists on the header search path.
  • Removed unnecessary unused parameter casts-to-void.
jroelofs marked an inline comment as done.Feb 16 2020, 8:17 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
50 ↗(On Diff #244829)

Sorry, did this out of habit. Indeed it isn't actually needed.

jroelofs added inline comments.Feb 16 2020, 8:17 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
27

FIXME: still need to write docs for this.

jroelofs marked 4 inline comments as done and an inline comment as not done.Feb 16 2020, 8:20 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
62 ↗(On Diff #244829)

I'd imagine that people doing this intentionally (unity builds come to mind) would turn off this check. That said, is there something better this check could be doing to accommodate those people?

jroelofs updated this revision to Diff 244884.Feb 16 2020, 8:27 AM

git format-patch

jroelofs marked an inline comment as done.Feb 16 2020, 8:29 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
60

FIXME: is this a reasonable SourceLocation to give to LookupFile?

njames93 added inline comments.Feb 17 2020, 2:18 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
44

This function is used for #import on modules. Do we still want the same behaviour when importing modules? More thinking about the extension suggestions with that one.

60

Main thing is to make sure that the same location is used for the warnings and the notes.
I personally found this the most pleasing diagnostics

SourceLocation DiagLoc = FilenameRange.getBegin().getLocWithOffset(1);
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11: warning: suspicious #include of file with .cpp extension [bugprone-suspicious-include]
#include "a.cpp"
          ^
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11: note: did you mean to include 'a'?
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11: note: did you mean to include 'a.h'?
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11: note: did you mean to include 'a.hpp'?
jroelofs updated this revision to Diff 244994.Feb 17 2020, 9:34 AM

Implement review feedback re: DiagLoc

jroelofs marked an inline comment as done.Feb 17 2020, 9:38 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
60

I don't have much experience with modules to lean on for intuition on what this check should do with them. I've added a test to codify what the pass currently does in that case, but if there's something better it should be trying when searching for suggestions, let's make it do that instead.

jroelofs updated this revision to Diff 245031.Feb 17 2020, 1:39 PM

Make the diagnostics more accurate.

I'd suggest possibly adding 2 Options at Global Level SourceFileExtensions and HeaderFileExtensions, both would take semicolon seperated lists.
Reason they are Global is there are probably other checks that could use them.
The SourceFileExtensions could be defaulted to .c;.cc;.cpp;.cxx and the HeaderFileExtensions .h;.hh;.hpp;<empty>.
You need the <empty> because how Options get parsed, then it can manually be transformed to "" when you read it.

aaron.ballman added inline comments.Feb 23 2020, 7:46 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
51–52

We already have HeaderFileExtensionsUtils.h -- we should be using that for the header file extensions (which should be configurable).

It might make sense to make those utilities be general for other kinds of file extensions as well, such as source files. I think that list should also be configurable.

56

Should probably add ' quotes around the extension to make it more obvious where the extension starts and stops.

jroelofs updated this revision to Diff 246604.Feb 25 2020, 5:11 PM

Implement review feedback:

  • Re-purpose HeaderFileExtensionsUtils.h to support headers *and* sources.
  • Surround file extension in diagnostic with ''s.

I have these as two separate patches locally, but I don't know how to represent that in phabricator, so I've uploaded the diff across both. If this gets approved, I intend to push them as separate commits.

Implement review feedback:

  • Re-purpose HeaderFileExtensionsUtils.h to support headers *and* sources.
  • Surround file extension in diagnostic with ''s.

I have these as two separate patches locally, but I don't know how to represent that in phabricator, so I've uploaded the diff across both. If this gets approved, I intend to push them as separate commits.

You can create a review that just re-purposes HeaderFileExtensionsUtils then set that review as a parent of this review

Implement review feedback:

  • Re-purpose HeaderFileExtensionsUtils.h to support headers *and* sources.
  • Surround file extension in diagnostic with ''s.

I have these as two separate patches locally, but I don't know how to represent that in phabricator, so I've uploaded the diff across both. If this gets approved, I intend to push them as separate commits.

You could merge two patches (diff to base version) and update diff in this request.

aaron.ballman added inline comments.Feb 27 2020, 11:32 AM
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
77

Please don't use auto here (the type isn't spelled out in the initialization).

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
36

Delimiter here as well.

54

Extension

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
9

Can drop the HEADER from these macros.

48

delimiter -> Delimiter per our usual naming conventions.

njames93 added inline comments.Feb 27 2020, 1:05 PM
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
45

Follow the convention of the other checks and use HeaderFileExtensions as the option name. Probably call the other option ImplementationFileExtensions

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
42

This could be changed to

if (!llvm::all_of(Extension, isAlphanumeric))
  return false;
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
38

A lot of configuration options for clang tidy use semi-colon ; seperated lists. Would it not be wise to carry on the convention and use semi colon here (and below) as well. It could break a few peoples configuration but that would be realised fairly quickly and updated

jroelofs retitled this revision from [clang-tidy] New check: misc-no-include-cpp to [clang-tidy] New check: bugprone-suspicious-include.Mar 2 2020, 4:11 PM
jroelofs updated this revision to Diff 247764.Mar 2 2020, 4:59 PM

implement review feedback

jroelofs marked 8 inline comments as done.Mar 2 2020, 5:00 PM
jroelofs marked 2 inline comments as done.

Adding the parent revision means you don't need to have those changes in this patch. It will cause issues down the line. best thing is to run a diff from this to the parent and just include those changes

jroelofs marked 2 inline comments as done.Mar 2 2020, 8:51 PM

Adding the parent revision means you don't need to have those changes in this patch.

IIUC, that is what I've already done:

https://reviews.llvm.org/D74669 is git show c4dd6f5903e -U999
https://reviews.llvm.org/D75489 is git show 0cda7e0b9ed -U999

where c4dd6f5903e is the parent of 0cda7e0b9ed.

The suggestion to switch over to ;s as delimiters is what's causing this diff to still touch so many files.

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
24 ↗(On Diff #247764)

";h;hh;hpp;hxx"

clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
25 ↗(On Diff #247764)

";h;hh;hpp;hxx"

jroelofs updated this revision to Diff 248208.Mar 4 2020, 9:10 AM
jroelofs marked an inline comment as not done.
jroelofs updated this revision to Diff 248246.Mar 4 2020, 10:45 AM
lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
38

I think i'm missing the point of D75621.
Why can't we keep using , as delimitter?
Why do we have to change everything, introduce inconsistency
(is it still documented that , is still valid?),
and spam into terminal if , is found?

njames93 added inline comments.Mar 5 2020, 1:24 AM
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
38

This reason

➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}" -- -std=c++17
YAML:1:59: error: unknown key 'hpp'
{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}
                                                          ^
Error: invalid configuration specified.
Invalid argument
➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, value: h;;hpp;hxx }]}" -- -std=c++17
➜  ~
lebedev.ri added inline comments.Mar 5 2020, 1:45 AM
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
38
$ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}" -- -std=c++17
YAML:1:59: error: unknown key 'hpp'
{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}
                                                          ^
Error: invalid configuration specified.
Invalid argument
$ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, value: \"h,,hpp,hxx\" }]}" -- -std=c++17
Error while processing /repositories/llvm-project/test.cpp.
error: no input files [clang-diagnostic-error]
error: no such file or directory: '/repositories/llvm-project/test.cpp' [clang-diagnostic-error]
error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Found compiler error(s).
jroelofs updated this revision to Diff 249164.Mar 9 2020, 10:53 AM

Fix comment, add docs.

jroelofs marked 3 inline comments as done.Mar 9 2020, 11:15 AM
jroelofs added inline comments.
clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
9
jroelofs marked 4 inline comments as done.Mar 9 2020, 11:16 AM
aaron.ballman accepted this revision.Mar 9 2020, 2:24 PM

LGTM aside from some documentation and testing nits.

clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
15–17

Missing full stops at the end of the comments.

24

The -> the

25

contain "." -> contain a "."

26

using -> use
leaving -> leave

29

ImplementationFileExtensions ;-)

clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
7

For those of you following along at home, a.h already exists on ToT (as does b.h). :-D

10

Can you add a test for:

#include "i.cpp"

to show that it suggests i.h (which already exists in-tree) but not i or i.hpp?

This revision is now accepted and ready to land.Mar 9 2020, 2:24 PM
jroelofs updated this revision to Diff 249223.Mar 9 2020, 2:49 PM

Implement review feedback.

jroelofs updated this revision to Diff 249224.Mar 9 2020, 2:55 PM

Add missing release notes.

jroelofs closed this revision.Mar 9 2020, 3:00 PM
jroelofs marked 7 inline comments as done.
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
10

happy to :)

Harbormaster completed remote builds in B48610: Diff 249224.
Eugene.Zelenko added inline comments.Mar 9 2020, 3:13 PM
clang-tools-extra/docs/ReleaseNotes.rst
101 ↗(On Diff #249224)

Please keep alphabetical order.

104 ↗(On Diff #249224)

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
7

Please omit The checker. Clang-tidy uses check in its terminology.

24

Please use single back-ticks for option values. Same above.

jroelofs updated this revision to Diff 249377.Mar 10 2020, 7:39 AM

Don't fire on #imports.

The error on Windows suggests it would normally do the "right" thing anyway:

http://45.33.8.238/win/10088/step_8.txt

C:\src\llvm-project\out\gn\obj\clang-tools-extra\test\clang-tidy\checkers\Output\bugprone-suspicious-include.cpp.tmp.cpp:18:2: error: #import of type library is an unsupported Microsoft feature [clang-diagnostic-error]
#import "c.c"

LGTM with the changes for #import.

Firefox uses a unified build model. For better performances in the binary, the C++ files are compiled as the same time from a single file (ex: Unified_cpp_netwerk_base3.cpp) which will include the .cpp files.

This isn't a common use case, so, don't hesitate to ignore me but checks triggers 4286 new issues. About 20 of them are legit.