[Clang-Tidy] Empty Check

[denik]

Summary

Adds a clang-tidy check for the incorrect use of empty() on a
container when the result of the call is ignored.

Authored-by: Abraham Corea Diaz <[e-mail]>
Co-authored-by: Denis Nikitin <[e-mail]>

[abrahamcd]

Addressed review edits and clarity feedback.

[denik]

Thanks for adding a check! Please check my comments.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

77

Check getAsCXXRecordDecl() for null and add a test case.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

62

I think you also need no-warning test cases when the empty() result is used.

[cjdb]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

77

We should also have a test to ensure empty(0) doesn't segfault.

[abrahamcd]

Progress Update

Hit a roadblock with determining unused return value,
uploading current state for further work.

[abrahamcd]

Stuck on determining unused return value, uploaded current state for @denik and @cjdb to take a look.

[abrahamcd]

Adds functionality for only warning on the case of unused return value
of the call to empty() and removes using-directive.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

147

nullptr.

[cjdb]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

129

Let's eliminate some of these else-clauses by using early exits.

[abrahamcd]

Adds argument 0 test case and refactoring based on feedback.

[cjdb]

Thanks for working on this! I'll merge this on your behalf once there's maintainer approval.

[denik]

Thanks Abraham!

[abrahamcd]

Gentle ping!

[abrahamcd]

Hi all, I just wanted to check in on this again and see if any more feedback or progress could be made. Thank you!

[rsmith]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

24

150

This only looks for direct members, not ones found in a base class. Eg, this will not find clear() in llvm::SmallVector<T, N>. If you have a Sema object here, you could do a real name lookup; otherwise, you can use CXXRecordDecl::lookupInBases to do the lookup. You should be able to find some examples by searching for existing calls to that function.

[abrahamcd]

Checks for clear() in base classes as well.

[abrahamcd]

Adds non-dependent base class test case.

[njames93]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

58–73

These using declarations are annoying, the common convention in tidy checks is to just use a global using namespace ast_matchers at the top of the file.

121–122

We also need to check qualifiers. If there exists a clear method but its unavailable due to it not being const when our member is const. The fix would result in a compile error.

125

dyn_cast isn't needed here as we have already checked we are dealing with a CXXMethodDecl.

133

The 'did you mean' diagnostics in clang all use a semicolon ; instead of a comma between the message and suggestion.

178–179

Diagnostics can accept args as const NamedDecl *, so you can omit the call to getQualifiedNameAsString().
The use of dyn_cast here is suspicious. If the CalleeDecl isn't a NamedDecl, this would assert when you try and call getQualifiedNameAsString, but equally I can't see any case why the CalleeDecl wouldn't be a NamedDecl. @rsmith Can you think of any situation where this could happen?

[abrahamcd]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

178–179

Seems that without getQualifiedNameAsString() I get longer less-readable versions of the name, e.g. 'empty<std::vector<int> &>' instead of just 'std::empty'. Do you think the extra information is helpful here?

[cjdb]

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

58–73

Adding them just after the namespaces are opened would be good.

I've had issues with refactoring llvm-project files in the past that have global using-directives, so I'd prefer it if we stuck with the using-declarations please. If they're a part of the preamble, they become additional lines that will be scrolled over.

121–122

Good catch. We probably shouldn't be suggesting clear() on a const object anyway.

178–179

I'm partial to 'std::empty' because that's presumably what the user will type, and so it'll be easier for them to understand. 'empty<std::vector<int> &>' isn't useful because we almost always defer to type deduction.

[abrahamcd]

Adds check for the compatibility of qualifiers on the clear method and the object it is called on.

[abrahamcd]

Minor formatting/naming fixes.

[abrahamcd]

Hi, checking in on this again, thanks!

[cjdb]

@njames93, @LegalizeAdulthood, are there any further concerns regarding this CL?

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

104

Please synchronize with first statement in documentation.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

630

