This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add readability-use-early-exits check
Needs ReviewPublic

Authored by njames93 on Jul 20 2022, 9:32 AM.

Details

Summary

Add a check to flag loops and functions where an if at the end of the body could be negated and made into an early exit.

Diff Detail

Event Timeline

njames93 created this revision.Jul 20 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 9:32 AM
njames93 requested review of this revision.Jul 20 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 9:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.Jul 20 2022, 9:58 AM
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
109

Please add newline.

njames93 updated this revision to Diff 446197.Jul 20 2022, 10:13 AM

Add option to wrap early exit in braces.

njames93 marked an inline comment as done.Jul 20 2022, 10:14 AM
njames93 added inline comments.Jul 20 2022, 10:28 AM
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
218

I have an idea that we could infer this using the readability-braces-around-statements and its hicpp alias.
If enabled, probe the ShortStatementLines option to infer whether or not that check would fire if we make this transformation without using braces.

njames93 updated this revision to Diff 446219.Jul 20 2022, 11:22 AM

Added logic to infer WrapInBraces option if unspecified.

njames93 updated this revision to Diff 446255.Jul 20 2022, 1:46 PM

Fix pre-build failing.

njames93 updated this revision to Diff 447236.Jul 25 2022, 2:07 AM

Add option SplitConjunctions to alter fix-it for if statements with && in the condition.

njames93 updated this revision to Diff 447967.Jul 27 2022, 2:38 AM

Fix typo in check list documentation.

njames93 updated this revision to Diff 449048.Aug 1 2022, 9:42 AM

Rebase and Ping??

njames93 updated this revision to Diff 451856.Aug 11 2022, 7:38 AM

Refactor some of the impl.

njames93 updated this revision to Diff 454275.Aug 20 2022, 11:47 PM

Add NestedControlFlow options

This option can be used to silence diagnostics when there aren't any nested blocks inside the if statement.

just my 2 cents

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
67

did you consider comma expressions?

if (myDirtyCode(), myCondition && yourCondition). It seems to me, that the transformation would be incorrect.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
56

typo cunjunctions -> conjunctions

64

if this option is false, the transformation would be if(!(A && B)), right?

should demorgan rules be applied or at least be mentioned here? I think transforming to if (!A || !B) is at least a viable option for enough users.

113

maybe highlight the if as code with double quotes?

njames93 marked an inline comment as done.Aug 30 2022, 1:54 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
67

Comma operator is a binary operator, so the transformation would wrap the whole expression in parens.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
64

Once this is in, I plan to merge some common code with the simplify-boolean-expr logic for things like demorgan processing. Right now the transformation happens, the simplify boolean suggests a demorgan transformation of you run the output through clang tidy.

JonasToth added inline comments.Aug 31 2022, 6:44 AM
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
64

a short reference to the readability-simplify-boolean-expr check in the user facing docs would be great.
i personally find it ok if the users "pipe" multiple checks together to reach a final transformation.

would this check then use the same settings as the readability-simplify-boolean-expr check? (that of course off topic and does not relate to this patch :) )

njames93 updated this revision to Diff 457199.Sep 1 2022, 1:35 AM
njames93 marked 3 inline comments as done.

Fix documentation.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
64

I'm not sure it's really necessary to mention that the fix would likely need another fix, besides that comment would just be removed in the follow up.

JonasToth added a comment.EditedSep 4 2022, 1:40 AM

_Edit_: If overloaded operators are ignored and the check only applies to pointers, int and bool the following is irrelevant. But I did not spot such a guard.
If this is the case, it of course opens the question about at least optional<> types and how to handle those.

IMHO the check looks good, but I do have concerns about correctness of the transformation in specific cases, especially overloaded operators.

If the conditions are inverted, it might be the case that the inverted operator is not overloaded, resulting in compilation errors.
I think that should be guarded against.

And then there is the more subtle issue of different semantics of the operators.

Hypothetical Matrix Algebra Situation:

// A, B are matrices of booleans with identical dimensions
// A && B will do '&&' on each matrix element
// The matrices overload 'operator bool()' for implicit conversion to 'bool',
// true := any element != false
// false := all false
if (A && B) {
  // !C inverts each boolean in C, same '&&' application
  if (B && !C) {
    padLines();
  }
}

Transformed to

if (!A )
  continue;
if ( !B)
  continue;
if (!B )
  continue;
if ( C)
  continue;
padLines();

is highly likely not a correct and equivalent transformation, both in this instance and in general for sure.

