[clang-tidy] Added option to readability-else-after-return

[njames93]

Summary

Added a 'RefactorConditionVariables' option to control how the check handles condition variables

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

200

Please use single back-ticks for option name.

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

67

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

[njames93]

Address doc backticks.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

67

Emit -> emit

74

The -> the

I'm a bit unclear on what "attempt to refactor" means -- I sort of understand it to mean that if this option is true then the check will not produce a fix-it for variables defined inside the condition of the if statement that is used in the else branch, but will produce a diagnostic. However, the check behavior seems to also remove the diagnostic in this case (not just the fix-it), so I'm not certain I'm reading this right.

[njames93]

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

74

Good spot, the check behaviour is also removing the diagnostic as well as the fix it.
That behaviour should probably be changed to removing the Fix-It when this option is false, but then diagnostic behaviour should follow what WarnOnUnfixable dictates.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

74

I think that makes sense. Then the option can be renamed to remove the "Refactor" bit and probably be something more like WarnOnConditionVariables or something?

[njames93]

Renamed new option to WarnOnConditionVariables

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

193

Would it make sense to hoist this into the previous if statement so we don't bother checking the condition var use in the first place if we're just going to ignore the results?

228

Same here.

[njames93]

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

193

That wouldn't work, we need to see if there is a condition variable that needs refactoring first before we can disregard it, Or am I missing something?

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

193

I was suggesting:

if (WarnOnConditionVariables && checkConditionVarUsageInElse(If)) {
 ...
}
if (WarnOnConditionVariables && checkInitDeclUsageInElse(If) {
...
}

The effect is that we don't bother checking for the situations we weren't going to warn about anyway. But maybe I'm the one missing something?

[njames93]

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

193

I think you may be this time. The reason being those if statements have a return at the end. If I followed how you have it layed out, those returns would never hit regardless of whether there was a condition variable that needed handling or not.

[aaron.ballman]

LGTM once the documentation nit is fixed up.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

193

Oh! So you're talking about the case where IsLastInScope and WarnOnUnfixable are both false, okay I see that now. I think the logic could still be rearranged to avoid doing work you're just going to immediately throw away, but I don't think it's critical (or really needed for this patch).

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

74

Still have to fix the capitalization issues with The and Emit.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h

public:
   ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   const bool WarnOnUnfixable;
+  const bool WarnOnConditionVariables;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

 
 namespace {
 static const char ReturnStr[] = "return";
 static const char ContinueStr[] = "continue";
 static const char BreakStr[] = "break";
 static const char ThrowStr[] = "throw";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
+static const char WarnOnConditionVariablesStr[] = "WarnOnConditionVariables";
 
 const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   if (!Node)
     return nullptr;
   if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Node)) {
     if (DeclRef->getDecl()->getID() == DeclIdentifier) {
       return DeclRef;
     }
Diag << tooling::fixit::createReplacement(
         SourceRange(ElseExpandedLoc, EndLoc), Repl);
   }
 }
 } // namespace
 
 ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      WarnOnUnfixable(Options.get(WarnOnUnfixableStr, true)) {}
+      WarnOnUnfixable(Options.get(WarnOnUnfixableStr, true)),
+      WarnOnConditionVariables(Options.get(WarnOnConditionVariablesStr, true)) {
+}
 
 void ElseAfterReturnCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, WarnOnUnfixableStr, WarnOnUnfixable);
+  Options.store(Opts, WarnOnConditionVariablesStr, WarnOnConditionVariables);
 }
 
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
   const auto InterruptsControlFlow =
       stmt(anyOf(returnStmt().bind(ReturnStr), continueStmt().bind(ContinueStr),
                  breakStmt().bind(BreakStr),
                  expr(ignoringImplicit(cxxThrowExpr().bind(ThrowStr)))));
   Finder->addMatcher(
if (!IsLastInScope && containsDeclInScope(Else)) {
     if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }
 
   if (checkConditionVarUsageInElse(If) != nullptr) {
+    if (!WarnOnConditionVariables)
+      return;
     if (IsLastInScope) {
       // If the if statement is the last statement its enclosing statements
       // scope, we can pull the decl out of the if statement.
       DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
                                << ControlFlowInterruptor;
       if (checkInitDeclUsageInElse(If) != nullptr) {
         Diag << tooling::fixit::createReplacement(
                     SourceRange(If->getIfLoc()),
if (checkConditionVarUsageInElse(If) != nullptr) {
     } else if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }
 
   if (checkInitDeclUsageInElse(If) != nullptr) {
+    if (!WarnOnConditionVariables)
+      return;
     if (IsLastInScope) {
       // If the if statement is the last statement its enclosing statements
       // scope, we can pull the decl out of the if statement.
       DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
                                << ControlFlowInterruptor;
       Diag << tooling::fixit::createReplacement(
                   SourceRange(If->getIfLoc()),
                   (tooling::fixit::getText(*If->getInit(), *Result.Context) +

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-str34-c
   <clang-tidy/checks/cert-str34-c>` to
   :doc:`bugprone-signed-char-misuse
   <clang-tidy/checks/bugprone-signed-char-misuse>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`readability-else-after-return
+  <clang-tidy/checks/readability-else-after-return>` check now supports a 
+  `WarnOnConditionVariables` option to control whether to refactor condition
+  variables where possible.
+
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 
   Now able to rename member references in class template definitions with 
   explicit access.
 
 - Improved :doc:`readability-qualified-auto
   <clang-tidy/checks/readability-qualified-auto>` check now supports a

clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst

void foo(int Value) {
 
         if (Value == 3) {
           throw 42;
         }
         Local++;
       }
     }
 
+Options
+-------
+
+.. option:: WarnOnUnfixable
+
+   When `true`, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that
+   would have an extended lifetime if the ``else`` branch was removed.
+   Default value is `true`.
+
+.. option:: WarnOnConditionVariables
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch
+   defining them just before the ``if`` statement. This can only be done if 
+   the ``if`` statement is the last statement in its parents scope.
+   Default value is `true`.
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-<https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>`_.

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp

+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN:     -config='{CheckOptions: [ \
+// RUN:         {key: readability-else-after-return.WarnOnConditionVariables, value: false}, \
+// RUN:     ]}'
+
+bool foo(int Y) {
+  // Excess scopes are here so that the check would have to opportunity to
+  // refactor the variable out of the condition.
+
+  // Expect warnings here as we don't need to move declaration of 'X' out of the
+  // if condition as its not used in the else.
+  {
+    if (int X = Y)
+      return X < 0;
+    else
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+      return false;
+  }
+  {
+    if (int X = Y; X)
+      return X < 0;
+    else
+      // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+      return false;
+  }
+
+  // Expect no warnings for these cases, as even though its safe to move
+  // declaration of 'X' out of the if condition, that has been disabled
+  // by the options.
+  {
+    if (int X = Y)
+      return false;
+    else
+      return X < 0;
+  }
+  {
+    if (int X = Y; X)
+      return false;
+    else
+      return X < 0;
+  }
+}