Excessive newline.

[abrahamcd]

Formatting fixes and synchronization of documentation.

[cjdb]

This patch has been open for weeks without maintainer input, despite repeated requests, and @abrahamcd finishes his internship this Friday. We would very much appreciate direction on whether or not D128372 is ready to be merged.

[cjdb]

Ping.

[MaskRay]

Drive-by comments: this patch may need a review from common reviewers like @njames93.

I assume that @LegalizeAdulthood's comment (about the file hierarchy since there was a big reorganization few months ago) has been addressed. If you are concerned with unable to contact @LegalizeAdulthood, I think folks like @njames93 can decide moving forward the patch despite the "Request Changes" state. That said, it'll always be good to give @LegalizeAdulthood some time when the patch is in a ready-to-submit state.

[denik]

Any updates from the reviewers?

[denik]

@LegalizeAdulthood, @njames93, is there anything else we should address in this change?

[denik]

@LegalizeAdulthood, @njames93, friendly ping.

[cjdb]

@LegalizeAdulthood @njames93 is there anything actionable that's left for this patch?

[denik]

@Eugene.Zelenko , could you please stamp the patch if you don't have any other concerns?

@njames93 , if you don't have spare cycles could you please suggest other reviewers?

[Eugene.Zelenko]

In D128372#3883214, @denik wrote:

@Eugene.Zelenko , could you please stamp the patch if you don't have any other concerns?

Please wait for developers approval. I look only for trivial issues.

[cjdb]

@njames93 if you don't have any further concerns, I am going to merge this on Friday afternoon, as it will have been four months since there has been a maintainer's input.

[carlosgalvezp]

In D128372#3957982, @cjdb wrote:

@njames93 if you don't have any further concerns, I am going to merge this on Friday afternoon, as it will have been four months since there has been a maintainer's input.

@cjdb I have raised a concern about the lack of reviewer feedback in clang-tidy here:
https://discourse.llvm.org/t/rfc-improve-code-review-process-for-clang-tidy/66740

If more people express their concerns perhaps it can be escalated to the point where action is taken?

[MaskRay]

I just took a glance and the code looks reasonable.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp

132

delete blank line

161

delete blank line

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp

162

unneeded blank line

ditto below

They just make tests less compact without good values.

178

[[@LINE-1]] is deprecated lit syntax. Use [[#@LINE-1]]. Don't use the prevailing style in clang-tidy tests.

[denik]

Format changes.

Removed blank lines and fixed test types.

[denik]

Replaced deprecated [[@LINE-1]]

[denik]

In D128372#3965162, @MaskRay wrote:

I just took a glance and the code looks reasonable.

@MaskRay thanks for the review! I have updated the patch.

[MyDeveloperDay]

Ironically as an aside... looking back at the review because of the discource article.

The whole reason why I added the [[nodiscard]] checker to clang-tidy 4 years ago was to cover exactly this use case...and many others like it where users have a function which returns a boolean and takes no arguments are pointer or reference (empty() included)

https://reviews.llvm.org/D55433

Shouldn't people be using this checker to add nodiscard and let the compiler to the heavy lifting. Was there a use case as to why that solution doesn't solve this?

[Eugene.Zelenko]

Please take a loon on https://github.com/llvm/llvm-project/issues/59487.

[cjdb]

In D128372#3991516, @MyDeveloperDay wrote:

Ironically as an aside... looking back at the review because of the discource article.

The whole reason why I added the [[nodiscard]] checker to clang-tidy 4 years ago was to cover exactly this use case...and many others like it where users have a function which returns a boolean and takes no arguments are pointer or reference (empty() included)

https://reviews.llvm.org/D55433

Shouldn't people be using this checker to add nodiscard and let the compiler to the heavy lifting. Was there a use case as to why that solution doesn't solve this?

That check is for library authors: this one is for library users. There are a non-insignificant number of cases where an unused call to empty pops up, hence the desire for this check.