C++ allows to implement operators with different semantics and classes must not behave like 'int' for the operators. Such overloads are not necessarily incorrect. E.g. boost::sml uses the operator overloading to define state machines, which does not follow anything close to the semantics we need for this check.
Expression-template libraries (especially linear algebra and so on) might either break or change meaning as well.

In my personal opinion the check should at least have an option to disable transformations for classes with overloaded operators and verify that the transformation would still compile if done anyway.
I think even better would be a "concept check" to at least verify that the type in question models a boolean with its operators. But I am not sure how far such a check should go and I am aware that it would not be perfect anyway.

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
330

i would really prefer to just use strings here.
std::string Buff = CheckAlias.str() + OptName.str(); is much easier to understand and equivalent (?)

this function is called only once in the constructor as well, so speed and allocations are not valid in my opinion.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
64

ok, thats good with me.

njames93 added a comment.EditedSep 5 2022, 1:15 AM

...

Your concerns aren't actually that important. Because the transformations only work on for binary operators, and not CXXOperatorCallExpr, it would always never do any special logic, instead just wrap the whole thing in parens and negate it.

if (!(A && B))
  continue;

if (!(!B && C))
  continue;

padLines();

The only potential issue would be cases when the binary operator is type dependent, as binary operators where they type is unresolved are handled as BinaryOperators, even if every instantiation would be resolved to an operator call

template <size_t N, size_t M>
void fancyMatrix(Matrix<N,M> A, Matrix<M, M> B) {
  if (a && B) {
    ...
  }
}

I think the best way to resolve that could be delayed diagnostics for template operators.
So cache the locations of all dependent binary (and unary) operators, and see if any of them either stay as binary operators or get transformed into CXXOperatorCallExprs, if so don't emit the fix.
The second option is to just not emit any fix for template dependent operators.
WDYT?

...

Your concerns aren't actually that important. Because the transformations only work on for binary operators, and not CXXOperatorCallExpr, it would always never do any special logic, instead just wrap the whole thing in parens and negate it.

Ah yes, you are right! That makes it very much better.

I think the best way to resolve that could be delayed diagnostics for template operators.
So cache the locations of all dependent binary (and unary) operators, and see if any of them either stay as binary operators or get transformed into CXXOperatorCallExprs, if so don't emit the fix.
The second option is to just not emit any fix for template dependent operators.
WDYT?

I am not 100% sure that delayed diagnostics would solve the issue _always_

template <typename T1, typename T2>
bool type_dependent_logic(T1 foo, T2 bar) {
    return foo && !bar;
}

This template would be a counter example to yours, where I feel less comfortable.
The definition itself has BinaryOperator in the AST.

Now different TUs might use this template wildly different, e.g. TU1.cpp only with builtins, making a transformation possible and TU2.cpp with fancy and weird types making the transformation not possible.

clang-tidy (e.g. with run-clang-tidy) would now still change the template definition after it received the diagnostic from TU1.cpp, even though in TU2.cpp it is not considered as valid.
This issue is common to many checks, e.g. the const-correctness suffered from it as well. In the const case I had to just had to consider type dependent expressions as modifications.

For TU-local templates your strategy is certainly viable. But in the general case I would rather not do it, or at least have a "default-off" option for it.

Finally, what are your thoughts on optional types? They are kinda bool in the sense of this check, but suffer from the shortcomings with overloaded operators.
Should they be explicitly not considered now (i am fine with that!)? In that case, it should be mentioned as limitation in the documentation.

I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?

I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?

One of the driving factors of this is that it can be used to remove nesting levels, which can definitely aid with readability.

PiotrZSL added inline comments.Mar 23 2023, 12:30 AM
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
360

This will trigger on all system code, and then users will complain again that they see poor clang-tidy performance...

when it could be just something like

ifStmt(unless(isExpansionInSystemHeader()),
       unless(isConsteval()),
       unless(isConstexpr()),
       unless(hasElse(stmt())),
       unless(hasInitStatement(stmt()),
       hasThen(compoundStmt(hasSizeAboeLineTreshold())),
       ...

Simply everything that could be put into matcher should be put into matcher (easier to maintain), also what's a point of checking functions that doesn't have if's. On that point also some implicit functions or if statement should be ignored, to avoid checking templates twice.

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp
145
  • Missing test with if in template function & implicit specialization, just to make sure that fixes won't get to mess up.
  • Missing test with if under macro.
  • Missing test with if under switch under loop (to check if brak/continue will be used correctly, instead of return).
njames93 added inline comments.Mar 23 2023, 6:20 AM
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
360

Not triggering on system headers is a good point, but for raw performance, that code is better residing in the visitor, which can massively outperform matchers as we can straight up ignore huge blocks of code that aren't of interest.